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.

app/sidekiq/asset_manager_create_asset_worker.rb

@@ -32,7 +32,7 @@ def perform(temporary_location, asset_params, draft = false, attachable_model_cl
     FileUtils.rmdir(File.dirname(file))
   end
 
-private
+  private
 
   def create_asset(asset_options, asset_variant, assetable_id, assetable_type)
     response = asset_manager.create_asset(asset_options)
@@ -42,15 +42,15 @@ 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)
+      assetable.republish_on_assets_ready
     end
   end
 

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

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