From 861958717e9d2d1b1485392691e459686f674e8b Mon Sep 17 00:00:00 2001
From: Laura Ghiorghisor <laura.ghiorghisor@digital.cabinet-office.gov.uk>
Date: Wed, 5 Feb 2025 14:23:21 +0000
Subject: [PATCH] Ensure assets are deleted when policy group is deleted

Since Policy Groups are not editionable, their delete flows are different. Because of this, when deleting a group, the assets associated with their attachments did not get deleted in Asset Manager.

These changes address that bug, and ensure we delete both the attachments themselves and their corresponding assets in Asset Manager. Rather than trying to reuse some of the classes that serve the edition workflow, we preferred to use a callback on the PolicyGroup model, only reusing the asset specific DeleteAttachmentAssetJob.
---
 app/models/policy_group.rb                    | 12 +++++-
 .../attachment_deletion_integration_test.rb   | 40 ++++++++++++++-----
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/app/models/policy_group.rb b/app/models/policy_group.rb
index 984c1ae11a0..9d4b210740b 100644
--- a/app/models/policy_group.rb
+++ b/app/models/policy_group.rb
@@ -14,7 +14,7 @@ class PolicyGroup < ApplicationRecord
 
   after_create :extract_dependencies
   after_update :extract_dependencies
-  after_destroy :remove_all_dependencies
+  after_destroy :remove_all_dependencies, :delete_all_attachments
   after_update :publish_deleted_attachments
 
   def extract_dependencies
@@ -41,6 +41,16 @@ def remove_all_dependencies
     policy_group_dependencies.delete_all
   end
 
+  def delete_all_attachments
+    attachments.each do |attachment|
+      attachment.destroy!
+
+      next unless attachment.attachment_data
+
+      DeleteAttachmentAssetJob.perform_async(attachment.attachment_data.id)
+    end
+  end
+
   def access_limited_object
     nil
   end
diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb
index 0e3e9503760..aefa48d57e7 100644
--- a/test/integration/attachment_deletion_integration_test.rb
+++ b/test/integration/attachment_deletion_integration_test.rb
@@ -219,25 +219,28 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
     end
 
     context "given a policy group" do
-      let(:managing_editor) { create(:managing_editor) }
+      let(:gds_editor) { create(:gds_editor) }
       let(:attachable) { create(:policy_group) }
-      let(:attachment) { build(:file_attachment, attachable:) }
-      let(:asset_manager_id) { attachment.attachment_data.assets.first.asset_manager_id }
+      let(:first_attachment) { build(:file_attachment, attachable:) }
+      let(:second_attachment) { build(:file_attachment, attachable:) }
+      let(:first_asset_manager_id) { first_attachment.attachment_data.assets.first.asset_manager_id }
+      let(:second_asset_manager_id) { second_attachment.attachment_data.assets.first.asset_manager_id }
 
       before do
-        login_as(managing_editor)
-        stub_asset(asset_manager_id, { "draft" => false, "parent_document_url" => nil })
+        login_as(gds_editor)
+        stub_asset(first_asset_manager_id, { "draft" => false, "parent_document_url" => nil })
+        stub_asset(second_asset_manager_id, { "draft" => false, "parent_document_url" => nil })
 
-        attachable.attachments << [attachment]
+        attachable.attachments << [first_attachment, second_attachment]
         attachable.save!
       end
 
-      it "deletes the corresponding asset in Asset Manager when the policy group is saved" do
-        Services.asset_manager.expects(:delete_asset).once.with(asset_manager_id)
-        Services.asset_manager.expects(:update_asset).with(asset_manager_id).never
+      it "deletes the corresponding asset in Asset Manager for a deleted attachment, when the policy group is saved" do
+        Services.asset_manager.expects(:delete_asset).once.with(first_asset_manager_id)
+        Services.asset_manager.expects(:update_asset).with(first_asset_manager_id).never
 
         visit admin_policy_group_attachments_path(attachable)
-        within page.find("li", text: attachment.title) do
+        within page.find("li", text: first_attachment.title) do
           click_link "Delete attachment"
         end
         click_button "Delete attachment"
@@ -247,6 +250,23 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
 
         DeleteAttachmentAssetJob.drain
       end
+
+      it "deleted the corresponding assets of all attachments, when the policy group is deleted" do
+        Services.asset_manager.expects(:delete_asset).once.with(first_asset_manager_id)
+        Services.asset_manager.expects(:delete_asset).once.with(second_asset_manager_id)
+
+        visit admin_policy_groups_path
+        within page.find("tr", text: attachable.name) do
+          click_link "Delete"
+        end
+        click_button "Delete"
+        assert_text "\"#{attachable.name}\" deleted."
+
+        assert_equal first_attachment.reload.deleted, true
+        assert_equal second_attachment.reload.deleted, true
+
+        DeleteAttachmentAssetJob.drain
+      end
     end
 
   private