content-modelling/Handle incorrect content IDs found in embeds #3172
Conversation
This file contains 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
Harriethw
force-pushed
the
content-modelling/878-debug-embedded-content
branch
from
e406734 to
6e65bee
Compare
Harriethw
force-pushed
the
content-modelling/878-debug-embedded-content
branch
from
6e65bee to
c5cf3e7
Compare
pezholio
approved these changes
Mar 4, 2025
There was a problem hiding this comment.
Yeah, this sounds sensible to me. It's better for us to get some kind of error in the backend when this happens and the code to get ignored than the page to straight up not get published (especially as this could happen in a background job and not be immediately obvious to a user). I'd hope in "real life", anything like this would get picked up at the 2i process, but better to be safe than sorry!
Previously, an uncaught error was raised if a content block's content ID couldn't be found, which prevented any editions being published if they had an incorrect embed code within them. There was no feedback for the user on Mainstream when this happened and it looks like the edition *has* been published, on Whitehall a nondescript server error is shown to users when attempting to publish. When a Host Edition is updated at the content PUT endpoint and embedded content links are looked for, this change sends an exception to Sentry to alert developers the issue has occurred, and does not add the incorrect ID to the Links on an Edition. When the content is then rendered by the `ContentEmbedPresenter`, we again check whether the corresponding Edition exists and alert Sentry again. The user should be able to see their changes published, and the embed code will appear as plain text on the frontend, so it should be easy to spot and change.
Harriethw
force-pushed
the
content-modelling/878-debug-embedded-content
branch
from
c5cf3e7 to
c37b524
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://trello.com/c/S4xH4K03/878-debug-logging-and-error-handling-for-embed-codes
Previously, if an editor published an Edition with an embed code with an incorrect or missing content ID, then it would fail silently (on Mainstream) or show generic error page (on Whitehall). It seemed like the error for Mainstream wasn't propagating (possibly because it was being called in a Worker, so the Sidekiq job was just failing and retrying in background?).
This is a first pass at handling when we might have an incorrect Content ID, allowing the Edition to be published and the incorrect embed code rendered in plain text in the process, but also alerting Sentry so it will be easier to catch if this issue occurs.
The error handling was introduced last year here, before we started embedded Content Blocks in earnest and I think only Contacts were the embedded content being used?
Is it better to allow the Editor to publish an incorrect block, when they can see that it's not being rendered and correct it, instead of either failing in the background or throwing an error preventing publication? I'm not sure without touching more of the Mainstream architecture how we would throw a more helpful error....
Still TODO:
This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.