Add a custom attachment asset publish listener

We want to separate out the catch-all logic we currently have around asset updates. The `AttachmentUpdater` service listener was listening on all possible events, making it hard to understand which specific updates need to happen in which case. Additionally, in the `MetadataWorker` that it enqueues, the update and delete logic fired together.

These changes are being made in the context of a bug that causes assets to remain live when a new draft attachment gets replaced and deleted. The issue was that the `deleted?` method on the replaced `AttachmentData` evaluated to true when attempting to update the asset to live after deleting the attachment. By separating the publish logic we are confident in removing the `deleted?` on update.

We also removed the deletion call from the attachment controller's destroy method, so that the only delete calls to Asset Manager now happen from the publish listener, at the point of publish.

app/controllers/admin/attachments_controller.rb

@@ -48,9 +48,7 @@ def update
   def confirm_destroy; end
 
   def destroy
-    attachment_data = attachment.attachment_data
     attachment.destroy!
-    attachment_updater(attachment_data)
     redirect_to attachable_attachments_path(attachable), notice: "Attachment deleted"
   end
 

app/services/service_listeners/attachment_asset_publisher.rb

@@ -0,0 +1,11 @@
+module ServiceListeners
+  class AttachmentAssetPublisher
+    def self.call(attachable:)
+      Attachment.includes(:attachment_data).where(attachable: attachable.attachables).find_each do |attachment|
+        next unless attachment.attachment_data
+
+        PublishAttachmentAssetJob.perform_async(attachment.attachment_data.id)
+      end
+    end
+  end
+end

app/sidekiq/asset_manager_attachment_metadata_worker.rb

@@ -6,6 +6,8 @@ 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?

app/sidekiq/publish_attachment_asset_job.rb

@@ -0,0 +1,25 @@
+class PublishAttachmentAssetJob < WorkerBase
+  sidekiq_options queue: "asset_manager"
+
+  def perform(attachment_data_id)
+    attachment_data = AttachmentData.find(attachment_data_id)
+
+    asset_attributes = {
+      "draft" => false,
+    }
+
+    unless attachment_data.replaced?
+      asset_attributes.merge!({ "parent_document_url" => attachment_data.attachable_url })
+    end
+
+    # We need to delete first, because if we delete after updating, and the delete request fails,
+    # we will have a live asset. Asset Manager should let us update the deleted asset,
+    # in line with recent changes that now permit the update of a deleted draft asset.
+    # At this point in time, the asset is still in draft in Asset Manager, since the draft: false update
+    # has not yet been sent, as we do not send it from anywhere else in the code.
+    attachment_data.assets.each do |asset|
+      AssetManager::AssetDeleter.call(asset.asset_manager_id) if attachment_data.deleted?
+      AssetManager::AssetUpdater.call(asset.asset_manager_id, asset_attributes)
+    end
+  end
+end

config/initializers/edition_services.rb

@@ -1,9 +1,13 @@
 Whitehall::Application.config.to_prepare do
   Whitehall.edition_services.tap do |coordinator|
-    coordinator.subscribe do |_event, edition, _options|
+    coordinator.subscribe(/^(?!(force_publish|publish))/) 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("unpublish") do |_event, edition, _options|
       # handling edition's dependency on other content
       edition.edition_dependencies.destroy_all

test/integration/attachment_deletion_integration_test.rb

@@ -15,13 +15,15 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
       let(:second_attachment) { build(:file_attachment, attachable: edition) }
       let(:second_asset_id) { second_attachment.attachment_data.assets.first.asset_manager_id }
       let(:edition) { create(:news_article) }
+      let(:topic_taxon) { build(:taxon_hash) }
 
       before do
         login_as(managing_editor)
 
         setup_publishing_api_for(edition)
         stub_publishing_api_has_linkables([], document_type: "topic")
         stub_publishing_api_expanded_links_with_taxons(edition.content_id, [])
+        stub_publishing_api_links_with_taxons(edition.content_id, [topic_taxon["content_id"]])
 
         stub_asset(first_asset_id)
         stub_asset(second_asset_id)
@@ -31,27 +33,29 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
       end
 
       context "when one attachment is deleted" do
-        before do
+        it "deletes the corresponding asset in Asset Manager when the edition is published" do
+          Services.asset_manager.expects(:delete_asset).never
+
           visit admin_news_article_path(edition)
           click_link "Modify attachments"
           within page.find("li", text: first_attachment.title) do
             click_link "Delete attachment"
           end
           click_button "Delete attachment"
           assert_text "Attachment deleted"
-        end
 
-        it "deletes the corresponding asset in Asset Manager" do
           Services.asset_manager.expects(:delete_asset).once.with(first_asset_id)
-          assert_equal AssetManagerAttachmentMetadataWorker.jobs.count, 1
+          Services.asset_manager.expects(:update_asset).once.with(first_asset_id, has_entry({ "draft" => false }))
+          Services.asset_manager.expects(:update_asset).once.with(second_asset_id, has_entry({ "draft" => false }))
 
-          AssetManagerAttachmentMetadataWorker.drain
-        end
-
-        it "queues one worker to delete the asset" do
-          queued_ids = AssetManagerAttachmentMetadataWorker.jobs.map { |job| job["args"].first }
+          visit admin_news_article_path(edition)
+          click_link "Force publish"
+          assert_text "Reason for force publishing"
+          fill_in "Reason for force publishing", with: "testing"
+          click_button "Force publish"
+          assert_text "The document #{edition.title} has been published"
 
-          assert_equal queued_ids, [first_attachment.attachment_data.id]
+          PublishAttachmentAssetJob.drain
         end
       end
 
@@ -104,8 +108,10 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
         click_button "Delete attachment"
         assert_text "Attachment deleted"
 
-        Services.asset_manager.expects(:update_asset).once.with(original_asset_manager_id, has_entry({ "draft" => false }))
-        AssetManagerAttachmentMetadataWorker.drain
+        Services.asset_manager.expects(:update_asset)
+                .with(original_asset_manager_id, has_entry({ "draft" => false }))
+                .at_least_once
+        Services.asset_manager.expects(:delete_asset).once.with(original_asset_manager_id)
 
         visit admin_news_article_path(latest_attachable)
         click_link "Force publish"
@@ -114,8 +120,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
         click_button "Force publish"
         assert_text "The document #{latest_attachable.title} has been published"
 
-        Services.asset_manager.expects(:delete_asset).once.with(original_asset_manager_id)
-        AssetManagerAttachmentMetadataWorker.drain
+        PublishAttachmentAssetJob.drain
       end
 
       context "when the attachment has been replaced" do
@@ -145,8 +150,8 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
           click_button "Delete attachment"
           assert_text "Attachment deleted"
 
-          Services.asset_manager.expects(:update_asset).once.with(replacement_asset_manager_id, has_entry({ "draft" => true }))
-          AssetManagerAttachmentMetadataWorker.drain
+          Services.asset_manager.expects(:delete_asset).once.with(replacement_asset_manager_id)
+          Services.asset_manager.expects(:update_asset).once.with(replacement_asset_manager_id, has_entry({ "draft" => false }))
 
           visit admin_news_article_path(latest_attachable)
           click_link "Force publish"
@@ -155,9 +160,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
           click_button "Force publish"
           assert_text "The document #{latest_attachable.title} has been published"
 
-          Services.asset_manager.expects(:delete_asset).once.with(replacement_asset_manager_id)
-          Services.asset_manager.expects(:update_asset).once.with(replacement_asset_manager_id, has_entry({ "draft" => false }))
-          AssetManagerAttachmentMetadataWorker.drain
+          PublishAttachmentAssetJob.drain
         end
       end
     end

test/integration/attachment_draft_status_integration_test.rb

@@ -35,9 +35,12 @@ class AttachmentDraftStatusIntegrationTest < ActionDispatch::IntegrationTest
           stub_publishing_api_expanded_links_with_taxons(edition.content_id, [])
           stub_publishing_api_links_with_taxons(edition.content_id, [topic_taxon["content_id"]])
 
+          assert_sets_draft_status_in_asset_manager_to false
+
           visit admin_news_article_path(edition)
           force_publish_document
-          assert_sets_draft_status_in_asset_manager_to false
+
+          PublishAttachmentAssetJob.drain
         end
       end
 
@@ -64,9 +67,12 @@ class AttachmentDraftStatusIntegrationTest < ActionDispatch::IntegrationTest
           stub_publishing_api_expanded_links_with_taxons(edition.content_id, [])
           stub_publishing_api_links_with_taxons(edition.content_id, [topic_taxon["content_id"]])
 
+          assert_sets_draft_status_in_asset_manager_to false
+
           visit admin_consultation_path(edition)
           force_publish_document
-          assert_sets_draft_status_in_asset_manager_to false
+
+          PublishAttachmentAssetJob.drain
         end
       end
 
@@ -80,9 +86,12 @@ class AttachmentDraftStatusIntegrationTest < ActionDispatch::IntegrationTest
           stub_publishing_api_expanded_links_with_taxons(edition.content_id, [])
           stub_publishing_api_links_with_taxons(edition.content_id, [topic_taxon["content_id"]])
 
+          assert_sets_draft_status_in_asset_manager_to false
+
           visit admin_consultation_path(edition)
           force_publish_document
-          assert_sets_draft_status_in_asset_manager_to false
+
+          PublishAttachmentAssetJob.drain
         end
       end
     end
@@ -102,6 +111,7 @@ class AttachmentDraftStatusIntegrationTest < ActionDispatch::IntegrationTest
         AssetManagerCreateAssetWorker.drain
 
         assert_sets_draft_status_in_asset_manager_to false
+        AssetManagerAttachmentMetadataWorker.drain
       end
     end
 
@@ -133,7 +143,6 @@ def assert_sets_draft_status_in_asset_manager_to(draft, never: false)
                             .with(asset_manager_id, has_entry("draft", draft))
                             .at_least_once
       expectation.never if never
-      AssetManagerAttachmentMetadataWorker.drain
     end
 
     def refute_sets_draft_status_in_asset_manager_to(draft)

test/integration/attachment_link_header_test.rb

@@ -31,16 +31,15 @@ class AttachmentLinkHeaderIntegrationTest < ActionDispatch::IntegrationTest
         let(:asset_initially_draft) { true }
 
         it "sets link to parent document in Asset Manager when document is published" do
-          visit admin_news_article_path(edition)
-          force_publish_document
-
           parent_document_url = edition.public_url
-
           Services.asset_manager.expects(:update_asset)
                   .at_least_once
                   .with(asset_manager_id, has_entry("parent_document_url", parent_document_url))
 
-          AssetManagerAttachmentMetadataWorker.drain
+          visit admin_news_article_path(edition)
+          force_publish_document
+
+          PublishAttachmentAssetJob.drain
         end
       end
     end

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

@@ -0,0 +1,41 @@
+require "test_helper"
+
+module ServiceListeners
+  class AttachmentAssetPublisherTest < ActiveSupport::TestCase
+    extend Minitest::Spec::DSL
+
+    describe ServiceListeners::AttachmentAssetPublisher do
+      let(:edition) { create(:published_news_article) }
+      let(:attachment) { create(:file_attachment, attachable: edition, attachment_data: create(:attachment_data, attachable: edition)) }
+
+      it "sets the expected attributes" do
+        expected_attribute_hash = {
+          "draft" => false,
+          "parent_document_url" => edition.public_url(draft: false),
+        }
+
+        AssetManager::AssetUpdater.expects(:call).with(attachment.attachment_data.assets.first.asset_manager_id, expected_attribute_hash)
+
+        ServiceListeners::AttachmentAssetPublisher.call(attachable: edition)
+        PublishAttachmentAssetJob.drain
+      end
+
+      it "deletes the asset if the attachment is deleted" do
+        stub_asset(attachment.attachment_data.assets.first.asset_manager_id)
+        attachment.destroy!
+
+        AssetManager::AssetDeleter.expects(:call).with(attachment.attachment_data.assets.first.asset_manager_id)
+
+        ServiceListeners::AttachmentAssetPublisher.call(attachable: edition)
+        PublishAttachmentAssetJob.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/publish_attachment_asset_job_test.rb

@@ -0,0 +1,38 @@
+require "test_helper"
+
+class PublishAttachmentAssetJobTest < ActiveSupport::TestCase
+  extend Minitest::Spec::DSL
+
+  describe PublishAttachmentAssetJob 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) { PublishAttachmentAssetJob.new }
+
+    it "it deletes and updates the asset if attachment data is deleted" do
+      attachment = create(:file_attachment, attachable: attachable, attachment_data: attachment_data)
+      attachment.destroy!
+
+      AssetManager::AssetDeleter.expects(:call).with(asset_manager_id)
+      AssetManager::AssetUpdater.expects(:call).with(asset_manager_id, { "draft" => false, "parent_document_url" => attachment_data.attachable_url })
+
+      worker.perform(attachment_data.id)
+    end
+
+    it "only updates the asset if attachment data is not deleted" do
+      AssetManager::AssetUpdater.expects(:call).with(asset_manager_id, { "draft" => false, "parent_document_url" => attachment_data.attachable_url })
+
+      worker.perform(attachment_data.id)
+    end
+
+    context "attachment data is replaced" do
+      it "does not set the parent_document_url" do
+        attachment_data.update!(replaced_by: create(:attachment_data, attachable:))
+
+        AssetManager::AssetUpdater.expects(:call).with(asset_manager_id, { "draft" => false })
+
+        worker.perform(attachment_data.id)
+      end
+    end
+  end
+end