Page MenuHomePhabricator

Echo: Add tests for revert detection
Open, MediumPublic3 Estimated Story Points

Description

Echo doesn't test the expected behavior for different kinds of reverts (manual vs undo vs rollback). This led to an accidental change in behavior when working on T397103.

Original Report

We changed $isRevert = $editResult->getRevertMethod() === EditResult::REVERT_UNDO || $editResult->getRevertMethod() === EditResult::REVERT_ROLLBACK; to $isRevert = $event->isRevert();, which is not exactly the same. As a result, manual reverts (i.e. viewing an older version of the page, then clicking edit and save; not using the undo or rollback buttons) will now be treated as reverts by this code.

This doesn't affect the revert notifications themselves, since they are sent elsewhere, but it will affect some other notification types that check whether an edit is a revert before sending notifications: at a glance, this will affect the summary mention notifications too (they are not sent for reverts), possibly fixing T373750 but making T340883 a bit worse; and page linked notifications (also not sent for reverts), apparently fixing T148949.

We also need to add test cases to this.

Event Timeline

I'm not sure that this has to be reverted. It seems like a good change. I would just test and update these tasks that were probably affected. (But it's up to you.)

Do I understand this correctly: When a manual revert mentions the user who's edit is being reverted in the edit summary, the old behavior was for the user to get two notifications, one for the revert and one for the mention. The new behavior is for them to only get the revert notification. That would be consistent with the behavior when the edit summary doesn't mention the user, and when the "rollback" or "undo" feature is used to perform the revert.

If this analysis is correct, then I agree that this is probably an improvement. Formally I think this would be up to @KStoller-WMF to decide. I suppose the Growth-Team should prioritize this. The change is trivial, the hardest bit is really writing a test for it.

HCoplin-WMF triaged this task as Medium priority.Jul 1 2025, 2:57 PM
HCoplin-WMF updated the task description. (Show Details)
HCoplin-WMF set the point value for this task to 3.
daniel renamed this task from DomainEvents: Echo: Revert to the original revert logic to Echo: Add tests for revert detection .Jul 15 2025, 3:08 PM
daniel updated the task description. (Show Details)

Since it turns out the new behavior is fine, the MW-Interfaces-Team isn't going to work on this; the Growth-Team should determin the priority it has for them.

KStoller-WMF added a subscriber: Michael.

I wish we had more time to devote to Echo improvements, but as of now this does not fit within our current priorities.

Adding a Growth engineer since this task relates more to testing / maintenance. @Michael

That might be a nice onboarding task to learn a bit how Echo works.