Restore old "author can not be a reviewer" rule to Transactions

Summary:
This is a bit messy, but not tooo bad:

  - In general, stop the author from being added as a reviewer.
  - In the specific case that "self accept" is enabled, allow it. This is easier than trying to special case it.
  - When commandeering, we make the author a reviewer and make the actor the author, but these happen after validation. At validation time, it looks like we're making the author a reviewer. Just special case this.
  - Provide a slightly nicer message when trying to add yourself from `arc`. We hit the Transactions message anyway, but it's not formatted as cleanly.
  - Don't try to add the author via Herald.

Test Plan:
  - Edited a revision with author = reviewer, got stopped.
  - Commandeered revision.
  - Updated from `arc`.
  - Updated in general.
  - Fired a "add author as reviewer" Herald rule without things breaking.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

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

Author: epriestley

src/applications/differential/customfield/DifferentialReviewersField.php

@@ -177,4 +177,19 @@ public function renderCommitMessageValue(array $handles) {
     return $this->renderObjectList($handles);
   }
 
+  public function validateCommitMessageValue($value) {
+    $author_phid = $this->getObject()->getAuthorPHID();
+
+    $config_self_accept_key = 'differential.allow-self-accept';
+    $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key);
+
+    foreach ($value as $phid) {
+      if (($phid == $author_phid) && !$allow_self_accept) {
+        throw new DifferentialFieldValidationException(
+          pht('The author of a revision can not be a reviewer.'));
+      }
+    }
+  }
+
+
 }

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -395,10 +395,15 @@ protected function expandTransaction(
               );
             }
 
+            // 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;
@@ -625,8 +630,45 @@ protected function validateTransaction(
 
     $errors = parent::validateTransaction($object, $type, $xactions);
 
+    $config_self_accept_key = 'differential.allow-self-accept';
+    $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key);
+
     foreach ($xactions as $xaction) {
       switch ($type) {
+        case PhabricatorTransactions::TYPE_EDGE:
+          switch ($xaction->getMetadataValue('edge:type')) {
+            case PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER:
+
+              // Prevent the author from becoming a reviewer.
+
+              // NOTE: This is pretty gross, but this restriction is unusual.
+              // If we end up with too much more of this, we should try to clean
+              // this up -- maybe by moving validation to after transactions
+              // are adjusted (so we can just examine the final value) or adding
+              // a second phase there?
+
+              $author_phid = $object->getAuthorPHID();
+              $new = $xaction->getNewValue();
+
+              $add = idx($new, '+', array());
+              $eq = idx($new, '=', array());
+              $phids = array_keys($add + $eq);
+
+              foreach ($phids as $phid) {
+                if (($phid == $author_phid) &&
+                    !$allow_self_accept &&
+                    !$xaction->getIsCommandeerSideEffect()) {
+                  $errors[] =
+                    new PhabricatorApplicationTransactionValidationError(
+                      $type,
+                      pht('Invalid'),
+                      pht('The author of a revision can not be a reviewer.'),
+                      $xaction);
+                }
+              }
+              break;
+          }
+          break;
         case DifferentialTransaction::TYPE_UPDATE:
           $diff = $this->loadDiff($xaction->getNewValue());
           if (!$diff) {
@@ -1340,6 +1382,12 @@ protected function didApplyHeraldRules(
     $value = array();
     foreach ($reviewers as $status => $phids) {
       foreach ($phids as $phid) {
+        if ($phid == $object->getAuthorPHID()) {
+          // Don't try to add the revision's author as a reviewer, since this
+          // isn't valid and doesn't make sense.
+          continue;
+        }
+
         $value['+'][$phid] = array(
           'data' => array(
             'status' => $status,

src/applications/differential/storage/DifferentialTransaction.php

@@ -2,6 +2,18 @@
 
 final class DifferentialTransaction extends PhabricatorApplicationTransaction {
 
+  private $isCommandeerSideEffect;
+
+
+  public function setIsCommandeerSideEffect($is_side_effect) {
+    $this->isCommandeerSideEffect = $is_side_effect;
+    return $this;
+  }
+
+  public function getIsCommandeerSideEffect() {
+    return $this->isCommandeerSideEffect;
+  }
+
   const TYPE_INLINE = 'differential:inline';
   const TYPE_UPDATE = 'differential:update';
   const TYPE_ACTION = 'differential:action';