Refactor variable naming in AssetManagerCreateAssetWorkerTest

Co-authored-by: Syed Ali <[email protected]> Co-authored-by: Laura Ghiorghisor <[email protected]> Co-authored-by: Iris Lau <[email protected]>
alphagov · Oct 19, 2023 · 2a16048 · 2a16048
1 parent 1579fdc
commit 2a16048
Showing 1 changed file with 24 additions and 37 deletions.
diff --git a/test/unit/app/workers/asset_manager_create_asset_worker_test.rb b/test/unit/app/workers/asset_manager_create_asset_worker_test.rb
@@ -6,41 +6,43 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
     @worker = AssetManagerCreateAssetWorker.new
     @asset_manager_id = "asset_manager_id"
     @organisation = FactoryBot.create(:organisation)
-    @model = FactoryBot.create(:attachment_data)
     @model_without_assets = FactoryBot.create(:attachment_data_with_no_assets)
-    @asset_args_without_assets = { assetable_id: @model_without_assets.id, asset_variant: Asset.variants[:original], assetable_type: @model_without_assets.class.to_s }.deep_stringify_keys
     @asset_manager_response = {
       "id" => "http://asset-manager/assets/#{@asset_manager_id}",
       "name" => File.basename(@file),
     }
-    @asset_args = { assetable_id: @model.id, asset_variant: Asset.variants[:original], assetable_type: @model.class.to_s }.deep_stringify_keys
+    @asset_params = {
+      assetable_id: @model_without_assets.id,
+      asset_variant: Asset.variants[:original],
+      assetable_type: @model_without_assets.class.to_s,
+    }.deep_stringify_keys
   end
 
-  test "upload an asset using a file object at the correct path" do
+  test "uploads an asset using a file object at the correct path" do
     Services.asset_manager.expects(:create_asset).with { |args|
       args[:file].path == @file.path
     }.returns(@asset_manager_response)
 
-    @worker.perform(@file.path, @asset_args_without_assets)
+    @worker.perform(@file.path, @asset_params)
   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_args_without_assets, true)
+    @worker.perform(@file.path, @asset_params, true)
   end
 
   test "removes the file after it has been successfully uploaded" do
     Services.asset_manager.stubs(:create_asset).returns(@asset_manager_response)
 
-    @worker.perform(@file.path, @asset_args)
+    @worker.perform(@file.path, @asset_params)
     assert_not File.exist?(@file.path)
   end
 
   test "removes the directory after it has been successfully uploaded" do
     Services.asset_manager.stubs(:create_asset).returns(@asset_manager_response)
 
-    @worker.perform(@file.path, @asset_args)
+    @worker.perform(@file.path, @asset_params)
     assert_not Dir.exist?(File.dirname(@file))
   end
 
@@ -51,7 +53,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     Services.asset_manager.expects(:create_asset).with(has_entry(access_limited_organisation_ids: [@organisation.content_id])).returns(@asset_manager_response)
 
-    @worker.perform(@file.path, @asset_args_without_assets, true, consultation.class.to_s, consultation.id)
+    @worker.perform(@file.path, @asset_params, true, consultation.class.to_s, consultation.id)
   end
 
   test "marks attachments belonging to consultation responses as access limited" do
@@ -62,7 +64,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     Services.asset_manager.expects(:create_asset).with(has_entry(access_limited_organisation_ids: [@organisation.content_id])).returns(@asset_manager_response)
 
-    @worker.perform(@file.path, @asset_args_without_assets, true, consultation.class.to_s, consultation.id)
+    @worker.perform(@file.path, @asset_params, true, consultation.class.to_s, consultation.id)
   end
 
   test "does not mark attachments belonging to policy groups as access limited" do
@@ -72,7 +74,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     Services.asset_manager.expects(:create_asset).with(Not(has_key(:access_limited))).returns(@asset_manager_response)
 
-    @worker.perform(@file.path, @asset_args_without_assets, true, policy_group.class.to_s, policy_group.id)
+    @worker.perform(@file.path, @asset_params, true, policy_group.class.to_s, policy_group.id)
   end
 
   test "sends auth bypass ids to asset manager when these are passed through in the params" do
@@ -83,7 +85,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     Services.asset_manager.expects(:create_asset).with(has_entry(auth_bypass_ids: [consultation.auth_bypass_id])).returns(@asset_manager_response)
 
-    @worker.perform(@file.path, @asset_args_without_assets, true, consultation.class.to_s, consultation.id, [consultation.auth_bypass_id])
+    @worker.perform(@file.path, @asset_params, true, consultation.class.to_s, consultation.id, [consultation.auth_bypass_id])
   end
 
   test "doesn't run if the file is missing (e.g. job ran twice)" do
@@ -92,18 +94,18 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     Services.asset_manager.expects(:create_asset).never
 
-    @worker.perform(path, @asset_args)
+    @worker.perform(path, @asset_params)
   end
 
   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_args_without_assets)
+    @worker.perform(@file.path, @asset_params)
 
     assert_equal 1, Asset.where(asset_manager_id: @asset_manager_id, variant: Asset.variants[:original], filename: File.basename(@file)).count
   end
 
-  test "triggers an update to asset-manager" do
+  test "triggers an update to asset-manager for any edition based document-type" do
     # This happens via the edition services coordinator,
     # for the "update_draft" event triggered by the call to draft_updater.
 
@@ -112,7 +114,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     ServiceListeners::AttachmentUpdater.expects(:call).with(attachable: consultation).once
 
-    @worker.perform(@file.path, @asset_args, true, consultation.class.to_s, consultation.id)
+    @worker.perform(@file.path, @asset_params, true, consultation.class.to_s, consultation.id)
 
     PublishingApiDraftUpdateWorker.drain
   end
@@ -122,9 +124,9 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     Services.asset_manager.stubs(:create_asset).returns(@asset_manager_response)
 
-    AssetManagerAttachmentMetadataWorker.expects(:perform_async).with(@model.id).once
+    AssetManagerAttachmentMetadataWorker.expects(:perform_async).with(@model_without_assets.id).once
 
-    @worker.perform(@file.path, @asset_args, true, policy_group.class.to_s, policy_group.id)
+    @worker.perform(@file.path, @asset_params, true, policy_group.class.to_s, policy_group.id)
   end
 
   test "triggers an update to publishing api after asset has been saved" do
@@ -133,7 +135,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     PublishingApiDraftUpdateWorker.expects(:perform_async).with(consultation.class.to_s, consultation.id)
 
-    @worker.perform(@file.path, @asset_args, true, consultation.class.to_s, consultation.id)
+    @worker.perform(@file.path, @asset_params, true, consultation.class.to_s, consultation.id)
   end
 
   test "does not trigger an update to publishing api if attachable is not an edition" do
@@ -142,7 +144,7 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
 
     Services.publishing_api.stubs(:put_content).never
 
-    @worker.perform(@file.path, @asset_args, true, consultation_outcome.class.to_s, consultation_outcome.id)
+    @worker.perform(@file.path, @asset_params, true, consultation_outcome.class.to_s, consultation_outcome.id)
   end
 
   test "updates existing asset of same variant if it already exists - for Organisations" do
@@ -167,29 +169,14 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase
     assert_equal new_asset_manager_id, assets.first.asset_manager_id
   end
 
-  test "generates missing assets on retry" do
-    consultation = FactoryBot.create(:consultation)
-    Services.asset_manager.stubs(:create_asset).returns(@asset_manager_response)
-    @model = create(:attachment_data_with_no_assets)
-    @model.assets << build(:asset, assetable_id: @model.id, assetable_type: @model.class.to_s)
-    @model.save!
-    thumbnail_asset_args = { assetable_id: @model.id, asset_variant: Asset.variants[:thumbnail], assetable_type: @model.class.to_s }.deep_stringify_keys
-
-    assert_difference "Asset.where(assetable_id: @model.id).count", 1 do
-      @worker.perform(@file.path, thumbnail_asset_args, true, consultation.class.to_s, consultation.id)
-    end
-  end
-
   test "does not run if assetable (ImageData) has been deleted" do
     consultation = FactoryBot.create(:consultation)
-    @model = FactoryBot.create(:image_data)
-    @model.class.find(@model.id).delete
-    asset_args = { assetable_id: @model.id, asset_variant: Asset.variants[:original], assetable_type: @model.class.to_s }.deep_stringify_keys
+    @model_without_assets.delete
 
     Services.asset_manager.expects(:create_asset).never
     Services.publishing_api.expects(:put_content).never
     Sidekiq.logger.expects(:info).once
 
-    @worker.perform(@file.path, asset_args, true, consultation.class.to_s, consultation.id)
+    @worker.perform(@file.path, @asset_params, true, consultation.class.to_s, consultation.id)
   end
 end