Add custom asset deleter to deal with attachable discard

We want to separate out the catch-all logic we currently have around asset updates. In the `MetadataWorker` the update and delete logic were firing together, making it difficult to understand which specific updates, if any, we need when deleting.

Additionally, the same updates would fire on attachment deletion, edition publish and edition discard. We have now made it clear (previous commits) that an attachment deletion should only be sent to Asset Manager at the point of publish.

This commit deals with the remaining logic around discard. We used to call the `AttachmentDeleter` from inside the `MetadataWorker`, which suggested updates are being made at discard time. We have now extracted what needs to happen on edition discard into a service listener, making it clear that no updates are firing, only asset deletion.

app/services/service_listeners/attachment_asset_deleter.rb

@@ -0,0 +1,13 @@
+module ServiceListeners
+  class AttachmentAssetDeleter
+    def self.call(attachable:)
+      Attachment.includes(:attachment_data).where(attachable: attachable.attachables).find_each do |attachment|
+        attachment_data = attachment.attachment_data
+
+        next unless attachment_data&.deleted?
+
+        DeleteAttachmentAssetJob.perform_async(attachment_data.id)
+      end
+    end
+  end
+end

app/sidekiq/asset_manager_attachment_metadata_worker.rb

@@ -6,10 +6,6 @@ def perform(attachment_data_id)
 
     return if attachment_data.blank?
 
-    # this call needs to be here to run when we do an edition discard,
-    # unless we add a custom service for that
-    AssetManager::AttachmentDeleter.call(attachment_data)
-
     return unless attachment_data.all_asset_variants_uploaded?
 
     AssetManager::AttachmentUpdater.call(attachment_data)

app/sidekiq/delete_attachment_asset_job.rb

@@ -0,0 +1,13 @@
+class DeleteAttachmentAssetJob < WorkerBase
+  sidekiq_options queue: "asset_manager"
+
+  def perform(attachment_data_id)
+    attachment_data = AttachmentData.find(attachment_data_id)
+
+    attachment_data.assets.each do |asset|
+      AssetManager::AssetDeleter.call(asset.asset_manager_id)
+    rescue AssetManager::ServiceHelper::AssetNotFound
+      logger.info("Asset #{asset.asset_manager_id} has already been deleted from Asset Manager")
+    end
+  end
+end

config/initializers/edition_services.rb

@@ -1,13 +1,17 @@
 Whitehall::Application.config.to_prepare do
   Whitehall.edition_services.tap do |coordinator|
-    coordinator.subscribe(/^(?!(force_publish|publish))/) do |_event, edition, _options|
+    coordinator.subscribe(/^(?!(force_publish|publish|delete))/) do |_event, edition, _options|
       ServiceListeners::AttachmentUpdater.call(attachable: edition)
     end
 
     coordinator.subscribe(/^(force_publish|publish)$/) do |_event, edition, _options|
       ServiceListeners::AttachmentAssetPublisher.call(attachable: edition)
     end
 
+    coordinator.subscribe(/^(delete)$/) do |_event, edition, _options|
+      ServiceListeners::AttachmentAssetDeleter.call(attachable: edition)
+    end
+
     coordinator.subscribe("unpublish") do |_event, edition, _options|
       # handling edition's dependency on other content
       edition.edition_dependencies.destroy_all

test/integration/attachment_deletion_integration_test.rb

@@ -60,18 +60,16 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
       end
 
       context "when draft document is discarded" do
-        before do
-          visit admin_news_article_path(edition)
-          click_link "Delete draft"
-          click_button "Delete"
-        end
-
         it "deletes all corresponding assets in Asset Manager" do
           Services.asset_manager.expects(:delete_asset).once.with(first_asset_id)
           Services.asset_manager.expects(:delete_asset).once.with(second_asset_id)
-          assert_equal AssetManagerAttachmentMetadataWorker.jobs.count, 2
 
-          AssetManagerAttachmentMetadataWorker.drain
+          visit admin_news_article_path(edition)
+          click_link "Delete draft"
+          click_button "Delete"
+
+          assert_equal AssetManagerAttachmentMetadataWorker.jobs.count, 0
+          DeleteAttachmentAssetJob.drain
         end
       end
     end

test/unit/app/services/service_listeners/attachment_asset_deleter_test.rb

@@ -0,0 +1,36 @@
+require "test_helper"
+
+module ServiceListeners
+  class AttachmentAssetDeleterTest < ActiveSupport::TestCase
+    extend Minitest::Spec::DSL
+
+    describe ServiceListeners::AttachmentAssetDeleter do
+      let(:edition) { create(:draft_news_article) }
+      let(:first_attachment) { create(:file_attachment, attachable: edition, attachment_data: create(:attachment_data, attachable: edition)) }
+      let(:second_attachment) { create(:file_attachment, attachable: edition, attachment_data: create(:attachment_data, attachable: edition)) }
+
+      before do
+        stub_asset(first_attachment.attachment_data.assets.first.asset_manager_id)
+        stub_asset(second_attachment.attachment_data.assets.first.asset_manager_id)
+      end
+
+      it "calls deleter for all assets of all attachments" do
+        edition.delete!
+        edition.delete_all_attachments
+
+        AssetManager::AssetDeleter.expects(:call).with(first_attachment.attachment_data.assets.first.asset_manager_id)
+        AssetManager::AssetDeleter.expects(:call).with(second_attachment.attachment_data.assets.first.asset_manager_id)
+
+        ServiceListeners::AttachmentAssetDeleter.call(attachable: edition)
+        DeleteAttachmentAssetJob.drain
+      end
+
+      def stub_asset(asset_manger_id, attributes = {})
+        url_id = "http://asset-manager/assets/#{asset_manger_id}"
+        Services.asset_manager.stubs(:asset)
+                .with(asset_manger_id)
+                .returns(attributes.merge(id: url_id).stringify_keys)
+      end
+    end
+  end
+end

test/unit/app/sidekiq/asset_manager_attachment_metadata_worker_test.rb

@@ -8,13 +8,9 @@ class AssetManagerAttachmentMetadataWorkerTest < ActiveSupport::TestCase
     let(:attachment_data) { create(:attachment_data, attachable: edition) }
     let(:worker) { AssetManagerAttachmentMetadataWorker.new }
 
-    it "calls both updater and deleter" do
+    it "calls updater" do
       AssetManager::AttachmentUpdater.expects(:call).with(attachment_data)
 
-      AssetManager::AttachmentDeleter.expects(:call).with(
-        attachment_data,
-      )
-
       worker.perform(attachment_data.id)
     end
 
@@ -26,12 +22,6 @@ class AssetManagerAttachmentMetadataWorkerTest < ActiveSupport::TestCase
 
         worker.perform(attachment_data.id)
       end
-
-      it "calls deleter" do
-        AssetManager::AttachmentDeleter.expects(:call)
-
-        worker.perform(attachment_data.id)
-      end
     end
   end
 end

test/unit/app/sidekiq/delete_attachment_asset_job_test.rb

@@ -0,0 +1,25 @@
+require "test_helper"
+
+class DeleteAttachmentAssetJobTest < ActiveSupport::TestCase
+  extend Minitest::Spec::DSL
+
+  describe DeleteAttachmentAssetJob do
+    let(:attachable) { create(:draft_news_article) }
+    let(:attachment_data) { create(:attachment_data, attachable:) }
+    let(:asset_manager_id) { attachment_data.assets.first.asset_manager_id }
+    let(:worker) { DeleteAttachmentAssetJob.new }
+
+    it "deletes the asset" do
+      AssetManager::AssetDeleter.expects(:call).with(asset_manager_id)
+
+      worker.perform(attachment_data.id)
+    end
+
+    it "raises an error if the asset deletion fails for an unknown reason" do
+      expected_error = GdsApi::HTTPServerError.new(500)
+      AssetManager::AssetDeleter.expects(:call).raises(expected_error)
+
+      assert_raises(GdsApi::HTTPServerError) { worker.perform(attachment_data.id) }
+    end
+  end
+end