recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix some visibility issues with inline comments in Diffusion

Summary:
Fixes T10519. Two issues:

First, the acting user wasn't explicitly included in the mail. This usually didn't matter, but could matter if you unsubscribed and then interacted.

Second, we had some logic which tried to hide redundant "added inline comment" transactions, but could hide them inappropriately. In particular, if another action (like a subscribe) was present in the same group, we could hide the inlines because of that other transaction, then //also// hide the subscribe. This particular issue is likely an unintended consequence of hiding self-subscribes.

Instead of hiding inlines if //anything else// happened, hide them only if:

- there is another "added a comment" transaction; or
- there is another "added an inline comment" transaction.

This prevents the root issue in T10519 (incorrectly hiding every transaction, and thus not sending the mail) and should generally make behavior a little more consistent and future-proof.

Test Plan:
- Submitted //only// an inline comment on a commit I had not previously interacted with.
- Before patch: no mail was generated (entire mail was improperly hidden).
- After patch: got some mail with my comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10519

Differential Revision: https://secure.phabricator.com/D15407

epriestley 0569919e 090252a8

+18 -13
+2
src/applications/audit/editor/PhabricatorAuditEditor.php
··· 636 636 } 637 637 } 638 638 639 + $phids[] = $this->getActingAsPHID(); 640 + 639 641 return $phids; 640 642 } 641 643
+16 -13
src/applications/transactions/storage/PhabricatorApplicationTransaction.php
··· 605 605 break; 606 606 } 607 607 608 - // If a transaction publishes an inline comment: 609 - // 610 - // - Don't show it if there are other kinds of transactions. The 611 - // rationale here is that application mail will make the presence 612 - // of inline comments obvious enough by including them prominently 613 - // in the body. We could change this in the future if the obviousness 614 - // needs to be increased. 615 - // - If there are only inline transactions, only show the first 616 - // transaction. The rationale is that seeing multiple "added an inline 617 - // comment" transactions is not useful. 608 + if ($this->isInlineCommentTransaction()) { 609 + $inlines = array(); 618 610 619 - if ($this->isInlineCommentTransaction()) { 611 + // If there's a normal comment, we don't need to publish the inline 612 + // transaction, since the normal comment covers things. 620 613 foreach ($xactions as $xaction) { 621 - if (!$xaction->isInlineCommentTransaction()) { 614 + if ($xaction->isInlineCommentTransaction()) { 615 + $inlines[] = $xaction; 616 + continue; 617 + } 618 + 619 + // We found a normal comment, so hide this inline transaction. 620 + if ($xaction->hasComment()) { 622 621 return true; 623 622 } 624 623 } 625 - return ($this !== head($xactions)); 624 + 625 + // If there are several inline comments, only publish the first one. 626 + if ($this !== head($inlines)) { 627 + return true; 628 + } 626 629 } 627 630 628 631 return $this->shouldHide();