Merge pull request #8369 from alphagov/implement-non-legacy-urls-for-…

…organisation-logos

Implement non legacy urls for organisation logos

app/controllers/admin/organisations_controller.rb

@@ -117,6 +117,8 @@ def organisation_params
       topical_event_organisations_attributes: %i[topical_event_id ordering id _destroy],
       featured_links_attributes: %i[title url _destroy id],
     )
+
+    clear_file_cache(@organisation_params)
   end
 
   def build_topical_event_organisations
@@ -184,4 +186,12 @@ def clean_non_departmental_public_body_params
     organisation_params[:public_minutes] = nil
     organisation_params[:regulatory_function] = nil
   end
+
+  def clear_file_cache(organisation_params)
+    if organisation_params[:logo].present? && organisation_params[:logo_cache].present?
+      organisation_params.delete(:logo_cache)
+    end
+
+    organisation_params
+  end
 end

app/presenters/publishing_api/organisation_presenter.rb

@@ -154,7 +154,7 @@ def crest_is_publishable?
     end
 
     def image
-      return unless item.custom_logo_selected?
+      return unless item.custom_logo_selected? && item.all_asset_variants_uploaded?
 
       {
         url: item.logo.url,

app/workers/asset_manager_create_asset_worker.rb

@@ -16,19 +16,9 @@ def perform(temporary_location, asset_params, draft = false, attachable_model_cl
     authorised_organisation_uids = get_authorised_organisation_ids(attachable_model_class, attachable_model_id)
     asset_options[:access_limited_organisation_ids] = authorised_organisation_uids if authorised_organisation_uids
 
-    unless asset_already_exist?(assetable_id, assetable_type, asset_variant)
-      response = asset_manager.create_asset(asset_options)
-      asset_manager_id = get_asset_id(response)
-      filename = get_filename(response)
-      save_asset(assetable_id, assetable_type, asset_variant, asset_manager_id, filename)
-    end
-
-    if attachable_model_class && !attachable_model_class.constantize.ancestors.include?(Edition)
-      attachment_data = AttachmentData.find(assetable_id)
-      ServiceListeners::AttachmentUpdater.call(attachment_data:)
-    end
+    create_asset(asset_options, asset_variant, assetable_id, assetable_type)
 
-    perform_draft_update(attachable_model_class, attachable_model_id)
+    enqueue_downstream_service_updates(assetable_id, attachable_model_class, attachable_model_id)
 
     file.close
     FileUtils.rm(file)
@@ -37,16 +27,21 @@ def perform(temporary_location, asset_params, draft = false, attachable_model_cl
 
 private
 
-  def asset_already_exist?(assetable_id, assetable_type, asset_variant)
-    Asset.where(assetable_id:, assetable_type:, variant: asset_variant).exists?
+  def create_asset(asset_options, asset_variant, assetable_id, assetable_type)
+    response = asset_manager.create_asset(asset_options)
+    asset_manager_id = get_asset_id(response)
+    filename = get_filename(response)
+    save_asset(assetable_id, assetable_type, asset_variant, asset_manager_id, filename)
   end
 
-  def perform_draft_update(attachable_model_class, attachable_model_id)
-    if attachable_model_class && attachable_model_id && attachable_model_class.constantize.ancestors.include?(Edition)
-      attachable = attachable_model_class.constantize.find(attachable_model_id)
-      draft_updater = Whitehall.edition_services.draft_updater(attachable)
-
-      draft_updater.perform!
+  def enqueue_downstream_service_updates(assetable_id, attachable_model_class, attachable_model_id)
+    if attachable_model_class
+      if attachable_model_class.constantize.ancestors.include?(Edition)
+        PublishingApiDraftUpdateWorker.perform_async(attachable_model_class, attachable_model_id)
+      else
+        attachment_data = AttachmentData.find(assetable_id)
+        AssetManagerAttachmentMetadataWorker.perform_async(attachment_data.id)
+      end
     end
   end
 
@@ -60,7 +55,10 @@ def get_authorised_organisation_ids(attachable_model_class, attachable_model_id)
   end
 
   def save_asset(assetable_id, assetable_type, variant, asset_manager_id, filename)
-    Asset.create!(asset_manager_id:, assetable_id:, assetable_type:, variant:, filename:)
+    asset = Asset.where(assetable_id:, assetable_type:, variant:).first_or_initialize
+    asset.asset_manager_id = asset_manager_id
+    asset.filename = filename
+    asset.save!
   end
 
   def get_asset_id(response)

app/workers/asset_manager_delete_asset_worker.rb

@@ -2,6 +2,11 @@ class AssetManagerDeleteAssetWorker < WorkerBase
   sidekiq_options queue: "asset_manager"
 
   def perform(legacy_url_path, asset_manager_id)
-    AssetManager::AssetDeleter.call(legacy_url_path, asset_manager_id)
+    begin
+      AssetManager::AssetDeleter.call(legacy_url_path, asset_manager_id)
+    rescue AssetManager::ServiceHelper::AssetNotFound
+      logger.info("Asset #{asset_manager_id} has already been deleted from Asset Manager")
+    end
+    Asset.where(asset_manager_id:).delete_all
   end
 end

app/workers/publishing_api_draft_update_worker.rb

@@ -0,0 +1,8 @@
+class PublishingApiDraftUpdateWorker < WorkerBase
+  sidekiq_options queue: "publishing_api"
+  def perform(attachable_model_class, attachable_model_id)
+    attachable = attachable_model_class.constantize.find(attachable_model_id)
+    draft_updater = Whitehall.edition_services.draft_updater(attachable)
+    draft_updater.perform!
+  end
+end

lib/whitehall/asset_manager_storage.rb

@@ -102,7 +102,7 @@ def get_asset
   def self.use_non_legacy_behaviour?(model)
     return unless model
 
-    return true if model.instance_of?(AttachmentData) || model.instance_of?(ImageData)
+    return true if model.instance_of?(AttachmentData) || model.instance_of?(ImageData) || model.instance_of?(Organisation)
 
     model.respond_to?("use_non_legacy_endpoints") && model.use_non_legacy_endpoints
   end

test/factories/organisations.rb

@@ -11,6 +11,15 @@
       logo { image_fixture_file }
     end
 
+    trait(:with_logo_and_assets) do
+      organisation_logo_type_id { 14 }
+      logo { image_fixture_file }
+
+      after :build do |organisation|
+        organisation.assets.build(asset_manager_id: "logo_asset_manager_id", variant: Asset.variants[:original], filename: "960x640_jpeg.jpg")
+      end
+    end
+
     trait(:closed) do
       govuk_status { "closed" }
       govuk_closed_status { "no_longer_exists" }
@@ -46,6 +55,7 @@
   end
 
   factory :organisation_with_logo, parent: :organisation, traits: [:with_logo]
+  factory :organisation_with_logo_and_assets, parent: :organisation, traits: [:with_logo_and_assets]
   factory :closed_organisation, parent: :organisation, traits: [:closed]
 
   factory :ministerial_department, parent: :organisation do

test/functional/admin/organisations_controller_test.rb

@@ -372,4 +372,56 @@ def example_organisation_attributes
 
     assert_match(/A maximum of 6 documents will be featured on GOV.UK.*/, response.body)
   end
+
+  test "POST: create - discards file cache if file is present" do
+    filename = "logo.png"
+    Services.asset_manager.stubs(:create_asset).returns("id" => "http://asset-manager/assets/asset_manager_id", "name" => filename)
+    cached_organisation = FactoryBot.build(:organisation_with_logo_and_assets)
+
+    post :create,
+         params: {
+           organisation: example_organisation_attributes
+                           .merge(
+                             organisation_logo_type_id: OrganisationLogoType::CustomLogo.id,
+                             logo_cache: cached_organisation.logo_cache,
+                             logo: upload_fixture(filename, "image/png"),
+                           ),
+         }
+
+    AssetManagerCreateAssetWorker.drain
+
+    assert_equal 1, Organisation.last.assets.size
+    assert_equal filename, Organisation.last.assets.first.filename
+  end
+
+  test "POST: update - discards file cache if file is present" do
+    organisation = FactoryBot.create(
+      :organisation_with_logo_and_assets,
+      logo: upload_fixture("big-cheese.960x640.jpg", "image/png"),
+    )
+
+    replacement_filename = "logo.png"
+    cached_filename = "minister-of-funk.960x640.jpg"
+    Services.asset_manager.stubs(:create_asset).returns("id" => "http://asset-manager/assets/asset_manager_id", "name" => replacement_filename)
+    cached_organisation = FactoryBot.build(
+      :organisation_with_logo_and_assets,
+      logo: upload_fixture(cached_filename, "image/png"),
+    )
+
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/#{replacement_filename}/), anything, anything, anything, anything, anything).times(1)
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/#{cached_filename}/), anything, anything, anything, anything, anything).never
+
+    post :update,
+         params: { id: organisation.id,
+                   organisation: {
+                     organisation_logo_type_id: OrganisationLogoType::CustomLogo.id,
+                     logo: upload_fixture(replacement_filename, "image/png"),
+                     logo_cache: cached_organisation.logo_cache,
+                   } }
+
+    AssetManagerCreateAssetWorker.drain
+
+    assert_equal 1, Organisation.last.assets.size
+    assert_equal replacement_filename, Organisation.last.assets.first.filename
+  end
 end

test/integration/asset_manager_test.rb

@@ -52,18 +52,19 @@ class CreatingAnOrganisationLogo < ActiveSupport::TestCase
         organisation_logo_type_id: OrganisationLogoType::CustomLogo.id,
         logo: File.open(fixture_path.join("images", @filename)),
       )
+      @response = { "id" => "http://asset-manager/assets/asset-id", "name" => @filename }
     end
 
     test "sends the logo to Asset Manager" do
-      Services.asset_manager.expects(:create_whitehall_asset).with(file_and_legacy_url_path_matching(/#{@filename}/))
+      Services.asset_manager.expects(:create_asset).with { |args| File.basename(args[:file]) == @filename }.returns(@response)
 
       Sidekiq::Testing.inline! do
         @organisation.save!
       end
     end
 
     test "does not mark the logo as draft in Asset Manager" do
-      Services.asset_manager.expects(:create_whitehall_asset).with(Not(has_key(:draft)))
+      Services.asset_manager.expects(:create_asset).with(has_entry(draft: false)).returns(@response)
 
       Sidekiq::Testing.inline! do
         @organisation.save!
@@ -73,18 +74,12 @@ class CreatingAnOrganisationLogo < ActiveSupport::TestCase
 
   class RemovingAnOrganisationLogo < ActiveSupport::TestCase
     test "removing an organisation logo removes it from asset manager" do
-      logo_filename = "960x640_jpeg.jpg"
-      organisation = FactoryBot.create(
-        :organisation,
-        organisation_logo_type_id: OrganisationLogoType::CustomLogo.id,
-        logo: File.open(fixture_path.join("images", logo_filename)),
-      )
-      logo_asset_id = "asset-id"
-      Services.asset_manager.stubs(:whitehall_asset)
-              .with(regexp_matches(/#{logo_filename}/))
-              .returns("id" => "http://asset-manager/assets/#{logo_asset_id}")
+      logo_asset_manager_id = "logo_asset_manager_id"
+      response = { "id" => "http://asset-manager/assets/#{logo_asset_manager_id}", "name" => "960x640_jpeg.jpg" }
+      organisation = FactoryBot.create(:organisation_with_logo_and_assets)
 
-      Services.asset_manager.expects(:delete_asset).with(logo_asset_id)
+      Services.asset_manager.stubs(:asset).with(logo_asset_manager_id).returns(response)
+      Services.asset_manager.expects(:delete_asset).with(logo_asset_manager_id)
 
       Sidekiq::Testing.inline! do
         organisation.logo.remove!
@@ -94,18 +89,13 @@ class RemovingAnOrganisationLogo < ActiveSupport::TestCase
 
   class ReplacingAnOrganisationLogo < ActiveSupport::TestCase
     test "replacing an organisation logo removes the old logo from asset manager" do
-      old_logo_filename = "960x640_jpeg.jpg"
-      organisation = FactoryBot.create(
-        :organisation,
-        organisation_logo_type_id: OrganisationLogoType::CustomLogo.id,
-        logo: File.open(fixture_path.join("images", old_logo_filename)),
-      )
-      old_logo_asset_id = "asset-id"
-      Services.asset_manager.stubs(:whitehall_asset)
-              .with(regexp_matches(/#{old_logo_filename}/))
-              .returns("id" => "http://asset-manager/assets/#{old_logo_asset_id}")
+      logo_asset_manager_id = "logo_asset_manager_id"
+      response = { "id" => "http://asset-manager/assets/#{logo_asset_manager_id}", "name" => "960x640_jpeg.jpg" }
+      Services.asset_manager.stubs(:create_asset).returns(response)
+      organisation = FactoryBot.create(:organisation_with_logo_and_assets)
 
-      Services.asset_manager.expects(:delete_asset).with(old_logo_asset_id)
+      Services.asset_manager.stubs(:asset).with(logo_asset_manager_id).returns(response)
+      Services.asset_manager.expects(:delete_asset).with(logo_asset_manager_id)
 
       organisation.logo = File.open(fixture_path.join("images", "960x640_gif.gif"))
 

test/integration/attachment_replacement_integration_test.rb

@@ -34,6 +34,8 @@ class AttachmentReplacementIntegrationTest < ActionDispatch::IntegrationTest
 
       context "when attachment is replaced" do
         before do
+          Sidekiq::Worker.clear_all
+
           visit admin_news_article_path(edition)
           click_link "Modify attachments"
           within page.find("li", text: filename) do
@@ -48,12 +50,12 @@ class AttachmentReplacementIntegrationTest < ActionDispatch::IntegrationTest
         # We rely on Asset Manager to do the redirect immediately in this case,
         # because the replacement is visible to the user.
         it "updates replacement_id for attachment in Asset Manager" do
-          AssetManagerCreateAssetWorker.drain
-
           Services.asset_manager.expects(:update_asset)
                   .at_least_once
                   .with(asset_manager_id, { "replacement_id" => replacement_asset_manager_id })
 
+          AssetManagerCreateAssetWorker.drain
+          PublishingApiDraftUpdateWorker.drain
           AssetManagerAttachmentMetadataWorker.drain
         end
       end
@@ -64,6 +66,8 @@ class AttachmentReplacementIntegrationTest < ActionDispatch::IntegrationTest
 
       context "when new draft is created and attachment is replaced" do
         before do
+          Sidekiq::Worker.clear_all
+
           visit admin_news_article_path(edition)
           click_button "Create new edition"
           click_link "Attachments 1"

test/unit/app/models/organisation_test.rb

@@ -1185,15 +1185,14 @@ class OrganisationTest < ActiveSupport::TestCase
   end
 
   test "#all_asset_variants_uploaded? returns true if all asset variants present" do
-    organisation = build(:organisation)
-    organisation.assets.build(asset_manager_id: "asset_manager_id", variant: Asset.variants[:original], filename: "filename.png")
-    organisation.save!
+    organisation = build(:organisation_with_logo_and_assets)
 
     assert organisation.all_asset_variants_uploaded?
   end
 
   test "#all_asset_variants_uploaded? returns false if there are no assets" do
-    organisation = build(:organisation)
+    organisation = build(:organisation_with_logo_and_assets)
+    organisation.assets.delete_all
 
     assert_not organisation.all_asset_variants_uploaded?
   end