Page MenuHomePhabricator

Only evaluate newly added mentions/subscribers when applying transactions to remarkup blocks
Open, Needs TriagePublic

Description

Downstream: https://phabricator.wikimedia.org/T96464

Setup:

  • A user is @mention'd in a description field (for example, a task description)
  • They unsubscribe from the task.
  • The description field is edited to correct a typo, unrelated to the mention.

This should not re-notify or re-add the user: only newly introduced mentions should trigger mention behavior. If the text already mentioned the user, the mention should not be re-evaluated on edit.

I plan to resolve this by doing this:

  • Compute all old mentions in the text.
  • Compute all new mentions in the text.
  • Remove old mentions from the new mention list.
  • Move forward from there.

In particular, this means that @mentioning a user in a new comment will re-add them. I think this is expected/desirable: you're explicitly calling them back to the task.

Closely related: editing comment should have the same behavior as editing descriptions (evaluate only new mentions).


Related downstream: https://phabricator.wikimedia.org/T76993

If a user is quoted, I think it's reasonable that it shouldn't count as a mention (specifically: @username invocations inside of a quote block do not count as mentions). This may be more trouble to resolve than it's worth, though, since the "mention" part of the code and the "quote" part of the code are fairly oblivious to one another, but I think it's generally reasonable and some straightfoward approach may be available.

Event Timeline

I implemented this, sort of. However, it doesn't work cleanly because transactions are "adjusted" (have old values populated) after they are "expanded" (which generates the @user mention transactions). So when the code runs, it doesn't have access to the old text and can't easily figure out which mentions are new.

I don't immediately see a way to fix this cleanly. I think reordering transaction phases is probably the right approach, but that would affect everything and probably have some unintended side effects.

It's possible to hack this together, but I'm going to sit on it for a bit and hope we can pursue a cleaner fix in the relatively near future, maybe after something triggers T9789 to happen. That change would require extensive testing of things anyway, so it would be more reasonable to bundle changes to phase operation along with it.

epriestley moved this task from Backlog to Preflight on the Prioritized board.Jun 9 2016, 12:56 PM

Current state of the world after D16108 is:

  • editing comments uses the correct rule (only new mentions count).
  • editing other fields still uses the old/wrong rule (all mentions still count).

Fixing the "other fields" case is complicated, per above, and I plan to wait and see how T9789 and adjacent work develop before planning how to continue forward here.

epriestley moved this task from Preflight to The Queue on the Prioritized board.Jun 16 2016, 10:52 AM
epriestley moved this task from The Queue to Paused on the Prioritized board.Jul 2 2016, 7:11 PM

Can we please add the keyword subscribe or subscribers in this task title? I looked through 2 pages of subscribers search before filing a dup. I know now I was searching the wrong term, mention would have given it to me instead. But they are related.

epriestley renamed this task from Only evaluate newly added mentions when applying transactions to remarkup blocks to Only evaluate newly added mentions/subscribers when applying transactions to remarkup blocks.Feb 13 2017, 4:46 PM

T9789 has stabilized (the majority of major transaction types now run through ModularTransactions) although it didn't end up providing much clarity here. This is no longer blocked on anything, I just need to develop a specific technical plan to resolve the "adjust" vs "expand" sequence inside transaction application so the "expand" code has access to the old text by the time it runs.

Twilight removed a subscriber: Twilight.Jul 16 2017, 2:06 PM