Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update NewsImage to render multiple defaults #3096

Conversation

mec
Copy link

@mec mec commented Feb 15, 2024

The logic to render an image for a news item is:

  • render the details > image if there is one
  • render the organisation > default image if there is one
  • render a placeholder

Organisation here could be one of two types:

  • Organisation, presented in the primary_publishing_organisation link
  • WorldwideOrganisation, presented in the worldwide_organisations link

The placeholder is also dependent on which type of document and so which
type of organisation the content links to.

Here we allow the module to render the default image from the worldwide
organisation link, when the document is the appropriate type and one is
available.

Right now this change will not be used as Whitehall currently sets
details > image to the default image for worldwide organisation news
items, but this is set to change as we want this logic out of the
publishing applications and for all related concepts like 'the default
news image' to work the same way.

With this change, once we do turn the replacement in Whitehall off,
everything will continnue to work without change.

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

Follow these steps if you are doing a Rails upgrade.

@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3096 February 15, 2024 14:23 Inactive
@mec mec marked this pull request as ready for review February 15, 2024 14:31
mec and others added 2 commits February 27, 2024 15:50
The logic to render an image for a news item is:

- render the details > image if there is one
- render the organisation > default image if there is one
- render a placeholder

Organisation here could be one of two types:

- Organisation, presented in the primary_publishing_organisation link
- WorldwideOrganisation, presented in the worldwide_organisations link

The placeholder is also dependent on which type of document and so which
type of organisation the content links to.

Here we allow the module to render the default image from the worldwide
organisation link, when the document is the appropriate type and one is
available.

Right now this change will not be used as Whitehall currently sets
details > image to the default image for worldwide organisation news
items, but this is set to change as we want this logic out of the
publishing applications and for all related concepts like 'the default
news image' to work the same way.

With this change, once we do turn the replacement in Whitehall off,
everything will continnue to work without change.
The placeholder image and default news image both depend on whether the content
item is a world news story.
@jkempster34 jkempster34 force-pushed the feature/news-image-can-render-linked-worldwide-organisation-default-image branch from 645b30c to 822511a Compare February 27, 2024 15:58
@govuk-ci govuk-ci temporarily deployed to government-frontend-pr-3096 February 27, 2024 15:59 Inactive
Copy link
Member

@brucebolt brucebolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a minor nitpick about the syntax to get the worldwide organisation.

Comment on lines +25 to +26
worldwide_organisation = content_item.dig("links", "worldwide_organisations")
worldwide_organisation[0].dig("details", "default_news_image") if worldwide_organisation.present?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick where I think first would be nicer than accessing with a numeric position. The first method still returns nil if there's an empty array.

Suggested change
worldwide_organisation = content_item.dig("links", "worldwide_organisations")
worldwide_organisation[0].dig("details", "default_news_image") if worldwide_organisation.present?
worldwide_organisation = content_item.dig("links", "worldwide_organisations").first
worldwide_organisation.dig("details", "default_news_image") if worldwide_organisation.present?


def first_primary_publishing_organisation_default_news_image
organisation = content_item.dig("links", "primary_publishing_organisation")
organisation[0].dig("details", "default_news_image") if organisation.present?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as my previous comment.

jkempster34 added a commit to alphagov/whitehall that referenced this pull request Feb 28, 2024
Unless a user has added a specific news image for a news article, then a
default image is retrieved from an associated organisation or worldwide
organisation.

Currently, the default image is surfaced as an `image` in the details of a news
story. However, this is problematic because the default image does not update
through dependency resolution when the image is changed on the associated
organisation or worldwide organisation.

This sets the image as nil if the article has a default image. Government
Frontend will render the default news image when the details image is absent
(alphagov/government-frontend#3096), allowing us to replicate the functionality
we are removing here through link expansion instead.
jkempster34 added a commit to alphagov/whitehall that referenced this pull request Feb 28, 2024
Unless a user has added a specific news image for a news article, then a
default image is retrieved from an associated organisation or worldwide
organisation.

Currently, the default image is surfaced as an `image` in the details of a news
story. However, this is problematic because the default image does not update
through dependency resolution when the image is changed on the associated
organisation or worldwide organisation.

This sets the image as nil if the article has a default image. Government
Frontend will render the default news image when the details image is absent
(alphagov/government-frontend#3096), allowing us to replicate the functionality
we are removing here through link expansion instead.
@jkempster34
Copy link
Contributor

We considered including default news image as a link instead of relying on ActiveRecord callbacks. However:

  1. That leaves us with inconsistency between the records in Whitehall, and the content item (unless we add additional fields to the content item)
  2. There is already a callback pattern in place to deal with the delay in uploading assets.

See alphagov/whitehall#8877

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants