From 6cd6bebf296fd368eaee1df361484868871df0c5 Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Wed, 29 Nov 2023 11:40:43 +0000 Subject: [PATCH] Revert "Merge pull request #8537 from alphagov/feature-to-use-asset-ids" This reverts commit 4b9553d01824036c4468c3fb17371b6a44eb5a0f, reversing changes made to e27246ed42bc81ff96b3d9767bb9a4f1b0158786. --- app/controllers/admin/features_controller.rb | 6 +-- app/models/edition/lead_image.rb | 2 + app/models/feature.rb | 18 +++---- app/models/organisation.rb | 1 - app/models/person.rb | 2 +- app/models/worldwide_organisation.rb | 1 - .../featured_documents_presenter.rb | 44 ++++++++--------- .../publishing_api/speech_presenter.rb | 2 + app/views/admin/features/new.html.erb | 6 +-- test/factories/featured_image_data.rb | 14 +++--- test/factories/features.rb | 2 +- .../admin/features_controller_test.rb | 10 +--- .../admin/organisations_controller_test.rb | 4 +- .../app/models/edition/lead_image_test.rb | 8 +++ test/unit/app/models/feature_test.rb | 35 ++++++++++++- .../app/models/featured_image_data_test.rb | 46 ++--------------- .../featured_documents_presenter_test.rb | 49 +++---------------- .../news_article_presenter_test.rb | 1 - .../publishing_api/speech_presenter_test.rb | 20 +++----- 19 files changed, 107 insertions(+), 164 deletions(-) diff --git a/app/controllers/admin/features_controller.rb b/app/controllers/admin/features_controller.rb index e52755174f8..49acae4b4ce 100644 --- a/app/controllers/admin/features_controller.rb +++ b/app/controllers/admin/features_controller.rb @@ -45,11 +45,7 @@ def build_feature def feature_params params.fetch(:feature, {}).permit( - :alt_text, - :document_id, - :topical_event_id, - :offsite_link_id, - image_attributes: %i[file], + :image, :image_cache, :alt_text, :document_id, :topical_event_id, :offsite_link_id ) end diff --git a/app/models/edition/lead_image.rb b/app/models/edition/lead_image.rb index 103e949d005..f374bc9156c 100644 --- a/app/models/edition/lead_image.rb +++ b/app/models/edition/lead_image.rb @@ -34,6 +34,8 @@ def lead_image_caption end def lead_image_has_all_assets? + return true unless image_data.respond_to?(:all_asset_variants_uploaded?) + image_data.all_asset_variants_uploaded? end diff --git a/app/models/feature.rb b/app/models/feature.rb index d5d0e483e6f..3474533090c 100644 --- a/app/models/feature.rb +++ b/app/models/feature.rb @@ -4,13 +4,14 @@ class Feature < ApplicationRecord belongs_to :offsite_link belongs_to :feature_list - has_one :image, class_name: "FeaturedImageData", as: :featured_imageable, inverse_of: :featured_imageable - accepts_nested_attributes_for :image, reject_if: :all_blank + has_one :image_new, class_name: "FeaturedImageData", as: :featured_imageable, inverse_of: :featured_imageable + mount_uploader :image, FeaturedImageUploader, mount_on: :carrierwave_image validates :document, presence: true, unless: ->(feature) { feature.topical_event_id.present? || feature.offsite_link_id.present? } validates :started_at, presence: true - validate :image_is_present + validates :image, presence: true, on: :create validates :alt_text, length: { maximum: 255 } + validates_with ImageValidator, method: :image, size: [960, 640], if: :image_changed? before_validation :set_started_at!, on: :create @@ -49,20 +50,13 @@ def locale feature_list ? feature_list.locale : :en end - def republish_to_publishing_api_async - republish_featurable_to_publishing_api - if document&.editions&.select { |edition| edition.is_a?(Speech) }.present? - Whitehall::PublishingApi.republish_document_async(document) - end - end - private def set_started_at! self.started_at = Time.zone.now end - def image_is_present - errors.add(:"image.file", "can't be blank") if image.blank? + def image_changed? + changes["carrierwave_image"].present? end end diff --git a/app/models/organisation.rb b/app/models/organisation.rb index c157883cdbe..3b039132a36 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -242,7 +242,6 @@ def republish_dependent_documents .includes(:images) .where(images: { id: nil }) .map(&:document) - .uniq(&:id) documents.each { |d| Whitehall::PublishingApi.republish_document_async(d) } end diff --git a/app/models/person.rb b/app/models/person.rb index 27e7e0e5bbd..c6fdcfdf220 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -122,7 +122,7 @@ def publishing_api_presenter end def republish_dependent_documents - speeches.uniq { |speech| speech.document.id }.map { |speech| Whitehall::PublishingApi.republish_document_async(speech.document) } + speeches.map { |speech| Whitehall::PublishingApi.republish_document_async(speech.document) } historical_account&.republish_to_publishing_api_async end diff --git a/app/models/worldwide_organisation.rb b/app/models/worldwide_organisation.rb index 00c9478bf45..d75e2ef4815 100644 --- a/app/models/worldwide_organisation.rb +++ b/app/models/worldwide_organisation.rb @@ -144,7 +144,6 @@ def republish_dependent_documents .includes(:images) .where(images: { id: nil }) .map(&:document) - .uniq(&:id) documents.each { |d| Whitehall::PublishingApi.republish_document_async(d) } end end diff --git a/app/presenters/publishing_api/featured_documents_presenter.rb b/app/presenters/publishing_api/featured_documents_presenter.rb index c22cb60652e..e6c6e251868 100644 --- a/app/presenters/publishing_api/featured_documents_presenter.rb +++ b/app/presenters/publishing_api/featured_documents_presenter.rb @@ -1,18 +1,15 @@ module PublishingApi module FeaturedDocumentsPresenter def featured_documents(featurable_item, document_limit) - featurable_item - .feature_list_for_locale(I18n.locale).current.limit(document_limit) - .select { |feature| feature.image.all_asset_variants_uploaded? } - .map do |feature| - if feature.document - featured_documents_editioned(feature) - elsif feature.topical_event - featured_documents_topical_event(feature) - elsif feature.offsite_link - featured_documents_offsite_link(feature) - end + featurable_item.feature_list_for_locale(I18n.locale).current.limit(document_limit).map do |feature| + if feature.document + featured_documents_editioned(feature) + elsif feature.topical_event + featured_documents_topical_event(feature) + elsif feature.offsite_link + featured_documents_offsite_link(feature) end + end end private @@ -23,7 +20,10 @@ def featured_documents_editioned(feature) { title: edition.title, href: edition.public_path(locale: feature.feature_list.locale), - image: get_image(feature), + image: { + url: feature.image.url, + alt_text: feature.alt_text.presence || "", + }, summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(edition.summary), public_updated_at: edition.public_timestamp, document_type: edition.display_type, @@ -36,7 +36,10 @@ def featured_documents_topical_event(feature) { title: topical_event.name, href: topical_event.public_path(locale: feature.feature_list.locale), - image: get_image(feature), + image: { + url: feature.image.url, + alt_text: feature.alt_text, + }, summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(topical_event.summary), public_updated_at: topical_event.start_date, document_type: nil, # We don't want a type for topical events @@ -49,21 +52,14 @@ def featured_documents_offsite_link(feature) { title: offsite_link.title, href: offsite_link.url, - image: get_image(feature), + image: { + url: feature.image.url, + alt_text: feature.alt_text, + }, summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(offsite_link.summary), public_updated_at: offsite_link.date, document_type: offsite_link.display_type, } end - - def get_image(feature) - legacy_url_path = feature.image.file.path - { - url: URI.join(Plek.asset_root, Addressable::URI.encode(legacy_url_path)).to_s, - medium_resolution_url: feature.image.url(:s465), - high_resolution_url: feature.image.url(:s712), - alt_text: feature.alt_text.presence || "", - } - end end end diff --git a/app/presenters/publishing_api/speech_presenter.rb b/app/presenters/publishing_api/speech_presenter.rb index 468a726cbf6..01c507d8ef3 100644 --- a/app/presenters/publishing_api/speech_presenter.rb +++ b/app/presenters/publishing_api/speech_presenter.rb @@ -139,6 +139,8 @@ def has_image? end def image_has_all_assets? + return true unless image.instance_of?(FeaturedImageData) + image.all_asset_variants_uploaded? end diff --git a/app/views/admin/features/new.html.erb b/app/views/admin/features/new.html.erb index 1da1ffb0bc9..5a8d5a685f1 100644 --- a/app/views/admin/features/new.html.erb +++ b/app/views/admin/features/new.html.erb @@ -19,9 +19,9 @@ heading_size: "l", }, hint: "Select a 960px wide and 640px tall image to be shown when featuring", - name: "feature[image_attributes][file]", - id: "feature_image_file", - error_items: errors_for(@feature.errors, :"image.file"), + name: "feature[image]", + id: "feature_image", + error_items: errors_for(@feature.errors, :image), } %> <%= render "govuk_publishing_components/components/input", { diff --git a/test/factories/featured_image_data.rb b/test/factories/featured_image_data.rb index 223b77f68c0..cc7aea8e72c 100644 --- a/test/factories/featured_image_data.rb +++ b/test/factories/featured_image_data.rb @@ -4,13 +4,13 @@ featured_imageable { build(:topical_event) } after(:build) do |featured_image_data| - featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_original", variant: Asset.variants[:original], filename: featured_image_data.filename) - featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s960", variant: Asset.variants[:s960], filename: "s960_#{featured_image_data.filename}") - featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s712", variant: Asset.variants[:s712], filename: "s712_#{featured_image_data.filename}") - featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s630", variant: Asset.variants[:s630], filename: "s630_#{featured_image_data.filename}") - featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s465", variant: Asset.variants[:s465], filename: "s465_#{featured_image_data.filename}") - featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s300", variant: Asset.variants[:s300], filename: "s300_#{featured_image_data.filename}") - featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s216", variant: Asset.variants[:s216], filename: "s216_#{featured_image_data.filename}") + featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_original", variant: Asset.variants[:original], filename: "minister-of-funk.960x640.jpg") + featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s960", variant: Asset.variants[:s960], filename: "s960_minister-of-funk.960x640.jpg") + featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s712", variant: Asset.variants[:s712], filename: "s712_minister-of-funk.960x640.jpg") + featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s630", variant: Asset.variants[:s630], filename: "s630_minister-of-funk.960x640.jpg") + featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s465", variant: Asset.variants[:s465], filename: "s465_minister-of-funk.960x640.jpg") + featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s300", variant: Asset.variants[:s300], filename: "s300_minister-of-funk.960x640.jpg") + featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s216", variant: Asset.variants[:s216], filename: "s216_minister-of-funk.960x640.jpg") end end end diff --git a/test/factories/features.rb b/test/factories/features.rb index 5f19123f9ca..04b4b0564a5 100644 --- a/test/factories/features.rb +++ b/test/factories/features.rb @@ -1,7 +1,7 @@ FactoryBot.define do factory :feature do document - image { build(:featured_image_data) } + image { image_fixture_file } trait :with_topical_event_association do topical_event diff --git a/test/functional/admin/features_controller_test.rb b/test/functional/admin/features_controller_test.rb index 4688bcdb0e4..61bed4390ea 100644 --- a/test/functional/admin/features_controller_test.rb +++ b/test/functional/admin/features_controller_test.rb @@ -58,9 +58,7 @@ class Admin::FeaturesControllerTest < ActionController::TestCase params = { document_id: edition.document_id, - image_attributes: { - file: upload_fixture("images/960x640_gif.gif"), - }, + image: upload_fixture("images/960x640_gif.gif"), alt_text: "some text", } @@ -70,7 +68,6 @@ class Admin::FeaturesControllerTest < ActionController::TestCase post :create, params: { feature_list_id: feature_list.id, feature: params } assert_equal edition.document_id, Feature.last.document_id - assert_equal "960x640_gif.gif", Feature.last.image.filename end test "post :feature creates a feature and republishes the document & world location news when the featurable is an world location news" do @@ -81,9 +78,7 @@ class Admin::FeaturesControllerTest < ActionController::TestCase params = { document_id: edition.document_id, - image_attributes: { - file: upload_fixture("images/960x640_gif.gif"), - }, + image: upload_fixture("images/960x640_gif.gif"), alt_text: "some text", } @@ -93,6 +88,5 @@ class Admin::FeaturesControllerTest < ActionController::TestCase post :create, params: { feature_list_id: feature_list.id, feature: params } assert_equal edition.document_id, Feature.last.document_id - assert_equal "960x640_gif.gif", Feature.last.image.filename end end diff --git a/test/functional/admin/organisations_controller_test.rb b/test/functional/admin/organisations_controller_test.rb index 3478ec19c47..2716cf88d3b 100644 --- a/test/functional/admin/organisations_controller_test.rb +++ b/test/functional/admin/organisations_controller_test.rb @@ -356,9 +356,7 @@ def example_organisation_attributes feature_list = organisation.load_or_create_feature_list("en") feature_list.features.create!( topical_event:, - image_attributes: { - file: image_fixture_file, - }, + image: image_fixture_file, alt_text: "Image alternative text", ) diff --git a/test/unit/app/models/edition/lead_image_test.rb b/test/unit/app/models/edition/lead_image_test.rb index 1dfe27e5565..79296d904dd 100644 --- a/test/unit/app/models/edition/lead_image_test.rb +++ b/test/unit/app/models/edition/lead_image_test.rb @@ -57,4 +57,12 @@ class Edition::LeadImageTest < ActiveSupport::TestCase assert model.lead_image_has_all_assets? end + + test "#lead_image_has_all_assets? returns true if the lead image data doesn't implement all_asset_variants_uploaded?" do + image = build(:featured_image_data) + organisation = build(:organisation, default_news_image: image) + model = stub("Target", { lead_image: nil, lead_organisations: [], organisations: [organisation] }).extend(Edition::LeadImage) + + assert model.lead_image_has_all_assets? + end end diff --git a/test/unit/app/models/feature_test.rb b/test/unit/app/models/feature_test.rb index df7d5d15d5c..3ce4c530ed6 100644 --- a/test/unit/app/models/feature_test.rb +++ b/test/unit/app/models/feature_test.rb @@ -9,9 +9,18 @@ class FeatureTest < ActiveSupport::TestCase assert_not build(:feature, image: nil).valid? end + test "valid without image on update" do + # This is to work around the fact that carrierwave will consider the image invalid + # because the file will have been moved to the 'clean' folder by the virus scanner + # and therefore not in the location that it expects + feature = create(:feature) + feature.image = nil + assert feature.valid? + end + test "started_at set by default on creation" do feature = Feature.create!( - image: build(:featured_image_data), + image: image_fixture_file, feature_list: create(:feature_list), document: create(:document), alt_text: "foo", @@ -33,4 +42,28 @@ class FeatureTest < ActiveSupport::TestCase assert feature.end! assert_equal Time.zone.now, feature.reload.ended_at end + + test "rejects SVG logo uploads" do + svg_image = File.open(Rails.root.join("test/fixtures/images/test-svg.svg")) + feature = build(:feature, image: svg_image) + + assert_not feature.valid? + assert_includes feature.errors.map(&:full_message), "Image You are not allowed to upload \"svg\" files, allowed types: jpg, jpeg, gif, png" + end + + test "rejects non-image file uploads" do + non_image_file = File.open(Rails.root.join("test/fixtures/folders.zip")) + feature = build(:feature, image: non_image_file) + + assert_not feature.valid? + assert_includes feature.errors.map(&:full_message), "Image You are not allowed to upload \"zip\" files, allowed types: jpg, jpeg, gif, png" + end + + test "accepts valid image uploads" do + jpg_image = File.open(Rails.root.join("test/fixtures/big-cheese.960x640.jpg")) + feature = build(:feature, image: jpg_image) + + assert feature + assert_empty feature.errors + end end diff --git a/test/unit/app/models/featured_image_data_test.rb b/test/unit/app/models/featured_image_data_test.rb index 77693909df7..bdec3ee9b10 100644 --- a/test/unit/app/models/featured_image_data_test.rb +++ b/test/unit/app/models/featured_image_data_test.rb @@ -70,8 +70,7 @@ class FeaturedImageDataTest < ActiveSupport::TestCase test "#republish_on_assets_ready should republish organisation and associations if assets are ready" do organisation = create(:organisation, :with_default_news_image) - news_article = create(:news_article, :published, organisations: [organisation]) - create(:news_article, :draft, organisations: [organisation], document: news_article.document) + news_article = create(:news_article, organisations: [organisation]) Whitehall::PublishingApi.expects(:republish_async).with(organisation).once Whitehall::PublishingApi.expects(:republish_document_async).with(news_article.document).once @@ -81,11 +80,10 @@ class FeaturedImageDataTest < ActiveSupport::TestCase test "#republish_on_assets_ready should republish worldwide organisation and associations if assets are ready" do worldwide_organisation = create(:worldwide_organisation, :with_default_news_image) - news_article = create(:news_article_world_news_story, :published, worldwide_organisations: [worldwide_organisation]) - create(:news_article_world_news_story, :draft, worldwide_organisations: [worldwide_organisation], document: news_article.document) + news_article = create(:news_article_world_news_story, worldwide_organisations: [worldwide_organisation]) Whitehall::PublishingApi.expects(:republish_async).with(worldwide_organisation).once - Whitehall::PublishingApi.expects(:republish_document_async).with(news_article.document).once + Whitehall::PublishingApi.expects(:republish_document_async).with(news_article.document) worldwide_organisation.default_news_image.republish_on_assets_ready end @@ -100,13 +98,12 @@ class FeaturedImageDataTest < ActiveSupport::TestCase test "#republish_on_assets_ready should republish person and associations if assets are ready" do person = create(:person, :with_image) - speech = create(:published_speech, role_appointment: create(:role_appointment, role: create(:ministerial_role), person:)) - create(:draft_speech, role_appointment: create(:role_appointment, role: create(:ministerial_role), person:), document: speech.document) + speech = create(:speech, role_appointment: create(:role_appointment, role: create(:ministerial_role), person:)) create(:historical_account, person:) Whitehall::PublishingApi.expects(:republish_async).with(person).once Whitehall::PublishingApi.expects(:republish_async).with(person.historical_account).once - Whitehall::PublishingApi.expects(:republish_document_async).with(speech.document).once + Whitehall::PublishingApi.expects(:republish_document_async).with(speech.document) person.image.republish_on_assets_ready end @@ -119,39 +116,6 @@ class FeaturedImageDataTest < ActiveSupport::TestCase take_part_page.image.republish_on_assets_ready end - test "#republish_on_assets_ready should republish feature via organisation if assets are ready" do - speech1 = create(:speech) - speech2 = create(:speech) - document = create(:document, editions: [speech1, speech2]) - - organisation = create(:organisation) - feature_list = create(:feature_list, featurable: organisation, locale: :en) - feature = create(:feature, document:, feature_list:) - featurable = feature.feature_list.featurable - - Whitehall::PublishingApi.expects(:republish_async).with(featurable).once - Whitehall::PublishingApi.expects(:republish_document_async).with(document).once - - feature.image.republish_on_assets_ready - end - - test "#republish_on_assets_ready should republish feature via world location news if assets are ready" do - speech1 = create(:speech) - speech2 = create(:speech) - document = create(:document, editions: [speech1, speech2]) - - world_location = create(:world_location) - world_location_news = world_location.world_location_news - feature_list = create(:feature_list, featurable: world_location_news, locale: :en) - feature = create(:feature, document:, feature_list:) - featurable = feature.feature_list.featurable - - Whitehall::PublishingApi.expects(:republish_async).with(featurable).once - Whitehall::PublishingApi.expects(:republish_document_async).with(document).once - - feature.image.republish_on_assets_ready - end - test "#republish_on_assets_ready should not run any republishing action if assets are not ready" do person = create(:person, :with_image) person.image.assets.destroy_all diff --git a/test/unit/app/presenters/publishing_api/featured_documents_presenter_test.rb b/test/unit/app/presenters/publishing_api/featured_documents_presenter_test.rb index fc641c604c3..d38e9fb742c 100644 --- a/test/unit/app/presenters/publishing_api/featured_documents_presenter_test.rb +++ b/test/unit/app/presenters/publishing_api/featured_documents_presenter_test.rb @@ -24,18 +24,14 @@ class PublishingApi::FeaturedDocumentsPresenterTest < ActiveSupport::TestCase expected_ordered_featured_documents = [ { title: case_study.title, href: "/government/case-studies/case-study-title#{locale[:suffix]}", - image: { url: "#{Plek.asset_root}/government/uploads/system/uploads/featured_image_data/file/#{first_feature.image.id}/minister-of-funk.960x640.jpg", - medium_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s465/s465_minister-of-funk.960x640.jpg", - high_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s712/s712_minister-of-funk.960x640.jpg", + image: { url: first_feature.image.url, alt_text: "" }, summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(case_study.summary), public_updated_at: case_study.public_timestamp, document_type: I18n.t("document.type.case_study.one") }, { title: news_article.title, href: "/government/news/news-title#{locale[:suffix]}", - image: { url: "#{Plek.asset_root}/government/uploads/system/uploads/featured_image_data/file/#{second_feature.image.id}/minister-of-funk.960x640.jpg", - medium_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s465/s465_minister-of-funk.960x640.jpg", - high_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s712/s712_minister-of-funk.960x640.jpg", + image: { url: second_feature.image.url, alt_text: "" }, summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(news_article.summary), public_updated_at: news_article.public_timestamp, @@ -66,10 +62,8 @@ class PublishingApi::FeaturedDocumentsPresenterTest < ActiveSupport::TestCase expected_ordered_featured_documents = [ { title: topical_event.name, href: "/government/topical-events/topical_event_1#{locale[:suffix]}", - image: { url: "#{Plek.asset_root}/government/uploads/system/uploads/featured_image_data/file/#{feature.image.id}/minister-of-funk.960x640.jpg", - medium_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s465/s465_minister-of-funk.960x640.jpg", - high_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s712/s712_minister-of-funk.960x640.jpg", - alt_text: "" }, + image: { url: feature.image.url, + alt_text: feature.alt_text }, summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(topical_event.summary), public_updated_at: topical_event.start_date, document_type: nil }, @@ -96,10 +90,8 @@ class PublishingApi::FeaturedDocumentsPresenterTest < ActiveSupport::TestCase expected_ordered_featured_documents = [ { title: offsite_link.title, href: offsite_link.url, - image: { url: "#{Plek.asset_root}/government/uploads/system/uploads/featured_image_data/file/#{feature.image.id}/minister-of-funk.960x640.jpg", - medium_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s465/s465_minister-of-funk.960x640.jpg", - high_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s712/s712_minister-of-funk.960x640.jpg", - alt_text: "" }, + image: { url: feature.image.url, + alt_text: feature.alt_text }, summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(offsite_link.summary), public_updated_at: offsite_link.date, document_type: offsite_link.display_type }, @@ -123,33 +115,4 @@ class PublishingApi::FeaturedDocumentsPresenterTest < ActiveSupport::TestCase assert_equal [create(:published_case_study).title], (presented_locations.map { |presented_location| presented_location[:title] }) end - - test("filters out featured documents if feature image assets are missing") do - case_study = create(:published_case_study) - first_feature = build(:feature, document: case_study.document, ordering: 1) - news_article = create(:published_news_article) - second_feature = build(:feature, document: news_article.document, ordering: 2) - second_feature.image.assets = [] - featured_documents_display_limit = 5 - - world_location = create(:world_location) - - create(:feature_list, featurable: world_location.world_location_news, features: [second_feature, first_feature]) - - expected_ordered_featured_documents = [ - { - title: case_study.title, - href: "/government/case-studies/case-study-title", - image: { url: "#{Plek.asset_root}/government/uploads/system/uploads/featured_image_data/file/#{first_feature.image.id}/minister-of-funk.960x640.jpg", - medium_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s465/s465_minister-of-funk.960x640.jpg", - high_resolution_url: "#{Plek.asset_root}/media/asset_manager_id_s712/s712_minister-of-funk.960x640.jpg", - alt_text: "" }, - summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(case_study.summary), - public_updated_at: case_study.public_timestamp, - document_type: I18n.t("document.type.case_study.one"), - }, - ] - - assert_equal expected_ordered_featured_documents, featured_documents(world_location.world_location_news, featured_documents_display_limit) - end end diff --git a/test/unit/app/presenters/publishing_api/news_article_presenter_test.rb b/test/unit/app/presenters/publishing_api/news_article_presenter_test.rb index dce7702be7a..1c7e89dd023 100644 --- a/test/unit/app/presenters/publishing_api/news_article_presenter_test.rb +++ b/test/unit/app/presenters/publishing_api/news_article_presenter_test.rb @@ -238,7 +238,6 @@ class NewsArticleWithImage < TestCase lead_image_alt_text: "Bar", lead_image_caption: "Baz", images: [build(:image)], - lead_image: build(:image), ) end diff --git a/test/unit/app/presenters/publishing_api/speech_presenter_test.rb b/test/unit/app/presenters/publishing_api/speech_presenter_test.rb index 368e3f7a948..7d1906d7955 100644 --- a/test/unit/app/presenters/publishing_api/speech_presenter_test.rb +++ b/test/unit/app/presenters/publishing_api/speech_presenter_test.rb @@ -209,7 +209,9 @@ def iso8601_regex create( :feature, document: speech.document, - image: build(:featured_image_data), + image: File.open( + Rails.root.join("test/fixtures/images/960x640_gif.gif"), + ), alt_text: "featured image", ) end @@ -218,7 +220,9 @@ def iso8601_regex create( :feature, document: speech.document, - image: build(:featured_image_data, file: upload_fixture("big-cheese.960x640.jpg", "image/jpg")), + image: File.open( + Rails.root.join("test/fixtures/images/960x640_gif.gif"), + ), alt_text: "featured image two", ) end @@ -226,15 +230,7 @@ def iso8601_regex it "presents the most recent featured image" do details = presented.content[:details] assert_equal("featured image two", details[:image][:alt_text]) - assert_match(/big-cheese.960x640.jpg$/, details[:image][:url]) - end - - it "does not present the featured image if any assets are missing" do - feature_two.image.assets.destroy_all - - details = presented.content[:details] - - assert_nil details[:image] + assert_match(/960x640_gif.gif$/, details[:image][:url]) end end @@ -245,7 +241,7 @@ def iso8601_regex assert_match(/minister-of-funk.960x640.jpg$/, details[:image][:url]) end - test "does not present the person image if it has missing assets" do + test "it filters out the person image if it has missing assets" do person.image.assets.destroy_all details = presented.content[:details]