From 406b5b12a1fcf75d339eba3b42c086de097205aa Mon Sep 17 00:00:00 2001
From: ChrisBAshton <christopher.ashton@digital.cabinet-office.gov.uk>
Date: Fri, 6 Dec 2024 09:18:52 +0000
Subject: [PATCH] Remove `attachment_data_with_asset` factory

See https://github.com/alphagov/whitehall/pull/9682#discussion_r1872860891

> I recall that, at the time, we added a variety of factories to give us
> plenty to work with for both tests where "we only care if it has an asset,
> or not even an asset" and tests where "it would be really good to know
> that update fires twice". Now that we only have the one variant you could
> perhaps get rid of the doc factory and the attachment_data_with_asset and
> replace it with pdf. csv may have its own use cases so we should probably
> keep.

While here, I  tidied up some test file `let` declarations that were unused
(following the approach of "if it's only used once, define it
inline rather than as a `let`").

I've update the attachment_data factory to not include "_original" as a
suffix on the `asset_manager_id`, as it is no longer the case that file
attachments have variants per se, but only originals. That means we can
replace all usages of the previous `attachment_data_with_asset` factory
with a plain `attachment_data`, and all usages of `file_attachment_with_asset`
with plain `file_attachment` - the only tweak needed being setting the correct
`asset_manager_id` in some of the tests (without suffix).
---
 test/factories/attachment_data.rb                     |  9 ---------
 test/factories/attachments.rb                         | 11 -----------
 .../asset_access_options_integration_test.rb          |  8 ++++----
 .../attachment_deletion_integration_test.rb           | 10 +++++-----
 .../attachment_draft_status_integration_test.rb       |  4 +---
 test/integration/attachment_link_header_test.rb       |  9 +--------
 ...t_redirect_due_to_unpublishing_integration_test.rb |  6 +-----
 .../attachment_replacement_integration_test.rb        |  4 ++--
 .../app/models/attachment_data_visibility_test.rb     |  2 +-
 .../services/asset_manager/attachment_deleter_test.rb |  2 +-
 .../sidekiq/asset_manager_update_asset_worker_test.rb |  4 ++--
 test/unit/lib/tasks/asset_manager_test.rb             | 10 +++++-----
 test/unit/lib/whitehall/asset_manager_storage_test.rb |  4 ++--
 13 files changed, 25 insertions(+), 58 deletions(-)

diff --git a/test/factories/attachment_data.rb b/test/factories/attachment_data.rb
index 9f81a988ca4..231c358af79 100644
--- a/test/factories/attachment_data.rb
+++ b/test/factories/attachment_data.rb
@@ -5,14 +5,6 @@
     trait(:pdf) do
       content_type { AttachmentUploader::PDF_CONTENT_TYPE }
 
-      after(:build) do |attachment_data|
-        attachment_data.assets << build(:asset, asset_manager_id: "asset_manager_id_original", variant: Asset.variants[:original], filename: attachment_data.filename)
-      end
-    end
-
-    trait(:doc) do
-      file { File.open(Rails.root.join("test/fixtures/sample.docx")) }
-
       after(:build) do |attachment_data|
         attachment_data.assets << build(:asset, asset_manager_id: "asset_manager_id", variant: Asset.variants[:original], filename: attachment_data.filename)
       end
@@ -28,7 +20,6 @@
   end
 
   factory :attachment_data, parent: :generic_attachment_data, traits: [:pdf]
-  factory :attachment_data_with_asset, parent: :generic_attachment_data, traits: [:doc]
   factory :attachment_data_for_csv, parent: :generic_attachment_data, traits: [:csv]
   factory :attachment_data_with_no_assets, parent: :generic_attachment_data
 end
diff --git a/test/factories/attachments.rb b/test/factories/attachments.rb
index 3204c1c7860..ec0e4d1f1de 100644
--- a/test/factories/attachments.rb
+++ b/test/factories/attachments.rb
@@ -19,16 +19,6 @@
       end
     end
 
-    trait(:doc) do
-      transient do
-        file { File.open(Rails.root.join("test/fixtures/sample.docx")) }
-      end
-
-      after(:build) do |attachment, evaluator|
-        attachment.attachment_data ||= build(:attachment_data_with_asset, file: evaluator.file, attachable: attachment.attachable)
-      end
-    end
-
     trait(:csv) do
       transient do
         file { File.open(Rails.root.join("test/fixtures/sample.csv")) }
@@ -47,7 +37,6 @@
   end
 
   factory :file_attachment, parent: :attachment, traits: [:pdf]
-  factory :file_attachment_with_asset, parent: :attachment, traits: [:doc]
   factory :csv_attachment, parent: :attachment, traits: [:csv]
   factory :file_attachment_with_no_assets, parent: :attachment, traits: [:with_no_assets]
 
diff --git a/test/integration/asset_access_options_integration_test.rb b/test/integration/asset_access_options_integration_test.rb
index 763a967eeb5..6ecb027fa4b 100644
--- a/test/integration/asset_access_options_integration_test.rb
+++ b/test/integration/asset_access_options_integration_test.rb
@@ -256,7 +256,7 @@ def setup_publishing_api_for(edition)
 
     def add_file_attachment_with_asset(filename, to:)
       to.attachments << FactoryBot.build(
-        :file_attachment_with_asset,
+        :file_attachment,
         title: filename,
         attachable: to,
       )
@@ -266,10 +266,10 @@ def path_to_attachment(filename)
       fixture_path.join(filename)
     end
 
-    def stub_asset(asset_manger_id, attributes = {})
-      url_id = "http://asset-manager/assets/#{asset_manger_id}"
+    def stub_asset(asset_manager_id, attributes = {})
+      url_id = "http://asset-manager/assets/#{asset_manager_id}"
       Services.asset_manager.stubs(:asset)
-              .with(asset_manger_id)
+              .with(asset_manager_id)
               .returns(attributes.merge(id: url_id).stringify_keys)
     end
   end
diff --git a/test/integration/attachment_deletion_integration_test.rb b/test/integration/attachment_deletion_integration_test.rb
index 0f6d0bf8058..29cd80e07c5 100644
--- a/test/integration/attachment_deletion_integration_test.rb
+++ b/test/integration/attachment_deletion_integration_test.rb
@@ -10,10 +10,10 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
   describe "attachment deletion" do
     context "given a draft document with multiple file attachments" do
       let(:managing_editor) { create(:managing_editor) }
-      let(:first_attachment) { build(:file_attachment_with_asset, attachable: edition, title: "first attachment") }
-      let(:first_asset_id) { "asset_manager_id" }
+      let(:first_attachment) { build(:csv_attachment, attachable: edition, title: "first attachment") }
+      let(:first_asset_id) { first_attachment.attachment_data.assets.first.asset_manager_id }
       let(:second_attachment) { build(:file_attachment, attachable: edition) }
-      let(:second_asset_id_original) { "asset_manager_id_original" }
+      let(:second_asset_id) { second_attachment.attachment_data.assets.first.asset_manager_id }
       let(:edition) { create(:news_article) }
 
       before do
@@ -24,7 +24,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
         stub_publishing_api_expanded_links_with_taxons(edition.content_id, [])
 
         stub_asset(first_asset_id)
-        stub_asset(second_asset_id_original)
+        stub_asset(second_asset_id)
 
         edition.attachments << [first_attachment, second_attachment]
         edition.save!
@@ -64,7 +64,7 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
 
         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_original)
+          Services.asset_manager.expects(:delete_asset).once.with(second_asset_id)
           assert_equal AssetManagerAttachmentMetadataWorker.jobs.count, 2
 
           AssetManagerAttachmentMetadataWorker.drain
diff --git a/test/integration/attachment_draft_status_integration_test.rb b/test/integration/attachment_draft_status_integration_test.rb
index f296242b4f0..0887a510760 100644
--- a/test/integration/attachment_draft_status_integration_test.rb
+++ b/test/integration/attachment_draft_status_integration_test.rb
@@ -11,7 +11,6 @@ class AttachmentDraftStatusIntegrationTest < ActionDispatch::IntegrationTest
     let(:filename) { "sample.docx" }
     let(:asset_manager_id) { "asset_manager_id" }
     let(:topic_taxon) { build(:taxon_hash) }
-    let(:variant) { Asset.variants[:original] }
 
     before do
       login_as create(:managing_editor)
@@ -20,12 +19,11 @@ class AttachmentDraftStatusIntegrationTest < ActionDispatch::IntegrationTest
     end
 
     context "given a file attachment" do
-      let(:attachment) { build(:file_attachment_with_asset, attachable:) }
       let(:attachable) { edition }
 
       before do
         setup_publishing_api_for(edition)
-        attachable.attachments << attachment
+        attachable.attachments << build(:file_attachment, attachable:)
         attachable.save!
       end
 
diff --git a/test/integration/attachment_link_header_test.rb b/test/integration/attachment_link_header_test.rb
index d37d014246b..322ba1ceca8 100644
--- a/test/integration/attachment_link_header_test.rb
+++ b/test/integration/attachment_link_header_test.rb
@@ -8,9 +8,7 @@ class AttachmentLinkHeaderIntegrationTest < ActionDispatch::IntegrationTest
   include TaxonomyHelper
 
   describe "attachment link header" do
-    let(:filename) { "sample.docx" }
     let(:asset_manager_id) { "asset_manager_id" }
-    let(:variant) { Asset.variants[:original] }
 
     before do
       login_as create(:managing_editor)
@@ -20,12 +18,11 @@ class AttachmentLinkHeaderIntegrationTest < ActionDispatch::IntegrationTest
     end
 
     context "given a file attachment" do
-      let(:attachment) { build(:file_attachment_with_asset, attachable:) }
       let(:attachable) { edition }
       let(:topic_taxon) { build(:taxon_hash) }
 
       before do
-        attachable.attachments << attachment
+        attachable.attachments << build(:file_attachment, attachable:)
         attachable.save!
       end
 
@@ -55,10 +52,6 @@ def setup_publishing_api_for(edition)
       stub_publishing_api_links_with_taxons(edition.content_id, [topic_taxon["content_id"]])
     end
 
-    def path_to_attachment(filename)
-      fixture_path.join(filename)
-    end
-
     def stub_asset(asset_manger_id, attributes = {})
       url_id = "http://asset-manager/assets/#{asset_manger_id}"
       Services.asset_manager.stubs(:asset)
diff --git a/test/integration/attachment_redirect_due_to_unpublishing_integration_test.rb b/test/integration/attachment_redirect_due_to_unpublishing_integration_test.rb
index 977d41385d0..63279971758 100644
--- a/test/integration/attachment_redirect_due_to_unpublishing_integration_test.rb
+++ b/test/integration/attachment_redirect_due_to_unpublishing_integration_test.rb
@@ -8,14 +8,10 @@ class AttachmentRedirectDueToUnpublishingIntegrationTest < ActionDispatch::Integ
   include TaxonomyHelper
 
   describe "attachment redirect due to unpublishing" do
-    let(:filename) { "sample.docx" }
     let(:attachable) { edition }
     let(:asset_manager_id) { "asset_manager_id" }
     let(:redirect_path) { edition.public_path }
     let(:redirect_url) { edition.public_url }
-    let(:topic_taxon) { build(:taxon_hash) }
-    let(:variant) { Asset.variants[:original] }
-    let(:attachment) { build(:file_attachment_with_asset, attachable:) }
 
     before do
       login_as create(:managing_editor)
@@ -25,7 +21,7 @@ class AttachmentRedirectDueToUnpublishingIntegrationTest < ActionDispatch::Integ
       stub_publishing_api_expanded_links_with_taxons(edition.content_id, [])
       stub_asset(asset_manager_id)
 
-      attachable.attachments << attachment
+      attachable.attachments << build(:file_attachment, attachable:)
       attachable.save!
     end
 
diff --git a/test/integration/attachment_replacement_integration_test.rb b/test/integration/attachment_replacement_integration_test.rb
index 2c265a2e920..fe2d6aabd0d 100644
--- a/test/integration/attachment_replacement_integration_test.rb
+++ b/test/integration/attachment_replacement_integration_test.rb
@@ -9,12 +9,12 @@ class AttachmentReplacementIntegrationTest < ActionDispatch::IntegrationTest
 
   describe "attachment replacement" do
     let(:managing_editor) { create(:managing_editor) }
-    let(:filename) { "sample.docx" }
+    let(:filename) { "sample.csv" }
     let(:asset_manager_id) { "asset_manager_id" }
     let(:replacement_filename) { "sample.rtf" }
     let(:replacement_asset_manager_id) { "replacement-asset-id" }
     let(:variant) { Asset.variants[:original] }
-    let(:attachment) { build(:file_attachment_with_asset, title: "attachment-title", attachable: edition) }
+    let(:attachment) { build(:csv_attachment, title: "attachment-title", attachable: edition) }
 
     before do
       login_as(managing_editor)
diff --git a/test/unit/app/models/attachment_data_visibility_test.rb b/test/unit/app/models/attachment_data_visibility_test.rb
index 1eb002ffa7d..82766a650b0 100644
--- a/test/unit/app/models/attachment_data_visibility_test.rb
+++ b/test/unit/app/models/attachment_data_visibility_test.rb
@@ -116,7 +116,7 @@ class AttachmentDataVisibilityTest < ActiveSupport::TestCase
 
         context "when attachment is replaced" do
           before do
-            replacement_attachment_data = build(:attachment_data_with_asset, to_replace_id: attachment_data.id, attachable:)
+            replacement_attachment_data = build(:attachment_data, to_replace_id: attachment_data.id, attachable:)
             attachment.update!(attachment_data: replacement_attachment_data)
           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
index a5dc291dc77..5aae8a0bbfb 100644
--- a/test/unit/app/services/asset_manager/attachment_deleter_test.rb
+++ b/test/unit/app/services/asset_manager/attachment_deleter_test.rb
@@ -34,7 +34,7 @@ class AssetManager::AttachmentDeleterTest < ActiveSupport::TestCase
       let(:deleted) { true }
 
       it "deletes attachment asset in Asset Manager" do
-        delete_worker.expects(:call).with("asset_manager_id_original")
+        delete_worker.expects(:call).with("asset_manager_id")
 
         worker.call(attachment_data)
       end
diff --git a/test/unit/app/sidekiq/asset_manager_update_asset_worker_test.rb b/test/unit/app/sidekiq/asset_manager_update_asset_worker_test.rb
index cda986dd4b7..139f45024df 100644
--- a/test/unit/app/sidekiq/asset_manager_update_asset_worker_test.rb
+++ b/test/unit/app/sidekiq/asset_manager_update_asset_worker_test.rb
@@ -8,8 +8,8 @@ class AssetManagerUpdateAssetWorkerTest < ActiveSupport::TestCase
   end
   let(:attachment_data) { FactoryBot.create(:attachment_data, attachable: create(:draft_publication)) }
 
-  test "updates an attachment and its variant" do
-    AssetManager::AssetUpdater.expects(:call).with("asset_manager_id_original", auth_bypass_id_attributes)
+  test "updates a file attachment's original asset" do
+    AssetManager::AssetUpdater.expects(:call).with("asset_manager_id", auth_bypass_id_attributes)
 
     AssetManagerUpdateAssetWorker.perform_async_in_queue("asset_manager_updater", "AttachmentData", attachment_data.id, auth_bypass_id_attributes)
     AssetManagerUpdateAssetWorker.drain
diff --git a/test/unit/lib/tasks/asset_manager_test.rb b/test/unit/lib/tasks/asset_manager_test.rb
index 43259da56a8..682afada00e 100644
--- a/test/unit/lib/tasks/asset_manager_test.rb
+++ b/test/unit/lib/tasks/asset_manager_test.rb
@@ -15,7 +15,7 @@ class AssetManagerTest < ActiveSupport::TestCase
   %w[unpublished superseded].each do |state|
     context "for an asset attached only to a #{state} edition" do
       let(:edition) { create(:"#{state}_publication") }
-      let(:attachment) { create(:file_attachment_with_asset, attachable: edition) }
+      let(:attachment) { create(:file_attachment, attachable: edition) }
 
       context "when asset is not deleted from asset manager" do
         before do
@@ -57,7 +57,7 @@ class AssetManagerTest < ActiveSupport::TestCase
 
   context "for an asset attached only to a draft edition" do
     let(:edition) { create(:draft_publication) }
-    let(:attachment) { create(:file_attachment_with_asset, attachable: edition) }
+    let(:attachment) { create(:file_attachment, attachable: edition) }
 
     context "when asset is not deleted from asset manager" do
       before do
@@ -86,7 +86,7 @@ class AssetManagerTest < ActiveSupport::TestCase
 
   context "for an asset attached only to a published edition" do
     let(:edition) { create(:published_publication) }
-    let(:attachment) { create(:file_attachment_with_asset, attachable: edition) }
+    let(:attachment) { create(:file_attachment, attachable: edition) }
 
     context "when asset is not deleted from asset manager" do
       before do
@@ -114,7 +114,7 @@ class AssetManagerTest < ActiveSupport::TestCase
   end
 
   context "for an asset attached to both published and superseded editions" do
-    let(:attachment) { create(:file_attachment_with_asset) }
+    let(:attachment) { create(:file_attachment) }
     let(:edition_1) { create(:superseded_publication, :with_alternative_format_provider, attachments: [attachment]) }
     let(:edition_2) { create(:published_publication, :with_alternative_format_provider, attachments: [attachment]) }
 
@@ -144,7 +144,7 @@ class AssetManagerTest < ActiveSupport::TestCase
   end
 
   context "for an asset attached only to something that is not an edition" do
-    let(:attachment) { create(:file_attachment_with_asset, attachable: create(:double)) }
+    let(:attachment) { create(:file_attachment, attachable: create(:double)) }
 
     test "it should include no output in the report" do
       assert_output("") { task.invoke }
diff --git a/test/unit/lib/whitehall/asset_manager_storage_test.rb b/test/unit/lib/whitehall/asset_manager_storage_test.rb
index 00987e98c9c..d0a429e4e1b 100644
--- a/test/unit/lib/whitehall/asset_manager_storage_test.rb
+++ b/test/unit/lib/whitehall/asset_manager_storage_test.rb
@@ -46,11 +46,11 @@ class Whitehall::AssetManagerStorage::FileTest < ActiveSupport::TestCase
   end
 
   test "returns file url using asset_manager_id when the model has the original asset" do
-    model = build(:attachment_data_with_asset, attachable: build(:draft_edition, id: 1))
+    model = build(:attachment_data, attachable: build(:draft_edition, id: 1))
     model.save!
     model.reload
 
-    assert_equal "http://assets-host/media/asset_manager_id/sample.docx", model.file.url
+    assert_equal "http://assets-host/media/asset_manager_id/greenpaper.pdf", model.file.url
   end
 
   test "returns file url using asset_manager_id when the model has an asset variant" do