From fb98fd05408ea934172625f0641241109d0afb2b Mon Sep 17 00:00:00 2001 From: syed-ali-tw Date: Thu, 2 Nov 2023 10:41:55 +0000 Subject: [PATCH] Allow WWOrganisation to use FeaturedImageData This Change makes WWOrganisation use Asset manager ids instead of legacy url path for WWOrganisation default news images. This should make things more consistent across these classes. Co-authored-by: Iris Lau --- app/models/worldwide_organisation.rb | 16 +--------------- .../asset_manager_create_asset_worker.rb | 11 ++++++++--- .../show/summary_list_component_test.rb | 2 +- test/factories/worldwide_organisations.rb | 5 +++++ .../unit/app/models/edition/lead_image_test.rb | 9 --------- .../app/models/worldwide_organisation_test.rb | 4 ++-- .../news_article_presenter_test.rb | 17 +++++++++++++++++ .../asset_manager_create_asset_worker_test.rb | 18 ++++++++++++++++++ 8 files changed, 52 insertions(+), 30 deletions(-) diff --git a/app/models/worldwide_organisation.rb b/app/models/worldwide_organisation.rb index b6ab3598358..62484a04cf8 100644 --- a/app/models/worldwide_organisation.rb +++ b/app/models/worldwide_organisation.rb @@ -19,7 +19,7 @@ class WorldwideOrganisation < ApplicationRecord has_many :editions, through: :edition_worldwide_organisations - belongs_to :default_news_image, class_name: "DefaultNewsOrganisationImageData", foreign_key: :default_news_organisation_image_data_id + has_one :default_news_image, class_name: "FeaturedImageData", as: :featured_imageable, inverse_of: :featured_imageable accepts_nested_attributes_for :default_news_image, reject_if: :all_blank @@ -43,20 +43,6 @@ class WorldwideOrganisation < ApplicationRecord extend FriendlyId friendly_id - after_save do - # If the default news organisation image changes we need to republish all - # news articles belonging to the worldwide organisation - if saved_change_to_default_news_organisation_image_data_id? - documents = NewsArticle - .in_worldwide_organisation(self) - .includes(:images) - .where(images: { id: nil }) - .map(&:document) - - documents.each { |d| Whitehall::PublishingApi.republish_document_async(d) } - end - end - after_commit :republish_embassies_index_page_to_publishing_api, :republish_worldwide_offices # I'm trying to use a domain centric design rather than a persistence diff --git a/app/workers/asset_manager_create_asset_worker.rb b/app/workers/asset_manager_create_asset_worker.rb index 06a097e77d9..ab14c08d1d6 100644 --- a/app/workers/asset_manager_create_asset_worker.rb +++ b/app/workers/asset_manager_create_asset_worker.rb @@ -42,7 +42,8 @@ def enqueue_downstream_service_updates(assetable_id, assetable_type, attachable_ if assetable_type == FeaturedImageData.to_s assetable = FeaturedImageData.find(assetable_id) featured_imageable = assetable.featured_imageable - if assetable.featured_imageable_type.to_s == Organisation.to_s + + if %w[Organisation WorldwideOrganisation].include?(assetable.featured_imageable_type) # If the default news organisation image changes we need to republish all # news articles belonging to the organisation republish_news_articles_with_default_news_image(featured_imageable) @@ -61,8 +62,12 @@ def enqueue_downstream_service_updates(assetable_id, assetable_type, attachable_ end def republish_news_articles_with_default_news_image(featured_imageable) - documents = NewsArticle - .in_organisation(featured_imageable) + query = if featured_imageable.instance_of?(Organisation) + NewsArticle.in_organisation(featured_imageable) + else + NewsArticle.in_worldwide_organisation(featured_imageable) + end + documents = query .includes(:images) .where(images: { id: nil }) .map(&:document) diff --git a/test/components/admin/worldwide_organisations/show/summary_list_component_test.rb b/test/components/admin/worldwide_organisations/show/summary_list_component_test.rb index 7951b0c7d40..c15cb5838f8 100644 --- a/test/components/admin/worldwide_organisations/show/summary_list_component_test.rb +++ b/test/components/admin/worldwide_organisations/show/summary_list_component_test.rb @@ -18,7 +18,7 @@ class Admin::WorldwideOrganisations::Show::SummaryListComponentTest < ViewCompon test "renders the correct items when all fields are completed and only 1 world location & sponsoring organisation" do world_location = build_stubbed(:world_location) sponsoring_organisation = build_stubbed(:organisation) - news_image = build_stubbed(:default_news_organisation_image_data) + news_image = build_stubbed(:featured_image_data) worldwide_organisation = build_stubbed( :worldwide_organisation, logo_formatted_name: "Optional log formatted name", diff --git a/test/factories/worldwide_organisations.rb b/test/factories/worldwide_organisations.rb index 8376f08e86f..f1c9c0479c3 100644 --- a/test/factories/worldwide_organisations.rb +++ b/test/factories/worldwide_organisations.rb @@ -39,5 +39,10 @@ worldwide_organisation.world_locations << FactoryBot.create(:world_location) end end + trait(:with_default_news_image) do + after :build do |organisation| + organisation.default_news_image = build(:featured_image_data) + end + end end end diff --git a/test/unit/app/models/edition/lead_image_test.rb b/test/unit/app/models/edition/lead_image_test.rb index 0cbcc2e88a3..017df55d43c 100644 --- a/test/unit/app/models/edition/lead_image_test.rb +++ b/test/unit/app/models/edition/lead_image_test.rb @@ -57,13 +57,4 @@ 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 - # e.g. DefaultNewsOrganisationImageData doesn't yet implement all_asset_variants_uploaded? - image = build(:featured_image_data) - organisation = build(:organisation, default_news_image: image) - model = stub("Target", { images: [], lead_organisations: [], organisations: [organisation] }).extend(Edition::LeadImage) - - assert model.lead_image_has_all_assets? - end end diff --git a/test/unit/app/models/worldwide_organisation_test.rb b/test/unit/app/models/worldwide_organisation_test.rb index e9fa03802a3..99eb9a249bb 100644 --- a/test/unit/app/models/worldwide_organisation_test.rb +++ b/test/unit/app/models/worldwide_organisation_test.rb @@ -41,7 +41,7 @@ class WorldwideOrganisationTest < ActiveSupport::TestCase end test "can have a default news article image" do - image = build(:default_news_organisation_image_data) + image = build(:featured_image_data) worldwide_organisation = build(:worldwide_organisation, default_news_image: image) assert_equal image, worldwide_organisation.default_news_image end @@ -334,7 +334,7 @@ class WorldwideOrganisationTest < ActiveSupport::TestCase end world_organisation.update!( - default_news_image: create(:default_news_organisation_image_data), + default_news_image: create(:featured_image_data), ) 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 c0b55b21ff2..e8df3280ea2 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 @@ -254,6 +254,23 @@ class NewsArticleWithImage < TestCase assert_details_attribute :image, expected_image end + + test "should return lead image from worldwide_organisations when lead_image_has_all_assets?" do + image = build(:featured_image_data) + worldwide_organisation = build(:worldwide_organisation, default_news_image: image) + self.news_article = create(:news_article_world_news_story, worldwide_organisations: [worldwide_organisation]) + + assert presented_news_article.content[:details][:image][:url].include?("s300_minister-of-funk.960x640.jpg") + end + + test "should not return lead image from worldwide_organisations when lead_image dont have assets?" do + image = build(:featured_image_data) + worldwide_organisation = build(:worldwide_organisation, default_news_image: image) + worldwide_organisation.default_news_image.assets.delete_all + + self.news_article = create(:news_article_world_news_story, worldwide_organisations: [worldwide_organisation]) + assert presented_news_article.content[:details][:image].nil? + end end class NewsArticleWithMinisterialRoleAppointments < TestCase 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 index 9904a9a0b05..b4b7fd10c11 100644 --- a/test/unit/app/workers/asset_manager_create_asset_worker_test.rb +++ b/test/unit/app/workers/asset_manager_create_asset_worker_test.rb @@ -246,4 +246,22 @@ class AssetManagerCreateAssetWorkerTest < ActiveSupport::TestCase @worker.perform(@file.path, asset_params, true, nil, nil) end + + test "should republish all related news article for worldwide-organisations if featured image changes" do + worldwide_organisation = create(:worldwide_organisation, :with_default_news_image) + last_asset = worldwide_organisation.default_news_image.assets.last + asset_params = { + assetable_id: worldwide_organisation.default_news_image.id, + asset_variant: last_asset.variant, + assetable_type: FeaturedImageData.to_s, + }.deep_stringify_keys + + asset_manager_response_with_new_id = { "id" => "http://asset-manager/assets/some_asset_manager_id", "name" => File.basename(@file) } + Services.asset_manager.stubs(:create_asset).returns(asset_manager_response_with_new_id) + news_article = create(:news_article_world_news_story, worldwide_organisations: [worldwide_organisation]) + + Whitehall::PublishingApi.expects(:republish_document_async).with(news_article.document) + + @worker.perform(@file.path, asset_params, true, nil, nil) + end end