Restore some minor state behaviors to Differential on EditEngine

Summary:
Ref T11114. This restores:

  - Commandeering should exeucte Herald.
  - Commandeering should swap reviewers.
  - "Request Review" on an "Accepted" revision should downgrade reviewers so they have to accept again.

Test Plan:
  - Commandeered, saw Herald run and reviewers swap.
  - Requested review of an accepted revision, saw it drop down to "Needs Review" with "Accepted Prior" on the reviewer.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

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

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -288,16 +288,26 @@ final class DifferentialTransactionEditor
             $downgrade_accepts = true;
           }
           break;
+        case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE:
+          $downgrade_rejects = true;
+          if ((!$is_sticky_accept) ||
+              ($object->getStatus() != $status_plan)) {
+            // If the old state isn't "changes planned", downgrade the accepts.
+            // This exception allows an accepted revision to go through
+            // "Plan Changes" -> "Request Review" and return to "accepted" if
+            // the author didn't update the revision, essentially undoing the
+            // "Plan Changes".
+            $downgrade_accepts = true;
+          }
+          break;
+
+        // TODO: Remove this, obsoleted by ModularTransactions above.
         case DifferentialTransaction::TYPE_ACTION:
           switch ($xaction->getNewValue()) {
             case DifferentialAction::ACTION_REQUEST:
               $downgrade_rejects = true;
               if ((!$is_sticky_accept) ||
                   ($object->getStatus() != $status_plan)) {
-                // If the old state isn't "changes planned", downgrade the
-                // accepts. This exception allows an accepted revision to
-                // go through Plan Changes -> Request Review to return to
-                // "accepted" if the author didn't update the revision.
                 $downgrade_accepts = true;
               }
               break;
@@ -353,6 +363,7 @@ final class DifferentialTransactionEditor
       }
     }
 
+    $is_commandeer = false;
     switch ($xaction->getTransactionType()) {
       case DifferentialTransaction::TYPE_UPDATE:
         if ($this->getIsCloseByCommit()) {
@@ -424,6 +435,10 @@ final class DifferentialTransactionEditor
         }
         break;
 
+      case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE:
+        $is_commandeer = true;
+        break;
+
       case DifferentialTransaction::TYPE_ACTION:
         $action_type = $xaction->getNewValue();
 
@@ -463,41 +478,7 @@ final class DifferentialTransactionEditor
             break;
 
           case DifferentialAction::ACTION_CLAIM:
-            // If the user is commandeering, add the previous owner as a
+            $is_commandeer = true;
-            // reviewer and remove the actor.
-
-            $edits = array(
-              '-' => array(
-                $actor_phid => $actor_phid,
-              ),
-            );
-
-            $owner_phid = $object->getAuthorPHID();
-            if ($owner_phid) {
-              $reviewer = new DifferentialReviewerProxy(
-                $owner_phid,
-                array(
-                  'status' => DifferentialReviewerStatus::STATUS_ADDED,
-                ));
-
-              $edits['+'] = array(
-                $owner_phid => array(
-                  'data' => $reviewer->getEdgeData(),
-                ),
-              );
-            }
-
-            // NOTE: We're setting setIsCommandeerSideEffect() on this because
-            // normally you can't add a revision's author as a reviewer, but
-            // this action swaps them after validation executes.
-
-            $results[] = id(new DifferentialTransaction())
-              ->setTransactionType($type_edge)
-              ->setMetadataValue('edge:type', $edge_reviewer)
-              ->setIgnoreOnNoEffect(true)
-              ->setIsCommandeerSideEffect(true)
-              ->setNewValue($edits);
-
             break;
           case DifferentialAction::ACTION_RESIGN:
             // If the user is resigning, add a separate reviewer edit
@@ -519,6 +500,10 @@ final class DifferentialTransactionEditor
       break;
     }
 
+    if ($is_commandeer) {
+      $results[] = $this->newCommandeerReviewerTransaction($object);
+    }
+
     if (!$this->didExpandInlineState) {
       switch ($xaction->getTransactionType()) {
         case PhabricatorTransactions::TYPE_COMMENT:
@@ -1499,6 +1484,10 @@ final class DifferentialTransactionEditor
             return true;
           }
           break;
+        case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE:
+          // When users commandeer revisions, we may need to trigger
+          // signatures or author-based rules.
+          return true;
         case DifferentialTransaction::TYPE_ACTION:
           switch ($xaction->getNewValue()) {
             case DifferentialAction::ACTION_CLAIM:
@@ -1922,4 +1911,35 @@ final class DifferentialTransactionEditor
     return $this;
   }
 
+  private function newCommandeerReviewerTransaction(
+    DifferentialRevision $revision) {
+
+    $actor_phid = $this->getActingAsPHID();
+    $owner_phid = $revision->getAuthorPHID();
+
+    // If the user is commandeering, add the previous owner as a
+    // reviewer and remove the actor.
+
+    $edits = array(
+      '-' => array(
+        $actor_phid,
+      ),
+      '+' => array(
+        $owner_phid,
+      ),
+    );
+
+    // NOTE: We're setting setIsCommandeerSideEffect() on this because normally
+    // you can't add a revision's author as a reviewer, but this action swaps
+    // them after validation executes.
+
+    $xaction_type = DifferentialRevisionReviewersTransaction::TRANSACTIONTYPE;
+
+    return id(new DifferentialTransaction())
+      ->setTransactionType($xaction_type)
+      ->setIgnoreOnNoEffect(true)
+      ->setIsCommandeerSideEffect(true)
+      ->setNewValue($edits);
+  }
+
 }

src/applications/transactions/editengine/PhabricatorEditEngine.php

@@ -1805,6 +1805,10 @@ abstract class PhabricatorEditEngine
 
     try {
       $xactions = $editor->applyTransactions($object, $xactions);
+    } catch (PhabricatorApplicationTransactionValidationException $ex) {
+      return id(new PhabricatorApplicationTransactionValidationResponse())
+        ->setCancelURI($view_uri)
+        ->setException($ex);
     } catch (PhabricatorApplicationTransactionNoEffectException $ex) {
       return id(new PhabricatorApplicationTransactionNoEffectResponse())
         ->setCancelURI($view_uri)