diff --git a/app/controllers/admin/attachments_controller.rb b/app/controllers/admin/attachments_controller.rb index 8ade796d3be..c464d0e1299 100644 --- a/app/controllers/admin/attachments_controller.rb +++ b/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 diff --git a/app/services/service_listeners/attachment_asset_publisher.rb b/app/services/service_listeners/attachment_asset_publisher.rb new file mode 100644 index 00000000000..7fd9f65051b --- /dev/null +++ b/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 diff --git a/app/sidekiq/asset_manager_attachment_metadata_worker.rb b/app/sidekiq/asset_manager_attachment_metadata_worker.rb index d0d828b3069..b9f8b55eab6 100644 --- a/app/sidekiq/asset_manager_attachment_metadata_worker.rb +++ b/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? diff --git a/app/sidekiq/publish_attachment_asset_job.rb b/app/sidekiq/publish_attachment_asset_job.rb new file mode 100644 index 00000000000..e86c5c2b6c9 --- /dev/null +++ b/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 diff --git a/config/initializers/edition_services.rb b/config/initializers/edition_services.rb index 3ef03adb110..ae9560a2120 100644 --- a/config/initializers/edition_services.rb +++ b/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 diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index 717468d3723..c05562c1684 100644 --- a/test/integration/attachment_deletion_integration_test.rb +++ b/test/integration/attachment_deletion_integration_test.rb @@ -15,6 +15,7 @@ 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) @@ -22,6 +23,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest 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,7 +33,9 @@ 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 @@ -39,19 +43,19 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest 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 diff --git a/test/integration/attachment_draft_status_integration_test.rb b/test/integration/attachment_draft_status_integration_test.rb index 0887a510760..f9200ffa2e5 100644 --- a/test/integration/attachment_draft_status_integration_test.rb +++ b/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) diff --git a/test/integration/attachment_link_header_test.rb b/test/integration/attachment_link_header_test.rb index 322ba1ceca8..208f7c346e4 100644 --- a/test/integration/attachment_link_header_test.rb +++ b/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 diff --git a/test/unit/app/services/service_listeners/attachment_asset_publisher_test.rb b/test/unit/app/services/service_listeners/attachment_asset_publisher_test.rb new file mode 100644 index 00000000000..01300efde4d --- /dev/null +++ b/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 diff --git a/test/unit/app/sidekiq/publish_attachment_asset_job_test.rb b/test/unit/app/sidekiq/publish_attachment_asset_job_test.rb new file mode 100644 index 00000000000..124f3b34bc9 --- /dev/null +++ b/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