Skip to content

Commit

Permalink
Disable remove_previously_stored_files_after_update
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
lauraghiorghisor-tw committed Nov 2, 2023
1 parent e50b4ed commit dc4a683
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 7 deletions.
7 changes: 7 additions & 0 deletions app/uploaders/featured_image_uploader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 0 additions & 4 deletions test/functional/admin/topical_events_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
5 changes: 2 additions & 3 deletions test/integration/asset_manager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"))

Expand Down
8 changes: 8 additions & 0 deletions test/unit/app/models/featured_image_data_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit dc4a683

Please sign in to comment.