Last modified: 2014-02-09 04:23:26 UTC
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.
*** 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.
(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)
(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
