Bulk Uploading Edition Images [WHIT-1812][WHIT-2434] #10625

patrickpatrickpatrick · 2025-09-02T09:01:39Z

What

Images now save crop_data instead of transforming the uploaded image on save
Use embed_url (if specified within ImageConfig) within govspeak_helper
Images can be bulk uploaded on Editions that can display images

`ImageData` changes

remove Dimensions struct and replace with calls to dimensions

`ImageUploader` changes

on upload, dimensions is populated by process
if an image is too large and doesn't have crop_data, don't generate versions
if an image has crop_data, crop the first dependent version (not the original image)

`ImageCropper` changes

Remove JS that saves the cropped canvas as an image on form submit
Add JS that updates a hidden input fields with the crop data (height, width, x, y)

`ImageComponent` changes

If an image requires cropping, display "Requires crop" badge
If an image requires cropping, don't display markdown copy input
If an image requires cropping, don't display "Select as lead image" button

`EditionImagesController` changes

Remove crop view, instead redirect to index on successful upload
Add ImageCropper component to edit (if image can be cropped)
Add support for multiple images to create

`GovspeakHelper` changes

Use embed_url instead of url for each image (this is so the cropped version of images actually gets used)

Visual Differences

The user is no longer redirected to edit on successful image upload. If the images uploaded require cropping then the user is prompted to crop the image in edit.

Scenario	Before	After
After uploading an image that doesn't need cropping
After uploading an image that does need cropping
Edition Image edit page

Why

The advantages of having the cropping information be saved separately from the image is that:

the user can bulk upload multiple images and then crop them after the upload, previously the validation would block this
the user can adjust the crop of an image, they don't need to reupload (which would involve deleting the original image and then starting the process of uploading an image) to re-crop
we could add functionality for multiple crops of the same image, which would be useful for hero images
paves the way for image optimisation by a CDN or image processing to be moved to Asset Manager/S3 itself

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.

Follow these steps if you are doing a Rails upgrade.

ryanb-gds

This is looking really good now. I think the controller code could be a little bit tidier, but to be honest if it merged as-is I wouldn't have major concerns.

ryanb-gds · 2025-10-23T13:34:39Z

app/models/concerns/replaceable.rb

+  end
+
+  def replace_with!(replacement)
+    # NOTE: we're doing this manually because carrierwave is setup such


I reckon you can probably drop this comment, because Asset Manager handles virus checking these days, not Whitehall

ryanb-gds · 2025-10-23T13:56:58Z

app/controllers/admin/edition_images_controller.rb

+              else
+                invalid_image = @edition.images.build
+                invalid_image.build_image_data({})
+                [invalid_image]


Apologies if I should have caught this on the first review, but this building an invalid image seems a bit weird. I can kind of see why we'd do it to save ourselves checking if the @images value is set but I think to be honest we'd be better off just doing the null checks.

Or perhaps empty checks - would @images = images_params.map { |image| create_image(image) } work?

It's a bit strange looking but it's to avoid having to make any changes to the ErrorSummaryComponent in this PR. Happy to make a follow-up PR in which I make those changes.

ryanb-gds · 2025-10-23T13:59:45Z

app/controllers/admin/edition_images_controller.rb

-      # Remove @new_image from @edition.images array, otherwise the view will render it in the 'Uploaded images' list
-      @edition.images.delete(@new_image)
-      render :index
+      flash[:notice] = nil


Should be nil by default, shouldn't it?

ryanb-gds · 2025-10-23T14:01:34Z

app/controllers/admin/edition_images_controller.rb

+
+    new_image.image_data.validate_on_image = new_image
+
+    new_image.image_data.images << new_image


I don't think you should need this line, the association keys should be set automatically when you save the image on line 51

It seems we do need it otherwise the comment is correct, AssetManagerStorage isn't able to get the auth_bypass_id.

ryanb-gds · 2025-10-23T14:03:02Z

app/models/attachment_data.rb

 require "timeout"

 class AttachmentData < ApplicationRecord
+  include Replaceable


Nice to be able to share this behaviour across "data" classes, good work

ChrisBAshton

Looking good! I have every confidence this is an improvement on the current code and I wouldn't block merging it.

But where did we arrive at with the decision around overwriting files of the same name? I thought we'd said we should raise a validation error in that scenario, forcing the publisher to delete the original if they want to upload a new file of the same name.

ChrisBAshton · 2025-10-24T14:07:43Z

db/migrate/20250902091240_add_fields_to_image_data.rb

+  def change
+    change_table :image_data, bulk: true do |t|
+      t.json "dimensions"
+      t.json "crop_data"


Strictly speaking I'd move this into its own, earlier, commit. (Not entirely sure of the sequencing of db migrate vs pod deployment, if we roll out both the DB changes and the code that references the new fields, in the same PR. Don't we need to ship the DB migration first as a separate PR?)

Yeah, I wasn't sure about what to do with regards to the migration. What you're saying about the separate PR for the migrations makes sense.

ChrisBAshton · 2025-10-24T14:08:25Z

app/uploaders/image_uploader.rb

    config.storage = Storage::PreviewableStorage
  end

+  def downloader


Where is this called?

This is overloading the base downloader from CarrierWave::Uploader::Base. We need to override skip_ssrf_protection, without doing this an error will occur when EditionImagesController calls download! in test and development environments.

I can put a comment saying this in the image_uploader if that makes it clearer.

Sure, thanks!

ChrisBAshton · 2025-10-24T14:09:58Z

features/step_definitions/image_steps.rb

-      attach_file Rails.root.join("test/fixtures/horrible-image.64x96.jpg")
-    elsif width == 960 && height == 960
-      attach_file Rails.root.join("test/fixtures/images/960x960_jpeg.jpg")
+  if running_javascript?


patrickpatrickpatrick · 2025-10-24T14:30:52Z

Looking good! I have every confidence this is an improvement on the current code and I wouldn't block merging it.

But where did we arrive at with the decision around overwriting files of the same name? I thought we'd said we should raise a validation error in that scenario, forcing the publisher to delete the original if they want to upload a new file of the same name.

Yep, that's what happens now! The overwriting files functionality was in a different commit, so I just dropped it.

ChrisBAshton

LGTM 👍 nice catch

Allows images that are too big to be saved. When this happens the user will be asked to crop the image on the `index` page. This change means that images can be recropped, the cropping data can be used in other contexts and multiple images can be uploaded without validation errors if all/some of them are too big.

Users can now bulk upload images in a similar way to file attachments. The JS enhanced version of the `FileUpload` component is now used and the `EditionImagesController` has been changed to use an array of `images` for `create` instead of a single image.

As we save processed images and original images separately now, we need to ensure that the processed images are referenced by presenters instead of the original images. If we don't do this, then uncropped images will be used for embeddable images. To this end, `embed_version` has been added to `ImageKindConfig` and `embed_url` has been added to `Image`. If `embed_version` is defined, then the version specified will be used to generate the image url. Otherwise, the original url will returned. Tests referencing `image.url` within `govspeak` have been updated.

patrickpatrickpatrick changed the title ~~bulk image upload~~ bulk image upload [WHIT-1812] Sep 2, 2025

patrickpatrickpatrick force-pushed the bulk-image-upload branch 2 times, most recently from accc163 to e496119 Compare September 2, 2025 09:18

patrickpatrickpatrick changed the title ~~bulk image upload [WHIT-1812]~~ Changes to Image Cropping in Whitehall [WHIT-1812] Sep 2, 2025

patrickpatrickpatrick changed the title ~~Changes to Image Cropping in Whitehall [WHIT-1812]~~ Changes to Image Cropping in Whitehall [WHIT-1812][WHIT-2434] Sep 2, 2025

patrickpatrickpatrick force-pushed the bulk-image-upload branch from 9b3aad3 to fa4e301 Compare September 26, 2025 15:47

patrickpatrickpatrick force-pushed the bulk-image-upload branch 24 times, most recently from c1ce4da to 016aa8a Compare October 8, 2025 11:24

patrickpatrickpatrick force-pushed the bulk-image-upload branch from 1470600 to 32dfabf Compare October 23, 2025 14:03

ryanb-gds approved these changes Oct 23, 2025

View reviewed changes

patrickpatrickpatrick force-pushed the bulk-image-upload branch 6 times, most recently from 20e5d29 to a9ca8ed Compare October 24, 2025 13:31

ChrisBAshton reviewed Oct 24, 2025

View reviewed changes

patrickpatrickpatrick force-pushed the bulk-image-upload branch 11 times, most recently from 6d7590a to 9df74d9 Compare October 31, 2025 14:27

patrickpatrickpatrick requested review from ChrisBAshton and ryanb-gds October 31, 2025 14:51

ChrisBAshton approved these changes Nov 3, 2025

View reviewed changes

patrickpatrickpatrick added 3 commits November 3, 2025 09:46

Bulk upload Images

662ab30

Users can now bulk upload images in a similar way to file attachments. The JS enhanced version of the `FileUpload` component is now used and the `EditionImagesController` has been changed to use an array of `images` for `create` instead of a single image.

patrickpatrickpatrick force-pushed the bulk-image-upload branch from 9df74d9 to 3b74dbb Compare November 3, 2025 09:46

patrickpatrickpatrick merged commit 892c8e2 into main Nov 3, 2025
25 checks passed

patrickpatrickpatrick deleted the bulk-image-upload branch November 3, 2025 13:48


		new_image.image_data.validate_on_image = new_image

		new_image.image_data.images << new_image

Bulk Uploading Edition Images [WHIT-1812][WHIT-2434] #10625

Bulk Uploading Edition Images [WHIT-1812][WHIT-2434] #10625

Uh oh!

Conversation

patrickpatrickpatrick commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

ImageData changes

ImageUploader changes

ImageCropper changes

ImageComponent changes

EditionImagesController changes

GovspeakHelper changes

Visual Differences

Why

Uh oh!

ryanb-gds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisBAshton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickpatrickpatrick Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickpatrickpatrick commented Oct 24, 2025

Uh oh!

ChrisBAshton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patrickpatrickpatrick commented Sep 2, 2025 •

edited

Loading

`ImageData` changes

`ImageUploader` changes

`ImageCropper` changes

`ImageComponent` changes

`EditionImagesController` changes

`GovspeakHelper` changes

patrickpatrickpatrick Oct 24, 2025 •

edited

Loading