Page MenuHomePhabricator

Decision Required: Handling Admin Watchlist notifications
Closed, ResolvedPublic

Description

When working on Notifications, I found out an interesting use case with our ENotif system, and I think we need to clarify the use case for Admin notifications. I had an idea to deprecate and remove the AbortEmailNotification hook - T389618, as the Middleware could take care of everything.

Previously, we had a system that was doing

  • On RecentChange save - fire onAbortEmailNotification that allows to stop ENotif from happening entirely
  • when onAbortEmailNotification hook didn't stop the execution system would trigger three flows:
    • if User_Talk edit and AbortTalkPageEmailNotificationdin't stop - send Talk page email
    • take all users who watch changed page and for each execute SendWatchlistEmailNotification hook, if didn't stop - send watchlist email
    • take users from UsersNotifiedOnAllChanges config, and send email for each user.

As you can see, we had two types of hooks:

  • AbortTalkPageEmailNotification that could be used to stop entire notification. For example Translate is using it to prevent translationreviews from triggering notifications.
  • AbortTalkPageEmailNotification and SendWatchlistEmailNotification could stop specific Talk or watchlist notifications.

Now, we can utilize Middleware Pattern to deprecate and remove all three hooks - but it got me thinking - do we want to keep notifications as MediaWiki Notifications ? I mean, this seems an internal, and maybe the logic that handles notifying people via UsersNotifiedOnAllChanges conflig flag shouldn't trigger notifications but send emails immediately?

For explanation, the current flow is:

RecentChange -> EmailNotification -> NotificationService -> RecentChangeMailComposer.

and here I'm thinking out loud, this makes perfect sense for Talk and Watchlist notifications. But I started wondering, maybe Admin notifications should go

RecentChange -> EmailNotification -> RecentChangeMailComposer

Without becoming a notification that Echo could suppress and turn into something else. This would also mean that we would need to keep the AbortEmailNotification hook to let extensions abort entire flow. The Middleware would allow only to filter Watchlist and Talk notifications.

See also:

Decision:

Treat admin notifications the same way as other notifications. Deprecate AbortEmailNotification hook.

Event Timeline

I had to chew on it for a bit, and I'm in favor to deprecate AbortEmailNotification hook and make the Admin notifications behave like regular notifications. Extensions can already stop those from happening, if we could deprecate another hook and let everyone use just the Middleware - it will simplify the system.
The only challenge is that we need to be little bit more cautious when migrating Echo watchlist notifications and allow Echo only to override watchlist and talk.

An example patch that uses Middleware pattern to migrate away from AbortEmailNotification: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Translate/+/1138814

Change #1138814 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/extensions/Translate@master] Use middleware instead of AbortEmailNotification hook

https://gerrit.wikimedia.org/r/1138814

We decided to treat all the notifications in the same way, this will allow us to deprecate AbortEmailNotification. Extensions will have to use the NotificationsMiddleware to stop watchlist notifications from happening