Remove RenderedAttachmentsHelper [WHIT-1922]#11366
Open
patrickpatrickpatrick wants to merge 3 commits intomainfrom
Open
Remove RenderedAttachmentsHelper [WHIT-1922]#11366patrickpatrickpatrick wants to merge 3 commits intomainfrom
RenderedAttachmentsHelper [WHIT-1922]#11366patrickpatrickpatrick wants to merge 3 commits intomainfrom
Conversation
patrickpatrickpatrick
force-pushed
the
remove-attachments-rendering
branch
from
cf4e372 to
1b57496
Compare
patrickpatrickpatrick
changed the title
documents from detailsdocuments from details [WHIT-1922]
patrickpatrickpatrick
force-pushed
the
remove-attachments-rendering
branch
from
835e82c to
0842234
Compare
patrickpatrickpatrick
changed the title
documents from details [WHIT-1922]RenderedAttachmentsHelper [WHIT-1922]
patrickpatrickpatrick
force-pushed
the
remove-attachments-rendering
branch
from
0842234 to
62373cf
Compare
| attachments = item.attachments_ready_for_publishing | ||
| # nil/"" locale should always be returned | ||
| locales_that_match = [I18n.locale.to_s, ""] | ||
| attachments.to_a.select { |attachment| locales_that_match.include?(attachment.locale.to_s) }.map(&:publishing_api_details) |
Contributor
There was a problem hiding this comment.
This commit seems a bit rushed - no commit message explaining the 'why', no corresponding test to ensure we don't accidentally regress, etc. Please revisit 🙏
Contributor
There was a problem hiding this comment.
Great to delete all this code 🚀 Would be good to amend the commit message with the 'why', so a future dev doesn't have to navigate to the PR itself to find out why.
patrickpatrickpatrick
force-pushed
the
remove-attachments-rendering
branch
from
62373cf to
e58edd4
Compare
Currently `attachments` does not use `locale` to determine what documents should be included in the `payload`. This has not been noticed yet but for a document type in which attachments are `featured` (the user does not choose where in the document they are included, they are instead rendered as a list) all the attachments for the every locale the document is available in will be rendered. This functionality was present within the `documents` payload (which is subsequently being removed) and so therefore needs to be moved here.
patrickpatrickpatrick
force-pushed
the
remove-attachments-rendering
branch
from
e58edd4 to
1b31d98
Compare
The frontend rendering applications have moved away from using pre-rendered mark-up in the Publishing API payload and instead generating the attachment mark-up themselves using `details.attachments`. This means we can remove `details.documents` from the the Publishing API payload.
As we no longer include rendered attachments in the API payload, we no longer use the `RenderedAttachmentsHelper` and so it can be removed from Whitehall (along with it's associated tests).
patrickpatrickpatrick
force-pushed
the
remove-attachments-rendering
branch
from
1b31d98 to
1e07de4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
RenderedAttachmentsHelperRenderedAttachmentsHelperfrom Publishing API payload presenters (details.documents,details.final_outcome_documents,details.public_feedback_documents) that are no longer referenced by frontend rendering applicationslocalewhen buildingdetails.attachments, preserve previous behaviour of attachments and update testsWhy
Frontend rendering applications now render attachments themselves instead of using pre-rendered markup delivered by Whitehall. Therefore we can remove the
RenderedAttachmentsHelperas well as the fields in the payload that contained the output of this helper.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.