From daa5c60a3019ec95ff3984fed36719c50c31ca68 Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Tue, 14 Jan 2025 16:12:32 +0000 Subject: [PATCH 1/7] Add deletion integration test These tests now explicitly cover how deletion works, namely that, in the case of already published editions, an attachment deletion on a new draft will only ever be sent to Asset Manager at the point of publish. Changes: - Add test to cover replaced assets - to capture a known bug where attachments that are replaced and then deleted, cause the original replaced attachment's assets to remain live, rather than redirect to their replacement. - Fix existing test so that it triggers the edition service listener when publishing. --- .../attachment_deletion_integration_test.rb | 67 +++++++++++++++++-- 1 file changed, 60 insertions(+), 7 deletions(-) diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index 29cd80e07c5..717468d3723 100644 --- a/test/integration/attachment_deletion_integration_test.rb +++ b/test/integration/attachment_deletion_integration_test.rb @@ -77,7 +77,8 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest let(:earliest_attachable) { create(:published_news_article, :with_file_attachment) } let(:latest_attachable) { earliest_attachable.reload.create_draft(managing_editor) } let(:attachment) { latest_attachable.attachments.first } - let(:original_asset) { attachment.attachment_data.assets.first.asset_manager_id } + let(:original_asset_manager_id) { attachment.attachment_data.assets.first.asset_manager_id } + let(:topic_taxon) { build(:taxon_hash) } before do login_as(managing_editor) @@ -85,8 +86,13 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest setup_publishing_api_for(latest_attachable) stub_publishing_api_has_linkables([], document_type: "topic") stub_publishing_api_expanded_links_with_taxons(latest_attachable.content_id, []) + stub_publishing_api_links_with_taxons(latest_attachable.content_id, [topic_taxon["content_id"]]) - stub_asset(original_asset) + stub_asset(original_asset_manager_id) + + latest_attachable.update!(minor_change: true) + + AssetManagerAttachmentMetadataWorker.drain end it "deletes the corresponding asset in Asset Manager only when the new draft gets published" do @@ -98,15 +104,62 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest click_button "Delete attachment" assert_text "Attachment deleted" - Services.asset_manager.expects(:delete_asset).never.with(original_asset) - - latest_attachable.update!(minor_change: true) - latest_attachable.force_publish! + Services.asset_manager.expects(:update_asset).once.with(original_asset_manager_id, has_entry({ "draft" => false })) + AssetManagerAttachmentMetadataWorker.drain - Services.asset_manager.expects(:delete_asset).once.with(original_asset) + visit admin_news_article_path(latest_attachable) + 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 #{latest_attachable.title} has been published" + Services.asset_manager.expects(:delete_asset).once.with(original_asset_manager_id) AssetManagerAttachmentMetadataWorker.drain end + + context "when the attachment has been replaced" do + let(:replacement_asset_manager_id) { "replacement_asset_manager_id" } + + before do + stub_asset(replacement_asset_manager_id) + + replacement_data = create(:attachment_data, attachable: latest_attachable) + attachment.attachment_data.replaced_by = replacement_data + attachment.attachment_data.save! + + asset = replacement_data.assets.first + asset.asset_manager_id = replacement_asset_manager_id + asset.save! + + attachment.attachment_data = replacement_data + attachment.save! + end + + it "deletes the corresponding asset in Asset Manager and updates the asset to live, only when the new draft gets published" do + visit admin_news_article_path(latest_attachable) + click_link "Modify attachments" + within page.find("li", text: attachment.title) do + click_link "Delete attachment" + end + 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 + + visit admin_news_article_path(latest_attachable) + 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 #{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 + end + end end private From 5131ca537cdd5f8df696d1723f09d3fd64bd1ed5 Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Wed, 15 Jan 2025 17:23:57 +0000 Subject: [PATCH 2/7] 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. --- .../admin/attachments_controller.rb | 2 - .../attachment_asset_publisher.rb | 11 +++++ ...sset_manager_attachment_metadata_worker.rb | 2 + app/sidekiq/publish_attachment_asset_job.rb | 25 +++++++++++ config/initializers/edition_services.rb | 8 +++- .../attachment_deletion_integration_test.rb | 43 ++++++++++--------- ...ttachment_draft_status_integration_test.rb | 17 ++++++-- .../attachment_link_header_test.rb | 9 ++-- .../attachment_asset_publisher_test.rb | 41 ++++++++++++++++++ .../publish_attachment_asset_job_test.rb | 38 ++++++++++++++++ 10 files changed, 162 insertions(+), 34 deletions(-) create mode 100644 app/services/service_listeners/attachment_asset_publisher.rb create mode 100644 app/sidekiq/publish_attachment_asset_job.rb create mode 100644 test/unit/app/services/service_listeners/attachment_asset_publisher_test.rb create mode 100644 test/unit/app/sidekiq/publish_attachment_asset_job_test.rb 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..232c83a4c2e --- /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..a75da616f78 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. + # See next commit where we remove this call in favour of a custom discard service listener. 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..23f4d5e7551 100644 --- a/config/initializers/edition_services.rb +++ b/config/initializers/edition_services.rb @@ -1,7 +1,11 @@ Whitehall::Application.config.to_prepare do Whitehall.edition_services.tap do |coordinator| - coordinator.subscribe do |_event, edition, _options| - ServiceListeners::AttachmentUpdater.call(attachable: edition) + coordinator.subscribe do |event, edition, _options| + if %w[publish force_publish].include?(event) + ServiceListeners::AttachmentAssetPublisher.call(edition) + else + ServiceListeners::AttachmentUpdater.call(attachable: edition) + end end coordinator.subscribe("unpublish") do |_event, edition, _options| diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index 717468d3723..2b7733f1556 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 @@ -91,8 +95,6 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest stub_asset(original_asset_manager_id) latest_attachable.update!(minor_change: true) - - AssetManagerAttachmentMetadataWorker.drain end it "deletes the corresponding asset in Asset Manager only when the new draft gets published" do @@ -104,8 +106,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 +118,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 +148,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 +158,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..311f9001a32 --- /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(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(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 From 57be018bdf36bfb4ca1e00ab6837243c17f2c43b Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Thu, 16 Jan 2025 14:09:03 +0000 Subject: [PATCH 3/7] 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. --- .../asset_manager/attachment_deleter.rb | 9 ---- .../attachment_asset_deleter.rb | 13 ++++++ ...sset_manager_attachment_metadata_worker.rb | 4 -- app/sidekiq/delete_attachment_asset_job.rb | 13 ++++++ config/initializers/edition_services.rb | 2 + .../attachment_deletion_integration_test.rb | 13 +++--- .../asset_manager/attachment_deleter_test.rb | 43 ------------------- .../attachment_asset_deleter_test.rb | 36 ++++++++++++++++ ...manager_attachment_metadata_worker_test.rb | 12 +----- .../delete_attachment_asset_job_test.rb | 25 +++++++++++ 10 files changed, 95 insertions(+), 75 deletions(-) delete mode 100644 app/services/asset_manager/attachment_deleter.rb create mode 100644 app/services/service_listeners/attachment_asset_deleter.rb create mode 100644 app/sidekiq/delete_attachment_asset_job.rb delete mode 100644 test/unit/app/services/asset_manager/attachment_deleter_test.rb create mode 100644 test/unit/app/services/service_listeners/attachment_asset_deleter_test.rb create mode 100644 test/unit/app/sidekiq/delete_attachment_asset_job_test.rb diff --git a/app/services/asset_manager/attachment_deleter.rb b/app/services/asset_manager/attachment_deleter.rb deleted file mode 100644 index a7841521d55..00000000000 --- a/app/services/asset_manager/attachment_deleter.rb +++ /dev/null @@ -1,9 +0,0 @@ -class AssetManager::AttachmentDeleter - def self.call(attachment_data) - return unless attachment_data.deleted? - - attachment_data.assets.each do |asset| - AssetManager::AssetDeleter.call(asset.asset_manager_id) - end - end -end diff --git a/app/services/service_listeners/attachment_asset_deleter.rb b/app/services/service_listeners/attachment_asset_deleter.rb new file mode 100644 index 00000000000..f5bd37195f7 --- /dev/null +++ b/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 diff --git a/app/sidekiq/asset_manager_attachment_metadata_worker.rb b/app/sidekiq/asset_manager_attachment_metadata_worker.rb index a75da616f78..2503b33df1d 100644 --- a/app/sidekiq/asset_manager_attachment_metadata_worker.rb +++ b/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. - # See next commit where we remove this call in favour of a custom discard service listener. - AssetManager::AttachmentDeleter.call(attachment_data) - return unless attachment_data.all_asset_variants_uploaded? AssetManager::AttachmentUpdater.call(attachment_data) diff --git a/app/sidekiq/delete_attachment_asset_job.rb b/app/sidekiq/delete_attachment_asset_job.rb new file mode 100644 index 00000000000..fc1ebc2071d --- /dev/null +++ b/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 diff --git a/config/initializers/edition_services.rb b/config/initializers/edition_services.rb index 23f4d5e7551..0aac920741c 100644 --- a/config/initializers/edition_services.rb +++ b/config/initializers/edition_services.rb @@ -3,6 +3,8 @@ coordinator.subscribe do |event, edition, _options| if %w[publish force_publish].include?(event) ServiceListeners::AttachmentAssetPublisher.call(edition) + elsif event == "delete" + ServiceListeners::AttachmentAssetDeleter.call(edition) else ServiceListeners::AttachmentUpdater.call(attachable: edition) end diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index 2b7733f1556..312e5b230e6 100644 --- a/test/integration/attachment_deletion_integration_test.rb +++ b/test/integration/attachment_deletion_integration_test.rb @@ -60,18 +60,15 @@ 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" + + DeleteAttachmentAssetJob.drain end end end diff --git a/test/unit/app/services/asset_manager/attachment_deleter_test.rb b/test/unit/app/services/asset_manager/attachment_deleter_test.rb deleted file mode 100644 index 5aae8a0bbfb..00000000000 --- a/test/unit/app/services/asset_manager/attachment_deleter_test.rb +++ /dev/null @@ -1,43 +0,0 @@ -require "test_helper" - -class AssetManager::AttachmentDeleterTest < ActiveSupport::TestCase - extend Minitest::Spec::DSL - - describe AssetManager::AttachmentDeleter do - let(:worker) { AssetManager::AttachmentDeleter } - let(:delete_worker) { mock("delete-worker") } - let(:attachment_data) { build(:attachment_data) } - - around do |test| - AssetManager.stub_const(:AssetDeleter, delete_worker) do - test.call - end - end - - before do - attachment_data.stubs(:deleted?).returns(deleted) - end - - context "attachment data is not deleted" do - let(:deleted) { false } - - it "does not delete any assets from Asset Manager" do - delete_worker.expects(:call).never - - worker.call(attachment_data) - - assert AssetManagerDeleteAssetWorker.jobs.empty? - end - end - - context "attachment data is deleted" do - let(:deleted) { true } - - it "deletes attachment asset in Asset Manager" do - delete_worker.expects(:call).with("asset_manager_id") - - worker.call(attachment_data) - end - end - end -end diff --git a/test/unit/app/services/service_listeners/attachment_asset_deleter_test.rb b/test/unit/app/services/service_listeners/attachment_asset_deleter_test.rb new file mode 100644 index 00000000000..40c3e7f020c --- /dev/null +++ b/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(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 diff --git a/test/unit/app/sidekiq/asset_manager_attachment_metadata_worker_test.rb b/test/unit/app/sidekiq/asset_manager_attachment_metadata_worker_test.rb index 9c7385588d7..6bda340177a 100644 --- a/test/unit/app/sidekiq/asset_manager_attachment_metadata_worker_test.rb +++ b/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 diff --git a/test/unit/app/sidekiq/delete_attachment_asset_job_test.rb b/test/unit/app/sidekiq/delete_attachment_asset_job_test.rb new file mode 100644 index 00000000000..64a8722aecc --- /dev/null +++ b/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 From 74b05441f567a0323a15ed3390d32c9395eb14cc Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Fri, 17 Jan 2025 17:05:11 +0000 Subject: [PATCH 4/7] Remove log for asset not found This log tracked how many times we attempted to update a deleted asset, with the assumption that that is fault in the system. We now know that we actually need to update deleted draft assets for replacements to work, therefore removing the logging. --- app/services/asset_manager/asset_updater.rb | 5 ----- .../app/services/asset_manager/asset_updater_test.rb | 10 ---------- 2 files changed, 15 deletions(-) diff --git a/app/services/asset_manager/asset_updater.rb b/app/services/asset_manager/asset_updater.rb index 9f8a58becec..065fbdab9b5 100644 --- a/app/services/asset_manager/asset_updater.rb +++ b/app/services/asset_manager/asset_updater.rb @@ -21,11 +21,6 @@ def update_with_asset_manager_id(asset_manager_id, new_attributes) raise AssetAttributesEmpty, asset_manager_id if new_attributes.empty? attributes = find_asset_by_id(asset_manager_id) - asset_deleted = attributes["deleted"] - - if asset_deleted - logger.warn("Asset with asset_manager_id: '#{asset_manager_id}' expected not to be deleted in Asset Manager") - end keys = new_attributes.keys unless attributes.slice(*keys) == new_attributes.slice(*keys) diff --git a/test/unit/app/services/asset_manager/asset_updater_test.rb b/test/unit/app/services/asset_manager/asset_updater_test.rb index 398ce4309d7..d694870aa17 100644 --- a/test/unit/app/services/asset_manager/asset_updater_test.rb +++ b/test/unit/app/services/asset_manager/asset_updater_test.rb @@ -20,16 +20,6 @@ class AssetManager::AssetUpdaterTest < ActiveSupport::TestCase @asset_updater.call(@asset_manager_id, { "auth_bypass_ids" => [] }) end - test "warns if asset has been deleted in asset manager and attachment_data isn't deleted" do - @asset_updater.stubs(:find_asset_by_id).with(@asset_manager_id) - .returns("id" => @asset_url, "deleted" => true) - @attachment_data.stubs(:deleted?).returns(false) - - Logger.any_instance.stubs(:warn).with("Asset with asset_manager_id: '#{@asset_manager_id}' expected not to be deleted in Asset Manager").once - - @asset_updater.call(@asset_manager_id, { "draft" => false }) - end - test "does not update asset if no attributes are supplied" do assert_raises(AssetManager::AssetUpdater::AssetAttributesEmpty) do @asset_updater.call(@asset_manager_id, {}) From e1e6724ea3d826e7ce84c72a2312e9329fd9c0c8 Mon Sep 17 00:00:00 2001 From: Ryan Brown Date: Tue, 21 Jan 2025 11:57:56 +0000 Subject: [PATCH 5/7] Only update attachments that have never been published before when publishing and include parent document urls We want to ensure that the draft value for each asset is set to false when the attachable is published, but Asset Manager does not support updates to assets that are already deleted and not drafts. There's also no real need to update any asset that is not a draft in Asset Manager when publishing. The only time an update might be required is after editing a slug in Whitehall, but that should be handled following the slug editing operation (however it isn't currently). Note that we may still need to delete assets that are not drafts, so we always attempt to delete any assets where attachment_data.deleted? is true. The only way we can think of identifying which attachment data objects need publishing is to check if the attachment data has a single publicly visible attachable, which would imply that the attachment data was created during the editing of the just published edition. The previous test was inaccurate in its setup in that it assumed the PublishAttachmentAssetJob would operate with a draft attachable, but in reality it is only enqueued after the state of the attachable has been set to published in the database. We also include the parent document URL for every asset, to ensure that updating a deleted asset for an attachable that has just been published sets a public Asset Manager URL instead of the draft-origin URL. Asset Manager validates that non-draft assets do not have a draft-origin URL. --- app/models/attachment_data.rb | 5 ++ app/sidekiq/publish_attachment_asset_job.rb | 13 ++--- .../attachment_deletion_integration_test.rb | 18 +++--- .../publish_attachment_asset_job_test.rb | 57 +++++++++++++------ 4 files changed, 59 insertions(+), 34 deletions(-) diff --git a/app/models/attachment_data.rb b/app/models/attachment_data.rb index 350c366de69..50d6219aade 100644 --- a/app/models/attachment_data.rb +++ b/app/models/attachment_data.rb @@ -95,6 +95,10 @@ def draft? !significant_attachable.publicly_visible? end + def needs_publishing? + attachments.size == 1 && attachments.first.attachable.publicly_visible? + end + delegate :accessible_to?, to: :significant_attachable delegate :access_limited?, to: :last_attachable @@ -125,6 +129,7 @@ def visible_attachable_for(user) def visible_edition_for(user) visible_attachable = visible_attachable_for(user) + # below code seems wrong, policy group is not a edition but could be visible visible_attachable.is_a?(Edition) ? visible_attachable : nil end diff --git a/app/sidekiq/publish_attachment_asset_job.rb b/app/sidekiq/publish_attachment_asset_job.rb index e86c5c2b6c9..4a22dfa7e71 100644 --- a/app/sidekiq/publish_attachment_asset_job.rb +++ b/app/sidekiq/publish_attachment_asset_job.rb @@ -8,18 +8,15 @@ def perform(attachment_data_id) "draft" => false, } - unless attachment_data.replaced? - asset_attributes.merge!({ "parent_document_url" => attachment_data.attachable_url }) + if attachment_data.last_attachable.respond_to?(:public_url) + asset_attributes.merge!({ "parent_document_url" => attachment_data.last_attachable.public_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. + # We need to delete the asset first, because if we delete it after updating the draft value to false, and the delete request fails, + # the asset will remain live. 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) + AssetManager::AssetUpdater.call(asset.asset_manager_id, asset_attributes) if attachment_data.needs_publishing? end end end diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index 312e5b230e6..6a4494bcfe5 100644 --- a/test/integration/attachment_deletion_integration_test.rb +++ b/test/integration/attachment_deletion_integration_test.rb @@ -25,8 +25,8 @@ class AttachmentDeletionIntegrationTest < 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"]]) - stub_asset(first_asset_id) - stub_asset(second_asset_id) + stub_asset(first_asset_id, { "draft" => true, "parent_document_url" => edition.public_url(draft: true) }) + stub_asset(second_asset_id, { "draft" => true, "parent_document_url" => edition.public_url(draft: true) }) edition.attachments << [first_attachment, second_attachment] edition.save! @@ -45,8 +45,8 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest assert_text "Attachment deleted" Services.asset_manager.expects(:delete_asset).once.with(first_asset_id) - 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 })) + Services.asset_manager.expects(:update_asset).once.with(first_asset_id, has_entries({ "draft" => false, "parent_document_url" => edition.public_url(draft: false) })) + Services.asset_manager.expects(:update_asset).once.with(second_asset_id, has_entries({ "draft" => false, "parent_document_url" => edition.public_url(draft: false) })) visit admin_news_article_path(edition) click_link "Force publish" @@ -89,7 +89,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest stub_publishing_api_expanded_links_with_taxons(latest_attachable.content_id, []) stub_publishing_api_links_with_taxons(latest_attachable.content_id, [topic_taxon["content_id"]]) - stub_asset(original_asset_manager_id) + stub_asset(original_asset_manager_id, { "draft" => false, "parent_document_url" => latest_attachable.public_url(draft: false) }) latest_attachable.update!(minor_change: true) end @@ -103,10 +103,8 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest click_button "Delete attachment" assert_text "Attachment deleted" - 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) + Services.asset_manager.expects(:update_asset).never visit admin_news_article_path(latest_attachable) click_link "Force publish" @@ -122,7 +120,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest let(:replacement_asset_manager_id) { "replacement_asset_manager_id" } before do - stub_asset(replacement_asset_manager_id) + stub_asset(replacement_asset_manager_id, { "draft" => true, "parent_document_url" => latest_attachable.public_url(draft: true) }) replacement_data = create(:attachment_data, attachable: latest_attachable) attachment.attachment_data.replaced_by = replacement_data @@ -146,7 +144,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest assert_text "Attachment deleted" 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 })) + Services.asset_manager.expects(:update_asset).once.with(replacement_asset_manager_id, has_entries({ "draft" => false, "parent_document_url" => latest_attachable.public_url(draft: false) })) visit admin_news_article_path(latest_attachable) click_link "Force publish" diff --git a/test/unit/app/sidekiq/publish_attachment_asset_job_test.rb b/test/unit/app/sidekiq/publish_attachment_asset_job_test.rb index 124f3b34bc9..2bb4a435a13 100644 --- a/test/unit/app/sidekiq/publish_attachment_asset_job_test.rb +++ b/test/unit/app/sidekiq/publish_attachment_asset_job_test.rb @@ -4,32 +4,57 @@ 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! + context "attachment was created on the latest edition" do + let(:attachable) { create(:published_news_article, title: "news-title") } + let(:attachment_data) { create(:attachment_data, attachable:) } + let(:attachment) { create(:file_attachment, attachable:, attachment_data: attachment_data) } - 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 }) + before do + attachment_data.attachments = [attachment] + attachment_data.save! + end - worker.perform(attachment_data.id) - end + it "it deletes and updates the asset if attachment data is deleted" do + attachment.destroy! + + AssetManager::AssetDeleter.expects(:call).with(asset_manager_id) + AssetManager::AssetUpdater.expects(:call).with(asset_manager_id, { "draft" => false, "parent_document_url" => "https://www.test.gov.uk/government/news/news-title" }) + + 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 }) + it "updates the asset if attachment data is not deleted" do + AssetManager::AssetUpdater.expects(:call).with(asset_manager_id, { "draft" => false, "parent_document_url" => "https://www.test.gov.uk/government/news/news-title" }) - worker.perform(attachment_data.id) + worker.perform(attachment_data.id) + end 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:)) + context "attachment was created on the previous edition" do + let(:previous_attachable) { create(:superseded_news_article) } + let(:previous_attachment) { create(:attachment, attachable: previous_attachable, attachment_data:) } + let(:attachable) { create(:published_news_article, document: previous_attachable.document) } + let(:attachment_data) { create(:attachment_data, attachable:) } + let(:attachment) { create(:file_attachment, attachable:, attachment_data:) } + + before do + attachment_data.attachments = [previous_attachment, attachment] + attachment_data.save! + end + + it "it deletes the asset if attachment data is deleted" do + attachment.destroy! + + AssetManager::AssetDeleter.expects(:call).with(asset_manager_id) + + worker.perform(attachment_data.id) + end - AssetManager::AssetUpdater.expects(:call).with(asset_manager_id, { "draft" => false }) + it "does not update the asset" do + AssetManager::AssetUpdater.expects(:call).never worker.perform(attachment_data.id) end From 77a59a5627ab1148e5dc2070b9b9a409933005db Mon Sep 17 00:00:00 2001 From: Ryan Brown Date: Tue, 21 Jan 2025 16:42:21 +0000 Subject: [PATCH 6/7] Ensure discarding draft consultations and calls for evidence removes outcome attachment assets Until now the delete all attachments methods for these edition types was not capturing the outcome attachments. We fix this by ensuring we destroy attachments for all of the attachables associated with the edition. --- app/models/call_for_evidence.rb | 4 ++ app/models/consultation.rb | 4 ++ .../attachment_deletion_integration_test.rb | 60 +++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/app/models/call_for_evidence.rb b/app/models/call_for_evidence.rb index 0be3c27e419..09b1f168df9 100644 --- a/app/models/call_for_evidence.rb +++ b/app/models/call_for_evidence.rb @@ -60,6 +60,10 @@ def attachables [self, outcome].compact end + def delete_all_attachments + attachables.map(&:attachments).flatten.each(&:destroy) + end + def rendering_app Whitehall::RenderingApp::GOVERNMENT_FRONTEND end diff --git a/app/models/consultation.rb b/app/models/consultation.rb index 136bc402473..01fcaadf19f 100644 --- a/app/models/consultation.rb +++ b/app/models/consultation.rb @@ -70,6 +70,10 @@ def attachables [self, outcome, public_feedback].compact end + def delete_all_attachments + attachables.map(&:attachments).flatten.each(&:destroy) + end + def rendering_app Whitehall::RenderingApp::GOVERNMENT_FRONTEND end diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index 6a4494bcfe5..bc81b996b1c 100644 --- a/test/integration/attachment_deletion_integration_test.rb +++ b/test/integration/attachment_deletion_integration_test.rb @@ -158,6 +158,66 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest end end + context "given an outcome on a draft consultation" do + let(:managing_editor) { create(:managing_editor) } + let(:topic_taxon) { build(:taxon_hash) } + let(:edition) { create(:draft_consultation) } + let(:attachable) { edition.create_outcome!(FactoryBot.attributes_for(:consultation_outcome)) } + let(:attachment) { build(:csv_attachment, attachable:) } + let(:asset_id) { attachment.attachment_data.assets.first.asset_manager_id } + + 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(asset_id, { "draft" => true, "parent_document_url" => nil }) + + attachable.attachments << [attachment] + attachable.save! + end + + it "deletes the corresponding asset in Asset Manager when the edition is published and updates its draft setting to false" do + Services.asset_manager.expects(:delete_asset).never + + visit admin_consultation_path(edition) + click_link "Edit draft" + click_link "Final outcome" + within page.find("li", text: attachment.title) do + click_link "Delete attachment" + end + click_button "Delete attachment" + assert_text "Attachment deleted" + + Services.asset_manager.expects(:delete_asset).once.with(asset_id) + Services.asset_manager.expects(:update_asset).once.with(asset_id, has_entries({ "draft" => false })) + + visit admin_consultation_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" + + PublishAttachmentAssetJob.drain + end + + context "when draft consultation is discarded" do + it "deletes all outcome attachment assets in Asset Manager" do + Services.asset_manager.expects(:delete_asset).once.with(asset_id) + + visit admin_consultation_path(edition) + click_link "Delete draft" + click_button "Delete" + + DeleteAttachmentAssetJob.drain + end + end + end + private def setup_publishing_api_for(edition) From ab930d7105d881a948aa8cb281e7972a631a378a Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Tue, 21 Jan 2025 17:50:11 +0000 Subject: [PATCH 7/7] Enqueue job to delete assets when destroying a policy group attachment Previously, when deleting a policy group attachment, we called the `AttachmentUpdater` - this enqueued the `MetadataWorker`, which dealt with both deletion and updates. We have since separated the update and delete logic for editions. We are now explicit about the fact that the delete request only gets sent to Asset Manager on publish. This logic is captured in the `AttachmentAssetDeleter`. Nonetheless, since Policy Groups are not editionable, they do not use that workflow - their attachments go live upon creation and should be removed from live immediately upon being destroyed. This commit ensures that assets belonging to a policy group's deleted attachments are deleted from Asset Manager on update. --- app/models/policy_group.rb | 9 ++++++ .../attachment_deletion_integration_test.rb | 31 +++++++++++++++++++ test/unit/app/models/policy_group_test.rb | 30 ++++++++++++++++++ 3 files changed, 70 insertions(+) diff --git a/app/models/policy_group.rb b/app/models/policy_group.rb index 3bec4e943e5..984c1ae11a0 100644 --- a/app/models/policy_group.rb +++ b/app/models/policy_group.rb @@ -15,6 +15,7 @@ class PolicyGroup < ApplicationRecord after_create :extract_dependencies after_update :extract_dependencies after_destroy :remove_all_dependencies + after_update :publish_deleted_attachments def extract_dependencies remove_all_dependencies @@ -28,6 +29,14 @@ def extract_dependencies end end + def publish_deleted_attachments + deleted_attachments.each do |attachment| + next unless attachment.attachment_data.present? && attachment.deleted? + + DeleteAttachmentAssetJob.perform_async(attachment.attachment_data.id) + end + end + def remove_all_dependencies policy_group_dependencies.delete_all end diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index bc81b996b1c..0e3e9503760 100644 --- a/test/integration/attachment_deletion_integration_test.rb +++ b/test/integration/attachment_deletion_integration_test.rb @@ -218,6 +218,37 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest end end + context "given a policy group" do + let(:managing_editor) { create(:managing_editor) } + let(:attachable) { create(:policy_group) } + let(:attachment) { build(:file_attachment, attachable:) } + let(:asset_manager_id) { 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 }) + + attachable.attachments << [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 + + visit admin_policy_group_attachments_path(attachable) + within page.find("li", text: attachment.title) do + click_link "Delete attachment" + end + click_button "Delete attachment" + assert_text "Attachment deleted" + click_link "Group" + click_button "Save" + + DeleteAttachmentAssetJob.drain + end + end + private def setup_publishing_api_for(edition) diff --git a/test/unit/app/models/policy_group_test.rb b/test/unit/app/models/policy_group_test.rb index a92970c7373..213b6ddaf76 100644 --- a/test/unit/app/models/policy_group_test.rb +++ b/test/unit/app/models/policy_group_test.rb @@ -108,4 +108,34 @@ class PolicyGroupTest < ActiveSupport::TestCase assert_empty policy_group.depended_upon_contacts end + + test "deletes assets belonging to deleted attachments on save" do + first_attachment = create(:file_attachment, ordering: 1) + second_attachment = create(:file_attachment, ordering: 2) + policy_group = FactoryBot.create( + :policy_group, + attachments: [first_attachment, second_attachment], + ) + first_attachment.destroy! + second_attachment.destroy! + + DeleteAttachmentAssetJob.expects(:perform_async).with(first_attachment.attachment_data.id).once + DeleteAttachmentAssetJob.expects(:perform_async).with(second_attachment.attachment_data.id).once + + policy_group.save! + end + + test "does not delete assets belonging to not deleted attachments on save" do + first_attachment = create(:file_attachment, ordering: 1) + second_attachment = create(:file_attachment, ordering: 2) + policy_group = FactoryBot.create( + :policy_group, + attachments: [first_attachment, second_attachment], + ) + + DeleteAttachmentAssetJob.expects(:perform_async).with(first_attachment.attachment_data.id).never + DeleteAttachmentAssetJob.expects(:perform_async).with(second_attachment.attachment_data.id).never + + policy_group.save! + end end