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..97bfc9c4b80 --- /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 b9f8b55eab6..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, - # 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) 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 ae9560a2120..ad6acc4b4a3 100644 --- a/config/initializers/edition_services.rb +++ b/config/initializers/edition_services.rb @@ -1,6 +1,6 @@ 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 @@ -8,6 +8,10 @@ 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 diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb index c05562c1684..a8700250ff7 100644 --- a/test/integration/attachment_deletion_integration_test.rb +++ b/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 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..f8c30873102 --- /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(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 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