Merge pull request #6590 from tchak/fix-create-deleted-dossier

fix(dossiers): wrap dossier discard in a transaction

app/models/deleted_dossier.rb

@@ -17,8 +17,6 @@
 class DeletedDossier < ApplicationRecord
   belongs_to :procedure, -> { with_discarded }, inverse_of: :deleted_dossiers, optional: false
 
-  validates :dossier_id, uniqueness: true
-
   scope :order_by_updated_at, -> (order = :desc) { order(created_at: order) }
   scope :deleted_since,       -> (since) { where('deleted_dossiers.deleted_at >= ?', since) }
 
@@ -32,16 +30,17 @@ class DeletedDossier < ApplicationRecord
   }
 
   def self.create_from_dossier(dossier, reason)
-    create!(
+    # We have some bad data because of partially deleted dossiers in the past.
+    # For now use find_or_create_by! to avoid errors.
+    create_with(
       reason: reasons.fetch(reason),
-      dossier_id: dossier.id,
       groupe_instructeur_id: dossier.groupe_instructeur_id,
       revision_id: dossier.revision_id,
       user_id: dossier.user_id,
       procedure: dossier.procedure,
       state: dossier.state,
       deleted_at: Time.zone.now
-    )
+    ).create_or_find_by!(dossier_id: dossier.id)
   end
 
   def procedure_removed?

app/models/dossier.rb

@@ -606,7 +606,7 @@ class Dossier < ApplicationRecord
   end
 
   def keep_track_on_deletion?
-    !procedure.brouillon?
+    !procedure.brouillon? && !brouillon?
   end
 
   def expose_legacy_carto_api?
@@ -645,56 +645,68 @@ class Dossier < ApplicationRecord
     end
   end
 
-  def expired_keep_track!
+  def expired_keep_track_and_destroy!
-    if keep_track_on_deletion?
+    transaction do
-      DeletedDossier.create_from_dossier(self, :expired)
+      if keep_track_on_deletion?
-      log_automatic_dossier_operation(:supprimer, self)
+        DeletedDossier.create_from_dossier(self, :expired)
+        log_automatic_dossier_operation(:supprimer, self)
+      end
+      destroy!
     end
+    true
+  rescue
+    false
   end
 
   def discard_and_keep_track!(author, reason)
-    if keep_track_on_deletion?
+    user_email = user_deleted? ? nil : user_email_for(:notification)
-      if en_construction?
+    deleted_dossier = nil
-        deleted_dossier = DeletedDossier.create_from_dossier(self, reason)
 
+    transaction do
+      if keep_track_on_deletion?
+        log_dossier_operation(author, :supprimer, self)
+        deleted_dossier = DeletedDossier.create_from_dossier(self, reason)
+      end
+
+      update!(dossier_transfer_id: nil)
+      discard!
+    end
+
+    if deleted_dossier.present?
+      if en_construction?
         administration_emails = followers_instructeurs.present? ? followers_instructeurs.map(&:email) : procedure.administrateurs.map(&:email)
         administration_emails.each do |email|
           DossierMailer.notify_deletion_to_administration(deleted_dossier, email).deliver_later
         end
+      end
 
-        if !user_deleted?
+      if user_email.present?
-          DossierMailer.notify_deletion_to_user(deleted_dossier, user_email_for(:notification)).deliver_later
+        if reason == :user_request
+          DossierMailer.notify_deletion_to_user(deleted_dossier, user_email).deliver_later
+        else
+          DossierMailer.notify_instructeur_deletion_to_user(deleted_dossier, user_email).deliver_later
         end
-
-        log_dossier_operation(author, :supprimer, self)
-      elsif termine?
-        deleted_dossier = DeletedDossier.create_from_dossier(self, reason)
-
-        if !user_deleted?
-          DossierMailer.notify_instructeur_deletion_to_user(deleted_dossier, user_email_for(:notification)).deliver_later
-        end
-
-        log_dossier_operation(author, :supprimer, self)
       end
     end
-
-    update!(dossier_transfer_id: nil)
-    discard!
   end
 
-  def restore(author, only_discarded_with_procedure = false)
+  def restore(author)
     if discarded?
-      deleted_dossier = DeletedDossier.find_by(dossier_id: id)
+      transaction do
-
+        if undiscard && keep_track_on_deletion?
-      if !only_discarded_with_procedure || deleted_dossier&.procedure_removed?
+          deleted_dossier&.destroy!
-        if undiscard && keep_track_on_deletion? && en_construction?
-          deleted_dossier&.destroy
           log_dossier_operation(author, :restaurer, self)
         end
       end
     end
   end
 
+  def restore_if_discarded_with_procedure(author)
+    if deleted_dossier&.procedure_removed?
+      restore(author)
+    end
+  end
+
   def after_passer_en_construction
     update!(conservation_extension: 0.days)
     update!(en_construction_at: Time.zone.now) if self.en_construction_at.nil?
@@ -977,6 +989,10 @@ class Dossier < ApplicationRecord
 
   private
 
+  def deleted_dossier
+    @deleted_dossier ||= DeletedDossier.find_by(dossier_id: id)
+  end
+
   def defaut_groupe_instructeur?
     groupe_instructeur == procedure.defaut_groupe_instructeur
   end

app/models/procedure.rb

@@ -672,7 +672,7 @@ class Procedure < ApplicationRecord
   def restore(author)
     if discarded? && undiscard
       dossiers.with_discarded.discarded.find_each do |dossier|
-        dossier.restore(author, true)
+        dossier.restore_if_discarded_with_procedure(author)
       end
     end
   end

app/services/expired_dossiers_deletion_service.rb

@@ -19,22 +19,16 @@ class ExpiredDossiersDeletionService
       .brouillon_close_to_expiration
       .without_brouillon_expiration_notice_sent
 
-    dossiers_close_to_expiration
+    user_notifications = group_by_user_email(dossiers_close_to_expiration)
-      .with_notifiable_procedure
-      .includes(:user, :procedure)
-      .group_by(&:user)
-      .each do |(user, dossiers)|
-        DossierMailer.notify_brouillon_near_deletion(
-          dossiers,
-          user.email
-        ).deliver_later
 
-        # mark as sent dossiers from current notification
-        Dossier.where(id: dossiers).update_all(brouillon_close_to_expiration_notice_sent_at: Time.zone.now)
-      end
-
-    # mark as sent dossiers without notification
     dossiers_close_to_expiration.update_all(brouillon_close_to_expiration_notice_sent_at: Time.zone.now)
+
+    user_notifications.each do |(email, dossiers)|
+      DossierMailer.notify_brouillon_near_deletion(
+        dossiers,
+        email
+      ).deliver_later
+    end
   end
 
   def self.send_en_construction_expiration_notices
@@ -52,24 +46,17 @@ class ExpiredDossiersDeletionService
   end
 
   def self.delete_expired_brouillons_and_notify
-    dossiers_to_remove = Dossier.brouillon_expired
+    user_notifications = group_by_user_email(Dossier.brouillon_expired)
+      .map { |(email, dossiers)| [email, dossiers.map(&:hash_for_deletion_mail)] }
 
-    dossiers_to_remove
+    Dossier.brouillon_expired.destroy_all
-      .with_notifiable_procedure
-      .includes(:user, :procedure)
-      .group_by(&:user)
-      .each do |(user, dossiers)|
-        DossierMailer.notify_brouillon_deletion(
-          dossiers.map(&:hash_for_deletion_mail),
-          user.email
-        ).deliver_later
 
-        # destroy dossiers from current notification
+    user_notifications.each do |(email, dossiers_hash)|
-        Dossier.where(id: dossiers).destroy_all
+      DossierMailer.notify_brouillon_deletion(
-      end
+        dossiers_hash,
-
+        email
-    # destroy dossiers without notification
+      ).deliver_later
-    dossiers_to_remove.destroy_all
+    end
   end
 
   def self.delete_expired_en_construction_and_notify
@@ -83,57 +70,58 @@ class ExpiredDossiersDeletionService
   private
 
   def self.send_expiration_notices(dossiers_close_to_expiration, close_to_expiration_flag)
-    dossiers_close_to_expiration
+    user_notifications = group_by_user_email(dossiers_close_to_expiration)
-      .with_notifiable_procedure
+    administration_notifications = group_by_fonctionnaire_email(dossiers_close_to_expiration)
-      .includes(:user)
-      .group_by(&:user)
-      .each do |(user, dossiers)|
-        DossierMailer.notify_near_deletion_to_user(
-          dossiers,
-          user.email
-        ).deliver_later
-      end
 
-    group_by_fonctionnaire_email(dossiers_close_to_expiration).each do |(email, dossiers)|
-      DossierMailer.notify_near_deletion_to_administration(
-        dossiers.to_a,
-        email
-      ).deliver_later
-
-      # mark as sent dossiers from current notification
-      Dossier.where(id: dossiers.to_a).update_all(close_to_expiration_flag => Time.zone.now)
-    end
-
-    # mark as sent dossiers without notification
     dossiers_close_to_expiration.update_all(close_to_expiration_flag => Time.zone.now)
+
+    user_notifications.each do |(email, dossiers)|
+      DossierMailer.notify_near_deletion_to_user(dossiers, email).deliver_later
+    end
+    administration_notifications.each do |(email, dossiers)|
+      DossierMailer.notify_near_deletion_to_administration(dossiers, email).deliver_later
+    end
   end
 
   def self.delete_expired_and_notify(dossiers_to_remove, notify_on_closed_procedures_to_user: false)
-    dossiers_to_remove.each(&:expired_keep_track!)
+    user_notifications = group_by_user_email(dossiers_to_remove, notify_on_closed_procedures_to_user: notify_on_closed_procedures_to_user)
+      .map { |(email, dossiers)| [email, dossiers.map(&:id)] }
+    administration_notifications = group_by_fonctionnaire_email(dossiers_to_remove)
+      .map { |(email, dossiers)| [email, dossiers.map(&:id)] }
 
-    dossiers_to_remove
+    deleted_dossier_ids = []
-      .with_notifiable_procedure(notify_on_closed: notify_on_closed_procedures_to_user)
+    dossiers_to_remove.find_each do |dossier|
-      .includes(:user)
+      if dossier.expired_keep_track_and_destroy!
-      .group_by(&:user)
+        deleted_dossier_ids << dossier.id
-      .each do |(user, dossiers)|
-        DossierMailer.notify_automatic_deletion_to_user(
-          DeletedDossier.where(dossier_id: dossiers.map(&:id)).to_a,
-          user.email
-        ).deliver_later
       end
-
-    self.group_by_fonctionnaire_email(dossiers_to_remove).each do |(email, dossiers)|
-      DossierMailer.notify_automatic_deletion_to_administration(
-        DeletedDossier.where(dossier_id: dossiers.map(&:id)).to_a,
-        email
-      ).deliver_later
-
-      # destroy dossiers from current notification
-      Dossier.where(id: dossiers.to_a).destroy_all
     end
 
-    # destroy dossiers without notification
+    user_notifications.each do |(email, dossier_ids)|
-    dossiers_to_remove.destroy_all
+      dossier_ids = dossier_ids.intersection(deleted_dossier_ids)
+      if dossier_ids.present?
+        DossierMailer.notify_automatic_deletion_to_user(
+          DeletedDossier.where(dossier_id: dossier_ids).to_a,
+          email
+        ).deliver_later
+      end
+    end
+    administration_notifications.each do |(email, dossier_ids)|
+      dossier_ids = dossier_ids.intersection(deleted_dossier_ids)
+      if dossier_ids.present?
+        DossierMailer.notify_automatic_deletion_to_administration(
+          DeletedDossier.where(dossier_id: dossier_ids).to_a,
+          email
+        ).deliver_later
+      end
+    end
+  end
+
+  def self.group_by_user_email(dossiers, notify_on_closed_procedures_to_user: false)
+    dossiers
+      .with_notifiable_procedure(notify_on_closed: notify_on_closed_procedures_to_user)
+      .includes(:user, :procedure)
+      .group_by(&:user)
+      .map { |(user, dossiers)| [user.email, dossiers] }
   end
 
   def self.group_by_fonctionnaire_email(dossiers)
@@ -143,5 +131,6 @@ class ExpiredDossiersDeletionService
       .each_with_object(Hash.new { |h, k| h[k] = Set.new }) do |dossier, h|
         (dossier.followers_instructeurs + dossier.procedure.administrateurs).each { |destinataire| h[destinataire.email] << dossier }
       end
+      .map { |(email, dossiers)| [email, dossiers.to_a] }
   end
 end