Page MenuHomePhorge

Do not re-subscribe mentioned users when editing task description
Open, Needs TriagePublic

Description

Steps to reproduce:

  1. PHP 8.3.10, Phorge at 58f1c83a5ac56073a1960772c78c99ece72d4729
  2. Create a task with a task description which @mentions two usernames
  3. These users automatically become task subscribers.
  4. Remove the two users from the Subscribers field.
  5. Edit the task description, e.g. by adding a character.

Expected outcome:
As the users were unsubscribed, they should not get resubscribed, as no new interaction with these users happened and as the users were not (newly) mentioned (again).
(However, mentioned users should get subscribed at initial task creation when mentioned in the description.)

Actual outcome:
Users get forcefully re-subscribed to the task.

Other comments:

  • This is obviously different from quoting the previous comment of another user in a comment. Quoting Evan from https://secure.phabricator.com/T11035: "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."
  • Downstream https://phabricator.wikimedia.org/T96464 mentions clicking Mute Notifications as a workaround but this is not intuitive (and not immediately obvious why the user should use that hammer if the user already unsubscribed anyway).

Event Timeline

In your opinion, why is the current code not working? I mean, it should do exactly that, checking the text before, and the text after, so, to only mention newly-introduced mentions.

https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php;b9ea6f1ce823dcd83a431f5913d9b1fe29dd25bd$2050-2064

Ideally, what it "should do," is it correct? If yes, probably there is a bug in the diff or in the $old_text or something 🤔

I mean, it should do exactly that,

It does not. Inserting

phlog(pht('$old_phids: %s', json_encode($old_phids)));
phlog(pht('$new_phids: %s', json_encode($new_phids)));

into https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php$2063 outputs:

PHLOG: '$old_phids: []' at [/var/www/html/phorge/phorge/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2063]
PHLOG: '$new_phids: {"PHID-USER-rhylswrkfbga632672ps":"PHID-USER-rhylswrkfbga632672ps","PHID-USER-ak4lsy27fuwaq7jss7ex":"PHID-USER-ak4lsy27fuwaq7jss7ex","PHID-USER-pu3kb2cppwbdhc4gyzzt":"PHID-USER-pu3kb2cppwbdhc4gyzzt","PHID-USER-lgzmm2xvqnok3mdz7dzy":"PHID-USER-lgzmm2xvqnok3mdz7dzy","PHID-USER-trmm3xkkqygnrb5jk7k6":"PHID-USER-trmm3xkkqygnrb5jk7k6"}' at [/var/www/html/phorge/phorge/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2064]

https://secure.phabricator.com/T11035#179600 says "when the code runs, it doesn't have access to the old text and can't easily figure out which mentions are new"

Exactly. Like I said probably you will find interesting things debugging $old_text. I guess it's empty. That's probably our root cause. What do you think about?

In that case, we "just" probably need something that fills the old text when it is empty I guess.