From f1744ac6d9f89af9ee058ae84e7f274fd3d4e554 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 4 Dec 2015 10:53:15 -0800
Subject: [PATCH] Change/drop/reconcile some miscellaneous edit behaviors in
 Maniphest

Summary:
Ref T9132. Open to discussion here since it's mostly product stuff, but here's my gut on this:

  - Change Maniphest behavior to stop assigning tasks if they're unassigned when closed. I think this behavior often doesn't make much sense. We'll probably separately track "who closed this" in T4434 eventually.
  - Only add the actor as a subscriber if they comment, like in other applications. Previously, we added them as a subscriber for other types of changes (like priority and status changes). This is more consistent, but open to retaining the old behavior or some compromise between the two.
  - Retain the "when changing owner, subscribe the old owner" behavior.

Test Plan:
  - Added a comment, got CC'd.
  - Changed owners, saw old owner get CC'd.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132

Differential Revision: https://secure.phabricator.com/D14670
---
 .../maniphest/editor/ManiphestEditEngine.php  |  4 -
 .../editor/ManiphestTransactionEditor.php     | 74 +++++++++++++++++++
 ...habricatorApplicationTransactionEditor.php |  2 +-
 3 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php
index 017eaf6ad9..3dfb740009 100644
--- a/src/applications/maniphest/editor/ManiphestEditEngine.php
+++ b/src/applications/maniphest/editor/ManiphestEditEngine.php
@@ -60,10 +60,6 @@ final class ManiphestEditEngine
 
     $priority_map = ManiphestTaskPriority::getTaskPriorityMap();
 
-    // TODO: Restore these or toss them:
-    // - When closing an unassigned task, assign the closing user.
-    // - Make sure implicit CCs on actions are working reasonably.
-
     if ($object->isClosed()) {
       $priority_label = null;
       $owner_label = null;
diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
index 32b85f01a5..c0a763dbcb 100644
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -745,5 +745,79 @@ final class ManiphestTransactionEditor
     return $errors;
   }
 
+  protected function expandTransactions(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+    $results = parent::expandTransactions($object, $xactions);
+
+    $is_unassigned = ($object->getOwnerPHID() === null);
+
+    $any_assign = false;
+    foreach ($xactions as $xaction) {
+      if ($xaction->getTransactionType() == ManiphestTransaction::TYPE_OWNER) {
+        $any_assign = true;
+        break;
+      }
+    }
+
+    $is_open = !$object->isClosed();
+
+    $new_status = null;
+    foreach ($xactions as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case ManiphestTransaction::TYPE_STATUS:
+          $new_status = $xaction->getNewValue();
+          break;
+      }
+    }
+
+    if ($new_status === null) {
+      $is_closing = false;
+    } else {
+      $is_closing = ManiphestTaskStatus::isClosedStatus($new_status);
+    }
+
+    // If the task is not assigned, not being assigned, currently open, and
+    // being closed, try to assign the actor as the owner.
+    if ($is_unassigned && !$any_assign && $is_open && $is_closing) {
+      // Don't assign the actor if they aren't a real user.
+      $actor = $this->getActor();
+      $actor_phid = $actor->getPHID();
+      if ($actor_phid) {
+        $results[] = id(new ManiphestTransaction())
+          ->setTransactionType(ManiphestTransaction::TYPE_OWNER)
+          ->setNewValue($actor_phid);
+      }
+    }
+
+    return $results;
+  }
+
+  protected function expandTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    $results = parent::expandTransaction($object, $xaction);
+
+    switch ($xaction->getTransactionType()) {
+      case ManiphestTransaction::TYPE_OWNER:
+        // When a task is reassigned, move the old owner to the subscriber
+        // list so they're still in the loop.
+        $owner_phid = $object->getOwnerPHID();
+        if ($owner_phid) {
+          $results[] = id(new ManiphestTransaction())
+            ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
+            ->setIgnoreOnNoEffect(true)
+            ->setNewValue(
+              array(
+                '+' => array($owner_phid => $owner_phid),
+              ));
+        }
+        break;
+    }
+
+    return $results;
+  }
+
 
 }
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 4e4b9e751e..09ff0b3a76 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -1369,7 +1369,7 @@ abstract class PhabricatorApplicationTransactionEditor
    * resigning from a revision in Differential implies removing yourself as
    * a reviewer.
    */
-  private function expandTransactions(
+  protected function expandTransactions(
     PhabricatorLiskDAO $object,
     array $xactions) {