If users are on the email to Phabricator, do not send them the Phabricator reply.

Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.

Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.

Reviewers: epriestley, vrana

Reviewed By: vrana

CC: aran, Korvin, vrana

Maniphest Tasks: T1676

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

Author: bobtrahan

src/__phutil_library_map__.php

@@ -668,6 +668,7 @@
     'PhabricatorEdgeGraph' => 'infrastructure/edges/util/PhabricatorEdgeGraph.php',
     'PhabricatorEdgeQuery' => 'infrastructure/edges/query/PhabricatorEdgeQuery.php',
     'PhabricatorEdgeTestCase' => 'infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php',
+    'PhabricatorEditor' => 'infrastructure/PhabricatorEditor.php',
     'PhabricatorEmailLoginController' => 'applications/auth/controller/PhabricatorEmailLoginController.php',
     'PhabricatorEmailTokenController' => 'applications/auth/controller/PhabricatorEmailTokenController.php',
     'PhabricatorEmailVerificationController' => 'applications/people/controller/PhabricatorEmailVerificationController.php',
@@ -1450,6 +1451,7 @@
     'DifferentialChangesetParserTestCase' => 'ArcanistPhutilTestCase',
     'DifferentialChangesetViewController' => 'DifferentialController',
     'DifferentialComment' => 'DifferentialDAO',
+    'DifferentialCommentEditor' => 'PhabricatorEditor',
     'DifferentialCommentMail' => 'DifferentialMail',
     'DifferentialCommentPreviewController' => 'DifferentialController',
     'DifferentialCommentSaveController' => 'DifferentialController',
@@ -1507,6 +1509,7 @@
     'DifferentialRevisionCommentView' => 'AphrontView',
     'DifferentialRevisionDetailView' => 'AphrontView',
     'DifferentialRevisionEditController' => 'DifferentialController',
+    'DifferentialRevisionEditor' => 'PhabricatorEditor',
     'DifferentialRevisionIDFieldParserTestCase' => 'PhabricatorTestCase',
     'DifferentialRevisionIDFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialRevisionListController' => 'DifferentialController',
@@ -1714,6 +1717,7 @@
       1 => 'PhabricatorMarkupInterface',
     ),
     'ManiphestTransactionDetailView' => 'ManiphestView',
+    'ManiphestTransactionEditor' => 'PhabricatorEditor',
     'ManiphestTransactionListView' => 'ManiphestView',
     'ManiphestTransactionPreviewController' => 'ManiphestController',
     'ManiphestTransactionSaveController' => 'ManiphestController',
@@ -1765,6 +1769,7 @@
     'PhabricatorApplicationsListController' => 'PhabricatorController',
     'PhabricatorAuditAddCommentController' => 'PhabricatorAuditController',
     'PhabricatorAuditComment' => 'PhabricatorAuditDAO',
+    'PhabricatorAuditCommentEditor' => 'PhabricatorEditor',
     'PhabricatorAuditCommitListView' => 'AphrontView',
     'PhabricatorAuditController' => 'PhabricatorController',
     'PhabricatorAuditDAO' => 'PhabricatorLiskDAO',
@@ -1839,6 +1844,7 @@
     'PhabricatorDraftDAO' => 'PhabricatorLiskDAO',
     'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants',
     'PhabricatorEdgeCycleException' => 'Exception',
+    'PhabricatorEdgeEditor' => 'PhabricatorEditor',
     'PhabricatorEdgeGraph' => 'AbstractDirectedGraph',
     'PhabricatorEdgeQuery' => 'PhabricatorQuery',
     'PhabricatorEdgeTestCase' => 'PhabricatorTestCase',
@@ -2083,6 +2089,7 @@
     'PhabricatorProjectController' => 'PhabricatorController',
     'PhabricatorProjectCreateController' => 'PhabricatorProjectController',
     'PhabricatorProjectDAO' => 'PhabricatorLiskDAO',
+    'PhabricatorProjectEditor' => 'PhabricatorEditor',
     'PhabricatorProjectEditorTestCase' => 'PhabricatorTestCase',
     'PhabricatorProjectListController' => 'PhabricatorProjectController',
     'PhabricatorProjectMembersEditController' => 'PhabricatorProjectController',
@@ -2202,6 +2209,7 @@
     'PhabricatorStorageManagementWorkflow' => 'PhutilArgumentWorkflow',
     'PhabricatorSubscribersQuery' => 'PhabricatorQuery',
     'PhabricatorSubscriptionsEditController' => 'PhabricatorController',
+    'PhabricatorSubscriptionsEditor' => 'PhabricatorEditor',
     'PhabricatorSubscriptionsUIEventListener' => 'PhutilEventListener',
     'PhabricatorSymbolNameLinter' => 'ArcanistXHPASTLintNamingHook',
     'PhabricatorTaskmasterDaemon' => 'PhabricatorDaemon',
@@ -2229,6 +2237,7 @@
       1 => 'PhutilPerson',
     ),
     'PhabricatorUserDAO' => 'PhabricatorLiskDAO',
+    'PhabricatorUserEditor' => 'PhabricatorEditor',
     'PhabricatorUserEmail' => 'PhabricatorUserDAO',
     'PhabricatorUserLDAPInfo' => 'PhabricatorUserDAO',
     'PhabricatorUserLog' => 'PhabricatorUserDAO',
@@ -2304,6 +2313,7 @@
     'PhrictionDiffController' => 'PhrictionController',
     'PhrictionDocument' => 'PhrictionDAO',
     'PhrictionDocumentController' => 'PhrictionController',
+    'PhrictionDocumentEditor' => 'PhabricatorEditor',
     'PhrictionDocumentPreviewController' => 'PhrictionController',
     'PhrictionDocumentStatus' => 'PhrictionConstants',
     'PhrictionDocumentTestCase' => 'PhabricatorTestCase',
@@ -2318,6 +2328,7 @@
       1 => 'PhabricatorMarkupInterface',
       2 => 'PonderVotableInterface',
     ),
+    'PonderAnswerEditor' => 'PhabricatorEditor',
     'PonderAnswerListView' => 'AphrontView',
     'PonderAnswerPreviewController' => 'PonderController',
     'PonderAnswerQuery' => 'PhabricatorOffsetPagedQuery',
@@ -2329,6 +2340,7 @@
       0 => 'PonderDAO',
       1 => 'PhabricatorMarkupInterface',
     ),
+    'PonderCommentEditor' => 'PhabricatorEditor',
     'PonderCommentListView' => 'AphrontView',
     'PonderCommentMail' => 'PonderMail',
     'PonderCommentQuery' => 'PhabricatorQuery',
@@ -2347,6 +2359,7 @@
     ),
     'PonderQuestionAskController' => 'PonderController',
     'PonderQuestionDetailView' => 'AphrontView',
+    'PonderQuestionEditor' => 'PhabricatorEditor',
     'PonderQuestionPreviewController' => 'PonderController',
     'PonderQuestionQuery' => 'PhabricatorOffsetPagedQuery',
     'PonderQuestionSummaryView' => 'AphrontView',
@@ -2355,6 +2368,7 @@
     'PonderRuleQuestion' => 'PhabricatorRemarkupRuleObjectName',
     'PonderUserProfileView' => 'AphrontView',
     'PonderVotableView' => 'AphrontView',
+    'PonderVoteEditor' => 'PhabricatorEditor',
     'PonderVoteSaveController' => 'PonderController',
     'QueryFormattingTestCase' => 'PhabricatorTestCase',
   ),

src/applications/audit/PhabricatorAuditReplyHandler.php

@@ -61,7 +61,9 @@ protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
       ->setContent($mail->getCleanTextBody());
 
     $editor = new PhabricatorAuditCommentEditor($commit);
-    $editor->setUser($actor);
+    $editor->setActor($actor);
+    $editor->setExcludeMailRecipientPHIDs(
+      $this->getExcludeMailRecipientPHIDs());
     $editor->addComment($comment);
   }
 

src/applications/audit/controller/PhabricatorAuditAddCommentController.php

@@ -61,7 +61,7 @@ public function processRequest() {
     }
 
     id(new PhabricatorAuditCommentEditor($commit))
-      ->setUser($user)
+      ->setActor($user)
       ->setAttachInlineComments(true)
       ->addAuditors($auditors)
       ->addCCs($ccs)

src/applications/audit/editor/PhabricatorAuditCommentEditor.php

@@ -16,10 +16,9 @@
  * limitations under the License.
  */
 
-final class PhabricatorAuditCommentEditor {
+final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
 
   private $commit;
-  private $user;
 
   private $attachInlineComments;
   private $auditors = array();
@@ -30,11 +29,6 @@ public function __construct(PhabricatorRepositoryCommit $commit) {
     return $this;
   }
 
-  public function setUser(PhabricatorUser $user) {
-    $this->user = $user;
-    return $this;
-  }
-
   public function addAuditors(array $auditor_phids) {
     $this->auditors = array_merge($this->auditors, $auditor_phids);
     return $this;
@@ -53,7 +47,7 @@ public function setAttachInlineComments($attach_inline_comments) {
   public function addComment(PhabricatorAuditComment $comment) {
 
     $commit = $this->commit;
-    $user = $this->user;
+    $actor = $this->getActor();
 
     $other_comments = id(new PhabricatorAuditComment())->loadAllWhere(
       'targetPHID = %s',
@@ -64,12 +58,12 @@ public function addComment(PhabricatorAuditComment $comment) {
       $inline_comments = id(new PhabricatorAuditInlineComment())->loadAllWhere(
         'authorPHID = %s AND commitPHID = %s
           AND auditCommentID IS NULL',
-        $user->getPHID(),
+        $actor->getPHID(),
         $commit->getPHID());
     }
 
     $comment
-      ->setActorPHID($user->getPHID())
+      ->setActorPHID($actor->getPHID())
       ->setTargetPHID($commit->getPHID())
       ->save();
 
@@ -106,13 +100,13 @@ public function addComment(PhabricatorAuditComment $comment) {
       $ccs = array_merge($ccs, $metacc);
     }
 
-    // When a user submits an audit comment, we update all the audit requests
+    // When an actor submits an audit comment, we update all the audit requests
     // they have authority over to reflect the most recent status. The general
     // idea here is that if audit has triggered for, e.g., several packages, but
     // a user owns all of them, they can clear the audit requirement in one go
     // without auditing the commit for each trigger.
 
-    $audit_phids = self::loadAuditPHIDsForUser($this->user);
+    $audit_phids = self::loadAuditPHIDsForUser($actor);
     $audit_phids = array_fill_keys($audit_phids, true);
 
     $requests = id(new PhabricatorRepositoryAuditRequest())
@@ -128,7 +122,7 @@ public function addComment(PhabricatorAuditComment $comment) {
     // and handle the no-effect cases (e.g., closing and already-closed audit).
 
 
-    $user_is_author = ($user->getPHID() == $commit->getAuthorPHID());
+    $actor_is_author = ($actor->getPHID() == $commit->getAuthorPHID());
 
     if ($action == PhabricatorAuditActionConstants::CLOSE) {
       // "Close" means wipe out all the concerns.
@@ -144,33 +138,34 @@ public function addComment(PhabricatorAuditComment $comment) {
       // user row (never package/project rows), and always affects the user
       // row (other actions don't, if they were able to affect a package/project
       // row).
-      $user_request = null;
+      $actor_request = null;
       foreach ($requests as $request) {
-        if ($request->getAuditorPHID() == $user->getPHID()) {
-          $user_request = $request;
+        if ($request->getAuditorPHID() == $actor->getPHID()) {
+          $actor_request = $request;
           break;
         }
       }
-      if (!$user_request) {
-        $user_request = id(new PhabricatorRepositoryAuditRequest())
+      if (!$actor_request) {
+        $actor_request = id(new PhabricatorRepositoryAuditRequest())
           ->setCommitPHID($commit->getPHID())
-          ->setAuditorPHID($user->getPHID())
+          ->setAuditorPHID($actor->getPHID())
           ->setAuditReasons(array("Resigned"));
       }
 
-      $user_request
+      $actor_request
         ->setAuditStatus(PhabricatorAuditStatusConstants::RESIGNED)
         ->save();
 
-      $requests[] = $user_request;
+      $requests[] = $actor_request;
     } else {
       $have_any_requests = false;
       foreach ($requests as $request) {
         if (empty($audit_phids[$request->getAuditorPHID()])) {
           continue;
         }
 
-        $request_is_for_user = ($request->getAuditorPHID() == $user->getPHID());
+        $request_is_for_actor =
+          ($request->getAuditorPHID() == $actor->getPHID());
 
         $have_any_requests = true;
         $new_status = null;
@@ -181,15 +176,15 @@ public function addComment(PhabricatorAuditComment $comment) {
             // Commenting or adding cc's/auditors doesn't change status.
             break;
           case PhabricatorAuditActionConstants::ACCEPT:
-            if (!$user_is_author || $request_is_for_user) {
+            if (!$actor_is_author || $request_is_for_actor) {
               // When modifying your own commits, you act only on behalf of
               // yourself, not your packages/projects -- the idea being that
               // you can't accept your own commits.
               $new_status = PhabricatorAuditStatusConstants::ACCEPTED;
             }
             break;
           case PhabricatorAuditActionConstants::CONCERN:
-            if (!$user_is_author || $request_is_for_user) {
+            if (!$actor_is_author || $request_is_for_actor) {
               // See above.
               $new_status = PhabricatorAuditStatusConstants::CONCERNED;
             }
@@ -203,7 +198,7 @@ public function addComment(PhabricatorAuditComment $comment) {
         }
       }
 
-      // If the user has no current authority over any audit trigger, make a
+      // If the actor has no current authority over any audit trigger, make a
       // new one to represent their audit state.
       if (!$have_any_requests) {
         $new_status = null;
@@ -227,7 +222,7 @@ public function addComment(PhabricatorAuditComment $comment) {
 
         $request = id(new PhabricatorRepositoryAuditRequest())
           ->setCommitPHID($commit->getPHID())
-          ->setAuditorPHID($user->getPHID())
+          ->setAuditorPHID($actor->getPHID())
           ->setAuditStatus($new_status)
           ->setAuditReasons(array("Voluntary Participant"))
           ->save();
@@ -270,7 +265,7 @@ public function addComment(PhabricatorAuditComment $comment) {
           ->setAuditorPHID($auditor_phid)
           ->setAuditStatus($audit_requested)
           ->setAuditReasons(
-            array('Added by ' . $user->getUsername()))
+            array('Added by ' . $actor->getUsername()))
           ->save();
       }
     }
@@ -283,7 +278,7 @@ public function addComment(PhabricatorAuditComment $comment) {
           ->setAuditorPHID($cc_phid)
           ->setAuditStatus($audit_cc)
           ->setAuditReasons(
-            array('Added by ' . $user->getUsername()))
+            array('Added by ' . $actor->getUsername()))
           ->save();
       }
     }
@@ -322,13 +317,10 @@ public static function loadAuditPHIDsForUser(PhabricatorUser $user) {
     }
 
     // The user can audit on behalf of all projects they are a member of.
-    $query = new PhabricatorProjectQuery();
-
-    // TODO: As above.
-    $query->setViewer($user);
-
-    $query->withMemberPHIDs(array($user->getPHID()));
-    $projects = $query->execute();
+    $projects = id(new PhabricatorProjectQuery())
+      ->setViewer($user)
+      ->withMemberPHIDs(array($user->getPHID()))
+      ->execute();
     foreach ($projects as $project) {
       $phids[$project->getPHID()] = true;
     }
@@ -341,18 +333,18 @@ private function publishFeedStory(
     array $more_phids) {
 
     $commit = $this->commit;
-    $user = $this->user;
+    $actor = $this->getActor();
 
     $related_phids = array_merge(
       array(
-        $user->getPHID(),
+        $actor->getPHID(),
         $commit->getPHID(),
       ),
       $more_phids);
 
     id(new PhabricatorFeedStoryPublisher())
       ->setRelatedPHIDs($related_phids)
-      ->setStoryAuthorPHID($user->getPHID())
+      ->setStoryAuthorPHID($actor->getPHID())
       ->setStoryTime(time())
       ->setStoryType(PhabricatorFeedStoryTypeConstants::STORY_AUDIT)
       ->setStoryData(
@@ -445,6 +437,7 @@ private function sendMail(
       ->setThreadID($thread_id, $is_new)
       ->addHeader('Thread-Topic', $thread_topic)
       ->setRelatedPHID($commit->getPHID())
+      ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
       ->setIsBulk(true)
       ->setBody($body);
 
@@ -484,8 +477,8 @@ private function renderMailBody(
     assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
 
     $commit = $this->commit;
-    $user = $this->user;
-    $name = $user->getUsername();
+    $actor = $this->getActor();
+    $name = $actor->getUsername();
 
     $verb = PhabricatorAuditActionConstants::getActionPastTenseVerb(
       $comment->getAction());

src/applications/conduit/method/differential/ConduitAPI_differential_close_Method.php

@@ -63,8 +63,8 @@ protected function execute(ConduitAPIRequest $request) {
 
     $editor = new DifferentialCommentEditor(
       $revision,
-      $request->getUser()->getPHID(),
       DifferentialAction::ACTION_CLOSE);
+    $editor->setActor($request->getUser());
     $editor->save();
 
     $revision->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);

src/applications/conduit/method/differential/ConduitAPI_differential_createcomment_Method.php

@@ -63,8 +63,8 @@ protected function execute(ConduitAPIRequest $request) {
 
     $editor = new DifferentialCommentEditor(
       $revision,
-      $request->getUser()->getPHID(),
       $action);
+    $editor->setActor($request->getUser());
     $editor->setContentSource($content_source);
     $editor->setMessage($request->getValue('message'));
     $editor->setNoEmail($request->getValue('silent'));

src/applications/conduit/method/differential/ConduitAPI_differential_createrevision_Method.php

@@ -54,7 +54,7 @@ protected function execute(ConduitAPIRequest $request) {
     $revision = DifferentialRevisionEditor::newRevisionFromConduitWithDiff(
       $fields,
       $diff,
-      $request->getUser()->getPHID());
+      $request->getUser());
 
     return array(
       'revisionid'  => $revision->getID(),