Update overall revision status after reviewers change

Summary:
Ref T2222. This doesn't feel super clean, but doesn't feel too bad either.

Basically, Differential transactions can have secondary state-based effects (changing the overall revision status) when reviewers resign, are removed, accept, or reject revisions.

To deal with this in ApplicationTransactions, I did this:

  - `applyFinalEffects()` can now alter the transaction set (notably, add new ones). This mostly matters for email, notifications and feed.
  - In Differential, check for an overall revision state transition in `applyFinalEffects()` (e.g., your reject moving the revision to a rejected state).
  - I'm only writing the transaction if the transition is implied and indirect.
    - For example, if you "Plan Changes", that action changes the state on its own so there's no implicit state change transaction added.

The transactions themselves are kind of fluff, but it seems useful to keep a record of when state changes occurred in the transaction log. If people complain we can hide/remove them.

Test Plan: {F118143}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

src/applications/conpherence/editor/ConpherenceEditor.php

@@ -304,6 +304,8 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
       }
       $participant->save();
     }
+
+    return $xactions;
   }
 
   protected function mergeTransactions(

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -13,6 +13,7 @@ final class DifferentialTransactionEditor
 
     $types[] = DifferentialTransaction::TYPE_ACTION;
     $types[] = DifferentialTransaction::TYPE_INLINE;
+    $types[] = DifferentialTransaction::TYPE_STATUS;
 
 /*
 
@@ -145,7 +146,6 @@ final class DifferentialTransactionEditor
       case DifferentialTransaction::TYPE_INLINE:
         return;
       case PhabricatorTransactions::TYPE_EDGE:
-        // TODO: Update review status.
         return;
       case DifferentialTransaction::TYPE_ACTION:
         $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
@@ -166,15 +166,12 @@ final class DifferentialTransactionEditor
             return;
           case DifferentialAction::ACTION_RECLAIM:
             $object->setStatus($status_review);
-            // TODO: Update review status?
             return;
           case DifferentialAction::ACTION_REOPEN:
             $object->setStatus($status_review);
-            // TODO: Update review status?
             return;
           case DifferentialAction::ACTION_REQUEST:
             $object->setStatus($status_review);
-            // TODO: Update review status?
             return;
           case DifferentialAction::ACTION_CLOSE:
             $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
@@ -315,6 +312,98 @@ final class DifferentialTransactionEditor
     return parent::applyCustomExternalTransaction($object, $xaction);
   }
 
+  protected function applyFinalEffects(
+    PhabricatorLiskDAO $object,
+    array $xactions) {
+
+    $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
+    $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
+    $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
+
+    $old_status = $object->getStatus();
+    switch ($old_status) {
+      case $status_accepted:
+      case $status_revision:
+      case $status_review:
+        // Load the most up-to-date version of the revision and its reviewers,
+        // so we don't need to try to deduce the state of reviewers by examining
+        // all the changes made by the transactions.
+        $new_revision = id(new DifferentialRevisionQuery())
+          ->setViewer($this->getActor())
+          ->needReviewerStatus(true)
+          ->withIDs(array($object->getID()))
+          ->executeOne();
+        if (!$new_revision) {
+          throw new Exception(
+            pht('Failed to load revision from transaction finalization.'));
+        }
+
+        // Try to move a revision to "accepted". We look for:
+        //
+        //   - at least one accepting reviewer who is a user; and
+        //   - no rejects; and
+        //   - no blocking reviewers.
+
+        $has_accepting_user = false;
+        $has_rejecting_reviewer = false;
+        $has_blocking_reviewer = false;
+        foreach ($new_revision->getReviewerStatus() as $reviewer) {
+          $reviewer_status = $reviewer->getStatus();
+          switch ($reviewer_status) {
+            case DifferentialReviewerStatus::STATUS_REJECTED:
+              $has_rejecting_reviewer = true;
+              break;
+            case DifferentialReviewerStatus::STATUS_BLOCKING:
+              $has_blocking_reviewer = true;
+              break;
+            case DifferentialReviewerStatus::STATUS_ACCEPTED:
+              if ($reviewer->isUser()) {
+                $has_accepting_user = true;
+              }
+              break;
+          }
+        }
+
+        $new_status = null;
+        if ($has_accepting_user &&
+            !$has_rejecting_reviewer &&
+            !$has_blocking_reviewer) {
+          $new_status = $status_accepted;
+        } else if ($has_rejecting_reviewer) {
+          // This isn't accepted, and there's at least one rejecting reviewer,
+          // so the revision needs changes. This usually happens after a
+          // "reject".
+          $new_status = $status_revision;
+        } else if ($old_status == $status_accepted) {
+          // This revision was accepted, but it no longer satisfies the
+          // conditions for acceptance. This usually happens after an accepting
+          // reviewer resigns or is removed.
+          $new_status = $status_review;
+        }
+
+        if ($new_status !== null && $new_status != $old_status) {
+          $xaction = id(new DifferentialTransaction())
+            ->setTransactionType(DifferentialTransaction::TYPE_STATUS)
+            ->setOldValue($old_status)
+            ->setNewValue($new_status);
+
+          $xaction = $this->populateTransaction($object, $xaction)->save();
+
+          $xactions[] = $xaction;
+
+          $object->setStatus($new_status)->save();
+        }
+        break;
+      default:
+        // Revisions can't transition out of other statuses (like closed or
+        // abandoned) as a side effect of reviewer status changes.
+        break;
+    }
+
+    return $xactions;
+  }
+
+
   protected function validateTransaction(
     PhabricatorLiskDAO $object,
     $type,

src/applications/differential/storage/DifferentialTransaction.php

@@ -5,6 +5,7 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
   const TYPE_INLINE = 'differential:inline';
   const TYPE_UPDATE = 'differential:update';
   const TYPE_ACTION = 'differential:action';
+  const TYPE_STATUS = 'differential:status';
 
   public function getApplicationName() {
     return 'differential';
@@ -18,6 +19,28 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
     return new DifferentialTransactionComment();
   }
 
+  public function shouldHide() {
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_EDGE:
+        $old = $this->getOldValue();
+        $new = $this->getNewValue();
+        $add = array_diff_key($new, $old);
+        $rem = array_diff_key($old, $new);
+
+        // Hide metadata-only edge transactions. These correspond to users
+        // accepting or rejecting revisions, but the change is always explicit
+        // because of the TYPE_ACTION transaction. Rendering these transactions
+        // just creates clutter.
+
+        if (!$add && !$rem) {
+          return true;
+        }
+        break;
+    }
+
+    return false;
+  }
+
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
     $author_handle = $this->renderHandleLink($author_phid);
@@ -45,6 +68,18 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
         }
       case self::TYPE_ACTION:
         return DifferentialAction::getBasicStoryText($new, $author_handle);
+      case self::TYPE_STATUS:
+        switch ($this->getNewValue()) {
+          case ArcanistDifferentialRevisionStatus::ACCEPTED:
+            return pht(
+              'This revision is now accepted and ready to land.');
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+            return pht(
+              'This revision now requires changes to proceed.');
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+            return pht(
+              'This revision now requires review to proceed.');
+        }
     }
 
     return parent::getTitle();
@@ -56,6 +91,16 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
         return 'comment';
       case self::TYPE_UPDATE:
         return 'refresh';
+      case self::TYPE_STATUS:
+        switch ($this->getNewValue()) {
+          case ArcanistDifferentialRevisionStatus::ACCEPTED:
+            return 'enable';
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+            return 'delete';
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+            return 'refresh';
+        }
+        break;
       case self::TYPE_ACTION:
         switch ($this->getNewValue()) {
           case DifferentialAction::ACTION_CLOSE:
@@ -82,10 +127,41 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
     return parent::getIcon();
   }
 
+  public function shouldDisplayGroupWith(array $group) {
+
+    // Never group status changes with other types of actions, they're indirect
+    // and don't make sense when combined with direct actions.
+
+    $type_status = self::TYPE_STATUS;
+
+    if ($this->getTransactionType() == $type_status) {
+      return false;
+    }
+
+    foreach ($group as $xaction) {
+      if ($xaction->getTransactionType() == $type_status) {
+        return false;
+      }
+    }
+
+    return parent::shouldDisplayGroupWith($group);
+  }
+
+
   public function getColor() {
     switch ($this->getTransactionType()) {
       case self::TYPE_UPDATE:
         return PhabricatorTransactions::COLOR_SKY;
+      case self::TYPE_STATUS:
+        switch ($this->getNewValue()) {
+          case ArcanistDifferentialRevisionStatus::ACCEPTED:
+            return PhabricatorTransactions::COLOR_GREEN;
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+            return PhabricatorTransactions::COLOR_RED;
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+            return PhabricatorTransactions::COLOR_ORANGE;
+        }
+        break;
       case self::TYPE_ACTION:
         switch ($this->getNewValue()) {
           case DifferentialAction::ACTION_CLOSE:

src/applications/legalpad/editor/LegalpadDocumentEditor.php

@@ -104,6 +104,8 @@ final class LegalpadDocumentEditor
 
       $object->save();
     }
+
+    return $xactions;
   }
 
   protected function mergeTransactions(

src/applications/paste/editor/PhabricatorPasteEditor.php

@@ -113,6 +113,8 @@ final class PhabricatorPasteEditor
         $this->getActor(),
         $object->getPHID());
     }
+
+    return $xactions;
   }
 
 

src/applications/pholio/editor/PholioMockEditor.php

@@ -275,6 +275,8 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
       $image->setMockID($object->getID());
       $image->save();
     }
+
+    return $xactions;
   }
 
   protected function mergeTransactions(

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -377,9 +377,36 @@ abstract class PhabricatorApplicationTransactionEditor
       "implementation!");
   }
 
+  /**
+   * Fill in a transaction's common values, like author and content source.
+   */
+  protected function populateTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    $actor = $this->getActor();
+
+    // TODO: This needs to be more sophisticated once we have meta-policies.
+    $xaction->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC);
+    $xaction->setEditPolicy($actor->getPHID());
+
+    $xaction->setAuthorPHID($actor->getPHID());
+    $xaction->setContentSource($this->getContentSource());
+    $xaction->attachViewer($actor);
+    $xaction->attachObject($object);
+
+    if ($object->getPHID()) {
+      $xaction->setObjectPHID($object->getPHID());
+    }
+
+    return $xaction;
+  }
+
+
   protected function applyFinalEffects(
     PhabricatorLiskDAO $object,
     array $xactions) {
+    return $xactions;
   }
 
   public function setContentSource(PhabricatorContentSource $content_source) {
@@ -421,14 +448,7 @@ abstract class PhabricatorApplicationTransactionEditor
     $xactions = $this->combineTransactions($xactions);
 
     foreach ($xactions as $xaction) {
-      // TODO: This needs to be more sophisticated once we have meta-policies.
+      $xaction = $this->populateTransaction($object, $xaction);
-      $xaction->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC);
-      $xaction->setEditPolicy($actor->getPHID());
-
-      $xaction->setAuthorPHID($actor->getPHID());
-      $xaction->setContentSource($this->getContentSource());
-      $xaction->attachViewer($this->getActor());
-      $xaction->attachObject($object);
     }
 
     $is_preview = $this->getIsPreview();
@@ -557,7 +577,7 @@ abstract class PhabricatorApplicationTransactionEditor
         $this->applyHeraldRules($object, $xactions);
       }
 
-      $this->applyFinalEffects($object, $xactions);
+      $xactions = $this->applyFinalEffects($object, $xactions);
 
       if ($read_locking) {
         $object->endReadLocking();