Allow blocking reviewers to be added via the CLI

Summary: Ref T10939. Fixes T4887. Supports "username!" to add a reviewer as blocking.

Test Plan: Added and removed blocking and non-blocking reviewers via CLI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4887, T10939

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

src/applications/differential/customfield/DifferentialCustomField.php

@@ -35,27 +35,39 @@ abstract class DifferentialCustomField
   protected function parseObjectList(
     $value,
     array $types,
-    $allow_partial = false) {
+    $allow_partial = false,
+    array $suffixes = array()) {
     return id(new PhabricatorObjectListQuery())
       ->setViewer($this->getViewer())
       ->setAllowedTypes($types)
       ->setObjectList($value)
       ->setAllowPartialResults($allow_partial)
+      ->setSuffixes($suffixes)
       ->execute();
   }
 
-  protected function renderObjectList(array $handles) {
+  protected function renderObjectList(
+    array $handles,
+    array $suffixes = array()) {
+
     if (!$handles) {
       return null;
     }
 
     $out = array();
     foreach ($handles as $handle) {
+      $phid = $handle->getPHID();
+
       if ($handle->getPolicyFiltered()) {
-        $out[] = $handle->getPHID();
+        $token = $phid;
       } else if ($handle->isComplete()) {
-        $out[] = $handle->getCommandLineObjectName();
+        $token = $handle->getCommandLineObjectName();
       }
+
+      $suffix = idx($suffixes, $phid);
+      $token = $token.$suffix;
+
+      $out[] = $token;
     }
 
     return implode(', ', $out);

src/applications/differential/customfield/DifferentialReviewersField.php

@@ -36,38 +36,37 @@ final class DifferentialReviewersField
   }
 
   public function readValueFromRequest(AphrontRequest $request) {
-    // Compute a new set of reviewer objects. We're going to respect the new
-    // reviewer order, add or remove any missing or new reviewers, and respect
-    // any blocking or unblocking changes. For reviewers who were there before
-    // and are still there, we're going to keep the current value because it
-    // may be something like "Accept", "Reject", etc.
-
-    $old_status = $this->getValue();
-    $old_status = mpull($old_status, 'getStatus', 'getReviewerPHID');
-
     $datasource = id(new DifferentialBlockingReviewerDatasource())
       ->setViewer($request->getViewer());
 
     $new_phids = $request->getArr($this->getFieldKey());
     $new_phids = $datasource->evaluateTokens($new_phids);
 
-    $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING;
+    $reviewers = array();
-
-    $specs = array();
     foreach ($new_phids as $spec) {
       if (!is_array($spec)) {
-        $spec = array(
+        $reviewers[$spec] = DifferentialReviewerStatus::STATUS_ADDED;
-          'type' => DifferentialReviewerStatus::STATUS_ADDED,
+      } else {
-          'phid' => $spec,
+        $reviewers[$spec['phid']] = $spec['type'];
-        );
       }
-      $specs[$spec['phid']] = $spec;
     }
 
-    $new_status = array();
+    $this->updateReviewers($this->getValue(), $reviewers);
-    foreach ($specs as $phid => $spec) {
+  }
-      $new = $spec['type'];
+
-      $old = idx($old_status, $phid);
+  private function updateReviewers(array $old_reviewers, array $new_map) {
+    // Compute a new set of reviewer objects. We're going to respect the new
+    // reviewer order, add or remove any new or missing reviewers, and respect
+    // any blocking or unblocking changes. For reviewers who were there before
+    // and are still there, we're going to keep the old value because it
+    // may be something like "Accept", "Reject", etc.
+
+    $old_map = mpull($old_reviewers, 'getStatus', 'getReviewerPHID');
+    $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING;
+
+    $new_reviewers = array();
+    foreach ($new_map as $phid => $new) {
+      $old = idx($old_map, $phid);
 
       // If we have an old status and this didn't make the reviewer blocking
       // or nonblocking, just retain the old status. This makes sure we don't
@@ -76,23 +75,23 @@ final class DifferentialReviewersField
         $is_block = ($old !== $status_blocking && $new === $status_blocking);
         $is_unblock = ($old === $status_blocking && $new !== $status_blocking);
         if (!$is_block && !$is_unblock) {
-          $new_status[$phid] = $old;
+          $new_reviewers[$phid] = $old;
           continue;
         }
       }
 
-      $new_status[$phid] = $new;
+      $new_reviewers[$phid] = $new;
     }
 
-    foreach ($new_status as $phid => $status) {
+    foreach ($new_reviewers as $phid => $status) {
-      $new_status[$phid] = new DifferentialReviewer(
+      $new_reviewers[$phid] = new DifferentialReviewer(
         $phid,
         array(
           'status' => $status,
         ));
     }
 
-    $this->setValue($new_status);
+    $this->setValue($new_reviewers);
   }
 
   public function renderEditControl(array $handles) {
@@ -187,7 +186,9 @@ final class DifferentialReviewersField
         PhabricatorPeopleUserPHIDType::TYPECONST,
         PhabricatorProjectProjectPHIDType::TYPECONST,
         PhabricatorOwnersPackagePHIDType::TYPECONST,
-      ));
+      ),
+      false,
+      array('!'));
   }
 
   public function getRequiredHandlePHIDsForCommitMessage() {
@@ -195,29 +196,40 @@ final class DifferentialReviewersField
   }
 
   public function readValueFromCommitMessage($value) {
-    $current_reviewers = $this->getObject()->getReviewerStatus();
-    $current_reviewers = mpull($current_reviewers, null, 'getReviewerPHID');
-
     $reviewers = array();
-    foreach ($value as $phid) {
+    foreach ($value as $spec) {
-      $reviewer = idx($current_reviewers, $phid);
+      $phid = $spec['phid'];
-      if ($reviewer) {
+
-        $reviewers[] = $reviewer;
+      $is_blocking = isset($spec['suffixes']['!']);
+      if ($is_blocking) {
+        $status = DifferentialReviewerStatus::STATUS_BLOCKING;
       } else {
-        $data = array(
+        $status = DifferentialReviewerStatus::STATUS_ADDED;
-          'status' => DifferentialReviewerStatus::STATUS_ADDED,
-        );
-        $reviewers[] = new DifferentialReviewer($phid, $data);
       }
+
+      $reviewers[$phid] = $status;
     }
 
-    $this->setValue($reviewers);
+    $this->updateReviewers(
+      $this->getObject()->getReviewerStatus(),
+      $reviewers);
 
     return $this;
   }
 
   public function renderCommitMessageValue(array $handles) {
-    return $this->renderObjectList($handles);
+    $suffixes = array();
+
+    $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING;
+
+    foreach ($this->getValue() as $reviewer) {
+      if ($reviewer->getStatus() == $status_blocking) {
+        $phid = $reviewer->getReviewerPHID();
+        $suffixes[$phid] = '!';
+      }
+    }
+
+    return $this->renderObjectList($handles, $suffixes);
   }
 
   public function validateCommitMessageValue($value) {
@@ -226,7 +238,9 @@ final class DifferentialReviewersField
     $config_self_accept_key = 'differential.allow-self-accept';
     $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key);
 
-    foreach ($value as $phid) {
+    foreach ($value as $spec) {
+      $phid = $spec['phid'];
+
       if (($phid == $author_phid) && !$allow_self_accept) {
         throw new DifferentialFieldValidationException(
           pht('The author of a revision can not be a reviewer.'));
@@ -265,4 +279,12 @@ final class DifferentialReviewersField
     return $warnings;
   }
 
+  public function getProTips() {
+    return array(
+      pht(
+        'You can mark a reviewer as blocking by adding an exclamation '.
+        'mark ("!") after their name.'),
+    );
+  }
+
 }

src/applications/phid/query/PhabricatorObjectListQuery.php

@@ -6,6 +6,7 @@ final class PhabricatorObjectListQuery extends Phobject {
   private $objectList;
   private $allowedTypes = array();
   private $allowPartialResults;
+  private $suffixes = array();
 
   public function setAllowPartialResults($allow_partial_results) {
     $this->allowPartialResults = $allow_partial_results;
@@ -16,6 +17,15 @@ final class PhabricatorObjectListQuery extends Phobject {
     return $this->allowPartialResults;
   }
 
+  public function setSuffixes(array $suffixes) {
+    $this->suffixes = $suffixes;
+    return $this;
+  }
+
+  public function getSuffixes() {
+    return $this->suffixes;
+  }
+
   public function setAllowedTypes(array $allowed_types) {
     $this->allowedTypes = $allowed_types;
     return $this;
@@ -87,6 +97,37 @@ final class PhabricatorObjectListQuery extends Phobject {
       }
     }
 
+    // If we're parsing with suffixes, strip them off any tokens and keep
+    // track of them for later.
+    $suffixes = $this->getSuffixes();
+    if ($suffixes) {
+      $suffixes = array_fuse($suffixes);
+      $suffix_map = array();
+      $stripped_map = array();
+      foreach ($name_map as $key => $name) {
+        $found_suffixes = array();
+        do {
+          $has_any_suffix = false;
+          foreach ($suffixes as $suffix) {
+            if (!$this->hasSuffix($name, $suffix)) {
+              continue;
+            }
+
+            $key = $this->stripSuffix($key, $suffix);
+            $name = $this->stripSuffix($name, $suffix);
+
+            $found_suffixes[] = $suffix;
+            $has_any_suffix = true;
+            break;
+          }
+        } while ($has_any_suffix);
+
+        $stripped_map[$key] = $name;
+        $suffix_map[$key] = array_fuse($found_suffixes);
+      }
+      $name_map = $stripped_map;
+    }
+
     $objects = $this->loadObjects(array_keys($name_map));
 
     $types = array();
@@ -140,7 +181,18 @@ final class PhabricatorObjectListQuery extends Phobject {
       }
     }
 
-    return array_values(array_unique(mpull($objects, 'getPHID')));
+    $result = array_unique(mpull($objects, 'getPHID'));
+
+    if ($suffixes) {
+      foreach ($result as $key => $phid) {
+        $result[$key] = array(
+          'phid' => $phid,
+          'suffixes' => idx($suffix_map, $key, array()),
+        );
+      }
+    }
+
+    return array_values($result);
   }
 
   private function loadObjects($names) {
@@ -186,5 +238,16 @@ final class PhabricatorObjectListQuery extends Phobject {
     return $results;
   }
 
+  private function hasSuffix($key, $suffix) {
+    return (substr($key, -strlen($suffix)) === $suffix);
+  }
+
+  private function stripSuffix($key, $suffix) {
+    if ($this->hasSuffix($key, $suffix)) {
+      return substr($key, 0, -strlen($suffix));
+    }
+
+    return $key;
+  }
 
 }

src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php

@@ -28,6 +28,15 @@ final class PhabricatorObjectListQueryTestCase extends PhabricatorTestCase {
     $result = $this->parseObjectList('');
     $this->assertEqual(array(), $result);
 
+    $result = $this->parseObjectList("{$name}!", array(), false, array('!'));
+    $this->assertEqual(
+      array(
+        array(
+          'phid' => $phid,
+          'suffixes' => array('!' => '!'),
+        ),
+      ),
+      $result);
 
     $package = PhabricatorOwnersPackage::initializeNewPackage($user)
       ->setName(pht('Query Test Package'))
@@ -42,6 +51,23 @@ final class PhabricatorObjectListQueryTestCase extends PhabricatorTestCase {
     $result = $this->parseObjectList("{$package_mono} Any Text, {$name}");
     $this->assertEqual(array($package_phid, $phid), $result);
 
+    $result = $this->parseObjectList(
+      "{$package_mono} Any Text!, {$name}",
+      array(),
+      false,
+      array('!'));
+    $this->assertEqual(
+      array(
+        array(
+          'phid' => $package_phid,
+          'suffixes' => array('!' => '!'),
+        ),
+        array(
+          'phid' => $phid,
+          'suffixes' => array(),
+        ),
+      ),
+      $result);
 
     // Expect failure when loading a user if objects must be of type "DUCK".
     $caught = null;
@@ -81,7 +107,8 @@ final class PhabricatorObjectListQueryTestCase extends PhabricatorTestCase {
   private function parseObjectList(
     $list,
     array $types = array(),
-    $allow_partial = false) {
+    $allow_partial = false,
+    $suffixes = array()) {
 
     $query = id(new PhabricatorObjectListQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
@@ -95,6 +122,10 @@ final class PhabricatorObjectListQueryTestCase extends PhabricatorTestCase {
       $query->setAllowPartialResults(true);
     }
 
+    if ($suffixes) {
+      $query->setSuffixes($suffixes);
+    }
+
     return $query->execute();
   }