Merge pull request #9682 from alphagov/remove-thumbnails

Remove thumbnail generation code

app/helpers/attachments_helper.rb

@@ -42,9 +42,7 @@ def attachment_component_params(attachment, alternative_format_contact_email: ni
       params[:file_size] = attachment.file_size
     end
 
-    # PDF attachments used to have a thumbnail, now just a generic icon.
     if attachment.pdf?
-      params[:thumbnail_url] = attachment.file.thumbnail.url
       params[:number_of_pages] = attachment.number_of_pages
     end
 

app/models/asset.rb

@@ -18,7 +18,6 @@ def unique_variant_for_each_assetable
 
   enum variant: {
     original: "original".freeze,
-    thumbnail: "thumbnail".freeze,
   }.merge(
     Whitehall.image_kinds.values
              .flat_map(&:version_names)

app/models/attachment_data.rb

@@ -60,10 +60,9 @@ def indexable?
   end
 
   def all_asset_variants_uploaded?
-    asset_variants = assets.map(&:variant).map(&:to_sym)
-    required_variants = pdf? ? AttachmentUploader.versions.keys.push(:original) : [Asset.variants[:original].to_sym]
+    asset_variants = assets.map(&:variant).compact.map(&:to_sym)
 
-    (required_variants - asset_variants).empty?
+    (%i[original] - asset_variants).empty?
   end
 
   def update_file_attributes

app/models/concerns/attachable.rb

@@ -143,18 +143,6 @@ def allows_attachment_type?(type)
     end
   end
 
-  def has_thumbnail?
-    thumbnailable_attachments.any?
-  end
-
-  def thumbnail_url
-    thumbnailable_attachments.first.url(:thumbnail)
-  end
-
-  def thumbnailable_attachments
-    attachments.select { |a| a.content_type == AttachmentUploader::PDF_CONTENT_TYPE }
-  end
-
   def has_official_document?
     has_command_paper? || has_act_paper?
   end

app/uploaders/attachment_uploader.rb

@@ -1,9 +1,6 @@
 class AttachmentUploader < WhitehallUploader
   PDF_CONTENT_TYPE = "application/pdf".freeze
   INDEXABLE_TYPES = %w[csv doc docx ods odp odt pdf ppt pptx rdf rtf txt xls xlsx xml].freeze
-
-  THUMBNAIL_GENERATION_TIMEOUT = 10.seconds
-  FALLBACK_PDF_THUMBNAIL = File.expand_path("../assets/images/pub-cover.png", __dir__)
   EXTENSION_ALLOW_LIST = %w[chm csv diff doc docx dot dxf eps gif gml ics jpg kml odp ods odt pdf png ppt pptx ps rdf ris rtf sch txt vcf wsdl xls xlsm xlsx xlt xml xsd xslt zip].freeze
 
   before :cache, :validate_zipfile_contents!
@@ -21,27 +18,6 @@ def set_content_type
     file.content_type = content_type
   end
 
-  version :thumbnail, if: :pdf? do
-    def full_filename(for_file)
-      "#{super}.png"
-    end
-
-    def full_original_filename
-      "#{super}.png"
-    end
-
-    process :generate_thumbnail
-    before :store, :set_correct_content_type
-
-    def set_correct_content_type(_ignore_argument)
-      @file.content_type = "image/png"
-    end
-  end
-
-  def generate_thumbnail
-    FileUtils.cp(FALLBACK_PDF_THUMBNAIL, path)
-  end
-
   def pdf?(file)
     file.content_type == PDF_CONTENT_TYPE
   end

features/step_definitions/attachment_steps.rb

@@ -1,5 +1,5 @@
 When(/^the attachment has been uploaded to the asset-manager$/) do
-  expect(Attachment.last.attachment_data.assets.size).to equal 2
+  expect(Attachment.last.attachment_data.assets.size).to equal 1
 end
 
 When(/^I start editing the attachments from the .*? page$/) do

features/support/attachment_helper.rb

@@ -1,10 +1,4 @@
 module AttachmentHelper
-  def attachment_thumbnail_path
-    within record_css_selector(@attachment) do
-      find("img")[:src]
-    end
-  end
-
   def attachment_path
     within record_css_selector(@attachment) do
       find_link(@attachment_title)[:href]

test/factories/attachment_data.rb

@@ -5,15 +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)
-        attachment_data.assets << build(:asset, asset_manager_id: "asset_manager_id_thumbnail", variant: Asset.variants[:thumbnail], filename: "thumbnail_#{attachment_data.filename}.png")
-      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
@@ -29,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/functional/admin/attachments_controller_test.rb

@@ -183,7 +183,6 @@ def self.supported_attachable_types
     model_type = AttachmentData.to_s
 
     AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:original], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id]).once
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:thumbnail], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
 
     post :create, params: { edition_id: @edition.id, type: "file", attachment: }
   end
@@ -404,7 +403,6 @@ def self.supported_attachable_types
     model_type = attachment.attachment_data.class.to_s
 
     AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:original], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:thumbnail], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
 
     put :update,
         params: {
@@ -476,7 +474,7 @@ def self.supported_attachable_types
     whitepaper_attachment_data = build(:attachment_data, file: whitepaper_pdf, attachable: @edition)
 
     AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/whitepaper/), anything, anything, anything, anything, anything).never
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/greenpaper/), anything, anything, anything, anything, anything).times(2)
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/greenpaper/), anything, anything, anything, anything, anything).once
 
     post :create,
          params: {
@@ -497,7 +495,7 @@ def self.supported_attachable_types
     whitepaper_attachment_data = build(:attachment_data, file: whitepaper_pdf)
 
     AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/whitepaper/), anything, anything, anything, anything, anything).never
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/greenpaper/), anything, anything, anything, anything, anything).times(2)
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/greenpaper/), anything, anything, anything, anything, anything).once
 
     put :update,
         params: {

test/integration/asset_access_options_integration_test.rb

@@ -141,12 +141,6 @@ class AssetAccessOptionsIntegrationTest < ActionDispatch::IntegrationTest
               auth_bypass_ids: [edition.auth_bypass_id],
             ),
           ).returns(asset_manager_response)
-          Services.asset_manager.expects(:create_asset).with(
-            has_entries(
-              access_limited_organisation_ids: [organisation.content_id],
-              auth_bypass_ids: [edition.auth_bypass_id],
-            ),
-          ).returns(asset_manager_response)
 
           AssetManagerCreateAssetWorker.drain
         end
@@ -262,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,
       )
@@ -272,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,11 +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_thumbnail) { "asset_manager_id_thumbnail" }
+      let(:second_asset_id) { second_attachment.attachment_data.assets.first.asset_manager_id }
       let(:edition) { create(:news_article) }
 
       before do
@@ -25,8 +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_thumbnail)
+        stub_asset(second_asset_id)
 
         edition.attachments << [first_attachment, second_attachment]
         edition.save!
@@ -66,8 +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_thumbnail)
+          Services.asset_manager.expects(:delete_asset).once.with(second_asset_id)
           assert_equal AssetManagerAttachmentMetadataWorker.jobs.count, 2
 
           AssetManagerAttachmentMetadataWorker.drain
@@ -81,7 +78,6 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
       let(:latest_attachable) { earliest_attachable.reload.create_draft(managing_editor) }
       let(:attachment) { latest_attachable.attachments.first }
       let(:original_asset) { attachment.attachment_data.assets.first.asset_manager_id }
-      let(:thumbnail_asset) { attachment.attachment_data.assets.second.asset_manager_id }
 
       before do
         login_as(managing_editor)
@@ -91,7 +87,6 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
         stub_publishing_api_expanded_links_with_taxons(latest_attachable.content_id, [])
 
         stub_asset(original_asset)
-        stub_asset(thumbnail_asset)
       end
 
       it "deletes the corresponding asset in Asset Manager only when the new draft gets published" do
@@ -103,13 +98,11 @@ class AttachmentDeletionIntegrationTest < ActionDispatch::IntegrationTest
         click_button "Delete attachment"
         assert_text "Attachment deleted"
 
-        Services.asset_manager.expects(:delete_asset).never.with(thumbnail_asset)
         Services.asset_manager.expects(:delete_asset).never.with(original_asset)
 
         latest_attachable.update!(minor_change: true)
         latest_attachable.force_publish!
 
-        Services.asset_manager.expects(:delete_asset).once.with(thumbnail_asset)
         Services.asset_manager.expects(:delete_asset).once.with(original_asset)
 
         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/helpers/attachments_helper_test.rb

@@ -85,7 +85,6 @@ class AttachmentsHelperTest < ActionView::TestCase
       content_type: "application/pdf",
       filename: attachment.filename,
       file_size: attachment.file_size,
-      thumbnail_url: attachment.file.thumbnail.url,
       number_of_pages: 2,
     }
     assert_equal expect_params, attachment_component_params(attachment)