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:
Description
Description
Details
Details
- Reference
- bz47143
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | None | T45833 Bugs that should be fixed before deploying Echo to en.wiki (tracking) | |||
| Open | None | T49143 Edit revert should not trigger a talk page post notification if the edit being reverted was performed by the owner of the talk page |
Event Timeline
Comment Actions
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.
Comment Actions
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
Comment Actions
(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.
Comment Actions
Related URL: https://gerrit.wikimedia.org/r/61902 (Gerrit Change Ic2d990057d88c27e6ed25c8d2d4adb995d196f65)
Comment Actions
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 anotherI 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 iIn 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
Comment Actions
Prioritization and scheduling of this bug is tracked on Mingle card https://mingle.corp.wikimedia.org/projects/flow/cards/203