Flow and DiscussionTools make their own notifications, we don't need a duplicate notification saying that the relevant watched page was also modified.
Acceptance criteria:
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | None | T125653 Create new types of notifications | |||
| Resolved | None | T311850 [Epic] FY 2022-23 Growth Maintenance Work | |||
| Open | None | T309855 Deploy WatchlistNotifications feature to production wikis | |||
| Resolved | Feature | kostajh | T203941 Allow watchlist notifications to be delivered as web notification (through Echo) | ||
| Open | None | T309839 Echo web watchlist notifications should not include Flow or DiscussionTools |
Change 802734 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):
[mediawiki/extensions/Echo@master] WatchlistNotifications: Don't generate notifications for Flow posts
Change 802747 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):
[mediawiki/extensions/DiscussionTools@master] TagHooks: Make static method for obtaining DiscussionTools tags
Change 802748 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):
[mediawiki/extensions/Echo@master] WatchlistNotifications: Don't generate notifications for DiscussionTools edits
I'll note that it's not going to be possible to catch all edits that will send DiscussionTools notifications, because they're generated from Echo's post-save deferredupdate by parsing new revisions for comments. Thus it also sends notifications when people edit via WikiEditor and so don't use the new tools at all.
Also, I'm not sure this is a good idea. The set of people receiving the notifications are different -- DiscussionTools sends to people who have subscribed to a thread on the page, watchlist sends to people who have added the page to their watchlist. There's not necessarily an overlap between these groups. If you must block this, it should only be for people who're getting the DT notifications.
Change 802747 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] TagHooks: Make static method for obtaining DiscussionTools tags
Getting two functionally almost identical notifications for the same edit definitely seems annoying. (It does seem somewhat pointless to watch a page and be subscribed to some of its talk page sections at the same time, but with so many tools automatically watching/subscribing, it's bound to happen regularly, possibly without the user knowing about it.)
It would be easy to deduplicate them with an Echo user filter, I think.
@Tgr There are some edge cases in there which are more complex. E.g. a given revision might trigger a DiscussionTools subscription notification and also have made changes elsewhere on the page, and that might make the user miss the non-talk-topic portion of the edits. (This is presumably uncommon, of course.)
Ignoring that overlap, and because of the nature of the watchlist (i.e. watching a page means you're watching it + its talk page, and you can't watch just one or the other), I'd be inclined to view a DT notification as a more-specific version of a watchlist notification that overrides whether the watchlist notification for a given edit is sent. I.e. if you're watching a page then you should get a watchlist notification... unless you're going to get a subscription notification because a specific section you're subscribed to had a comment added.
I'm not really familiar with Echo filters (and codesearch didn't turn up any uses of EchoFilteredSequentialIterator outside of the default few in Echo's NotificationController), but am I correct to assume that a skeleton of your idea would be:
If so, I think this would involve a certain amount of refactoring of how DiscussionTools sends its own notifications, as currently it doesn't parse the revision to diff the comment trees until it's in the middle of generating its own events. Because calling that is handled by Echo (which uses the PageSaveComplete hook to run it in a DeferredUpdate, I think?) we can't rely on the data being available before the equivalent watchlist event dispatch runs.
(Personally, I do feel that this might be overcomplicating things, and we should let it go and see if people actually notice and complain before we put in the work on this. 🤷🏻)
EchoGetDefaultNotifiedUsers can be used to filter out users, although in theory it's possible that one hook handler does some filtering and then another hook handler adds a user which should have been filtered, so having a separate filter step is cleaner, but I'm not sure that's worth worrying about in practice.
The really nice approach would be some kind of suppression option in Echo (where event type X can just be configured to suppress events of type Y to the same user and revision), but that would be a major change (and revision isn't even a standard parameter of an Echo event).
EchoGetDefaultNotifiedUsers can be used to filter out users
I don't think this is right -- it seems to just be a way to get additional users to be added to the event.
$users = [];
Hooks::run( 'EchoGetDefaultNotifiedUsers', [ $event, &$users ] );
// @phan-suppress-next-line PhanImpossibleCondition May be set by hook
if ( $users ) {
$notify->add( $users );
}(With the user-locators having already run before this.)
Change 802734 abandoned by Kosta Harlan:
[mediawiki/extensions/Echo@master] WatchlistNotifications: Don't generate notifications for Flow posts
Reason:
Change 802748 abandoned by Kosta Harlan:
[mediawiki/extensions/Echo@master] WatchlistNotifications: Don't generate notifications for DiscussionTools edits
Reason: