Merge pull request #4914 from tchak/fix-brouillon-deletions

Always add a reason when deleting a dossier

app/controllers/manager/dossiers_controller.rb

@@ -22,7 +22,7 @@ module Manager
 
     def hide
       dossier = Dossier.find(params[:id])
-      dossier.delete_and_keep_track(current_administration)
+      dossier.delete_and_keep_track!(current_administration, :manager_request)
 
       logger.info("Le dossier #{dossier.id} est supprimé par #{current_administration.email}")
       flash[:notice] = "Le dossier #{dossier.id} a été supprimé."

app/controllers/users/dossiers_controller.rb

@@ -203,7 +203,7 @@ module Users
       dossier = current_user.dossiers.includes(:user, procedure: :administrateurs).find(params[:id])
 
       if dossier.can_be_deleted_by_user?
-        dossier.delete_and_keep_track(current_user)
+        dossier.delete_and_keep_track!(current_user, :user_request)
         flash.notice = 'Votre dossier a bien été supprimé.'
         redirect_to dossiers_path
       else

app/models/deleted_dossier.rb

@@ -1,7 +1,20 @@
 class DeletedDossier < ApplicationRecord
   belongs_to :procedure
 
-  def self.create_from_dossier(dossier)
-    DeletedDossier.create!(dossier_id: dossier.id, procedure: dossier.procedure, state: dossier.state, deleted_at: Time.zone.now)
+  enum reason: {
+    user_request:    'user_request',
+    manager_request: 'manager_request',
+    user_removed:    'user_removed',
+    expired:         'expired'
+  }
+
+  def self.create_from_dossier(dossier, reason)
+    create!(
+      reason: reasons.fetch(reason),
+      dossier_id: dossier.id,
+      procedure: dossier.procedure,
+      state: dossier.state,
+      deleted_at: Time.zone.now
+    )
   end
 end

app/models/dossier.rb

@@ -404,21 +404,27 @@ class Dossier < ApplicationRecord
     end
   end
 
-  def delete_and_keep_track(author)
-    deleted_dossier = DeletedDossier.create_from_dossier(self)
-    discard!
+  def expired_keep_track!
+    DeletedDossier.create_from_dossier(self, :expired)
+    log_automatic_dossier_operation(:supprimer, self)
+  end
 
+  def delete_and_keep_track!(author, reason)
     if en_construction?
+      deleted_dossier = DeletedDossier.create_from_dossier(self, reason)
+
       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
       DossierMailer.notify_deletion_to_user(deleted_dossier, user.email).deliver_later
 
       log_dossier_operation(author, :supprimer, self)
     end
 
+    discard!
+  end
+
   def after_passer_en_instruction(instructeur)
     instructeur.follow(self)
 

app/models/user.rb

@@ -120,7 +120,7 @@ class User < ApplicationRecord
     end
 
     dossiers.each do |dossier|
-      dossier.delete_and_keep_track(administration)
+      dossier.delete_and_keep_track!(administration, :user_removed)
     end
     dossiers.with_discarded.destroy_all
     destroy!

app/services/expired_dossiers_deletion_service.rb

@@ -65,18 +65,12 @@ class ExpiredDossiersDeletionService
         ).deliver_later
       end
 
-    dossiers_to_remove.each do |dossier|
-      DeletedDossier.create_from_dossier(dossier)
-      dossier.destroy
-    end
+    dossiers_to_remove.destroy_all
   end
 
   def self.delete_expired_en_construction_and_notify
     dossiers_to_remove = Dossier.en_construction_expired
-
-    dossiers_to_remove.each do |dossier|
-      DeletedDossier.create_from_dossier(dossier)
-    end
+    dossiers_to_remove.each(&:expired_keep_track!)
 
     dossiers_to_remove
       .includes(:user)

db/migrate/20200319103836_add_reason_to_deleted_dossiers.rb

@@ -0,0 +1,5 @@
+class AddReasonToDeletedDossiers < ActiveRecord::Migration[5.2]
+  def change
+    add_column :deleted_dossiers, :reason, :string
+  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: 2020_03_04_155418) do
+ActiveRecord::Schema.define(version: 2020_03_19_103836) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -213,6 +213,7 @@ ActiveRecord::Schema.define(version: 2020_03_04_155418) do
     t.string "state"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.string "reason"
     t.index ["procedure_id"], name: "index_deleted_dossiers_on_procedure_id"
   end
 

spec/mailers/dossier_mailer_spec.rb

@@ -98,7 +98,7 @@ RSpec.describe DossierMailer, type: :mailer do
 
   describe '.notify_automatic_deletion_to_user' do
     let(:dossier) { create(:dossier) }
-    let(:deleted_dossier) { DeletedDossier.create_from_dossier(dossier) }
+    let(:deleted_dossier) { DeletedDossier.create_from_dossier(dossier, :expired) }
 
     before do
       duree = dossier.procedure.duree_conservation_dossiers_dans_ds
@@ -116,7 +116,7 @@ RSpec.describe DossierMailer, type: :mailer do
 
   describe '.notify_automatic_deletion_to_administration' do
     let(:dossier) { create(:dossier) }
-    let(:deleted_dossier) { DeletedDossier.create_from_dossier(dossier) }
+    let(:deleted_dossier) { DeletedDossier.create_from_dossier(dossier, :expired) }
 
     subject { described_class.notify_automatic_deletion_to_administration([deleted_dossier], dossier.user.email) }
 

spec/models/dossier_spec.rb

@@ -661,23 +661,42 @@ describe Dossier do
     end
   end
 
-  describe "#delete_and_keep_track" do
-    let(:dossier) { create(:dossier) }
-    let(:deleted_dossier) { DeletedDossier.find_by!(dossier_id: dossier.id) }
+  describe "#delete_and_keep_track!" do
+    let(:dossier) { create(:dossier, :en_construction) }
+    let(:deleted_dossier) { DeletedDossier.find_by(dossier_id: dossier.id) }
     let(:last_operation) { dossier.dossier_operation_logs.last }
+    let(:reason) { :user_request }
 
     before do
       allow(DossierMailer).to receive(:notify_deletion_to_user).and_return(double(deliver_later: nil))
       allow(DossierMailer).to receive(:notify_deletion_to_administration).and_return(double(deliver_later: nil))
     end
 
-    subject! { dossier.delete_and_keep_track(dossier.user) }
+    subject! { dossier.delete_and_keep_track!(dossier.user, reason) }
 
+    context 'brouillon' do
+      let(:dossier) { create(:dossier) }
+
+      it 'hides the dossier' do
+        expect(dossier.discarded?).to be_truthy
+      end
+
+      it 'do not creates a DeletedDossier record' do
+        expect(deleted_dossier).to be_nil
+      end
+
+      it 'do not records the operation in the log' do
+        expect(last_operation).to be_nil
+      end
+    end
+
+    context 'en_construction' do
       it 'hides the dossier' do
         expect(dossier.hidden_at).to be_present
       end
 
       it 'creates a DeletedDossier record' do
+        expect(deleted_dossier.reason).to eq DeletedDossier.reasons.fetch(reason)
         expect(deleted_dossier.dossier_id).to eq dossier.id
         expect(deleted_dossier.procedure).to eq dossier.procedure
         expect(deleted_dossier.state).to eq dossier.state
@@ -721,6 +740,33 @@ describe Dossier do
           expect(DossierMailer).not_to have_received(:notify_deletion_to_administration)
         end
       end
+
+      context 'with reason: manager_request' do
+        let(:reason) { :manager_request }
+
+        it 'hides the dossier' do
+          expect(dossier.discarded?).to be_truthy
+        end
+
+        it 'records the operation in the log' do
+          expect(last_operation.operation).to eq("supprimer")
+          expect(last_operation.automatic_operation?).to be_falsey
+        end
+      end
+
+      context 'with reason: user_removed' do
+        let(:reason) { :user_removed }
+
+        it 'hides the dossier' do
+          expect(dossier.discarded?).to be_truthy
+        end
+
+        it 'records the operation in the log' do
+          expect(last_operation.operation).to eq("supprimer")
+          expect(last_operation.automatic_operation?).to be_falsey
+        end
+      end
+    end
   end
 
   describe 'webhook' do

spec/models/user_spec.rb

@@ -264,7 +264,7 @@ describe User, type: :model do
           user.delete_and_keep_track_dossiers(administration)
 
           expect(DeletedDossier.find_by(dossier_id: dossier_en_construction)).to be_present
-          expect(DeletedDossier.find_by(dossier_id: dossier_brouillon)).to be_present
+          expect(DeletedDossier.find_by(dossier_id: dossier_brouillon)).to be_nil
           expect(User.find_by(id: user.id)).to be_nil
         end
       end
@@ -278,16 +278,16 @@ describe User, type: :model do
         end
 
         it "keep track of dossiers and delete user" do
-          dossier_cache.delete_and_keep_track(administration)
+          dossier_cache.delete_and_keep_track!(administration, :user_request)
           user.delete_and_keep_track_dossiers(administration)
 
           expect(DeletedDossier.find_by(dossier_id: dossier_en_construction)).to be_present
-          expect(DeletedDossier.find_by(dossier_id: dossier_brouillon)).to be_present
+          expect(DeletedDossier.find_by(dossier_id: dossier_brouillon)).to be_nil
           expect(User.find_by(id: user.id)).to be_nil
         end
 
         it "doesn't destroy dossiers of another user" do
-          dossier_cache.delete_and_keep_track(administration)
+          dossier_cache.delete_and_keep_track!(administration, :user_request)
           user.delete_and_keep_track_dossiers(administration)
 
           expect(Dossier.find_by(id: dossier_from_another_user.id)).to be_present

spec/services/expired_dossiers_deletion_service_spec.rb

@@ -34,7 +34,7 @@ describe ExpiredDossiersDeletionService do
       it 'deletes and notify expired brouillon' do
         expect(DossierMailer).to have_received(:notify_brouillon_deletion).once
         expect(DossierMailer).to have_received(:notify_brouillon_deletion).with([expired_brouillon.hash_for_deletion_mail], expired_brouillon.user.email)
-        expect(DeletedDossier.find_by(dossier_id: expired_brouillon.id)).to be_present
+        expect(DeletedDossier.find_by(dossier_id: expired_brouillon.id)).not_to be_present
         expect { expired_brouillon.reload }.to raise_error(ActiveRecord::RecordNotFound)
       end
     end