Merge pull request #8176 from tchak/refactor-groupe_instructeurs_controller

refactor(groupe_instructeur): improve controller

app/controllers/administrateurs/groupe_instructeurs_controller.rb

@@ -40,7 +40,7 @@ module Administrateurs
         @instructeurs = paginated_instructeurs
         @groupes_instructeurs = paginated_groupe_instructeurs
 
-        flash[:alert] = "le nom « #{@groupe_instructeur.label} » est déjà pris par un autre groupe."
+        flash[:alert] = @groupe_instructeur.errors.full_messages
         render :index
       end
     end
@@ -56,7 +56,7 @@ module Administrateurs
         @instructeurs = paginated_instructeurs
         @available_instructeur_emails = available_instructeur_emails
 
-        flash[:alert] = @groupe_instructeur.errors.full_messages.to_sentence
+        flash[:alert] = @groupe_instructeur.errors.full_messages
         render :show
       end
     end
@@ -64,7 +64,7 @@ module Administrateurs
     def destroy
       @groupe_instructeur = groupe_instructeur
 
-      if !@groupe_instructeur.dossiers.empty?
+      if @groupe_instructeur.dossiers.present?
         flash[:alert] = "Impossible de supprimer un groupe avec des dossiers. Il faut le réaffecter avant"
       elsif procedure.groupe_instructeurs.one?
         flash[:alert] = "Suppression impossible : il doit y avoir au moins un groupe instructeur sur chaque procédure"
@@ -109,47 +109,30 @@ module Administrateurs
 
     def add_instructeur
       emails = params['emails'].presence || [].to_json
-      emails = JSON.parse(emails).map(&:strip).map(&:downcase)
+      emails = JSON.parse(emails)
 
-      correct_emails, bad_emails = emails
+      instructeurs, invalid_emails = Instructeur.find_or_invite(emails:, groupe_instructeur:)
-        .partition { |email| URI::MailTo::EMAIL_REGEXP.match?(email) }
 
-      if bad_emails.present?
+      if invalid_emails.present?
         flash[:alert] = t('.wrong_address',
-          count: bad_emails.count,
+          count: invalid_emails.size,
-          value: bad_emails.join(', '))
+          emails: invalid_emails.join(', '))
       end
 
-      email_to_adds = correct_emails - groupe_instructeur.instructeurs.map(&:email)
+      if instructeurs.present?
-
+        instructeurs.each { groupe_instructeur.add(_1) }
-      if email_to_adds.present?
-        instructeurs = email_to_adds.map do |instructeur_email|
-          Instructeur.by_email(instructeur_email) ||
-            create_instructeur(instructeur_email)
-        end
-
-        if procedure.routing_enabled?
-          instructeurs.each do |instructeur|
-            groupe_instructeur.add(instructeur)
-          end
 
+        flash[:notice] = if procedure.routing_enabled?
           GroupeInstructeurMailer
-            .add_instructeurs(groupe_instructeur, instructeurs, current_user.email)
+            .add_instructeurs(groupe_instructeur, instructeurs, current_administrateur.email)
             .deliver_later
 
-          flash[:notice] = t('.assignment',
+          t('.assignment',
-            count: email_to_adds.count,
+            count: instructeurs.size,
-            value: email_to_adds.join(', '),
+            emails: instructeurs.map(&:email).join(', '),
             groupe: groupe_instructeur.label)
-
         else
-
+          "Les instructeurs ont bien été affectés à la démarche"
-          if instructeurs.present?
-            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
       end
 
@@ -164,21 +147,27 @@ module Administrateurs
       if groupe_instructeur.instructeurs.one?
         flash[:alert] = "Suppression impossible : il doit y avoir au moins un instructeur dans le groupe"
       else
-        instructeur = Instructeur.find(instructeur_id)
+        instructeur = groupe_instructeur.instructeurs.find_by(id: instructeur_id)
-        if procedure.routing_enabled?
+
-          if groupe_instructeur.remove(instructeur)
+        if groupe_instructeur.remove(instructeur)
-            flash[:notice] = "L’instructeur « #{instructeur.email} » a été retiré du groupe."
+          flash[:notice] = if procedure.routing_enabled?
             GroupeInstructeurMailer
-              .remove_instructeur(groupe_instructeur, instructeur, current_user.email)
+              .remove_instructeurs(groupe_instructeur, [instructeur], current_administrateur.email)
               .deliver_later
+
+            "L’instructeur « #{instructeur.email} » a été retiré du groupe."
           else
-            flash[:alert] = "L’instructeur « #{instructeur.email} » n’est pas dans le groupe."
+            "L’instructeur a bien été désaffecté de la démarche"
           end
         else
-          if procedure.defaut_groupe_instructeur.remove(instructeur)
+          flash[:alert] = if procedure.routing_enabled?
-            flash[:notice] = "L’instructeur a bien été désaffecté de la démarche"
+            if instructeur.present?
+              "L’instructeur « #{instructeur.email} » n’est pas dans le groupe."
+            else
+              "L’instructeur n’est pas dans le groupe."
+            end
           else
-            flash[:alert] = "L’instructeur n’est pas affecté à la démarche"
+            "L’instructeur n’est pas affecté à la démarche"
           end
         end
       end
@@ -258,16 +247,6 @@ module Administrateurs
 
     private
 
-    def create_instructeur(email)
-      user = User.create_or_promote_to_instructeur(
-        email,
-        SecureRandom.hex,
-        administrateurs: [current_administrateur]
-      )
-      user.invite!
-      user.instructeur
-    end
-
     def procedure
       current_administrateur
         .procedures

app/mailers/groupe_instructeur_mailer.rb

@@ -12,6 +12,17 @@ class GroupeInstructeurMailer < ApplicationMailer
     mail(bcc: emails, subject: subject)
   end
 
+  def remove_instructeurs(group, removed_instructeurs, current_instructeur_email)
+    @removed_instructeur_emails = removed_instructeurs.map(&:email)
+    @group = group
+    @current_instructeur_email = current_instructeur_email
+
+    subject = "Suppression d’un instructeur dans le groupe \"#{group.label}\""
+
+    emails = @group.instructeurs.map(&:email)
+    mail(bcc: emails, subject: subject)
+  end
+
   def remove_instructeur(group, instructeur, current_instructeur_email)
     @email = instructeur.email
     @group = group

app/models/groupe_instructeur.rb

@@ -33,6 +33,7 @@ class GroupeInstructeur < ApplicationRecord
   scope :closed, -> { where(closed: true) }
 
   def add(instructeur)
+    return if instructeur.nil?
     return if in?(instructeur.groupe_instructeurs)
 
     default_notification_settings = instructeur.notification_settings(procedure_id)
@@ -40,6 +41,7 @@ class GroupeInstructeur < ApplicationRecord
   end
 
   def remove(instructeur)
+    return if instructeur.nil?
     return if !in?(instructeur.groupe_instructeurs)
 
     instructeur.groupe_instructeurs.destroy(self)

app/models/instructeur.rb

@@ -49,7 +49,45 @@ class Instructeur < ApplicationRecord
   default_scope { eager_load(:user) }
 
   def self.by_email(email)
-    Instructeur.eager_load(:user).find_by(users: { email: email })
+    find_by(users: { email: email })
+  end
+
+  def self.find_or_invite(emails: [], ids: [], groupe_instructeur:)
+    valid_emails, invalid_emails = emails
+      .map(&:strip)
+      .map(&:downcase)
+      .partition { URI::MailTo::EMAIL_REGEXP.match?(_1) }
+    valid_ids = ids.map { _1.is_a?(String) ? id_from_typed_id(_1) : _1 }
+
+    instructeurs_by_ids = where(id: valid_ids)
+    instructeurs_by_emails = where(users: { email: valid_emails })
+
+    instructeurs = if valid_ids.present? && valid_emails.present?
+      instructeurs_by_ids.or(instructeurs_by_emails).distinct(:id)
+    elsif valid_emails.present?
+      instructeurs_by_emails
+    else
+      instructeurs_by_ids
+    end
+
+    not_found_emails = valid_emails - instructeurs.map(&:email)
+
+    # Send invitations to users without account
+    if not_found_emails.present?
+      administrateurs = groupe_instructeur.procedure.administrateurs
+      instructeurs += not_found_emails.map { invite(_1, administrateurs) }
+    end
+
+    # We dont't want to assign a user to a groupe_instructeur if they are already assigned to it
+    instructeurs -= groupe_instructeur.instructeurs
+
+    [instructeurs, invalid_emails]
+  end
+
+  def self.invite(email, administrateurs)
+    user = User.create_or_promote_to_instructeur(email, SecureRandom.hex, administrateurs:)
+    user.invite!
+    user.instructeur
   end
 
   def email

app/views/groupe_instructeur_mailer/add_instructeurs.html.haml

@@ -2,7 +2,7 @@
   Bonjour,
 
 %p
-  #{t('administrateurs.groupe_instructeurs.add_instructeur.assignment', count: @new_instructeur_emails.count, value: @new_instructeur_emails.join(', '), groupe: @group.label).chomp('.')} par « #{@current_instructeur_email} », en charge de la démarche « #{@group.procedure.libelle} ».
+  = t('administrateurs.groupe_instructeurs.add_instructeur.email_body', count: @new_instructeur_emails.size, emails: @new_instructeur_emails.join(', '), groupe: @group.label, email: @current_instructeur_email, procedure: @group.procedure.libelle)
 
 %p
   Cliquez sur le lien ci-dessous pour voir la liste des instructeurs de ce groupe :

app/views/groupe_instructeur_mailer/remove_instructeur.html.haml

@@ -2,7 +2,7 @@
   Bonjour,
 
 %p
-  L’instructeur « #{@email} » a été retiré du groupe « #{@group.label} » par « #{@current_instructeur_email} », en charge de la démarche « #{@group.procedure.libelle} ».
+  = t('administrateurs.groupe_instructeurs.remove_instructeur.email_body', count: 1, emails: @email, groupe: @group.label, email: @current_instructeur_email, procedure: @group.procedure.libelle)
 
 %p
   Cliquez sur le lien ci-dessous pour voir la liste des instructeurs de ce groupe :

app/views/groupe_instructeur_mailer/remove_instructeurs.html.haml

@@ -0,0 +1,11 @@
+%p
+  Bonjour,
+
+%p
+  = t('administrateurs.groupe_instructeurs.remove_instructeur.email_body', count: @removed_instructeur_emails.size, emails: @removed_instructeur_emails.join(', '), groupe: @group.label, email: @current_instructeur_email, procedure: @group.procedure.libelle)
+
+%p
+  Cliquez sur le lien ci-dessous pour voir la liste des instructeurs de ce groupe :
+  = link_to(@group.label, admin_procedure_groupe_instructeur_url(@group.procedure, @group))
+
+= render partial: "layouts/mailers/signature"

config/locales/views/administrateurs/groupe_instructeurs/en.yml

@@ -11,11 +11,18 @@ en:
         instructors_group: Group of instructors
       add_instructeur:
         wrong_address:
-          one: "%{value} is not a valid email address"
+          one: "%{emails} is not a valid email address"
-          other: "%{value} are not valid email addresses"
+          other: "%{emails} are not valid email addresses"
         assignment:
-          one: "The instructor %{value} was assigned to the group « %{groupe} »."
+          one: "The instructor %{emails} was assigned to the group « %{groupe} »."
-          other: "The instructors %{value} were assigned to the group « %{groupe} »."
+          other: "The instructors %{emails} were assigned to the group « %{groupe} »."
+        email_body:
+          one: "The instructor %{emails} was assigned to the group « %{groupe} » by « %{email} », in charge of procedure « %{procedure} »."
+          other: "The instructors %{emails} were assigned to the group « %{groupe} » by « %{email} », in charge of procedure « %{procedure} »."
+      remove_instructeur:
+        email_body:
+          one: "The instructor %{emails} was removed from the group « %{groupe} » by « %{email} », in charge of procedure « %{procedure} »."
+          other: "The instructors %{emails} were removed from the group « %{groupe} » by « %{email} », in charge of procedure « %{procedure} »."
       reaffecter_dossiers:
         existing_groupe:
           one: "%{count} group exist"

config/locales/views/administrateurs/groupe_instructeurs/fr.yml

@@ -17,11 +17,18 @@ fr:
           other: "%{count} groupes existent"
       add_instructeur:
         wrong_address:
-          one: "%{value} n’est pas une adresse email valide"
+          one: "%{emails} n’est pas une adresse email valide"
-          other: "%{value} ne sont pas des adresses emails valides"
+          other: "%{emails} ne sont pas des adresses emails valides"
         assignment:
-          one: "L’instructeur %{value} a été affecté au groupe « %{groupe} »."
+          one: "L’instructeur %{emails} a été affecté au groupe « %{groupe} »."
-          other: "Les instructeurs %{value} ont été affectés au groupe « %{groupe} »."
+          other: "Les instructeurs %{emails} ont été affectés au groupe « %{groupe} »."
+        email_body:
+          one: "L’instructeur %{emails} a été affecté au groupe « %{groupe} » par « %{email} », en charge de la démarche « %{procedure} »."
+          other: "Les instructeurs %{emails} ont été affectés au groupe « %{groupe} » par « %{email} », en charge de la démarche « %{procedure} »."
+      remove_instructeur:
+        email_body:
+          one: "L’instructeur %{emails} a été retiré du groupe « %{groupe} » par « %{email} », en charge de la démarche « %{procedure} »."
+          other: "Les instructeurs %{emails} ont été retirés du groupe « %{groupe} » par « %{email} », en charge de la démarche « %{procedure} »."
       reaffecter_dossiers:
         existing_groupe:
           one: "%{count} groupe existe"

spec/controllers/administrateurs/groupe_instructeurs_controller_spec.rb

@@ -206,7 +206,7 @@ describe Administrateurs::GroupeInstructeursController, type: :controller do
       it { expect(response).to render_template(:show) }
       it { expect(gi_1_1.label).not_to eq(new_name) }
       it { expect(gi_1_1.closed).to eq(false) }
-      it { expect(flash.alert).to eq('Il doit y avoir au moins un groupe instructeur actif sur chaque démarche') }
+      it { expect(flash.alert).to eq(['Il doit y avoir au moins un groupe instructeur actif sur chaque démarche']) }
     end
 
     context 'when the name is already taken' do
@@ -214,7 +214,7 @@ describe Administrateurs::GroupeInstructeursController, type: :controller do
       let(:new_name) { gi_1_2.label }
 
       it { expect(gi_1_1.label).not_to eq(new_name) }
-      it { expect(flash.alert).to eq('Le libellé est déjà utilisé(e)') }
+      it { expect(flash.alert).to eq(['Le libellé est déjà utilisé(e)']) }
     end
   end