Merge pull request #6599 from betagouv/main

2021-10-28-01

app/controllers/champs/repetition_controller.rb

@@ -2,12 +2,9 @@ class Champs::RepetitionController < ApplicationController
   before_action :authenticate_logged_user!
 
   def show
-    @champ = policy_scope(Champ).find(params[:champ_id])
+    @champ = policy_scope(Champ).includes(:champs).find(params[:champ_id])
-
     @position = params[:position]
-    row = (@champ.champs.empty? ? 0 : @champ.champs.last.row) + 1
+    @champ.add_row
-
-    @champ.add_row(row)
 
     if @champ.private?
       @attribute = "dossier[champs_private_attributes][#{@position}][champs_attributes]"

app/controllers/instructeurs/groupe_instructeurs_controller.rb

@@ -14,17 +14,16 @@ module Instructeurs
     end
 
     def add_instructeur
-      @instructeur = Instructeur.by_email(instructeur_email) ||
+      instructeur = Instructeur.by_email(instructeur_email) ||
         create_instructeur(instructeur_email)
 
-      if groupe_instructeur.instructeurs.include?(@instructeur)
+      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.instructeurs << instructeur
         flash[:notice] = "L’instructeur « #{instructeur_email} » a été affecté au groupe."
         GroupeInstructeurMailer
-          .add_instructeurs(groupe_instructeur, [@instructeur], current_user.email)
+          .add_instructeurs(groupe_instructeur, [instructeur], current_user.email)
           .deliver_later
       end
 
@@ -34,14 +33,16 @@ module Instructeurs
     def remove_instructeur
       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 = Instructeur.find(instructeur_id)
-        groupe_instructeur.instructeurs.destroy(@instructeur)
+        if instructeur.remove_from_groupe_instructeur(groupe_instructeur)
-        flash[:notice] = "L’instructeur « #{@instructeur.email} » a été retiré du groupe."
+          flash[:notice] = "L’instructeur « #{instructeur.email} » a été retiré du groupe."
-        GroupeInstructeurMailer
+          GroupeInstructeurMailer
-          .remove_instructeur(groupe_instructeur, @instructeur, current_user.email)
+            .remove_instructeur(groupe_instructeur, instructeur, current_user.email)
-          .deliver_later
+            .deliver_later
+        else
+          flash[:alert] = "L’instructeur « #{instructeur.email} » n’est pas dans le groupe."
+        end
       end
 
       redirect_to instructeur_groupe_path(procedure, groupe_instructeur)

app/controllers/manager/users_controller.rb

@@ -14,19 +14,7 @@ module Manager
           flash[:error] = user.errors.full_messages.to_sentence
         end
       else
-        user.dossiers.update_all(user_id: targeted_user.id)
+        targeted_user.merge(user)
-
-        [
-          [user.instructeur, targeted_user.instructeur],
-          [user.expert, targeted_user.expert],
-          [user.administrateur, targeted_user.administrateur]
-        ].each do |old_role, targeted_role|
-          if targeted_role.nil?
-            old_role&.update(user: targeted_user)
-          else
-            targeted_role.merge(old_role)
-          end
-        end
 
         flash[:notice] = "Le compte « #{targeted_email} » a absorbé le compte « #{user.email} »."
       end
@@ -89,7 +77,7 @@ module Manager
       if Sendinblue::API.new.unblock_user(@user.email)
         flash.notice = "L'adresse email a été débloquée auprès de Sendinblue"
       else
-        flash.alert = "Impossible de débloquer cette addresse email auprès de Sendinblue"
+        flash.alert = "Impossible de débloquer cette adresse email auprès de Sendinblue"
       end
       redirect_to emails_manager_user_path(@user)
     end

app/controllers/new_administrateur/groupe_instructeurs_controller.rb

@@ -155,22 +155,22 @@ module NewAdministrateur
     def remove_instructeur
       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)
         if procedure.routee?
-          @instructeur = Instructeur.find(instructeur_id)
+          if instructeur.remove_from_groupe_instructeur(groupe_instructeur)
-          groupe_instructeur.instructeurs.destroy(@instructeur)
+            flash[:notice] = "L’instructeur « #{instructeur.email} » a été retiré du groupe."
-          flash[:notice] = "L’instructeur « #{@instructeur.email} » a été retiré du groupe."
+            GroupeInstructeurMailer
-          GroupeInstructeurMailer
+              .remove_instructeur(groupe_instructeur, instructeur, current_user.email)
-            .remove_instructeur(groupe_instructeur, @instructeur, current_user.email)
+              .deliver_later
-            .deliver_later
-        else
-
-          instructeur = Instructeur.find(instructeur_id)
-          if instructeur.remove_from_procedure(procedure)
-            flash[:notice] = "L'instructeur a bien été désaffecté de la démarche"
           else
-            flash[:alert] = "Suppression impossible : il doit y avoir au moins un instructeur dans le groupe"
+            flash[:alert] = "L’instructeur « #{instructeur.email} » n’est pas dans le groupe."
+          end
+        else
+          if instructeur.remove_from_groupe_instructeur(procedure.defaut_groupe_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"
           end
         end
       end

app/controllers/new_administrateur/procedure_administrateurs_controller.rb

@@ -11,7 +11,7 @@ module NewAdministrateur
       # Find the admin
       administrateur = Administrateur.by_email(email)
       if administrateur.nil?
-        flash.alert = "L’administrateur « #{email} » n’existe pas. Invitez-le à demander un compte administrateur à l’addresse <a href=#{DEMANDE_INSCRIPTION_ADMIN_PAGE_URL}>#{DEMANDE_INSCRIPTION_ADMIN_PAGE_URL}</a>."
+        flash.alert = "L’administrateur « #{email} » n’existe pas. Invitez-le à demander un compte administrateur à l’adresse <a href=#{DEMANDE_INSCRIPTION_ADMIN_PAGE_URL}>#{DEMANDE_INSCRIPTION_ADMIN_PAGE_URL}</a>."
         return
       end
 

app/models/champs/repetition_champ.rb

@@ -26,9 +26,12 @@ class Champs::RepetitionChamp < Champ
     champs.group_by(&:row).values
   end
 
-  def add_row(row = 0)
+  def add_row
-    type_de_champ.types_de_champ.each do |type_de_champ|
+    transaction do
-      self.champs << type_de_champ.champ.build(row: row)
+      row = (blank? ? -1 : champs.last.row) + 1
+      type_de_champ.types_de_champ.each do |type_de_champ|
+        self.champs << type_de_champ.champ.build(row: row)
+      end
     end
   end
 

app/models/concerns/dossier_rebase_concern.rb

@@ -64,7 +64,7 @@ module DossierRebaseConcern
   def add_new_champs_for_revision(published_type_de_champ)
     if published_type_de_champ.parent
       find_champs_by_stable_id(published_type_de_champ.parent.stable_id).each do |champ_repetition|
-        champ_repetition.rows.size.times do |row|
+        champ_repetition.champs.map(&:row).uniq.each do |row|
           champ = published_type_de_champ.champ.build(row: row)
           champ_repetition.champs << champ
         end

app/models/instructeur.rb

@@ -80,9 +80,13 @@ class Instructeur < ApplicationRecord
     end
   end
 
-  def remove_from_procedure(procedure)
+  def remove_from_groupe_instructeur(groupe_instructeur)
-    if procedure.defaut_groupe_instructeur.in?(groupe_instructeurs)
+    if groupe_instructeur.in?(groupe_instructeurs)
-      groupe_instructeurs.destroy(procedure.defaut_groupe_instructeur)
+      groupe_instructeurs.destroy(groupe_instructeur)
+      follows
+        .joins(:dossier)
+        .where(dossiers: { groupe_instructeur: groupe_instructeur })
+        .update_all(unfollowed_at: Time.zone.now)
     end
   end
 

app/models/merge_log.rb

@@ -0,0 +1,14 @@
+# == Schema Information
+#
+# Table name: merge_logs
+#
+#  id              :bigint           not null, primary key
+#  from_user_email :string           not null
+#  created_at      :datetime         not null
+#  updated_at      :datetime         not null
+#  from_user_id    :bigint           not null
+#  user_id         :bigint           not null
+#
+class MergeLog < ApplicationRecord
+  belongs_to :user
+end

app/models/user.rb

@@ -47,10 +47,11 @@ class User < ApplicationRecord
   has_many :invites, dependent: :destroy
   has_many :dossiers_invites, through: :invites, source: :dossier
   has_many :deleted_dossiers
+  has_many :merge_logs, dependent: :destroy
   has_one :france_connect_information, dependent: :destroy
-  belongs_to :instructeur, optional: true
+  belongs_to :instructeur, optional: true, dependent: :destroy
-  belongs_to :administrateur, optional: true
+  belongs_to :administrateur, optional: true, dependent: :destroy
-  belongs_to :expert, optional: true
+  belongs_to :expert, optional: true, dependent: :destroy
 
   accepts_nested_attributes_for :france_connect_information
 
@@ -194,6 +195,29 @@ class User < ApplicationRecord
     end
   end
 
+  def merge(old_user)
+    transaction do
+      old_user.dossiers.update_all(user_id: id)
+      old_user.invites.update_all(user_id: id)
+      old_user.merge_logs.update_all(user_id: id)
+
+      [
+        [old_user.instructeur, instructeur],
+        [old_user.expert, expert],
+        [old_user.administrateur, administrateur]
+      ].each do |old_role, targeted_role|
+        if targeted_role.nil?
+          old_role&.update(user: self)
+        else
+          targeted_role.merge(old_role)
+        end
+      end
+
+      merge_logs.create(from_user_id: old_user.id, from_user_email: old_user.email)
+      old_user.destroy
+    end
+  end
+
   private
 
   def link_invites!

app/views/instructeurs/procedures/email_usagers.html.haml

@@ -6,7 +6,7 @@
 .messagerie.container
   - if @email_usagers_dossiers.present?
     %p.notice.mb-2.mt-4 Vous allez envoyer un message à #{pluralize(@dossiers_count, 'personne')} dont les dossiers sont en brouillon, dans les groupes instructeurs : #{@groupe_instructeurs.join(', ')}.
-    = render partial: 'shared/dossiers/messages/form', locals: { commentaire: @commentaire, form_url: create_multiple_commentaire_instructeur_procedure_path(@procedure) }
+    = render partial: 'shared/dossiers/messages/form', locals: { commentaire: @commentaire, form_url: create_multiple_commentaire_instructeur_procedure_path(@procedure), disable_piece_jointe: true }
 
     - if @bulk_messages.present?
       %section.list-avis.mt-8

app/views/shared/dossiers/editable_champs/_repetition.html.haml

@@ -18,7 +18,7 @@
             Supprimer l’élément
 
 - if champ.persisted?
-  = link_to champs_repetition_path(form.index), class: 'button add-row', data: { remote: true, method: 'POST', params: { champ_id: champ&.id }.to_query } do
+  = link_to champs_repetition_path(form.index), class: 'button add-row', data: { remote: true, disable: true, method: 'POST', params: { champ_id: champ&.id }.to_query } do
     %span.icon.add
     Ajouter un élément pour « #{champ.libelle} »
 - else

app/views/shared/dossiers/messages/_form.html.haml

@@ -5,11 +5,13 @@
     - placeholder = t('views.shared.dossiers.messages.form.write_message_placeholder')
   = f.text_area :body, rows: 5, placeholder: placeholder, required: true, class: 'message-textarea persisted-input'
   .flex.justify-between.wrap
+    - disable_piece_jointe = defined?(disable_piece_jointe) ? disable_piece_jointe : false
     %div
-      = f.label :piece_jointe, for: :piece_jointe do
+      - if !disable_piece_jointe
-        = t('views.shared.dossiers.messages.form.attach_dossier')
+        = f.label :piece_jointe, for: :piece_jointe do
-        %span.notice= t('views.shared.dossiers.messages.form.attachment_size')
+          = t('views.shared.dossiers.messages.form.attach_dossier')
-      = f.file_field :piece_jointe, id: 'piece_jointe', direct_upload: true
+          %span.notice= t('views.shared.dossiers.messages.form.attachment_size')
+        = f.file_field :piece_jointe, id: 'piece_jointe', direct_upload: true
 
     %div
       = f.submit t('views.shared.dossiers.messages.form.send_message'), class: 'button primary send', data: { disable: true }

db/migrate/20211026131800_create_merge_logs.rb

@@ -0,0 +1,11 @@
+class CreateMergeLogs < ActiveRecord::Migration[6.1]
+  def change
+    create_table :merge_logs do |t|
+      t.bigint :from_user_id, null: false
+      t.string :from_user_email, null: false
+      t.references :user, null: false, foreign_key: true
+
+      t.timestamps
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2021_10_20_114237) do
+ActiveRecord::Schema.define(version: 2021_10_26_131800) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -546,6 +546,15 @@ ActiveRecord::Schema.define(version: 2021_10_20_114237) do
     t.index ["email", "dossier_id"], name: "index_invites_on_email_and_dossier_id", unique: true
   end
 
+  create_table "merge_logs", force: :cascade do |t|
+    t.bigint "from_user_id", null: false
+    t.string "from_user_email", null: false
+    t.bigint "user_id", null: false
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.index ["user_id"], name: "index_merge_logs_on_user_id"
+  end
+
   create_table "module_api_cartos", id: :serial, force: :cascade do |t|
     t.integer "procedure_id"
     t.boolean "use_api_carto", default: false
@@ -841,6 +850,7 @@ ActiveRecord::Schema.define(version: 2021_10_20_114237) do
   add_foreign_key "geo_areas", "champs"
   add_foreign_key "groupe_instructeurs", "procedures"
   add_foreign_key "initiated_mails", "procedures"
+  add_foreign_key "merge_logs", "users"
   add_foreign_key "procedure_presentations", "assign_tos"
   add_foreign_key "procedure_revision_types_de_champ", "procedure_revisions", column: "revision_id"
   add_foreign_key "procedure_revision_types_de_champ", "types_de_champ"

spec/controllers/instructeurs/groupe_instructeurs_controller_spec.rb

@@ -64,9 +64,14 @@ describe Instructeurs::GroupeInstructeursController, type: :controller do
   end
 
   describe '#remove_instructeur' do
-    let!(:new_instructeur) { create(:instructeur) }
+    let(:new_instructeur) { create(:instructeur) }
+    let(:dossier) { create(:dossier) }
 
-    before { gi_1_1.instructeurs << instructeur << new_instructeur }
+    before do
+      gi_1_1.instructeurs << instructeur << new_instructeur
+      gi_1_1.dossiers << dossier
+      new_instructeur.followed_dossiers << dossier
+    end
 
     def remove_instructeur(instructeur)
       delete :remove_instructeur,
@@ -82,6 +87,7 @@ describe Instructeurs::GroupeInstructeursController, type: :controller do
 
       it { expect(gi_1_1.instructeurs).to include(instructeur) }
       it { expect(gi_1_1.reload.instructeurs.count).to eq(1) }
+      it { expect(new_instructeur.reload.follows.count).to eq(0) }
       it { expect(response).to redirect_to(instructeur_groupe_path(procedure, gi_1_1)) }
     end
 

spec/controllers/manager/users_controller_spec.rb

@@ -48,43 +48,12 @@ describe Manager::UsersController, type: :controller do
       let(:targeted_user) { create(:user, email: 'email.existant@domaine.fr', password: '{My-$3cure-p4ssWord}') }
       let(:nouvel_email) { targeted_user.email }
 
-      context 'and the old account has a dossier' do
+      it 'launches the merge process' do
-        let!(:dossier) { create(:dossier, user: user) }
+        expect_any_instance_of(User).to receive(:merge).with(user)
 
-        it 'transfers the dossier' do
+        subject
-          subject
 
-          expect(targeted_user.dossiers).to match([dossier])
+        expect(flash[:notice]).to match("Le compte « email.existant@domaine.fr » a absorbé le compte « ancien.email@domaine.fr ».")
-        end
-      end
-
-      context 'and the old account belongs to an instructeur, expert and administrateur' do
-        let!(:instructeur) { create(:instructeur, user: user) }
-        let!(:expert) { create(:expert, user: user) }
-        let!(:administrateur) { create(:administrateur, user: user, instructeur: instructeur) }
-
-        it 'transfers instructeur account' do
-          subject
-          targeted_user.reload
-
-          expect(targeted_user.instructeur).to match(instructeur)
-          expect(targeted_user.expert).to match(expert)
-          expect(targeted_user.administrateur).to match(administrateur)
-          expect(flash[:notice]).to match("Le compte « email.existant@domaine.fr » a absorbé le compte « ancien.email@domaine.fr ».")
-        end
-
-        context 'and the targeted account owns an instructeur and expert as well' do
-          let!(:targeted_instructeur) { create(:instructeur, user: targeted_user) }
-          let!(:targeted_expert) { create(:expert, user: targeted_user) }
-          let!(:targeted_administrateur) { create(:administrateur, user: targeted_user) }
-
-          it 'merge the account' do
-            expect_any_instance_of(Instructeur).to receive(:merge)
-            expect_any_instance_of(Expert).to receive(:merge)
-            expect_any_instance_of(Administrateur).to receive(:merge)
-            subject
-          end
-        end
       end
     end
   end

spec/factories/user.rb

@@ -2,7 +2,7 @@ FactoryBot.define do
   sequence(:user_email) { |n| "user#{n}@user.com" }
   factory :user do
     email { generate(:user_email) }
-    password { 'my-s3cure-p4ssword' }
+    password { '{My-$3cure-p4ssWord}' }
     confirmed_at { Time.zone.now }
 
     trait :unconfirmed do

spec/models/concern/tags_substitution_concern_spec.rb

@@ -173,7 +173,7 @@ describe TagsSubstitutionConcern, type: :model do
       before do
         repetition = dossier.champs
           .find { |champ| champ.libelle == 'Répétition' }
-        repetition.add_row(1)
+        repetition.add_row
         paul_champs, pierre_champs = repetition.rows
 
         paul_champs.first.update(value: 'Paul')

spec/models/dossier_spec.rb

@@ -1497,6 +1497,11 @@ describe Dossier do
 
       datetime_champ.update(value: Date.today.to_s)
       text_champ.update(value: 'bonjour')
+      # Add two rows then remove previous to last row in order to create a "hole" in the sequence
+      repetition_champ.add_row
+      repetition_champ.add_row
+      repetition_champ.champs.where(row: repetition_champ.champs.last.row - 1).destroy_all
+      repetition_champ.reload
     end
 
     it "updates the brouillon champs with the latest revision changes" do
@@ -1505,8 +1510,9 @@ describe Dossier do
 
       expect(dossier.revision).to eq(procedure.published_revision)
       expect(dossier.champs.size).to eq(4)
-      expect(repetition_champ.rows.size).to eq(1)
+      expect(repetition_champ.rows.size).to eq(2)
       expect(repetition_champ.rows[0].size).to eq(1)
+      expect(repetition_champ.rows[1].size).to eq(1)
 
       procedure.publish_revision!
       perform_enqueued_jobs
@@ -1520,8 +1526,9 @@ describe Dossier do
       expect(rebased_text_champ.type_de_champ_id).not_to eq(text_champ.type_de_champ_id)
       expect(rebased_datetime_champ.type_champ).to eq(TypeDeChamp.type_champs.fetch(:date))
       expect(rebased_datetime_champ.value).to be_nil
-      expect(rebased_repetition_champ.rows.size).to eq(1)
+      expect(rebased_repetition_champ.rows.size).to eq(2)
       expect(rebased_repetition_champ.rows[0].size).to eq(2)
+      expect(rebased_repetition_champ.rows[1].size).to eq(2)
       expect(rebased_text_champ.rebased_at).not_to be_nil
       expect(rebased_datetime_champ.rebased_at).not_to be_nil
     end

spec/models/instructeur_spec.rb

@@ -84,8 +84,8 @@ describe Instructeur, type: :model do
     end
   end
 
-  describe "#remove_from_procedure" do
+  describe "#remove_from_groupe_instructeur" do
-    subject { instructeur.remove_from_procedure(procedure_to_remove) }
+    subject { instructeur.remove_from_groupe_instructeur(procedure_to_remove.defaut_groupe_instructeur) }
 
     context "with an assigned procedure" do
       let(:procedure_to_remove) { procedure }

spec/models/user_spec.rb

@@ -417,4 +417,63 @@ describe User, type: :model do
       end
     end
   end
+
+  describe '#merge' do
+    let(:old_user) { create(:user) }
+    let(:targeted_user) { create(:user) }
+
+    subject { targeted_user.merge(old_user) }
+
+    context 'and the old account has some stuff' do
+      let!(:dossier) { create(:dossier, user: old_user) }
+      let!(:invite) { create(:invite, user: old_user) }
+      let!(:merge_log) { MergeLog.create(user: old_user, from_user_id: 1, from_user_email: 'a') }
+
+      it 'transfers the dossier' do
+        subject
+
+        expect(targeted_user.dossiers).to match([dossier])
+        expect(targeted_user.invites).to match([invite])
+        expect(targeted_user.merge_logs.first).to eq(merge_log)
+
+        added_merge_log = targeted_user.merge_logs.last
+        expect(added_merge_log.from_user_id).to eq(old_user.id)
+        expect(added_merge_log.from_user_email).to eq(old_user.email)
+      end
+    end
+
+    context 'and the old account belongs to an instructeur, expert and administrateur' do
+      let!(:expert) { create(:expert, user: old_user) }
+      let!(:administrateur) { create(:administrateur, user: old_user) }
+      let!(:instructeur) { old_user.instructeur }
+
+      it 'transfers instructeur account' do
+        subject
+        targeted_user.reload
+
+        expect(targeted_user.instructeur).to match(instructeur)
+        expect(targeted_user.expert).to match(expert)
+        expect(targeted_user.administrateur).to match(administrateur)
+      end
+
+      context 'and the targeted account owns an instructeur and expert as well' do
+        let!(:targeted_administrateur) { create(:administrateur, user: targeted_user) }
+        let!(:targeted_instructeur) { targeted_user.instructeur }
+        let!(:targeted_expert) { create(:expert, user: targeted_user) }
+
+        it 'merge the account' do
+          expect(targeted_instructeur).to receive(:merge).with(instructeur)
+          expect(targeted_expert).to receive(:merge).with(expert)
+          expect(targeted_administrateur).to receive(:merge).with(administrateur)
+
+          subject
+
+          expect { instructeur.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          expect { expert.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          expect { administrateur.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          expect { old_user.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        end
+      end
+    end
+  end
 end