Page MenuHomePhabricator

Edit revert should not trigger a talk page post notification if the edit being reverted was performed by the owner of the talk page
Open, NormalPublic

Description

If you perform a rollback on an edit performed by the owner of the talk page, it sends 2 notifications to the owner. It should only send one - probably the rollback notification.


See Also:

Details

Reference
bz47143

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 1:30 AM
bzimport added a project: Notifications.
bzimport set Reference to bz47143.
kaldari created this task.Apr 11 2013, 11:31 PM
  • Bug 47155 has been marked as a duplicate of this bug. ***

This is actually a bit tricky to fix correctly.

Looked over this, basically the end goal is to not send more than one notification to a user regarding a specific change. Both of these changes come from the same hook which calls EchoHooks::onArticleSaved.

I'm thinking we can create an empty array, $notified that will contain a list of user ids that have already been notified regarding this change. First we do the revert event, if that fires we add the victim's user id to $notified. Next DiscussionParser::generateEventForRevision can run, which will be passed the list of notified users. This can then check before sending the notification about an updated talk page we can do !in_array( $user->getID(), $previouslyNotified ).

A quick test implementing this seems to work, but I may be missing several edge cases that I just dont know about yet. I cant push a fix to gerrit yet though as my ssh keys are not yet valid.

bsitu wrote:

(In reply to comment #3)

Looked over this, basically the end goal is to not send more than one
notification to a user regarding a specific change. Both of these changes
come
from the same hook which calls EchoHooks::onArticleSaved.
I'm thinking we can create an empty array, $notified that will contain a list
of user ids that have already been notified regarding this change. First we
do
the revert event, if that fires we add the victim's user id to $notified.
Next
DiscussionParser::generateEventForRevision can run, which will be passed the
list of notified users. This can then check before sending the notification
about an updated talk page we can do !in_array( $user->getID(),
$previouslyNotified ).
A quick test implementing this seems to work, but I may be missing several
edge
cases that I just dont know about yet. I cant push a fix to gerrit yet
though
as my ssh keys are not yet valid.

This may not work when we enable job queue to process notifications. We can't pass $notifiedUser from one job to another

(In reply to comment #4)

(In reply to comment #3)

Looked over this, basically the end goal is to not send more than one
notification to a user regarding a specific change. Both of these changes
come
from the same hook which calls EchoHooks::onArticleSaved.
I'm thinking we can create an empty array, $notified that will contain a list
of user ids that have already been notified regarding this change. First we
do
the revert event, if that fires we add the victim's user id to $notified.
Next
DiscussionParser::generateEventForRevision can run, which will be passed the
list of notified users. This can then check before sending the notification
about an updated talk page we can do !in_array( $user->getID(),
$previouslyNotified ).
A quick test implementing this seems to work, but I may be missing several
edge
cases that I just dont know about yet. I cant push a fix to gerrit yet
though
as my ssh keys are not yet valid.

This may not work when we enable job queue to process notifications. We
can't
pass $notifiedUser from one job to another

I will look over the job queueing code to get an idea of whats going on there, at which point in execution does it move into job queue? I was thinking i

In this case EchoHooks::onArticleSaved directly calls EchoDiscussionParser::generateEventsForRevision as opposed to going through any kind of indirection, so while not in the general case, in this particular case i think we can pass it through.

Related URL: https://gerrit.wikimedia.org/r/61902 (Gerrit Change Ic2d990057d88c27e6ed25c8d2d4adb995d196f65)

bsitu wrote:

(In reply to comment #5)

(In reply to comment #4)

(In reply to comment #3)

Looked over this, basically the end goal is to not send more than one
notification to a user regarding a specific change. Both of these changes
come
from the same hook which calls EchoHooks::onArticleSaved.
I'm thinking we can create an empty array, $notified that will contain a list
of user ids that have already been notified regarding this change. First we
do
the revert event, if that fires we add the victim's user id to $notified.
Next
DiscussionParser::generateEventForRevision can run, which will be passed the
list of notified users. This can then check before sending the notification
about an updated talk page we can do !in_array( $user->getID(),
$previouslyNotified ).
A quick test implementing this seems to work, but I may be missing several
edge
cases that I just dont know about yet. I cant push a fix to gerrit yet
though
as my ssh keys are not yet valid.

This may not work when we enable job queue to process notifications. We
can't
pass $notifiedUser from one job to another

I will look over the job queueing code to get an idea of whats going on
there,
at which point in execution does it move into job queue? I was thinking i
In this case EchoHooks::onArticleSaved directly calls
EchoDiscussionParser::generateEventsForRevision as opposed to going through
any
kind of indirection, so while not in the general case, in this particular
case
i think we can pass it through.

Yes, you are right. The target users are known before creating the event for both of them, it works in this case

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

EBernhardson removed EBernhardson as the assignee of this task.Apr 23 2015, 10:45 PM
EBernhardson set Security to None.
EBernhardson removed a subscriber: EBernhardson.
Restricted Application added a project: Growth-Team. · View Herald TranscriptSep 4 2018, 2:30 PM