Page MenuHomePhorge

Do not re-subscribe @mentions when editing task description
Needs RevisionPublic

Authored by aklapper on Aug 13 2024, 20:12.

Details

Summary

Do not re-subscribe users who are mentioned in a task description and are not task subscribers when editing an existing task description.
They should not forcefully get re-subscribed as these users were not "newly" mentioned (again).

Closes T15915

Test Plan
  • Create a new task with two users mentioned in the description: Both users get subscribed.
  • Create a new task with two users mentioned in the description and explicitly add both users to the Subscribers field: Both users get subscribed.
  • Edit an existing task description which mentions two users who are also subscribed, plus an additional user being a subscriber, remove the two mentions from the task description: The previous subscriber and the two previously mentioned users are still subscribed become subscribers.
  • Remove two users mentioned in the task description from the Subscribers field plus the task has an additional user being a subscriber, then edit the existing task description (by adding a random word) which mentions the two users who are not subscribed (anymore): Both mentioned users do not get re-subscribed anymore and third user is still subscribed.
  • Edit an existing task which mentions two users who are not subscribed (anymore) without changing the description, plus the task has an additional user being a subscriber, and explicitly re-add the two users mentioned in the description to the Subscribers field: The previous subscriber and the two mentioned users are subscribers.

Diff Detail

Repository
rP Phorge
Branch
T15915resubDescMentions
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1523
Build 1523: arc lint + arc unit

Event Timeline

Eh, there is a trade-off: Editing the description of an existing task to add a @mention will not add this newly mentioned user as a task subscriber anymore.

I guess there could be even more code to compare/parse the old description text and the new description text for new @mentions not existing in the old description, or we could just accept this corner case for the time being?

Thanks for this prototype but unfortunately this approach has the problem that it's verticalized on Maniphest and it should really not be expanded for Phriction or to any other component that may have a description and may suffer the same thing.

More understanding on the root cause is needed. Probably the root cause is "just" that getOldValue() returns an empty string.

In that case we should probably at least understand what object is that (sub-class of PhabricatorTransactionRemarkupChange?) and we probably we need something like a generateOldValue() or something similar.

More discussion needed. Thanks.

https://we.phorge.it/T15915#

This revision now requires changes to proceed.Aug 19 2024, 10:35

More understanding on the root cause is needed. Probably the root cause is "just" that getOldValue() returns an empty string. In that case we should probably at least understand what object is that (sub-class of PhabricatorTransactionRemarkupChange?) and we probably we need something like a generateOldValue() or something similar.

Per https://we.phorge.it/rP65634781b44fb6a5e182c896705d44e04490ba76 ,

The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order.

That commit deprecated getRemarkupBlocks() still called in PhabricatorAuditTransaction and in the foreach loop in PhabricatorApplicationTransaction which does ->setOldValue(null).