From eaa9ce15929bb5ec0503bf9129abf44dc78b26cb Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Mon, 13 Jan 2025 16:41:17 +0000 Subject: [PATCH] Ensure asset update happens at least once The `CreateAssetWorker` relies on the `DraftUpdaterWorker` successfully running in order to trigger a call to the `MetadataWorker` for editionable attachables. This happens due to the `AttachmentUpdater` service listening on the edition `draft_update` event. However, if the edition is not valid, e.g. the attachment has been updated/replaced before saving the edition with a change note, then the `DraftUpdaterWorker` will not run, and the MetadataWorker will not be triggered. This means the state of the asset in Asset Manager will not be updated. There are additional attempts that try to achieve the same result, that also fail: - The `save_attachment` method in the `AttachmentsController` calls the `draft_updater`, which again fails, as above, because the edition is invalid, causing the `MetadataWorker` to not be enqueued. - Although the `attachment_updater` in the `AttachmentsController` also triggers the MetadataWorker, it typically fails because the assets are not yet ready. There is currently no retry mechanism in place for this worker. The current change ensures that the `MetadataWorker` runs after the asset has been created irrespective of whether or not the edition is valid. This ensures that we send an update to Asset Manager. --- .../asset_manager_create_asset_worker.rb | 12 ++-- test/integration/asset_manager_test.rb | 1 + ...attachment_replacement_integration_test.rb | 69 ++++++++++++++++++- .../asset_manager_create_asset_worker_test.rb | 13 ++-- 4 files changed, 83 insertions(+), 12 deletions(-) diff --git a/app/sidekiq/asset_manager_create_asset_worker.rb b/app/sidekiq/asset_manager_create_asset_worker.rb index c560997f494..21361cdaadd 100644 --- a/app/sidekiq/asset_manager_create_asset_worker.rb +++ b/app/sidekiq/asset_manager_create_asset_worker.rb @@ -42,14 +42,16 @@ def create_asset(asset_options, asset_variant, assetable_id, assetable_type) end def enqueue_downstream_service_updates(assetable_id, assetable_type, attachable_model_class, attachable_model_id) - assetable = assetable_type.constantize.find(assetable_id) - assetable.republish_on_assets_ready if assetable.respond_to? :republish_on_assets_ready - if attachable_model_class if attachable_model_class.constantize.ancestors.include?(Edition) PublishingApiDraftUpdateWorker.perform_async(attachable_model_class, attachable_model_id) - else - AssetManagerAttachmentMetadataWorker.perform_async(assetable_id) + end + + AssetManagerAttachmentMetadataWorker.perform_async(assetable_id) + else + assetable = assetable_type.constantize.find(assetable_id) + if assetable.respond_to?(:republish_on_assets_ready) + assetable.republish_on_assets_ready end end end diff --git a/test/integration/asset_manager_test.rb b/test/integration/asset_manager_test.rb index 212156727a4..4b2db6f3d54 100644 --- a/test/integration/asset_manager_test.rb +++ b/test/integration/asset_manager_test.rb @@ -9,6 +9,7 @@ class CreatingAFileAttachment < ActiveSupport::TestCase @edition = create(:draft_publication) @attachment = FactoryBot.build(:file_attachment_with_no_assets, file: file_fixture(@filename), attachable: @edition) @asset_manager_response = { "id" => "http://asset-manager/assets/asset_manager_id", "name" => @filename } + Services.asset_manager.stubs(:asset).returns(@asset_manager_response) end test "sends the attachment to Asset Manager" do diff --git a/test/integration/attachment_replacement_integration_test.rb b/test/integration/attachment_replacement_integration_test.rb index fe2d6aabd0d..0b5307da350 100644 --- a/test/integration/attachment_replacement_integration_test.rb +++ b/test/integration/attachment_replacement_integration_test.rb @@ -10,9 +10,11 @@ class AttachmentReplacementIntegrationTest < ActionDispatch::IntegrationTest describe "attachment replacement" do let(:managing_editor) { create(:managing_editor) } let(:filename) { "sample.csv" } - let(:asset_manager_id) { "asset_manager_id" } let(:replacement_filename) { "sample.rtf" } + let(:double_replacement_filename) { "sample.docx" } + let(:asset_manager_id) { "asset_manager_id" } let(:replacement_asset_manager_id) { "replacement-asset-id" } + let(:double_replacement_asset_manager_id) { "double-replacement-asset-id" } let(:variant) { Asset.variants[:original] } let(:attachment) { build(:csv_attachment, title: "attachment-title", attachable: edition) } @@ -92,6 +94,71 @@ class AttachmentReplacementIntegrationTest < ActionDispatch::IntegrationTest AssetManagerAttachmentMetadataWorker.drain end end + + context "when new draft is created and attachment is replaced twice" do + before do + [ + { id: replacement_asset_manager_id, filename: replacement_filename }, + { id: double_replacement_asset_manager_id, filename: double_replacement_filename }, + ].each do |item| + Services.asset_manager.expects(:create_asset).with { |args| + args[:file].path =~ /#{item[:filename]}/ + }.returns("id" => "http://asset-manager/assets/#{item[:id]}", "name" => item[:filename]) + end + stub_asset(double_replacement_asset_manager_id) + + Sidekiq::Job.clear_all + end + + it "without pre-saving the edition - updates draft & replacement for asset in Asset Manager" do + visit admin_news_article_path(edition) + click_button "Create new edition" + + AssetManagerAttachmentMetadataWorker.drain + AssetManagerCreateAssetWorker.drain + PublishingApiDraftUpdateWorker.drain + AssetManagerAttachmentMetadataWorker.drain + + Services.asset_manager.expects(:update_asset) + .with(asset_manager_id, { "replacement_id" => replacement_asset_manager_id }) + .once + Services.asset_manager.expects(:update_asset) + .with(replacement_asset_manager_id, has_entry({ "draft" => true })) + .once + + click_link "Attachments 1" + within page.find("li", text: filename) do + click_link "Edit attachment" + end + attach_file "Replace file", path_to_attachment(replacement_filename) + click_button "Save" + assert_text "Attachment 'attachment-title' updated" + + AssetManagerAttachmentMetadataWorker.drain + AssetManagerCreateAssetWorker.drain + PublishingApiDraftUpdateWorker.drain + AssetManagerAttachmentMetadataWorker.drain + + Services.asset_manager.expects(:update_asset) + .with(replacement_asset_manager_id, { "replacement_id" => double_replacement_asset_manager_id }) + .once + Services.asset_manager.expects(:update_asset) + .with(double_replacement_asset_manager_id, has_entry({ "draft" => true })) + .once + + within page.find("li", text: replacement_filename) do + click_link "Edit attachment" + end + attach_file "Replace file", path_to_attachment(double_replacement_filename) + click_button "Save" + assert_text "Attachment 'attachment-title' updated" + + AssetManagerAttachmentMetadataWorker.drain + AssetManagerCreateAssetWorker.drain + PublishingApiDraftUpdateWorker.drain + AssetManagerAttachmentMetadataWorker.drain + end + end end private diff --git a/test/unit/app/sidekiq/asset_manager_create_asset_worker_test.rb b/test/unit/app/sidekiq/asset_manager_create_asset_worker_test.rb index c3aa588845b..679f9c51490 100644 --- a/test/unit/app/sidekiq/asset_manager_create_asset_worker_test.rb +++ b/test/unit/app/sidekiq/asset_manager_create_asset_worker_test.rb @@ -6,7 +6,8 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase @worker = AssetManagerCreateAssetWorker.new @asset_manager_id = "asset_manager_id" @organisation = FactoryBot.create(:organisation) - @model_without_assets = FactoryBot.create(:attachment_data_with_no_assets, attachable: create(:draft_publication)) + @attachable = create(:draft_publication) + @model_without_assets = FactoryBot.create(:attachment_data_with_no_assets, attachable: @attachable) @asset_manager_response = { "id" => "http://asset-manager/assets/#{@asset_manager_id}", "name" => File.basename(@file), @@ -23,26 +24,26 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase args[:file].path == @file.path }.returns(@asset_manager_response) - @worker.perform(@file.path, @asset_params) + @worker.perform(@file.path, @asset_params, false, @attachable.class.to_s, @attachable.id) end test "marks the asset as draft if instructed" do Services.asset_manager.expects(:create_asset).with(has_entry(draft: true)).returns(@asset_manager_response) - @worker.perform(@file.path, @asset_params, true) + @worker.perform(@file.path, @asset_params, true, @attachable.class.to_s, @attachable.id) end test "removes the local temp file after the file has been successfully uploaded" do Services.asset_manager.stubs(:create_asset).returns(@asset_manager_response) - @worker.perform(@file.path, @asset_params) + @worker.perform(@file.path, @asset_params, false, @attachable.class.to_s, @attachable.id) assert_not File.exist?(@file.path) end test "removes the local temp directory after the file has been successfully uploaded" do Services.asset_manager.stubs(:create_asset).returns(@asset_manager_response) - @worker.perform(@file.path, @asset_params) + @worker.perform(@file.path, @asset_params, false, @attachable.class.to_s, @attachable.id) assert_not Dir.exist?(File.dirname(@file)) end @@ -100,7 +101,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase test "stores corresponding asset_manager_id and filename for current file attachment" do Services.asset_manager.stubs(:create_asset).returns(@asset_manager_response) - @worker.perform(@file.path, @asset_params) + @worker.perform(@file.path, @asset_params, false, @attachable.class.to_s, @attachable.id) assert_equal 1, Asset.where(asset_manager_id: @asset_manager_id, variant: Asset.variants[:original], filename: File.basename(@file)).count end