To be precise, the example in screenshot was not a rollback but a simple revert. Rollback *must* suppress notifications, while other revert systems may need some more work. I suppose, however, that you already have some logic to detect reverts, given that you have notifications for those too.

Right now we are just mirroring the behavior of the existing talk page notification system. While I agree it would probably be better to suppress notifications about rolledback edits, I would consider this an enhancement rather than a 'major' bug as no one has complained about this in the many years we've been doing talk page notifications.

(In reply to comment #2)

Right now we are just mirroring the behavior of the existing talk page
notification system.

I'm not sure, what notification system are you talking about? The new messages bar collates all edits in a single notification, and if spam has not yet reverted when you open the diff then you revert it yourself and you don't receive any notification.
Moreover, both enotif and new messages bar don't produce any permanent clutter: email can be deleted on your client and the bar disappears by itself; the notifications, on the other hand, are by design extremely prominent, and also impossible to delete.

I was talking about the orange bar. If someone vandalizes your talk page and it's reverted, you still get the orange bar notification. It works the same way in Echo. The only difference here is that you get a notification for each action. I agree it's a bug and have sent it to Nischay to work on.

Also, Echo notifications are not permanent. They are transient and disappear completely after they expire. What sort of expiration times they should have hasn't been decided though.

I think some users may be against this. You don't want to see it if its spam, what if its not?

Hiding details like this is not always appreciated.

Also how should this be done? when we detect a revert we need to find out if its of the last edit and then not notify. Also find and remove the previous notification because we 'thought' its spam ?

tchay wrote:

I think some users may be against this. You don't want to see it if its spam,

what if its not?

The goal is not to suppress the notification, it's that one event is generating two notifications (one for the rollback, and the other is for the talk page edit) so the redundant one should be suppressed.

Also how should this be done? when we detect a revert we need to find out if

its of the last edit and then not notify. Also find and remove the previous
notification because we 'thought' its spam ?

Ask kaldari on #wikimedia-ee for information on merging multiple redundant notifications in Echo. There is a system in place for doing this, it just needs to be hooked up for this. :-)

(In reply to comment #6)

Ask kaldari on #wikimedia-ee for information on merging multiple redundant
notifications in Echo. There is a system in place for doing this, it just
needs
to be hooked up for this. :-)

Aha! A merging notification system is cool. I will try asking and finding myself.

Probably the way this would be implemented is in EchoHooks::onRollbackComplete take the ID of the rev that was rolled back and look for any echo events in the database related to this rev and delete them. Just be sure to do this before the rollback (edit-revert) notification itself is created.

(In reply to comment #8)

take the ID of the rev that was rolled back and look for any echo events in
the
database related to this rev and delete them. Just be sure to do this before
the rollback (edit-revert) notification itself is created.

The rev ID is stored in the extra column right? I don't think searching in that BLOB would be a good idea.

Oh yeah, looks like you're right. We may want to add revision as it's own column in that case.

(In reply to comment #10)

Oh yeah, looks like you're right. We may want to add revision as it's own
column in that case.

Does the design team have a say on this? or can we just add the column?
I think its better to take some time to decide on that :)

We can just add the column, but I'd like to make sure Benny thinks it's a good idea since he's handling the back-end components.

tchay wrote:

Adding Luke since this will affect Redis mailbox.

bsitu wrote:

It wouldn't help if we add a revision column + extra index to event table. Upon each event is created, web + email notifications would be either created immediately or a job would be scheduled right away to fire web + email notifications, there is no way to undo an email notification or unschedule a job.

The fast and easy way is just to check request variables: wpUndidRevision exists or action == rollback before creating a talk page notification, if the author of the rollback revision exists and is the same as the talk page owner, then don't send talk page notification.

I am not sure the best general way to solve this yet, just some ideas: We can hold all events creation till the end of a "request", based on the sets of events, use the de-duplication logic ( eg, when not to send talk page notification ) to create a do-not-notify-list in extra field for each event, then create the events. Server side cron may trigger notification and it doesn't have the concept of end of a "request", we may consider different rev_id as end of "request", otherwise, tons of events will be stacked up. Events without rev_id, which very unlikely would create duplicate, should just be handled specially within each notification if they do.

The fast and easy way is just to check request variables: wpUndidRevision
exists or action == rollback before creating a talk page notification, if the
author of the rollback revision exists and is the same as the talk page owner,
then don't send talk page notification."

We already cover for that case. This bug is about retroactively suppressing the notifications that were created for an edit that has been rolled back. (Not the notifications generated by the rollback edit itself). It sounds like there just isn't a scalable way to do this. If anyone has an idea, let add it.

Actually I guess this bug might be about both cases, but I'm not sure from the bug's initial description.

@Nemo: In the case that JarJar vandalizes Kirk's userpage, and BuckRogers reverts JarJar, should Kirk receive one notification about the rollback or none at all?

Either way, we wouldn't want to suppress the notification about the rollback unless we also suppressed the notification about the vandal edit that was rolled back, so if there is no solution for the latter, it's a moot question what to do with the former.

lwelling wrote:

Attempting to address this would take significant work and depending on the exact timing and user settings its behaviour could be very mysterious to users.

With a similar degree of effort to that taken to write the code in hooks to detect if a notification should be fired we could write mirrored logic for each one to detect situations where a notification should be unfired.

Depending on timing and user settings, a user may already have been emailed the notification or may already have viewed it in the web ui. Having it disappear from the list after it's been seen would be mysterious and confusing.

Having the unread counter decrement would also be mysterious.

Given that it cannot be relied on to behave in a consistent, clean way I don't think the significant effort needed to build it and add it to future notification types is worthwhile.

(In reply to comment #16)

@Nemo: In the case that JarJar vandalizes Kirk's userpage, and BuckRogers
reverts JarJar, should Kirk receive one notification about the rollback or
none
at all?

Ideally none (there's watchlist for that), or only one notification (about the rollback) as with the new user message bar.

(In reply to comment #18)

Attempting to address this would take significant work and depending on the
exact timing and user settings its behaviour could be very mysterious to
users.

I quite disagree on this, because Special:Notifications shows messages that don't actually exist (they've been removed from your talk page, possibly even deleted or suppressed?): this seems way more confusing and mysterious to me, and it's a regression.

Of course I can't help finding an implementation, but I wouldn't think the underlying database so important: the notifications special page and flyout should just check if the message actually exists before showing it, independently from its presence in whatever table. For this bug, in most cases it will be enough to check that rev_sha1 is not the same after JarJar's edit as it was before it.

Possibly related to the very old Bug 2939 ('"You've got new message" is displayed even after a revert')

(Which I found whilst trying to find an answer to this (new bug?) https://en.wikipedia.org/wiki/Wikipedia_talk:Notifications#Edit_reverts )

(In reply to comment #20)

Possibly related to the very old Bug 2939 ('"You've got new message" is
displayed even after a revert')

Well, it's related in the sense that Echo probably makes that bug disappear, but this bug is worse because it's not about the new message indicator but about the notifications, which 1) can't be deleted, 2) don't just tell you something happened on your talk as the old message bar, 3) don't go away after visiting the talk.

Prioritization and scheduling of this bug is tracked on Mingle card https://mingle.corp.wikimedia.org/projects/flow/cards/198

Is Nischay still working on this, or should the assignee be reset to default?

Saw this now. I am not working on it anymore.

Potential implementation: just as markbotedits marks edits as bot edits when they are reverted after the original edit was made, could it be extended to also mark such edits, when on user talk pages, as minor edits, and use nominornewtalk to suppress the notifications from both the original edit and the revert?

Thanks @DannyS712 for this idea. The Growth-Team is not planning to prioritize this at this time unless it becomes urgent.

I think this can now be implemented easily by checking whether the revision has mw-reverted tag (T254074) in canRender().