Remove attachment_data_with_asset factory

See #9682 (comment)

> 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

@@ -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

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]
 

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

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

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
 

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)

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
 

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)

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
 

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

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

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 }

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