From afec01129a09122aebb97c39409ab4606a9d3740 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 May 2016 17:10:13 -0700 Subject: [PATCH] Allow blocking reviewers to be added via the web UI Summary: Ref T10939. Adds a `blocking(...)` token. This code is pretty iffy and going to get worse before it gets better, but the fix (T10967 + EditEngine) is going to be a fair chunk of work down the road. Test Plan: {F1426966} Reviewers: chad Reviewed By: chad Subscribers: scode Maniphest Tasks: T10939 Differential Revision: https://secure.phabricator.com/D15933 --- src/__phutil_library_map__.php | 4 + .../DifferentialReviewersField.php | 78 ++++++++--- ...DifferentialBlockingReviewerDatasource.php | 128 ++++++++++++++++++ .../DifferentialReviewerDatasource.php | 27 ++++ .../view/DifferentialAddCommentView.php | 4 +- 5 files changed, 221 insertions(+), 20 deletions(-) create mode 100644 src/applications/differential/typeahead/DifferentialBlockingReviewerDatasource.php create mode 100644 src/applications/differential/typeahead/DifferentialReviewerDatasource.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 64b850fa1e..7cf85b984f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -361,6 +361,7 @@ phutil_register_library_map(array( 'DifferentialAuthorField' => 'applications/differential/customfield/DifferentialAuthorField.php', 'DifferentialBlameRevisionField' => 'applications/differential/customfield/DifferentialBlameRevisionField.php', 'DifferentialBlockHeraldAction' => 'applications/differential/herald/DifferentialBlockHeraldAction.php', + 'DifferentialBlockingReviewerDatasource' => 'applications/differential/typeahead/DifferentialBlockingReviewerDatasource.php', 'DifferentialBranchField' => 'applications/differential/customfield/DifferentialBranchField.php', 'DifferentialChangeDetailMailView' => 'applications/differential/mail/DifferentialChangeDetailMailView.php', 'DifferentialChangeHeraldFieldGroup' => 'applications/differential/herald/DifferentialChangeHeraldFieldGroup.php', @@ -497,6 +498,7 @@ phutil_register_library_map(array( 'DifferentialRevertPlanField' => 'applications/differential/customfield/DifferentialRevertPlanField.php', 'DifferentialReviewedByField' => 'applications/differential/customfield/DifferentialReviewedByField.php', 'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php', + 'DifferentialReviewerDatasource' => 'applications/differential/typeahead/DifferentialReviewerDatasource.php', 'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php', 'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php', 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php', @@ -4561,6 +4563,7 @@ phutil_register_library_map(array( 'DifferentialAuthorField' => 'DifferentialCustomField', 'DifferentialBlameRevisionField' => 'DifferentialStoredCustomField', 'DifferentialBlockHeraldAction' => 'HeraldAction', + 'DifferentialBlockingReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DifferentialBranchField' => 'DifferentialCustomField', 'DifferentialChangeDetailMailView' => 'DifferentialMailView', 'DifferentialChangeHeraldFieldGroup' => 'HeraldFieldGroup', @@ -4713,6 +4716,7 @@ phutil_register_library_map(array( 'DifferentialRevertPlanField' => 'DifferentialStoredCustomField', 'DifferentialReviewedByField' => 'DifferentialCoreCustomField', 'DifferentialReviewer' => 'Phobject', + 'DifferentialReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType', 'DifferentialReviewerStatus' => 'Phobject', 'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'DifferentialReviewersHeraldAction', diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php index 234d5c27f2..dad85d7bb9 100644 --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -36,43 +36,83 @@ final class DifferentialReviewersField } public function readValueFromRequest(AphrontRequest $request) { - // Compute a new set of reviewer objects. For reviewers who haven't been - // added or removed, retain their existing status. Also, respect the new - // order. + // 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, null, 'getReviewerPHID'); + $old_status = mpull($old_status, 'getStatus', 'getReviewerPHID'); + + $datasource = id(new DifferentialBlockingReviewerDatasource()) + ->setViewer($request->getViewer()); $new_phids = $request->getArr($this->getFieldKey()); - $new_phids = array_fuse($new_phids); + $new_phids = $datasource->evaluateTokens($new_phids); + + $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; + + $specs = array(); + foreach ($new_phids as $spec) { + if (!is_array($spec)) { + $spec = array( + 'type' => DifferentialReviewerStatus::STATUS_ADDED, + 'phid' => $spec, + ); + } + $specs[$spec['phid']] = $spec; + } $new_status = array(); - foreach ($new_phids as $new_phid) { - if (empty($old_status[$new_phid])) { - $new_status[$new_phid] = new DifferentialReviewer( - $new_phid, - array( - 'status' => DifferentialReviewerStatus::STATUS_ADDED, - )); - } else { - $new_status[$new_phid] = $old_status[$new_phid]; + foreach ($specs as $phid => $spec) { + $new = $spec['type']; + $old = idx($old_status, $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 + // throw away rejects, accepts, etc. + if ($old) { + $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; + continue; + } } + + $new_status[$phid] = $new; + } + + foreach ($new_status as $phid => $status) { + $new_status[$phid] = new DifferentialReviewer( + $phid, + array( + 'status' => $status, + )); } $this->setValue($new_status); } public function renderEditControl(array $handles) { - $phids = array(); - if ($this->getValue()) { - $phids = mpull($this->getValue(), 'getReviewerPHID'); + $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; + + $value = array(); + foreach ($this->getValue() as $reviewer) { + $phid = $reviewer->getReviewerPHID(); + if ($reviewer->getStatus() == $status_blocking) { + $value[] = 'blocking('.$phid.')'; + } else { + $value[] = $phid; + } } return id(new AphrontFormTokenizerControl()) ->setUser($this->getViewer()) ->setName($this->getFieldKey()) - ->setDatasource(new DiffusionAuditorDatasource()) - ->setValue($phids) + ->setDatasource(new DifferentialReviewerDatasource()) + ->setValue($value) ->setError($this->getFieldError()) ->setLabel($this->getFieldName()); } diff --git a/src/applications/differential/typeahead/DifferentialBlockingReviewerDatasource.php b/src/applications/differential/typeahead/DifferentialBlockingReviewerDatasource.php new file mode 100644 index 0000000000..bea7056ea2 --- /dev/null +++ b/src/applications/differential/typeahead/DifferentialBlockingReviewerDatasource.php @@ -0,0 +1,128 @@ + array( + 'name' => pht('Blocking: ...'), + 'arguments' => pht('reviewer'), + 'summary' => pht('Select a blocking reviewer.'), + 'description' => pht( + "This function allows you to add a reviewer as a blocking ". + "reviewer. For example, this will add `%s` as a blocking reviewer: ". + "\n\n%s\n\n", + 'alincoln', + '> blocking(alincoln)'), + ), + ); + } + + + protected function didLoadResults(array $results) { + foreach ($results as $result) { + $result + ->setColor('red') + ->setTokenType(PhabricatorTypeaheadTokenView::TYPE_FUNCTION) + ->setIcon('fa-asterisk') + ->setPHID('blocking('.$result->getPHID().')') + ->setDisplayName(pht('Blocking: %s', $result->getDisplayName())) + ->setName($result->getName().' blocking'); + } + + return $results; + } + + protected function evaluateFunction($function, array $argv_list) { + $phids = array(); + foreach ($argv_list as $argv) { + $phids[] = head($argv); + } + + $phids = $this->resolvePHIDs($phids); + + $results = array(); + foreach ($phids as $phid) { + $results[] = array( + 'type' => DifferentialReviewerStatus::STATUS_BLOCKING, + 'phid' => $phid, + ); + } + + return $results; + } + + public function renderFunctionTokens($function, array $argv_list) { + $phids = array(); + foreach ($argv_list as $argv) { + $phids[] = head($argv); + } + + $phids = $this->resolvePHIDs($phids); + + $tokens = $this->renderTokens($phids); + foreach ($tokens as $token) { + $token->setColor(null); + if ($token->isInvalid()) { + $token + ->setValue(pht('Blocking: Invalid Reviewer')); + } else { + $token + ->setIcon('fa-asterisk') + ->setTokenType(PhabricatorTypeaheadTokenView::TYPE_FUNCTION) + ->setColor('red') + ->setKey('blocking('.$token->getKey().')') + ->setValue(pht('Blocking: %s', $token->getValue())); + } + } + + return $tokens; + } + + private function resolvePHIDs(array $phids) { + $usernames = array(); + foreach ($phids as $key => $phid) { + if (phid_get_type($phid) != PhabricatorPeopleUserPHIDType::TYPECONST) { + $usernames[$key] = $phid; + } + } + + if ($usernames) { + $users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withUsernames($usernames) + ->execute(); + $users = mpull($users, null, 'getUsername'); + foreach ($usernames as $key => $username) { + $user = idx($users, $username); + if ($user) { + $phids[$key] = $user->getPHID(); + } + } + } + + return $phids; + } + +} diff --git a/src/applications/differential/typeahead/DifferentialReviewerDatasource.php b/src/applications/differential/typeahead/DifferentialReviewerDatasource.php new file mode 100644 index 0000000000..18aa9e48b7 --- /dev/null +++ b/src/applications/differential/typeahead/DifferentialReviewerDatasource.php @@ -0,0 +1,27 @@ +