diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php --- a/src/applications/differential/customfield/DifferentialCustomField.php +++ b/src/applications/differential/customfield/DifferentialCustomField.php @@ -35,27 +35,39 @@ 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); diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -36,38 +36,37 @@ } 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; - - $specs = array(); + $reviewers = array(); foreach ($new_phids as $spec) { if (!is_array($spec)) { - $spec = array( - 'type' => DifferentialReviewerStatus::STATUS_ADDED, - 'phid' => $spec, - ); + $reviewers[$spec] = DifferentialReviewerStatus::STATUS_ADDED; + } else { + $reviewers[$spec['phid']] = $spec['type']; } - $specs[$spec['phid']] = $spec; } - $new_status = array(); - foreach ($specs as $phid => $spec) { - $new = $spec['type']; - $old = idx($old_status, $phid); + $this->updateReviewers($this->getValue(), $reviewers); + } + + 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 @@ $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) { - $new_status[$phid] = new DifferentialReviewer( + foreach ($new_reviewers as $phid => $status) { + $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 @@ PhabricatorPeopleUserPHIDType::TYPECONST, PhabricatorProjectProjectPHIDType::TYPECONST, PhabricatorOwnersPackagePHIDType::TYPECONST, - )); + ), + false, + array('!')); } public function getRequiredHandlePHIDsForCommitMessage() { @@ -195,29 +196,40 @@ } public function readValueFromCommitMessage($value) { - $current_reviewers = $this->getObject()->getReviewerStatus(); - $current_reviewers = mpull($current_reviewers, null, 'getReviewerPHID'); - $reviewers = array(); - foreach ($value as $phid) { - $reviewer = idx($current_reviewers, $phid); - if ($reviewer) { - $reviewers[] = $reviewer; + foreach ($value as $spec) { + $phid = $spec['phid']; + + $is_blocking = isset($spec['suffixes']['!']); + if ($is_blocking) { + $status = DifferentialReviewerStatus::STATUS_BLOCKING; } else { - $data = array( - 'status' => DifferentialReviewerStatus::STATUS_ADDED, - ); - $reviewers[] = new DifferentialReviewer($phid, $data); + $status = DifferentialReviewerStatus::STATUS_ADDED; } + + $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 @@ $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 @@ return $warnings; } + public function getProTips() { + return array( + pht( + 'You can mark a reviewer as blocking by adding an exclamation '. + 'mark ("!") after their name.'), + ); + } + } diff --git a/src/applications/phid/query/PhabricatorObjectListQuery.php b/src/applications/phid/query/PhabricatorObjectListQuery.php --- a/src/applications/phid/query/PhabricatorObjectListQuery.php +++ b/src/applications/phid/query/PhabricatorObjectListQuery.php @@ -6,6 +6,7 @@ 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 @@ 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 @@ } } + // 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 @@ } } - 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 @@ 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; + } } diff --git a/src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php b/src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php --- a/src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php +++ b/src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php @@ -28,6 +28,15 @@ $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 @@ $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 @@ 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 @@ $query->setAllowPartialResults(true); } + if ($suffixes) { + $query->setSuffixes($suffixes); + } + return $query->execute(); }