Refactor image sizes so they can be made configurable #9575

richardTowers · 2024-11-04T16:42:00Z

This PR is probably best reviewed commit by commit.

There were several places where Whitehall hardcoded image dimensions to 960px x 640px:

image size validation
generating smaller image versions to save bandwidth
presenting the user with an interface for cropping images larger than the valid size

This PR pulls the configuration required for these three things into a config YAML file, and provides a "default" configuration which matches the existing behaviour.

We add an attribute of image_kind to the ImageData model, which will allow the user to choose which kind of image they are uploading, and get validation and smallert image versions appropriately.

Because other models (e.g. FeaturedImageData) don't need the attribute yet, so the ImageKind mixin provides a method which falls back to "default" in these cases.

I've chosed "image_kind", rather than "image_type" to try and avoid confusion with jpeg vs. png etc. An image_kind could be "default", or "mobile_hero", or "featured" etc. - it defines the dimensions and the versions which should be generated.

The most gnarly bit of this is the interaction with carrierwave. Carrierwave makes heavy use of class methods, which makes it unintuitive to configure it based on the state of the instance - in this case what the value of the model's image_kind method is. The way we achieve this is by providing a lambda to carrierwave's if: configuration parameter, which checks whether the version is applicable for the current image_kind. This approach is heavily inspired by Using one carrierwave image uploader with dynamic versions and Using one carrierwave image uploader with different sizes on several models - credit where credit is due bloggers from > 10 years ago 👏🏻. I also had to ensure that the image_kind attribute is assigned first, as it needs to be present when the carrierwave attribute is assigned (because at that point it needs to check the image_kind to work out which validations are present).

This PR is intended to be a straight refactoring - any behaviour changes are unintentional. Future PRs will add support for other image kinds.

Further context

For an example of how this would be used to provide different image sizes, see #9579 which adds support for mobile / tablet / desktop hero images for landing pages.

Trello cards

https://trello.com/c/00bwdpYL/124-images-from-whitehall
https://trello.com/c/lcaU5Lj4/533-allow-different-image-sizes-for-some-content-types

Boilerplate

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

dnkrj

👏 long overdue, I'll leave approval for the moment to give others a chance to take a look but as far as I'm concerned the code side of things all looks good.

I have one broader comment about the UX flow that potentially affects the code here and at the very least having image_kind as an attribute and was wondering if it came up in discussions at all.

The proposed flow begins with the user telling Whitehall what kind of image they're trying to upload and receiving an answer of "yes this is big enough for that purpose" or "no this image is too small". The alternative way to go about this would be that a user uploads an image and Whitehall tells them "this can be used as a desktop hero image, mobile hero image or embedded in your body copy".

I feel like the alternative I've outlined gives more long term flexibility to users (e.g. "this was the hero image, but we've got a new one so let's move this one to the body") and treats Whitehall more as a fully featured content management system, rather than a tool for one off publishing. At the same time, it might be more confusing to users why they can use images for one purpose but not another when they've not explicitly chosen that.

Ultimately I don't mind really mind which direction we go in, just want to make sure we've fully thought this through for whatever different kinds of blocks or image formats might come up in future.

richardTowers · 2024-11-08T10:40:29Z

Yeah, that's a good point @dnkrj. It is janky having the user select the kind from a radio button at the same time as uploading the image. Honestly, the reason I did it that way was because the code was easy to write, not because it was good UX.

Thinking about it a bit more, there are probably some missing validations where these images get used. At the moment, I don't think there's anything to stop a user from uploading a big hero image (so long as it's on a landing page), and then including it in some govspeak, other than it'll probably error when it tries to present a bunch of variants that don't exist. I'll have a quick look to see if there's some nicer validation I could do there.

I think your alternative flow is probably better, but it would be a much bigger refactoring. At the moment, we decide whether the image needs cropping and what variants to upload at the point the image is uploaded. If we wanted to delay that, we'd have to store the high res images, and then have a point later in the user journey where they specialise the image so it can be used in a particular context. E.g. if someone uploads a high res image, and then later wants to use it in the default govspeak context, we'd need to present them with a cropping interface at the point where they choose to use the image, and then create all the required variants once it's cropped. I think that would necessitate a different structure in the database, and we'd probably have to drop Carrierwave, as it wants to do everything at file upload time.

Hopefully this change is a small step in the right direction, and doesn't preclude a bigger refactoring if we later decide that Whitehall should have better support for images.

richardTowers · 2024-11-08T11:29:55Z

Added a check so that non-default image kinds can't be used in govspeak in f885b46

minhngocd · 2024-11-12T17:52:09Z

From a quick review (I think i need to digest it a little bit more), I think the term image_kind doesn't really tell me exactly what it is there for (would like to hear other people's opinions too). I was wondering if what we are trying to do is abstract image_dimensions and image_usage?

Another question I have is - do we want to have that image resizing functionality in Whitehall? Or should we rethink the image / uploading workflow and get asset manager to manage the sizes? user should be able to upload any image and crop, asset manager manages the storage of sizes for bandwidth, and rendering apps are in charge of querying for the correct size and rendering it.

Throwing a couple of questions out there from a user experience flow:

How are we envisioning users uploading images, will they be able to switch an image to become hero images after uploading them as normal size?
If the images are not switchable between the usages, does it make sense for them to share the same uploader?

Going to think a little more on it, but these are the initial questions I have.

richardTowers · 2024-11-13T16:32:49Z

Thanks Minno!

Some initial answers to those questions:

From a quick review (I think i need to digest it a little bit more), I think the term image_kind doesn't really tell me exactly what it is there for (would like to hear other people's opinions too). I was wondering if what we are trying to do is abstract image_dimensions and image_usage?

Yeah, it's some combination of dimensions and usage. You can get a feel for the things that make up an image_kind by looking at the YAML. It determines the valid width and height of the file the user uploads (or what the upload will be cropped down to, if the file is bigger than that), and then the smaller variants of the file that carrierwave will generate.

The reason I didn't go for image_dimensions (which sounds like it would be simpler) is that in theory you could have images with the same valid dimensions, but different variants. Say, a featured image might be 960x640, just like a govspeak image, but require different variants because it's used in different ways.

image_usage would be an okay name too I think - how the image will be used is the thing that should determine the required size and variants. Maybe that's a better name.

Another question I have is - do we want to have that image resizing functionality in Whitehall? Or should we rethink the image / uploading workflow and get asset manager to manage the sizes? user should be able to upload any image and crop, asset manager manages the storage of sizes for bandwidth, and rendering apps are in charge of querying for the correct size and rendering it.

Yes, possibly. But I think that's a much bigger refactoring, probably big enough to need pulling up into a team's objectives.

I think it's also good to think about this with @dnkrj 's comment in mind. Maybe Whitehall (or asset manager) shouldn't be determining how the image will be used (and therefore what dimensions are valid, and what variants are required) when it's uploaded. Maybe it should make that decision later, when the image actually gets used. So you could have like, a media library of high res images, and then when someone tries to use one in govspeak they're asked to crop it to the right size, and variants are generated (maybe by asset manager, maybe not).

I had a look at how a couple of third party CMSs do this, and it looks like there's also a common pattern where the cropping / resizing / whatever happens at request time (presumably with some caching). For example this hygraph URL with cropping parameters in the path:

https://eu-west-2.graphassets.com/cm3e4kmgk09pu07midhct1mgx/crop=dim:[800,10,1500,1500]/cm3eoqo8qfmdv07mp7lqfi82t

Or the Contentful images API.

I think it would definitely be nice to get the functionality for generating image variants out of whitehall, but I don't think it's totally clear that having it happen in asset manager at upload time is the right option.

As to whether this PR helps or hinders that direction of travel... I think it mostly helps. Introducing some concept of "how will this image be used" is still going to be necessary, even if it's asset-manager that does the resizing / variant production.

Throwing a couple of questions out there from a user experience flow:

How are we envisioning users uploading images, will they be able to switch an image to become hero images after uploading them as normal size?

Not at the moment - they get some radio buttons at upload time, and then the image kind is fixed from that point on. There's a screenshot on #9579 which kind of shows the idea.

I think @dnkrj 's comment is questioning whether this is the right model, and I take the point. But a system where users can upload images and then later customise them (crop / resize / generate variants) for various use cases would be quite a lot more involved to build.

If the images are not switchable between the usages, does it make sense for them to share the same uploader?

They're not switchable in this implementation, so I think this is moot.

If I was going to do switchable image kinds, I'd probably do something more like "user uploads some high res image data, and then user can later create many image_usage entities from it". Schema something like this:

erDiagram
    IMAGE ||--|| IMAGE_DATA : "has one"
    IMAGE_DATA ||--|{ IMAGE_USAGE : "has many"
    IMAGE_USAGE ||--|{ IMAGE_VARIANT : "has many"

... but the user journey to populate such a database schema would need some careful design.

Going to think a little more on it, but these are the initial questions I have.

🙇🏻 thank you very much for the thoughtful review so far.

ChrisBAshton

Thanks for DRYing up all the 960x640 references 👏

+1 to the suggestion of, long term, following the example of other CMS's in having a media library instead of uploads-per-document.
And +1 to the suggestion of, long term, moving 'variant generation' out of Whitehall and into Asset Manager.
I agree that your PR doesn't preclude us from doing either of the above in future.

I'm not terribly keen on image_kinds as a name, and image_usage also doesn't feel right. Perhaps image_constraints would be a good fit? Think it lends itself quite well to the domain ("The default image constraints are 960x640, with the following different variants...").

ChrisBAshton · 2024-11-13T09:56:22Z

config/image_kinds.yml

+  display_name: "Default (960 by 640)"
+  valid_width: 960
+  valid_height: 640
+  versions:


We seem to be using the term 'variant' elsewhere - so probably best to use that, unless you're deliberately trying to choose something different?

Yeah, it's already inconsistent sadly. When we're talking about files, Carrierwave calls them versions, but when we're talking about assets we call them variants.

I went with versions because the main place this bit of config is used is in a loop to call the Carrierwave version method, so it felt better to be consistent with that than with the assets code (that's mostly on the far side of Carrierwave).

app/models/concerns/image_kind.rb

ChrisBAshton · 2024-11-13T17:15:40Z

test/unit/app/helpers/govspeak_helper_test.rb

@@ -197,6 +197,17 @@ def edition_with_attachment(body:)
    refute_select_within_html html, ".image.embedded"
  end

+  test "should ignore images with non-default image kinds" do


It wouldn't be too much extra effort to add a can_be_embedded_in_govspeak: true property to the YAML config, so that we can set this at the config level rather than in code. Would also be less surprising for a dev following the breadcrumb trail when a support ticket comes in asking "why isn't my image rendering?" (explicit config as opposed to hardcoded conditional). What do you think?

Yeah, good idea.

I wonder if there's something more generic we could do in the config (so we don't end up with dozens of boolean flags like can_be_embedded_in_govspeak: true, can_be_featured: true, can_be_used_in_marquee_tag: false etc.).

Maybe something like:

permitted_usages: - govspeak_embed

?

Done in 60d8cc4 - although I might be getting a bit too fancy with the method_missing stuff there - let me know if you think it's too much. I could go with a permits?(string) method instead, or just permitted_uses.include?(string) directly.

Actually this is one of those where "if you have to ask 'is this too clever', it's probably too clever". I'll refactor it to use permits?(use_case).

Now done more simply in e0ade76

At the moment the fact that whitehall expects 960 x 640 px images is specified in various places in the ruby code. Specifically, there are three areas where the data is be needed: - Image size validation - Image variant generation - Image cropping At the moment information like "what size should an image be" is hardcoded in these three places, which is duplication. Pulling this information out into configuration will make it easier to make the code flexible, so we can support other images sizes in future. This commit only introduces the data structures, it doesn't use them. If you haven't encountered deconstruct_keys before, see https://docs.ruby-lang.org/en/master/syntax/pattern_matching_rdoc.html#label-Matching+non-primitive+objects-3A+deconstruct+and+deconstruct_keys . This allows us to use assert_pattern in the tests, which makes them a fair bit more elegant.

... instead of hardcoding sizes. Initially this just replaces the various hardcoded `size: [960, 640]` parameters with values from image_kind_config on the model, which is hardcoded to be "default". When we have multiple different image kinds (e.g. hero images, featured images, default images), we can make the image_kind method into an active record attribute and store the kind of image in the database instead of hardcoding it. This will allow different ImageData objects to have different valid sizes, depending on their kind. This commit should not change any behaviour, so many tests (e.g. the ones in image_data_test.rb) are unchanged.

We can get a list of the active versions from the mounted ImageUploader (which is mounted as file). The method we want is protected, but we can usefully wrap it up. This reduces a bit of duplication compared to the bitmap? ternary, which was doing the same thing as the `if: bitmap?` conditions we have on all the variants.

Previously we were duplicating the hardcoded list of image variants (s960, s712 etc.) in ImageUploader and Asset. Presumably only variants appearing in both lists would be uploaded as assets. This pulls the list of versions / variants from the image_kinds.yml config file. Once ImageUploader is similarly refactored, this will remove a bit of duplication. It will also allow versions from other (non-default) kinds of images (e.g. hero images) in future. It's now important that version names in the config are globally unique, so I've added a test that checks they are.

... instead of hardcoding them. Carrierwave makes this a little bit tricky - because version is a class method, we can't call it conditionally based on instance methods (like model.image_kind), because there is no instance when the method is called. Carrierwave provides an "if: " key to work around this, which previously we were just using to check if the uploaded file is a bitmap. By passing a lambda in instead, we can access both the newly uploaded file _and_ the model the uploader is mounted on. In a previous commit we added the image_kind method to all such models. This means we can add every possible version (for all different kinds of image) to ImageUploader, but have them check in their if condition that the model they're attached to has a relevant kind. At this point this is all a bit abstract, because there's only one image kind in the config ("default"). But when we later want other kinds of image (e.g. hero images), this approach should be flexible enough to support that.

... instead of hardcoding them. This is the same deal as for image_uploader, but this time we're explicitly only using the default versions, and we don't need the if conditional on each version.

This removes one more place where we were hardcoding 960 x 640, and seems to be good enough to make the image cropper work for other image dimensions.

This will allow users to choose from a list of image kinds, when models permit more than one kind of image. The ImageKind mixin still creates an image_kind method as a fallback for models which don't have an image_kind attribute (such as FeaturedImageData)

... and test the other methods in the mixin

We don't want people to be able to put Hero images etc. directly in govspeak, as they won't have the right aspect ratios so they won't work with GOV.UK's designs. They also won't have the right Carrierwave versions, so they probably wouldn't work even if we allowed them.

This means we don't have to hardcode the fact that only default images are allowed to be embedded in govspeak. If we later have other kinds of images which are allowed to be embedded in govspeak, that could be represented in config too. The permitted_uses array could be used in future for other situations where only some kinds of images are allowed to be used in a particular circumstance - for example in featured images, lead images, person images, etc.

richardTowers force-pushed the different-image-sizes branch 9 times, most recently from 7730b66 to d3ee291 Compare November 6, 2024 17:02

dnkrj reviewed Nov 7, 2024

View reviewed changes

ChrisBAshton reviewed Nov 13, 2024

View reviewed changes

richardTowers added 10 commits November 14, 2024 11:17

Use image versions from config for featured image uploader

f2af31f

... instead of hardcoding them. This is the same deal as for image_uploader, but this time we're explicitly only using the default versions, and we don't need the if conditional on each version.

Pass width and height through to image cropper

7e740ac

This removes one more place where we were hardcoding 960 x 640, and seems to be good enough to make the image cropper work for other image dimensions.

Ensure that image_kind attribute is assigned first

cfe78a8

... and test the other methods in the mixin

richardTowers force-pushed the different-image-sizes branch 2 times, most recently from 5f82aa5 to 60d8cc4 Compare November 14, 2024 13:40

richardTowers force-pushed the different-image-sizes branch from 60d8cc4 to e0ade76 Compare November 14, 2024 13:50

ChrisBAshton approved these changes Nov 18, 2024

View reviewed changes

richardTowers merged commit bff0387 into main Nov 18, 2024
19 checks passed

richardTowers deleted the different-image-sizes branch November 18, 2024 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor image sizes so they can be made configurable #9575

Refactor image sizes so they can be made configurable #9575

richardTowers commented Nov 4, 2024 •

edited

Loading

dnkrj left a comment

richardTowers commented Nov 8, 2024

richardTowers commented Nov 8, 2024

minhngocd commented Nov 12, 2024 •

edited

Loading

richardTowers commented Nov 13, 2024

ChrisBAshton left a comment

ChrisBAshton Nov 13, 2024

richardTowers Nov 14, 2024

ChrisBAshton Nov 13, 2024

richardTowers Nov 14, 2024

richardTowers Nov 14, 2024 •

edited

Loading

richardTowers Nov 14, 2024

richardTowers Nov 14, 2024

Refactor image sizes so they can be made configurable #9575

Refactor image sizes so they can be made configurable #9575

Conversation

richardTowers commented Nov 4, 2024 • edited Loading

Further context

Trello cards

Boilerplate

dnkrj left a comment

Choose a reason for hiding this comment

richardTowers commented Nov 8, 2024

richardTowers commented Nov 8, 2024

minhngocd commented Nov 12, 2024 • edited Loading

richardTowers commented Nov 13, 2024

ChrisBAshton left a comment

Choose a reason for hiding this comment

ChrisBAshton Nov 13, 2024

Choose a reason for hiding this comment

richardTowers Nov 14, 2024

Choose a reason for hiding this comment

ChrisBAshton Nov 13, 2024

Choose a reason for hiding this comment

richardTowers Nov 14, 2024

Choose a reason for hiding this comment

richardTowers Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

richardTowers Nov 14, 2024

Choose a reason for hiding this comment

richardTowers Nov 14, 2024

Choose a reason for hiding this comment

richardTowers commented Nov 4, 2024 •

edited

Loading

minhngocd commented Nov 12, 2024 •

edited

Loading

richardTowers Nov 14, 2024 •

edited

Loading