Merge pull request #9807 from alphagov/fix-multiple-replace-asset-bug_2

Ensure asset update happens at least once

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

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

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