From dc4a68316843e9947e259a895e534120f6b6dec9 Mon Sep 17 00:00:00 2001
From: Laura Ghiorghisor <laura.ghiorghisor@digital.cabinet-office.gov.uk>
Date: Thu, 2 Nov 2023 11:35:35 +0000
Subject: [PATCH] Disable remove_previously_stored_files_after_update

This config disables the default carrierwave behaviour of deleting previous images on update.
The issue with removing previous images is that the documents using them may have associations that also use the images.
Therefore, when the images get deleted from asset-manager, the associated published documents will 404 for the image, unless
an intentional update is run to inform the association that a new image has been provided.
Such an example is Speech document and the relates Person who made the speech. Speech page shows the image of the person.
When Person updates the picture, Speech needs to be republished as well, otherwise it will try to show the old image (404 if deletion enabled).
---
 app/uploaders/featured_image_uploader.rb                | 7 +++++++
 test/functional/admin/topical_events_controller_test.rb | 4 ----
 test/integration/asset_manager_test.rb                  | 5 ++---
 test/unit/app/models/featured_image_data_test.rb        | 8 ++++++++
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/app/uploaders/featured_image_uploader.rb b/app/uploaders/featured_image_uploader.rb
index ca11410a343..3c16216f693 100644
--- a/app/uploaders/featured_image_uploader.rb
+++ b/app/uploaders/featured_image_uploader.rb
@@ -6,6 +6,13 @@ def extension_allowlist
   end
 
   configure do |config|
+    # This config disables the default carrierwave behaviour of deleting previous images on update.
+    # The issue with removing previous images is that the documents using them may have associations that also use the images.
+    # Therefore, when the images get deleted from asset-manager, the associated published documents will 404 for the image, unless
+    # an intentional update is run to inform the association that a new image has been provided.
+    # Such an example is Speech document and the relates Person who made the speech. Speech page shows the image of the person.
+    # When Person updates the picture, Speech needs to be republished as well, otherwise it will try to show the old image (404 if deletion enabled).
+    config.remove_previously_stored_files_after_update = false
     config.validate_integrity = true
   end
 
diff --git a/test/functional/admin/topical_events_controller_test.rb b/test/functional/admin/topical_events_controller_test.rb
index a53dc97cec6..3fbdc4bf7f2 100644
--- a/test/functional/admin/topical_events_controller_test.rb
+++ b/test/functional/admin/topical_events_controller_test.rb
@@ -123,10 +123,6 @@ class Admin::TopicalEventsControllerTest < ActionController::TestCase
     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: {
diff --git a/test/integration/asset_manager_test.rb b/test/integration/asset_manager_test.rb
index 6b2e18f8bbd..15bb7117f94 100644
--- a/test/integration/asset_manager_test.rb
+++ b/test/integration/asset_manager_test.rb
@@ -195,9 +195,8 @@ class ReplacingAPersonImage < ActiveSupport::TestCase
       end
     end
 
-    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)
+    test "does not remove the original images from asset manager because other pages (e.g. Speech) might be using it" do
+      Services.asset_manager.expects(:delete_asset).never
 
       @person.image = File.open(fixture_path.join("big-cheese.960x640.jpg"))
 
diff --git a/test/unit/app/models/featured_image_data_test.rb b/test/unit/app/models/featured_image_data_test.rb
index 1872e1f8eac..94f16100e5b 100644
--- a/test/unit/app/models/featured_image_data_test.rb
+++ b/test/unit/app/models/featured_image_data_test.rb
@@ -52,4 +52,12 @@ class FeaturedImageDataTest < ActiveSupport::TestCase
 
     assert_not featured_image_data.all_asset_variants_uploaded?
   end
+
+  test "should not delete previous images when FeaturedImageData is updated" do
+    featured_image_data = create(:featured_image_data)
+
+    AssetManagerDeleteAssetWorker.expects(:perform_async).never
+
+    featured_image_data.update!(file: upload_fixture("images/960x640_jpeg.jpg"))
+  end
 end