Add substitute event for queue consumers #2664
Conversation
callumknights
force-pushed
the
add-substitute-event
branch
4 times, most recently
from
a2dadc2 to
f173321
Compare
There was a problem hiding this comment.
Looks good 👍. I've just added a few really small comments about the layout of the commits, but otherwise the code looks fine.
I think you'll also need to auto-regenerate the schemas again, because it looks like topic_taxonomy_taxons have been removed since they were last generated and there's now a failing test because of that.
app/downstream_payload.rb
Outdated
| @@ -79,7 +79,10 @@ def message_queue_presenter | |||
| when "redirect" then redirect_presenter | |||
| when "gone" then gone_presenter | |||
| when "vanish" then vanish_presenter | |||
| else content_presenter | |||
| when "substitute" then vanish_presenter | |||
There was a problem hiding this comment.
Should this not be substitute_presenter?
There was a problem hiding this comment.
Oh, I see the substitute_presenter method was added in a later commit. Maybe this first commit should just add the logging, then the when "substitute" case could be added in the same commit as the substitute_presenter method?
There was a problem hiding this comment.
Ultimately just decided to merge everything to do with the new unpublishing type into one commit, doesn't make much sense to split it 👍
content_schemas/dist/formats/substitute/notification/schema.json
Outdated
Show resolved
Hide resolved
callumknights
force-pushed
the
add-substitute-event
branch
3 times, most recently
from
73d920e to
4fff7e5
Compare
There was a problem hiding this comment.
I think we also need to switch the logic up a bit in message_queue_presenter - there's an issue at the moment where redirects take priority over unpublishings, which will mean we send "redirect" payloads when we're substituting old redirects, instead of sending "substitute" payloads.
Luckily, our test case is a redirect, so this is eash to show with an addition to the test.
Some commits to show what I mean in another branch:
(Feel free to merge these into this PR if you're happy with them)
|
I think there's also some refactoring that could be done to move some of the duplicated code into SubstitutionHelper, which I've sketched out on this branch - I think that could be taken out of scope an addressed in a future PR though. |
callumknights
force-pushed
the
add-substitute-event
branch
from
4fff7e5 to
5e0441d
Compare
Also added extra logging for unknown publishing types.
callumknights
force-pushed
the
add-substitute-event
branch
from
5e0441d to
b99667d
Compare
There was a problem hiding this comment.
Looks good to me - thanks for the perseverence on this 🙇🏻
It's probably worth getting back to @csutter as this gets deployed, as he might have an idea of a good way to test it / let us know whether the new behaviour works well enough for the search use case.
It was added in alphagov/publishing-api#2664 It'll resolves the following error: Etl::Edition::Content::Parser::InvalidSchemaError Schema does not exist: substitute (Etl::Edition::Content::Parser::InvalidSchemaError) https://govuk.sentry.io/issues/5176804130/events/7fb1eb6bf44a4e44a88a8008d5dccee9/
This PR is largely a copy of #2652 that I made to get my head around Richards thinking.
Trello: https://trello.com/c/xaN2Dhwk/1005-emit-an-event-when-documents-are-unpublished-through-subsitution
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.