From a5ca96a5903d2a367b58230ebe2dbb1705e6d191 Mon Sep 17 00:00:00 2001
From: Bob Trahan <bob.trahan@gmail.com>
Date: Thu, 20 Jun 2013 16:40:56 -0700
Subject: [PATCH] ApplicationTransactions - reload subscribers if there's a
 transaction that changes them

Summary: in applyExternalEffects, for subscriber transactions, we now re-load subscribers. also fixes a bug where a user can get emailed 2x when they take an action on a mock they created.

Test Plan: made some mocks. verified one copy sent to creator and one to each subscriber. (note having problems with email so I verified the phids mail was supposed to be sent to and did not get the actual email delivered)

Reviewers: epriestley, chad

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T3315

Differential Revision: https://secure.phabricator.com/D6206
---
 ...habricatorApplicationTransactionEditor.php | 37 +++++++++++++------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index e55032e50a..9eb493cc49 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -217,6 +217,15 @@ abstract class PhabricatorApplicationTransactionEditor
             array_diff_key($new_map, $old_map)));
 
         $subeditor->save();
+
+        // for the rest of these edits, subscribers should include those just
+        // added as well as those just removed.
+        $subscribers = array_unique(array_merge(
+          $this->subscribers,
+          $xaction->getOldValue(),
+          $xaction->getNewValue()));
+        $this->subscribers = $subscribers;
+
         break;
       case PhabricatorTransactions::TYPE_EDGE:
         $old = $xaction->getOldValue();
@@ -307,14 +316,7 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $actor = $this->requireActor();
 
-    if ($object->getPHID() &&
-        ($object instanceof PhabricatorSubscribableInterface)) {
-      $subs = PhabricatorSubscribersQuery::loadSubscribersForPHID(
-        $object->getPHID());
-      $this->subscribers = array_fuse($subs);
-    } else {
-      $this->subscribers = array();
-    }
+    $this->loadSubscribers($object);
 
     $xactions = $this->applyImplicitCC($object, $xactions);
 
@@ -505,6 +507,17 @@ abstract class PhabricatorApplicationTransactionEditor
     }
   }
 
+  private function loadSubscribers(PhabricatorLiskDAO $object) {
+    if ($object->getPHID() &&
+        ($object instanceof PhabricatorSubscribableInterface)) {
+      $subs = PhabricatorSubscribersQuery::loadSubscribersForPHID(
+        $object->getPHID());
+      $this->subscribers = array_fuse($subs);
+    } else {
+      $this->subscribers = array();
+    }
+  }
+
   private function validateEditParameters(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -1017,8 +1030,8 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorLiskDAO $object,
     array $xactions) {
 
-    $email_to = $this->getMailTo($object);
-    $email_cc = $this->getMailCC($object);
+    $email_to = array_unique($this->getMailTo($object));
+    $email_cc = array_unique($this->getMailCC($object));
 
     $phids = array_merge($email_to, $email_cc);
     $handles = id(new PhabricatorObjectHandleData($phids))
@@ -1203,9 +1216,9 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorLiskDAO $object,
     array $xactions) {
 
-    return array_merge(
+    return array_unique(array_merge(
       $this->getMailTo($object),
-      $this->getMailCC($object));
+      $this->getMailCC($object)));
   }