Merge pull request #3935 from betagouv/fix-pj-migration-changing-timestamps

Corrige la tâche de migration des PJ

app/services/piece_justificative_to_champ_piece_jointe_migration_service.rb

@@ -25,9 +25,16 @@ class PieceJustificativeToChampPieceJointeMigrationService
     populate_champs_pjs!(procedure, types_de_champ_pj, &progress)
 
     # Only destroy the old types PJ once everything has been safely migrated to
-    # champs PJs. Destroying the types PJ will cascade and destroy the PJs,
-    # and delete the linked objects from remote storage. This means that no other
-    # cleanup action is required.
+    # champs PJs.
+
+    # First destroy the individual PJ champs on all dossiers.
+    # It will cascade and destroy the PJs, and delete the linked objects from remote storage.
+    procedure.dossiers.unscope(where: :hidden_at).includes(:champs).find_each do |dossier|
+      destroy_pieces_justificatives(dossier)
+    end
+
+    # Now we can destroy the type de champ themselves,
+    # without cascading the timestamp update on all attached dossiers.
     procedure.types_de_piece_justificative.destroy_all
   end
 
@@ -40,32 +47,7 @@ class PieceJustificativeToChampPieceJointeMigrationService
 
     # Unscope to make sure all dossiers are migrated, even the soft-deleted ones
     procedure.dossiers.unscope(where: :hidden_at).includes(:champs).find_each do |dossier|
-      # Add the new pieces justificatives champs to the dossier
-      champs_pj = types_de_champ_pj.map(&:build_champ)
-      dossier.champs += champs_pj
-
-      # Copy the dossier old pieces jointes to the new champs
-      # (even if the champs already existed, so that we ensure a clean state)
-      champs_pj.each do |champ|
-        type_pj_id = champ.type_de_champ.old_pj&.fetch('stable_id', nil)
-        pj = dossier.retrieve_last_piece_justificative_by_type(type_pj_id)
-
-        if pj.present?
-          convert_pj_to_champ!(pj, champ)
-
-          champ.update(
-            updated_at: pj.updated_at,
-            created_at: pj.created_at
-          )
-        else
-          champ.update(
-            updated_at: dossier.updated_at,
-            created_at: dossier.created_at
-          )
-        end
-
-        yield if block_given?
-      end
+      migrate_dossier!(dossier, types_de_champ_pj, &progress)
     end
 
   rescue StandardError, SignalException
@@ -74,8 +56,11 @@ class PieceJustificativeToChampPieceJointeMigrationService
     rake_puts "Error received. Rolling back migration of procedure #{procedure.id}…"
 
     types_de_champ_pj.each do |type_champ|
-      type_champ.champ.each { |c| c.piece_justificative_file.purge }
-      type_champ.destroy
+      # First destroy all the individual champs on dossiers
+      type_champ.champ.each { |c| destroy_champ_pj(c.dossier.reload, c) }
+      # Now we can destroy the type de champ itself,
+      # without cascading the timestamp update on all attached dossiers.
+      type_champ.reload.destroy
     end
 
     rake_puts "Migration of procedure #{procedure.id} rolled back."
@@ -84,6 +69,39 @@ class PieceJustificativeToChampPieceJointeMigrationService
     raise
   end
 
+  def migrate_dossier!(dossier, types_de_champ_pj)
+    # Add the new pieces justificatives champs to the dossier
+    champs_pj = types_de_champ_pj.map(&:build_champ)
+    preserving_updated_at(dossier) do
+      dossier.champs += champs_pj
+    end
+
+    # Copy the dossier old pieces jointes to the new champs
+    # (even if the champs already existed, so that we ensure a clean state)
+    champs_pj.each do |champ|
+      type_pj_id = champ.type_de_champ.old_pj&.fetch('stable_id', nil)
+      pj = dossier.retrieve_last_piece_justificative_by_type(type_pj_id)
+
+      if pj.present?
+        preserving_updated_at(dossier) do
+          convert_pj_to_champ!(pj, champ)
+        end
+
+        champ.update_columns(
+          updated_at: pj.updated_at,
+          created_at: pj.created_at
+        )
+      else
+        champ.update_columns(
+          updated_at: dossier.updated_at,
+          created_at: dossier.created_at
+        )
+      end
+
+      yield if block_given?
+    end
+  end
+
   def convert_pj_to_champ!(pj, champ)
     actual_file_exists = pj.content.file.send(:file)
     if actual_file_exists
@@ -133,4 +151,26 @@ class PieceJustificativeToChampPieceJointeMigrationService
   def make_empty_blob(pj)
     storage_service.make_empty_blob(pj.content, pj.updated_at.iso8601, filename: pj.original_filename)
   end
+
+  def preserving_updated_at(model)
+    original_modification_date = model.updated_at
+    begin
+      yield
+    ensure
+      model.update_column(:updated_at, original_modification_date)
+    end
+  end
+
+  def destroy_pieces_justificatives(dossier)
+    preserving_updated_at(dossier) do
+      dossier.pieces_justificatives.destroy_all
+    end
+  end
+
+  def destroy_champ_pj(dossier, champ)
+    preserving_updated_at(dossier) do
+      champ.piece_justificative_file.purge
+      champ.destroy
+    end
+  end
 end

spec/services/piece_justificative_to_champ_piece_jointe_migration_service_spec.rb

@@ -26,6 +26,18 @@ describe PieceJustificativeToChampPieceJointeMigrationService do
     end
   end
 
+  def timestamps(dossier)
+    # Reload dossier because the resolution of in-database timestamps is
+    # different from the resolution of in-memory timestamps, causing the
+    # tests to fail on fractional time differences.
+    dossier.reload
+
+    {
+      created_at: dossier.created_at,
+      updated_at: dossier.updated_at
+    }
+  end
+
   def expect_storage_service_to_convert_object
     expect(storage_service).to receive(:make_blob)
     expect(storage_service).to receive(:copy_from_carrierwave_to_active_storage!)
@@ -64,6 +76,8 @@ describe PieceJustificativeToChampPieceJointeMigrationService do
     end
 
     context 'no notifications are sent to instructeurs' do
+      let!(:initial_dossier_timestamps) { timestamps(dossier) }
+
       context 'when there is a PJ' do
         let(:pjs) { make_pjs }
 
@@ -80,24 +94,18 @@ describe PieceJustificativeToChampPieceJointeMigrationService do
           dossier.reload
         end
 
-        it 'the champ has the same created_at as the PJ' do
+        it 'the champ has the same timestamps as the PJ' do
           expect(dossier.champs.last.created_at).to eq(pjs.last.created_at)
+          expect(dossier.champs.last.updated_at).to eq(pjs.last.updated_at)
         end
 
-        it 'the champ has the same updated_at as the PJ' do
-          expect(dossier.champs.last.updated_at).to eq(pjs.last.updated_at)
+        it 'does not change the dossier timestamps' do
+          expect(dossier.created_at).to eq(initial_dossier_timestamps[:created_at])
+          expect(dossier.updated_at).to eq(initial_dossier_timestamps[:updated_at])
         end
       end
 
       context 'when there is no PJ' do
-        let!(:expected_updated_at) do
-          # Reload dossier because the resolution of in-database timestamps is
-          # different from the resolution of in-memory timestamps, causing the
-          # tests to fail on fractional time differences.
-          dossier.reload
-          dossier.updated_at
-        end
-
         before do
           Timecop.travel(1.hour) { service.convert_procedure_pjs_to_champ_pjs(procedure) }
 
@@ -105,12 +113,14 @@ describe PieceJustificativeToChampPieceJointeMigrationService do
           dossier.reload
         end
 
-        it 'the champ has the same created_at as the dossier' do
-          expect(dossier.champs.last.created_at).to eq(dossier.created_at)
+        it 'the champ has the same timestamps as the dossier' do
+          expect(dossier.champs.last.created_at).to eq(initial_dossier_timestamps[:created_at])
+          expect(dossier.champs.last.updated_at).to eq(initial_dossier_timestamps[:updated_at])
         end
 
-        it 'the champ has the same updated_at as the dossier' do
-          expect(dossier.champs.last.updated_at).to eq(expected_updated_at)
+        it 'does not change the dossier timestamps' do
+          expect(dossier.created_at).to eq(initial_dossier_timestamps[:created_at])
+          expect(dossier.updated_at).to eq(initial_dossier_timestamps[:updated_at])
         end
       end
     end
@@ -192,6 +202,9 @@ describe PieceJustificativeToChampPieceJointeMigrationService do
       )
     end
 
+    let!(:initial_dossier_timestamps) { timestamps(dossier) }
+    let!(:initial_failing_dossier_timestamps) { timestamps(failing_dossier) }
+
     before do
       allow(storage_service).to receive(:checksum).and_return('cafe')
       allow(storage_service).to receive(:fix_content_type)
@@ -204,8 +217,10 @@ describe PieceJustificativeToChampPieceJointeMigrationService do
     end
 
     def try_convert(procedure)
-      service.convert_procedure_pjs_to_champ_pjs(procedure)
+      Timecop.travel(1.hour) { service.convert_procedure_pjs_to_champ_pjs(procedure) }
     rescue StandardError, SignalException => e
+      dossier.reload
+      failing_dossier.reload
       e
     end
 
@@ -234,6 +249,12 @@ describe PieceJustificativeToChampPieceJointeMigrationService do
         .not_to change { procedure.types_de_piece_justificative.count }
     end
 
+    it 'does not change the dossiers timestamps' do
+      try_convert(procedure)
+      expect(dossier.updated_at).to eq(initial_dossier_timestamps[:updated_at])
+      expect(failing_dossier.updated_at).to eq(initial_failing_dossier_timestamps[:updated_at])
+    end
+
     it 'does not leave stale blobs behind' do
       expect { try_convert(procedure) }
         .not_to change { ActiveStorage::Blob.count }