Merge pull request #8423 from alphagov/topical-event-to-use-asset-ids

Topical event to use asset ids

app/controllers/admin/topical_events_controller.rb

@@ -43,8 +43,8 @@ def update
   def confirm_destroy; end
 
   def destroy
-    @topical_event.delete!
-    if @topical_event.deleted?
+    @topical_event.destroy!
+    if @topical_event.destroyed?
       redirect_to [:admin, TopicalEvent], notice: "Topical event destroyed"
     else
       redirect_to [:admin, TopicalEvent], alert: "Cannot destroy Topical event with associated content"
@@ -61,6 +61,7 @@ def load_object
 
   def build_associated_objects
     @topical_event.social_media_accounts.build if @topical_event.social_media_accounts.blank?
+    @topical_event.build_logo if @topical_event.logo.blank?
   end
 
   def destroy_blank_social_media_accounts
@@ -74,21 +75,29 @@ def destroy_blank_social_media_accounts
   end
 
   def object_params
-    params.require(:topical_event).permit(
+    topical_event_params = params.require(:topical_event).permit(
       :name,
       :summary,
       :description,
-      :logo,
       :logo_alt_text,
-      :logo_cache,
-      :remove_logo,
       :start_date,
       :end_date,
       related_topical_event_ids: [],
       topical_event_membership_attributes: %i[id ordering],
       social_media_accounts_attributes: %i[social_media_service_id url _destroy id],
       featured_links_attributes: %i[title url _destroy id],
       topical_event_organisations_attributes: %i[id lead lead_ordering],
+      logo_attributes: %i[file file_cache id],
     )
+
+    clear_file_cache(topical_event_params)
+  end
+
+  def clear_file_cache(topical_event_params)
+    if topical_event_params.dig(:logo_attributes, :file).present? && topical_event_params.dig(:logo_attributes, :file_cache).present?
+      topical_event_params[:logo_attributes].delete(:file_cache)
+    end
+
+    topical_event_params
   end
 end

app/models/featured_image_data.rb

@@ -9,6 +9,10 @@ class FeaturedImageData < ApplicationRecord
   validates :file, presence: true
   validates_with ImageValidator, size: [960, 640]
 
+  def filename
+    file&.file&.filename
+  end
+
   def all_asset_variants_uploaded?
     asset_variants = assets.map(&:variant).map(&:to_sym)
     required_variants = FeaturedImageUploader.versions.keys.push(:original)

app/models/social_media_account.rb

@@ -5,7 +5,7 @@ class SocialMediaAccount < ApplicationRecord
   after_save :republish_organisation_to_publishing_api
   after_destroy :republish_organisation_to_publishing_api
 
-  validates :social_media_service_id, presence: true
+  validates :social_media_service, presence: true
   validates :url, presence: true, uri: true
 
   include TranslatableModel

app/models/topical_event.rb

@@ -73,7 +73,9 @@ class TopicalEvent < ApplicationRecord
            -> { where("editions.state" => "published") },
            through: :topical_event_memberships
 
-  belongs_to :logo_new, class_name: "FeaturedImageData", foreign_key: :featured_image_data_id
+  has_one :logo, class_name: "FeaturedImageData", as: :featured_imageable, inverse_of: :featured_imageable, dependent: :destroy
+
+  accepts_nested_attributes_for :logo, reject_if: :all_blank
 
   scope :active, -> { where("end_date > ?", Time.zone.today) }
   scope :alphabetical, -> { order("name ASC") }
@@ -94,8 +96,6 @@ class TopicalEvent < ApplicationRecord
   accepts_nested_attributes_for :topical_event_featurings
   accepts_nested_attributes_for :social_media_accounts, allow_destroy: true
 
-  mount_uploader :logo, FeaturedImageUploader, mount_on: :carrierwave_image
-
   extend FriendlyId
   friendly_id
 
@@ -247,8 +247,4 @@ def start_and_end_dates
   def more_than_a_year(from_time, to_time = 0)
     to_time > from_time + 1.year + 1.day # allow 1 day's leeway
   end
-
-  def logo_changed?
-    changes["carrierwave_image"].present?
-  end
 end

app/presenters/publishing_api/topical_event_presenter.rb

@@ -38,7 +38,7 @@ def details
         details[:about_page_link_text] = item.topical_event_about_page.read_more_link_text if item.topical_event_about_page && item.topical_event_about_page.read_more_link_text
         details[:body] = body
         details[:emphasised_organisations] = item.lead_organisations.map(&:content_id)
-        details[:image] = image if item.logo_url
+        details[:image] = image if item.logo && item.logo.all_asset_variants_uploaded?
         details[:start_date] = item.start_date.rfc3339 if item.start_date
         details[:end_date] = item.end_date.rfc3339 if item.end_date
         details[:ordered_featured_documents] = ordered_featured_documents
@@ -52,7 +52,7 @@ def body
 
     def image
       {
-        url: item.logo_url(:s300),
+        url: item.logo.file.url(:s300),
         alt_text: item.logo_alt_text,
       }
     end

app/uploaders/featured_image_uploader.rb

@@ -6,7 +6,6 @@ def extension_allowlist
   end
 
   configure do |config|
-    config.remove_previously_stored_files_after_update = false
     config.validate_integrity = true
   end
 

app/views/admin/topical_events/_form.html.erb

@@ -40,20 +40,21 @@
   } %>
 
   <div class="govuk-!-margin-bottom-8">
+    <%= hidden_field_tag "topical_event[logo_attributes][id]", topical_event.logo.id %>
     <%= render "components/single-image-upload", {
       title: "Logo",
-      name: "topical_event",
-      image_id: "topical_event_logo",
-      image_name: "topical_event[logo]",
+      name: "topical_event[logo_attributes]",
+      image_id: "topical_event_logo_file",
+      image_name: "topical_event[logo_attributes][file]",
       alt_text_name: "topical_event[logo_alt_text]",
       alt_text_id: "topical_event_logo_alt_text",
       filename: topical_event.logo.filename,
       page_errors: topical_event.errors.any?,
-      error_items: errors_for(topical_event.errors, :logo),
-      image_src: topical_event.logo.url,
+      error_items: errors_for(topical_event.errors, :"logo.file"),
+      image_src: topical_event.logo.file.url,
       image_alt: topical_event.logo_alt_text,
-      image_cache_name: "topical_event[logo_cache]",
-      image_cache: (topical_event.logo.image_cache.presence),
+      image_cache_name: "topical_event[logo_attributes][file_cache]",
+      image_cache: (topical_event.logo.file_cache.presence),
     } %>
   </div>
 

db/migrate/20231027134216_drop_featured_image_id_from_topical_events.rb

@@ -0,0 +1,5 @@
+class DropFeaturedImageIdFromTopicalEvents < ActiveRecord::Migration[7.0]
+  def change
+    remove_column :topical_events, :featured_image_data_id, :bigint
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_10_27_095017) do
+ActiveRecord::Schema[7.0].define(version: 2023_10_27_134216) do
   create_table "assets", charset: "utf8mb3", force: :cascade do |t|
     t.string "asset_manager_id", null: false
     t.string "variant", null: false
@@ -1094,8 +1094,6 @@
     t.date "end_date"
     t.string "content_id"
     t.text "summary"
-    t.bigint "featured_image_data_id"
-    t.index ["featured_image_data_id"], name: "index_topical_events_on_featured_image_data_id"
     t.index ["slug"], name: "index_topical_events_on_slug"
   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) || model.instance_of?(Organisation)
+    return true if model.instance_of?(AttachmentData) || model.instance_of?(ImageData) || model.instance_of?(Organisation) || model.instance_of?(FeaturedImageData)
 
     model.respond_to?("use_non_legacy_endpoints") && model.use_non_legacy_endpoints
   end

test/factories/topical_events.rb

@@ -3,9 +3,21 @@
     sequence(:name) { |index| "topical-event-#{index}" }
     summary { "Topical event summary" }
     description { "Topical event description" }
+
     trait :active do
       start_date { Time.zone.today - 1.month }
       end_date { Time.zone.today + 1.month }
     end
+
+    trait :with_logo do
+      logo { build(:featured_image_data) }
+      logo_alt_text { "Alternative text" }
+    end
+
+    trait :with_social_media_accounts do
+      after :build do |topical_event|
+        topical_event.social_media_accounts << build(:social_media_account)
+      end
+    end
   end
 end

test/functional/admin/topical_events_controller_test.rb

@@ -24,14 +24,64 @@ class Admin::TopicalEventsControllerTest < ActionController::TestCase
 
   test "POST :create saves the topical event" do
     assert_difference("TopicalEvent.count") do
-      post :create, params: { topical_event: { name: "Event", description: "Event description", summary: "Event summary" } }
+      post :create, params: {
+        topical_event: {
+          name: "Event",
+          description: "Event description",
+          summary: "Event summary",
+          logo_attributes: {
+            file: upload_fixture("images/960x640_jpeg.jpg"),
+          },
+        },
+      }
     end
 
     assert_response :redirect
 
     topical_event = TopicalEvent.last
     assert_equal "Event", topical_event.name
     assert_equal "Event description", topical_event.description
+    assert topical_event.logo.present?
+  end
+
+  test "POST :create uses the file cache if present" do
+    cached_logo = build(:featured_image_data, file: upload_fixture("images/960x640_jpeg.jpg"))
+
+    post :create, params: {
+      topical_event: {
+        name: "Event",
+        description: "Event description",
+        summary: "Event summary",
+        logo_attributes: {
+          file_cache: cached_logo.file_cache,
+        },
+      },
+    }
+
+    topical_event = TopicalEvent.last
+    assert_equal "960x640_jpeg.jpg", topical_event.logo.filename
+  end
+
+  test "POST :create discards the file cache if file is present" do
+    cached_logo = build(:featured_image_data, file: upload_fixture("images/960x640_jpeg.jpg"))
+
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/960x640_jpeg.jpg/), anything, anything, anything, anything, anything).never
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/big-cheese.960x640.jpg/), anything, anything, anything, anything, anything).times(7)
+
+    post :create, params: {
+      topical_event: {
+        name: "Event",
+        description: "Event description",
+        summary: "Event summary",
+        logo_attributes: {
+          file: upload_fixture("big-cheese.960x640.jpg"),
+          file_cache: cached_logo.file_cache,
+        },
+      },
+    }
+
+    topical_event = TopicalEvent.last
+    assert_equal "big-cheese.960x640.jpg", topical_event.logo.filename
   end
 
   test "GET :index lists the topical events" do
@@ -59,12 +109,61 @@ class Admin::TopicalEventsControllerTest < ActionController::TestCase
     assert_select "input[name='topical_event[name]'][value='#{topical_event.name}']"
   end
 
+  view_test "GET :edit renders id for logo model if it exists" do
+    topical_event = create(:topical_event, :with_logo)
+
+    get :edit, params: { id: topical_event }
+
+    expected_hidden_field_name = "topical_event[logo_attributes][id]"
+    expected_hidden_field_value = topical_event.logo.id
+    assert_select "input[name='#{expected_hidden_field_name}'][value='#{expected_hidden_field_value}']"
+  end
+
   test "PUT :update saves changes to the topical event" do
-    topical_event = create(:topical_event)
-    put :update, params: { id: topical_event, topical_event: { name: "New name" } }
+    topical_event = create(:topical_event, :with_logo)
+    logo = topical_event.logo
+
+    logo.assets
+        .pluck(:asset_manager_id)
+        .map { |id| AssetManagerDeleteAssetWorker.expects(:perform_async).with(anything, id).once }
+
+    put :update, params: {
+      id: topical_event,
+      topical_event: {
+        name: "New name",
+        logo_attributes: {
+          id: logo.id,
+          file: upload_fixture("images/960x640_jpeg.jpg"),
+        },
+      },
+    }
 
     assert_response :redirect
     assert_equal "New name", topical_event.reload.name
+    assert_equal logo.id, topical_event.reload.logo.id
+    assert_equal "960x640_jpeg.jpg", topical_event.reload.logo.filename
+  end
+
+  test "PUT :update discards the file cache if file is present" do
+    topical_event = create(:topical_event, :with_logo)
+    cached_logo = build(:featured_image_data, file: upload_fixture("images/960x640_jpeg.jpg"))
+
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/960x640_jpeg.jpg/), anything, anything, anything, anything, anything).never
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(regexp_matches(/big-cheese.960x640.jpg/), anything, anything, anything, anything, anything).times(7)
+
+    put :update, params: {
+      id: topical_event,
+      topical_event: {
+        logo_attributes: {
+          id: topical_event.logo.id,
+          file: upload_fixture("big-cheese.960x640.jpg"),
+          file_cache: cached_logo.file_cache,
+        },
+      },
+    }
+
+    topical_event = TopicalEvent.last
+    assert_equal "big-cheese.960x640.jpg", topical_event.logo.filename
   end
 
   test "GET :confirm_destroy calls correctly" do
@@ -76,11 +175,15 @@ class Admin::TopicalEventsControllerTest < ActionController::TestCase
     assert_equal topical_event, assigns(:topical_event)
   end
 
-  test "DELETE :destroy deletes the topical event" do
-    topical_event = create(:topical_event)
+  test "DELETE :destroy deletes the topical event and dependent classes" do
+    topical_event = create(:topical_event, :with_logo, :with_social_media_accounts)
+    logo = topical_event.logo
+    social_media_account = topical_event.social_media_accounts.first
     delete :destroy, params: { id: topical_event }
 
     assert_response :redirect
-    assert topical_event.reload.deleted?
+    assert_nil TopicalEvent.find_by(id: topical_event.id)
+    assert_nil FeaturedImageData.find_by(id: logo.id)
+    assert_nil SocialMediaAccount.find_by(id: social_media_account.id)
   end
 end

test/integration/asset_manager_test.rb

@@ -185,6 +185,7 @@ class ReplacingAPersonImage < ActiveSupport::TestCase
 
     test "sends the new image and its versions to asset manager" do
       expected_number_of_versions = @person.image.versions.size + 1
+      Services.asset_manager.stubs(:whitehall_asset).returns("id" => "http://asset-manager/assets/asset-id")
       Services.asset_manager.expects(:create_whitehall_asset).times(expected_number_of_versions)
 
       @person.image = File.open(fixture_path.join("big-cheese.960x640.jpg"))
@@ -194,8 +195,9 @@ class ReplacingAPersonImage < ActiveSupport::TestCase
       end
     end
 
-    test "does not remove the original images from asset manager" do
-      Services.asset_manager.expects(:delete_asset).never
+    test "removes the original images from asset manager" do
+      Services.asset_manager.stubs(:whitehall_asset).returns("id" => "http://asset-manager/assets/asset-id")
+      Services.asset_manager.expects(:delete_asset).with("asset-id").times(7)
 
       @person.image = File.open(fixture_path.join("big-cheese.960x640.jpg"))