Update subscriber list titles on major / minor publication events

[huwd]

Trello

Depends on

alphagov/email-alert-api#1704
(and it's release)

What

The GOV.UK Accounts team added a feature where logged in users could
subscribe to email notifications for individual pages.

SubscriberLists store the titles of those pages so that we can:

• Render a list of subscriptions in the user's manage dashboard
• Include the title of the subscriber list in emails alerting the user
  that the page has been unpublished. (Especially as there's no-longer a
  content item we can query for title at that point).

We record these titles when the SubscriberList is first made, however
there has not been a mechanism to update it. This is a problem as
content titles do change over time.

This PR adds a class we can use to gather information from major and
minor change messages and update the subscriber list titles using a new
Email Alert API endpoint

Other parts of email-alert-service make calls to services that use GDS
API adaptors. However they seem not to use the test helpers that ship
with the gem through webmock.

A note on test strategy

In previous work we decided to keep the testing strategy consistent
across the repo, and also avoided gds-api-test helpers. Instead we
tested with double and spys.

This got us into a bit of trouble when our parameters and payloads were
not matching the gds-api-adaptors, and reminded us how helpful those
test helpers were.

So we've introduced stubs and webmock for this test, aware this
introduces an inconsistent test strategy.

We intend to go back and refactor tests for the recently introduced
unpublication alert work. And hopefully refactor the rest of the test
suite to.

If you are far in the future and that didn't happen. Sorry! But
hopefully you at least know why these tests don't look like the others.

(1): alphagov/email-alert-api#1704

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

[barrucadu]

As a general comment, it feels a bit heavyweight adding a second major change processor for this. Not only do we have two message queues with exactly the same events in them, it could be kind of confusing to someone encountering this for the first time.

So rather than adding a second major change queue, my inclination would be to do both the content change triggering and the UpdateSubscriberList.new(document, logger).trigger inside the existing major change queue - and then call the new minor change queue minor-change-queue-consumer to fit the naming pattern of the others.

[huwd]

#493 (review)

Yeah I see what you mean, and that would be slimmer, my fear is that if we entangle the two is there any risk of errors in our updates preventing content-changes from occuring?

I can't really think of a sensible secenario, given they are both hitting the same email-alert-api.
But given the importance of content change notificaitons, and the low cost to having more workers, seemed lower risk to make separate ones?

Do you think it remains confusing even though we've given them a very explicit name here?

[barrucadu]

Mmm, that's a good point. In that case, I think we should have one new worker which receives all major and minor publishing events - rather than two new workers which do the same thing but with slightly different inputs.

[huwd]

@alex9smith and I tried that initially.
we ran into a couple of problems. Trying to trigger two .run processes from the same rake task never worked. We suspect it's because .run never returns but just keeps the process open.

So we couldn't have two running side by side from the same rake task?

Then we went down a RabbitMQ hole of "can you use regex to do multiple key matches". I found some blog posts with someone talking about "a cool hack" they'd written that seemed to suggest it's not built in. Without a super easy, standard way to match multiple routing keys (major || minor), making two with their own processors seemed like the best route?

[barrucadu]

I think matching *.*.# would be fine - you get all events, the ones with titles and/or descriptions trigger an update to the list, the others don't.

[huwd]

Yeah that does work, but my concern there is that all our logger events just start clogging up logs.
For every irrelivant event there's now a validation logger saying our thing doesn't have enough info - or that it tried to check against subscriber list params and no update was needed or 404.

I don't know how many events there might be... but seems like a lot.
Future log splunkers may not thank us for it?

Having the two new processors seems to give us the targeting we want, with some admitted cruft.
I'd hope the naming makes it explicit and we've left clear commit messages here about what we are doing.

I'd hope at least that gives a solid base for a future refactor if more of these get added in future and the number of processors becomes unwealdy?

[barrucadu]

Ok, I'm not going to die on this hill - let's go with the approach of having two new queues.

[barrucadu]

"A note on test strategy" in 1175454 can be removed by approving #494

[barrucadu]

I personally would squash 506ff53 into 2177521, but it's not majorly important