Allow publishing of conflicting topical events to one base_path - attempt two#11210
Allow publishing of conflicting topical events to one base_path - attempt two#11210ChrisBAshton wants to merge 2 commits intomainfrom
Conversation
`base_path` isn't always present in the payload of items sent to Publishing API. Role Appointments have no base path, and neither do Contacts, and possibly other content types which have content IDs (and need to exist in Publishing API for the purposes of link expansion etc) but no page rendered on the frontend. This commit returns early in the base path uniqueness check, as currently we make pointless requests to Publishing API for these content items (the 'existing content id' will always be `nil` for these, and thus no uniqueness issue). This guardrail is also depended upon by the next commit.
Currently, Whitehall has a safeguard that raises an exception if you try to publish a Whitehall-owned piece of content under the same public_path as another Whitehall-owned piece of content. In the short term, however, we'd like to allow the publish (effectively an overwrite) in the case of topical events. This is to aid in the migration of topical events. Proposed migration workflow: 1. A legacy topical event exists 2. We write a migration script that creates a StandardEdition _copy_ of the legacy topical event 3. We can save and publish the StandardEdition version and see how that looks on the frontend 4. We can also interchangably swap back to the legacy version if for whatever reason the config-driven version is problematic. We would simply edit and 'save' the legacy version, which automatically publishes to Publishing API. Once all legacy topical events have been migrated, we will revert this change. Note that this was originally introduced in commit b7deee0, but then reverted in commit 69b6797, due to an issue described (and fixed) in the previous commit. We've done the fix first (rather than after this commit) so that this commit can be easily reverted without losing the fix, as we'll always want to retain the 'nil' check. We expect to revert this current commit when all legacy topical events have been migrated.
9ddb2f5 to
422e0b3
Compare
lauraghiorghisor-tw
left a comment
There was a problem hiding this comment.
I am ok with this, but am not super convinced that it's something we should merge now.
I really like the idea that you can flexibly publish one thing or another. But let's consider a scenario where we republish in bulk, and then maybe something goes wrong -> what measures do we have to check the data, other than logs? Publishing API won't know which sort of "type" it got, as far as it knows it is "topical_event". And in whitehall both those docs (legacy and config) would appear as published? I'd be inclined to have more rigid release steps where we know better what is to be migrated, which paths we need to release, and what has been republished. But also cool if we can do it with this, I'd just need to see exactly how we know what's going on.
And if we merge this, don't we still risk that issue that you first thought about, i.e. someone might create a new topical event at the same path as an old one and override it. I know there is governance around what topical events people can create, but it's still a technical possibility. So I'd suggest only enabling this when we are ready to migrate/test the migration, rather than by default 🤔
lauraghiorghisor-tw
left a comment
There was a problem hiding this comment.
Approved but holding off the merge for now, until we are ready to consider migration.
This is a second attempt of #11198, which had to be reverted because role appointments don't have a
base_pathsent to Publishing API, which caused Whitehall to crash on the introduced line. See Sentry error.That got reverted, and this PR is a second attempt at the change but accounting for the possibility of the
base_pathnot being present.Original PR description below.
Currently, Whitehall has a safeguard that raises an exception if you try to publish a Whitehall-owned piece of content under the same public_path as another Whitehall-owned piece of content.
In the short term, however, we'd like to allow the publish (effectively an overwrite) in the case of topical events. This is to aid in the migration of topical events.
Proposed migration workflow:
Once all legacy topical events have been migrated, we will revert this change.
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.