Merge pull request #6770 from tchak/fix-instructeur-notifications

app/controllers/administrateurs/groupe_instructeurs_controller.rb

@@ -125,7 +125,9 @@ module Administrateurs
         end
 
         if procedure.routee?
-          groupe_instructeur.instructeurs << instructeurs
+          instructeurs.each do |instructeur|
+            groupe_instructeur.add(instructeur)
+          end
 
           GroupeInstructeurMailer
             .add_instructeurs(groupe_instructeur, instructeurs, current_user.email)
@@ -139,7 +141,9 @@ module Administrateurs
         else
 
           if instructeurs.present?
-            procedure.defaut_groupe_instructeur.instructeurs << instructeurs
+            instructeurs.each do |instructeur|
+              procedure.defaut_groupe_instructeur.add(instructeur)
+            end
             flash[:notice] = "Les instructeurs ont bien été affectés à la démarche"
           end
         end
@@ -158,7 +162,7 @@ module Administrateurs
       else
         instructeur = Instructeur.find(instructeur_id)
         if procedure.routee?
-          if instructeur.remove_from_groupe_instructeur(groupe_instructeur)
+          if groupe_instructeur.remove(instructeur)
             flash[:notice] = "L’instructeur « #{instructeur.email} » a été retiré du groupe."
             GroupeInstructeurMailer
               .remove_instructeur(groupe_instructeur, instructeur, current_user.email)
@@ -167,7 +171,7 @@ module Administrateurs
             flash[:alert] = "L’instructeur « #{instructeur.email} » n’est pas dans le groupe."
           end
         else
-          if instructeur.remove_from_groupe_instructeur(procedure.defaut_groupe_instructeur)
+          if procedure.defaut_groupe_instructeur.remove(instructeur)
             flash[:notice] = "L’instructeur a bien été désaffecté de la démarche"
           else
             flash[:alert] = "L’instructeur n’est pas affecté à la démarche"

app/controllers/instructeurs/groupe_instructeurs_controller.rb

@@ -20,7 +20,7 @@ module Instructeurs
       if groupe_instructeur.instructeurs.include?(instructeur)
         flash[:alert] = "L’instructeur « #{instructeur_email} » est déjà dans le groupe."
       else
-        groupe_instructeur.instructeurs << instructeur
+        groupe_instructeur.add(instructeur)
         flash[:notice] = "L’instructeur « #{instructeur_email} » a été affecté au groupe."
         GroupeInstructeurMailer
           .add_instructeurs(groupe_instructeur, [instructeur], current_user.email)
@@ -35,7 +35,7 @@ module Instructeurs
         flash[:alert] = "Suppression impossible : il doit y avoir au moins un instructeur dans le groupe"
       else
         instructeur = Instructeur.find(instructeur_id)
-        if instructeur.remove_from_groupe_instructeur(groupe_instructeur)
+        if groupe_instructeur.remove(instructeur)
           flash[:notice] = "L’instructeur « #{instructeur.email} » a été retiré du groupe."
           GroupeInstructeurMailer
             .remove_instructeur(groupe_instructeur, instructeur, current_user.email)

app/controllers/instructeurs/procedures_controller.rb

@@ -216,11 +216,13 @@ module Instructeurs
 
     def email_notifications
       @procedure = procedure
-      @assign_to = assign_to
+      @assign_to = assign_tos.first
     end
 
     def update_email_notifications
-      assign_to.update!(assign_to_params)
+      assign_tos.each do |assign_to|
+        assign_to.update!(assign_to_params)
+      end
       flash.notice = 'Vos notifications sont enregistrées.'
       redirect_to instructeur_procedure_path(procedure)
     end
@@ -290,10 +292,6 @@ module Instructeurs
       @exports = Export.find_for_groupe_instructeurs(groupe_instructeur_ids)
     end
 
-    def assign_to
-      current_instructeur.assign_to.joins(:groupe_instructeur).find_by(groupe_instructeurs: { procedure: procedure })
-    end
-
     def assign_tos
       @assign_tos ||= current_instructeur
         .assign_to

app/models/groupe_instructeur.rb

@@ -24,4 +24,21 @@ class GroupeInstructeur < ApplicationRecord
 
   scope :without_group, -> (group) { where.not(id: group) }
   scope :for_api_v2, -> { includes(procedure: [:administrateurs]) }
+
+  def add(instructeur)
+    return if in?(instructeur.groupe_instructeurs)
+
+    default_notification_settings = instructeur.notification_settings(procedure_id)
+    instructeur.assign_to.create(groupe_instructeur: self, **default_notification_settings)
+  end
+
+  def remove(instructeur)
+    return if !in?(instructeur.groupe_instructeurs)
+
+    instructeur.groupe_instructeurs.destroy(self)
+    instructeur.follows
+      .joins(:dossier)
+      .where(dossiers: { groupe_instructeur: self })
+      .update_all(unfollowed_at: Time.zone.now)
+  end
 end

app/models/instructeur.rb

@@ -81,14 +81,13 @@ class Instructeur < ApplicationRecord
     end
   end
 
-  def remove_from_groupe_instructeur(groupe_instructeur)
+  NOTIFICATION_SETTINGS = [:daily_email_notifications_enabled, :instant_email_dossier_notifications_enabled, :instant_email_message_notifications_enabled, :weekly_email_notifications_enabled]
-    if groupe_instructeur.in?(groupe_instructeurs)
+
-      groupe_instructeurs.destroy(groupe_instructeur)
+  def notification_settings(procedure_id)
-      follows
+    assign_to
-        .joins(:dossier)
+      .joins(:groupe_instructeur)
-        .where(dossiers: { groupe_instructeur: groupe_instructeur })
+      .find_by(groupe_instructeurs: { procedure_id: procedure_id })
-        .update_all(unfollowed_at: Time.zone.now)
+      &.slice(*NOTIFICATION_SETTINGS) || {}
-    end
   end
 
   def last_week_overview

spec/models/groupe_instructeur_spec.rb

@@ -1,5 +1,17 @@
 describe GroupeInstructeur, type: :model do
-  let(:procedure) { create(:procedure) }
+  let(:admin) { create :administrateur }
+  let(:procedure) { create :procedure, :published, administrateur: admin }
+  let(:procedure_2) { create :procedure, :published, administrateur: admin }
+  let(:procedure_3) { create :procedure, :published, administrateur: admin }
+  let(:instructeur) { create :instructeur, administrateurs: [admin] }
+  let(:procedure_assign) { assign(procedure) }
+
+  before do
+    procedure_assign
+    assign(procedure_2)
+    procedure_3
+  end
+
   subject { GroupeInstructeur.new(label: label, procedure: procedure) }
 
   context 'with no label provided' do
@@ -33,4 +45,62 @@ describe GroupeInstructeur, type: :model do
 
     it { is_expected.to be_invalid }
   end
+
+  describe "#add" do
+    let(:another_groupe_instructeur) { create(:groupe_instructeur, procedure: procedure) }
+
+    subject { another_groupe_instructeur.add(instructeur) }
+
+    it 'adds the instructeur to the groupe instructeur' do
+      subject
+      expect(another_groupe_instructeur.reload.instructeurs).to include(instructeur)
+    end
+
+    context 'when joining another groupe instructeur on the same procedure' do
+      before do
+        procedure_assign.update(daily_email_notifications_enabled: true)
+        subject
+      end
+
+      it 'copies notifications settings from a previous group' do
+        expect(instructeur.assign_to.last.daily_email_notifications_enabled).to be_truthy
+      end
+    end
+  end
+
+  describe "#remove" do
+    subject { procedure_to_remove.defaut_groupe_instructeur.remove(instructeur) }
+
+    context "with an assigned procedure" do
+      let(:procedure_to_remove) { procedure }
+      let!(:procedure_presentation) { procedure_assign.procedure_presentation }
+
+      it { is_expected.to be_truthy }
+
+      describe "consequences" do
+        before do
+          procedure_assign.build_procedure_presentation
+          procedure_assign.save
+          subject
+        end
+
+        it "removes the assign_to and procedure_presentation" do
+          expect(AssignTo.where(id: procedure_assign).count).to eq(0)
+          expect(ProcedurePresentation.where(assign_to_id: procedure_assign.id).count).to eq(0)
+        end
+      end
+    end
+
+    context "with an already unassigned procedure" do
+      let(:procedure_to_remove) { procedure_3 }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  private
+
+  def assign(procedure_to_assign, instructeur_assigne: instructeur)
+    create :assign_to, instructeur: instructeur_assigne, procedure: procedure_to_assign, groupe_instructeur: procedure_to_assign.defaut_groupe_instructeur
+  end
 end

spec/models/instructeur_spec.rb

@@ -1,13 +1,15 @@
 describe Instructeur, type: :model do
   let(:admin) { create :administrateur }
-  let!(:procedure) { create :procedure, :published, administrateur: admin }
+  let(:procedure) { create :procedure, :published, administrateur: admin }
-  let!(:procedure_2) { create :procedure, :published, administrateur: admin }
+  let(:procedure_2) { create :procedure, :published, administrateur: admin }
-  let!(:procedure_3) { create :procedure, :published, administrateur: admin }
+  let(:procedure_3) { create :procedure, :published, administrateur: admin }
   let(:instructeur) { create :instructeur, administrateurs: [admin] }
-  let!(:procedure_assign) { assign(procedure) }
+  let(:procedure_assign) { assign(procedure) }
 
   before do
+    procedure_assign
     assign(procedure_2)
+    procedure_3
   end
 
   describe 'follow' do
@@ -84,36 +86,6 @@ describe Instructeur, type: :model do
     end
   end
 
-  describe "#remove_from_groupe_instructeur" do
-    subject { instructeur.remove_from_groupe_instructeur(procedure_to_remove.defaut_groupe_instructeur) }
-
-    context "with an assigned procedure" do
-      let(:procedure_to_remove) { procedure }
-      let!(:procedure_presentation) { procedure_assign.procedure_presentation }
-
-      it { is_expected.to be_truthy }
-
-      describe "consequences" do
-        before do
-          procedure_assign.build_procedure_presentation
-          procedure_assign.save
-          subject
-        end
-
-        it "removes the assign_to and procedure_presentation" do
-          expect(AssignTo.where(id: procedure_assign).count).to eq(0)
-          expect(ProcedurePresentation.where(assign_to_id: procedure_assign.id).count).to eq(0)
-        end
-      end
-    end
-
-    context "with an already unassigned procedure" do
-      let(:procedure_to_remove) { procedure_3 }
-
-      it { is_expected.to be_falsey }
-    end
-  end
-
   describe 'last_week_overview' do
     let!(:instructeur2) { create(:instructeur) }
     subject { instructeur2.last_week_overview }