Stop guessing urls for Topical Event image srcset attribute #3426

tunylund · 2023-10-30T12:01:05Z

Whitehall has changed to start using modern Asset Manager urls for each asset. This means that the image urls are no longer guessable. Each image variant has its own unique id. In order to have a srcset for images.

This pr gets the correct image urls from the topical event content.

Trello

Specifically this change is about the following acceptance criteria:

GIVEN a Topical Event with a logo
WHEN I open the Topical Event on the live stack (View on website btn)
THEN I should see the Topical event logo on the page
AND the logo should be loaded without using legacy_url_path
AND the page should have the image srcset-attribute using asset-id urls instead of legacy_url_path

The background context for this work is that the wayhow Whitehall references assets in Asset Manager - including images is changing. Previously Whitehall would have a hardcoded configuration for urls such that would make it possible for the various sizes of the same image urls to be guessable.

The new behaviour is that each individual image will have its own url provided by Asset Manager. The urls are therefore no longer guessable as they all have a unique id in them.

Please see these 2 related prs that show how the urls will be carried from Whitehall to Collections.
alphagov/whitehall#8432
alphagov/publishing-api#2534

leenagupte

Thanks for adding this. And it's great that the tests have been updated. 🎉

Are there any expected changes for the user?
Do you have an example of pages with medium and high resolution images so we can see it in action? I tried the top two from the topical events page on GOV.UK but they don't have the attribute set.

leenagupte · 2023-10-31T12:17:09Z

spec/features/topical_event_spec.rb

@@ -30,6 +30,11 @@
    expect(page).to have_css("img[src='https://www.gov.uk/some-image.png'][alt='Text describing the image']")
  end

+  it "includes image srcset for various image sizes" do
+    visit base_path
+    expect(page).to have_css("img[srcset='https://www.gov.uk/some-medium-image.png 2x, https://www.gov.uk/some-high-image.png 3x']")


It's great that you've added a test for this 🎉

leenagupte · 2023-10-31T12:17:57Z

app/views/topical_events/show.html.erb

@@ -58,7 +58,7 @@

    <% if @topical_event.image_url %>
      <div class="topical-events__image">
-      <%= image_tag(@topical_event.image_url, alt: @topical_event.image_alt_text, srcset: { @topical_event.image_srcset[2][:url] => "2x", @topical_event.image_srcset[0][:url] => "3x",}) %>
+      <%= image_tag(@topical_event.image_url, alt: @topical_event.image_alt_text, srcset: @topical_event.image_srcset) %>


What happens if image_srcset is empty?

browsers ignore the empty srcset attribute and show the image defined in the src-attribute

I made it so that srcset is not rendered if there are no image variants to avoid any possible issues with it

tunylund · 2023-10-31T14:06:57Z

Thanks for adding this. And it's great that the tests have been updated. 🎉

Are there any expected changes for the user? Do you have an example of pages with medium and high resolution images so we can see it in action? I tried the top two from the topical events page on GOV.UK but they don't have the attribute set.

This pr doesn't add any new behaviour - it is simply keeping the existing behaviour of providing 2x and 3x images for various devices to use if they so choose.

If you open this page on an iphone, it will pick one of the images from the srcset (i'm not sure which one)
https://www.gov.uk/government/topical-events/ai-safety-summit-2023

it looks like this. The page above is using the previous version of this code. This pr is aiming to keep the existing behaviour but simply get the urls from whitehall instead of guessing what the urls might be.

leenagupte

I'd missed the conditional in the view in the first review, and have added an inline comment about it.

The values being pulled out of the content item and used to determine the size of the image look ok and match the content schema. I guess these values aren't in use yet?

leenagupte · 2023-11-01T13:50:00Z

app/views/topical_events/show.html.erb

@@ -58,7 +58,9 @@

    <% if @topical_event.image_url %>
      <div class="topical-events__image">
-      <%= image_tag(@topical_event.image_url, alt: @topical_event.image_alt_text, srcset: { @topical_event.image_srcset[2][:url] => "2x", @topical_event.image_srcset[0][:url] => "3x",}) %>
+      <%= @topical_event.image_srcset.any? ?


Ah. I missed that you added the conditional as a ternary operator, hence my previous question about what happens if image_srcset is nil. I've never seen a ternary operator written over multiple lines like this before.

Given that logic in views is generally more difficult to read, I'd recommend using a standard if/else here instead.

hannako · 2023-11-01T14:14:33Z

spec/features/topical_event_spec.rb

+  end
+
+  context "when the image has no variants" do
+    let(:content_item) { fetch_fixture("topical_event_with_no_image_variants") }


I didn't realise collections had this fetch_fixture method. We usually something like this

It isn't a blocker for this PR, but maybe you could update your spec at some point!

oh interesting, the GovukSchemas::Example.find(schema_name, example_name: schema_name) seems to fetch test fixtures from the examples written in publishing_api. It basically makes these tests become sort of contract tests between publishing api schema and collections app. however that then means that the test data has to be written in the examples folder of publishing api and therefore makes management of these testes a bit strange.

I can prepare a separate pr for that as that would require a change in publishing api to provide specific example files for these tests.

You can either create a new example file in publishing api (which might not be a bad idea, as there's only one example for topical events which is quite basic ie)

Or you can just use the existing schema, and mutate the JSON response like in that link I posted. In that way you don't need to make a change in publishing api.

Step 7 on here! https://github.com/alphagov/publishing-api/blob/main/docs/content_schemas/changing-an-existing-content-schema.md#adding-a-new-field-to-a-content-schema

hmm, i'm not exited to set up test data in another repo for this repo to use. If the example files were desired to be used as contract tests - then publishing_api CI workflow should include an action that executes any tests in Collections app that depend on those example files. Otherwise a dev who changes the schema or examples, would not be aware of collections application breaking.

I did however do as you suggested, i removed the extra fixture file i had added and just modified the data in place.

This was a good catch @hannako. I have to admit I didn't delve into what fetch_fixture was doing too closely. Using the examples content items is a pattern that is reflected in other applications, so I agree that it would make sense to use it here too.

As a bit of historical context, the content used to exist in a separate gem, but I believe they were merged into publishing-api to remove our app overhead.

…y Whitehall. Whitehall has changed to start using modern Asset Manager urls for each asset. This means that the image urls are no longer guessable. Each image variant has its own unique id. In order for the srcset to work correctly, the image urls must be collected from content source.

tunylund · 2023-11-01T17:14:46Z

This fixes this situation in production in production on this page

https://www.gov.uk/government/topical-events/ai-safety-summit-2023

richardTowers

I think this looks good enough to release now (not sure if there are any outstanding stylistic comments, but the code looks fine to me).

Approving so we can fix the issue with the production image

leenagupte

Thanks for updating the conditional in the view, and providing extra context about the changes in the PR description. It makes it much easier to understand what is going on.

The PR has been tested in integration and seems to be working. i.e. The Logo image at the top of the page has started rendering.

leenagupte · 2023-11-01T17:01:07Z

app/views/topical_events/show.html.erb

@@ -58,7 +58,13 @@

    <% if @topical_event.image_url %>
      <div class="topical-events__image">
-      <%= image_tag(@topical_event.image_url, alt: @topical_event.image_alt_text, srcset: { @topical_event.image_srcset[2][:url] => "2x", @topical_event.image_srcset[0][:url] => "3x",}) %>
+      <%=
+        if @topical_event.image_srcset.any?


Thanks! This is much easier to read!

govuk-ci temporarily deployed to collections-pr-3426 October 30, 2023 12:01 Inactive

tunylund force-pushed the stop-guessing-image-urls branch from 15bb4b2 to cc29600 Compare October 30, 2023 15:36

govuk-ci temporarily deployed to collections-pr-3426 October 30, 2023 15:36 Inactive

tunylund changed the title ~~Stop rendering srcset for Topic pages because the urls for image variants are no longer guessable~~ Stop guessing urls for Topical Event image srcset attribute Oct 30, 2023

tunylund mentioned this pull request Oct 30, 2023

Provide a medium size image in whitehall image definitions alphagov/publishing-api#2534

Merged

leenagupte reviewed Oct 31, 2023

View reviewed changes

tunylund force-pushed the stop-guessing-image-urls branch from cc29600 to 38febda Compare October 31, 2023 14:18

govuk-ci temporarily deployed to collections-pr-3426 October 31, 2023 14:18 Inactive

leenagupte reviewed Nov 1, 2023

View reviewed changes

hannako reviewed Nov 1, 2023

View reviewed changes

tunylund force-pushed the stop-guessing-image-urls branch from 38febda to 42c1fdf Compare November 1, 2023 14:52

govuk-ci temporarily deployed to collections-pr-3426 November 1, 2023 14:53 Inactive

tunylund force-pushed the stop-guessing-image-urls branch from 42c1fdf to b80bcc1 Compare November 1, 2023 14:56

govuk-ci temporarily deployed to collections-pr-3426 November 1, 2023 14:56 Inactive

tunylund force-pushed the stop-guessing-image-urls branch from b80bcc1 to 3506297 Compare November 1, 2023 16:28

govuk-ci temporarily deployed to collections-pr-3426 November 1, 2023 16:28 Inactive

richardTowers approved these changes Nov 1, 2023

View reviewed changes

leenagupte approved these changes Nov 1, 2023

View reviewed changes

tunylund merged commit f7e5ac4 into main Nov 1, 2023
9 checks passed

tunylund deleted the stop-guessing-image-urls branch November 1, 2023 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop guessing urls for Topical Event image srcset attribute #3426

Stop guessing urls for Topical Event image srcset attribute #3426

tunylund commented Oct 30, 2023 •

edited

Loading

leenagupte left a comment

leenagupte Oct 31, 2023

leenagupte Oct 31, 2023

tunylund Oct 31, 2023

tunylund Oct 31, 2023

tunylund commented Oct 31, 2023

leenagupte left a comment

leenagupte Nov 1, 2023

tunylund Nov 1, 2023

hannako Nov 1, 2023

hannako Nov 1, 2023

tunylund Nov 1, 2023 •

edited

Loading

hannako Nov 1, 2023 •

edited

Loading

hannako Nov 1, 2023

tunylund Nov 1, 2023

leenagupte Nov 1, 2023

tunylund commented Nov 1, 2023

richardTowers left a comment

leenagupte left a comment

leenagupte Nov 1, 2023

Stop guessing urls for Topical Event image srcset attribute #3426

Stop guessing urls for Topical Event image srcset attribute #3426

Conversation

tunylund commented Oct 30, 2023 • edited Loading

leenagupte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunylund commented Oct 31, 2023

leenagupte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunylund Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

hannako Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunylund commented Nov 1, 2023

richardTowers left a comment

Choose a reason for hiding this comment

leenagupte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunylund commented Oct 30, 2023 •

edited

Loading

tunylund Nov 1, 2023 •

edited

Loading

hannako Nov 1, 2023 •

edited

Loading