From 0569919eab545806c1632a1ab49cde4f91266950 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 5 Mar 2016 13:49:46 -0800
Subject: [PATCH] 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
---
 .../audit/editor/PhabricatorAuditEditor.php   |  2 ++
 .../PhabricatorApplicationTransaction.php     | 29 ++++++++++---------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
index 51efdf175b..3df9013301 100644
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -636,6 +636,8 @@ final class PhabricatorAuditEditor
       }
     }
 
+    $phids[] = $this->getActingAsPHID();
+
     return $phids;
   }
 
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 8084ea302f..ee8a7a47a3 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -605,24 +605,27 @@ abstract class PhabricatorApplicationTransaction
         break;
     }
 
-    // If a transaction publishes an inline comment:
-    //
-    //   - Don't show it if there are other kinds of transactions. The
-    //     rationale here is that application mail will make the presence
-    //     of inline comments obvious enough by including them prominently
-    //     in the body. We could change this in the future if the obviousness
-    //     needs to be increased.
-    //   - If there are only inline transactions, only show the first
-    //     transaction. The rationale is that seeing multiple "added an inline
-    //     comment" transactions is not useful.
-
     if ($this->isInlineCommentTransaction()) {
+      $inlines = array();
+
+      // If there's a normal comment, we don't need to publish the inline
+      // transaction, since the normal comment covers things.
       foreach ($xactions as $xaction) {
-        if (!$xaction->isInlineCommentTransaction()) {
+        if ($xaction->isInlineCommentTransaction()) {
+          $inlines[] = $xaction;
+          continue;
+        }
+
+        // We found a normal comment, so hide this inline transaction.
+        if ($xaction->hasComment()) {
           return true;
         }
       }
-      return ($this !== head($xactions));
+
+      // If there are several inline comments, only publish the first one.
+      if ($this !== head($inlines)) {
+        return true;
+      }
     }
 
     return $this->shouldHide();