diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -549,6 +549,7 @@ 'DifferentialRevisionRequiredActionResultBucket' => 'applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php', 'DifferentialRevisionResultBucket' => 'applications/differential/query/DifferentialRevisionResultBucket.php', 'DifferentialRevisionReviewersHeraldField' => 'applications/differential/herald/DifferentialRevisionReviewersHeraldField.php', + 'DifferentialRevisionReviewersTransaction' => 'applications/differential/xaction/DifferentialRevisionReviewersTransaction.php', 'DifferentialRevisionSearchConduitAPIMethod' => 'applications/differential/conduit/DifferentialRevisionSearchConduitAPIMethod.php', 'DifferentialRevisionSearchEngine' => 'applications/differential/query/DifferentialRevisionSearchEngine.php', 'DifferentialRevisionStatus' => 'applications/differential/constants/DifferentialRevisionStatus.php', @@ -5202,6 +5203,7 @@ 'DifferentialRevisionRequiredActionResultBucket' => 'DifferentialRevisionResultBucket', 'DifferentialRevisionResultBucket' => 'PhabricatorSearchResultBucket', 'DifferentialRevisionReviewersHeraldField' => 'DifferentialRevisionHeraldField', + 'DifferentialRevisionReviewersTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'DifferentialRevisionSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DifferentialRevisionStatus' => 'Phobject', diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -32,7 +32,8 @@ } protected function newObjectQuery() { - return new DifferentialRevisionQuery(); + return id(new DifferentialRevisionQuery()) + ->needReviewerStatus(true); } protected function getObjectCreateTitleText($object) { @@ -104,6 +105,18 @@ } $fields[] = id(new PhabricatorDatasourceEditField()) + ->setKey('reviewerPHIDs') + ->setLabel(pht('Reviewers')) + ->setDatasource(new DifferentialReviewerDatasource()) + ->setUseEdgeTransactions(true) + ->setTransactionType( + DifferentialRevisionReviewersTransaction::TRANSACTIONTYPE) + ->setDescription(pht('Reviewers for this revision.')) + ->setConduitDescription(pht('Change the reviewers for this revision.')) + ->setConduitTypeDescription(pht('New reviewers.')) + ->setValue($object->getReviewerPHIDsForEdit()); + + $fields[] = id(new PhabricatorDatasourceEditField()) ->setKey('repositoryPHID') ->setLabel(pht('Repository')) ->setDatasource(new DiffusionRepositoryDatasource()) diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -409,6 +409,24 @@ return $this; } + public function getReviewerPHIDsForEdit() { + $reviewers = $this->getReviewerStatus(); + + $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; + + $value = array(); + foreach ($reviewers as $reviewer) { + $phid = $reviewer->getReviewerPHID(); + if ($reviewer->getStatus() == $status_blocking) { + $value[] = 'blocking('.$phid.')'; + } else { + $value[] = $phid; + } + } + + return $value; + } + public function getRepository() { return $this->assertAttached($this->repository); } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php @@ -0,0 +1,350 @@ +getReviewerStatus(); + $reviewers = mpull($reviewers, 'getStatus', 'getReviewerPHID'); + return $reviewers; + } + + public function generateNewValue($object, $value) { + $actor = $this->getActor(); + + $datasource = id(new DifferentialBlockingReviewerDatasource()) + ->setViewer($actor); + + $reviewers = $this->generateOldValue($object); + + // First, remove any reviewers we're getting rid of. + $rem = idx($value, '-', array()); + $rem = $datasource->evaluateTokens($rem); + foreach ($rem as $phid) { + unset($reviewers[$phid]); + } + + $add = idx($value, '+', array()); + $add = $datasource->evaluateTokens($add); + $add_map = array(); + foreach ($add as $spec) { + if (!is_array($spec)) { + $phid = $spec; + $status = DifferentialReviewerStatus::STATUS_ADDED; + } else { + $phid = $spec['phid']; + $status = $spec['type']; + } + + $add_map[$phid] = $status; + } + + $set = idx($value, '=', null); + if ($set !== null) { + $set = $datasource->evaluateTokens($set); + foreach ($set as $spec) { + if (!is_array($spec)) { + $phid = $spec; + $status = DifferentialReviewerStatus::STATUS_ADDED; + } else { + $phid = $spec['phid']; + $status = $spec['type']; + } + + $add_map[$phid] = $status; + } + + // We treat setting reviewers as though they were being added to an + // empty list, so we can share more code between pathways. + $reviewers = array(); + } + + $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; + foreach ($add_map as $phid => $new_status) { + $old_status = idx($reviewers, $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_status) { + $was_blocking = ($old_status == $status_blocking); + $now_blocking = ($new_status == $status_blocking); + + $is_block = ($now_blocking && !$was_blocking); + $is_unblock = (!$now_blocking && $was_blocking); + + if (!$is_block && !$is_unblock) { + continue; + } + } + + $reviewers[$phid] = $new_status; + } + + return $reviewers; + } + + public function applyExternalEffects($object, $value) { + $src_phid = $object->getPHID(); + + $old = $this->generateOldValue($object); + $new = $value; + $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; + + $editor = new PhabricatorEdgeEditor(); + + $rem = array_diff_key($old, $new); + foreach ($rem as $dst_phid => $status) { + $editor->removeEdge($src_phid, $edge_type, $dst_phid); + } + + foreach ($new as $dst_phid => $status) { + $old_status = idx($old, $dst_phid); + if ($old_status === $status) { + continue; + } + + $data = array( + 'data' => array( + 'status' => $status, + + // TODO: This seemes like it's buggy before the Modular Transactions + // changes. Figure out what's going on here? We don't have a very + // clean way to get the active diff ID right now. + 'diffID' => null, + ), + ); + + $editor->addEdge($src_phid, $edge_type, $dst_phid, $data); + } + + $editor->save(); + } + + public function getTitle() { + return $this->renderReviewerEditTitle(false); + } + + public function getTitleForFeed() { + return $this->renderReviewerEditTitle(true); + } + + private function renderReviewerEditTitle($is_feed) { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + $rem = array_diff_key($old, $new); + $add = array_diff_key($new, $old); + $rem_phids = array_keys($rem); + $add_phids = array_keys($add); + $total_count = count($rem) + count($add); + + $parts = array(); + + if ($rem && $add) { + if ($is_feed) { + $parts[] = pht( + '%s edited %s reviewer(s) for %s, added %s: %s; removed %s: %s.', + $this->renderAuthor(), + new PhutilNumber($total_count), + $this->renderObject(), + phutil_count($add_phids), + $this->renderHandleList($add_phids), + phutil_count($rem_phids), + $this->renderHandleList($rem_phids)); + } else { + $parts[] = pht( + '%s edited %s reviewer(s), added %s: %s; removed %s: %s.', + $this->renderAuthor(), + new PhutilNumber($total_count), + phutil_count($add_phids), + $this->renderHandleList($add_phids), + phutil_count($rem_phids), + $this->renderHandleList($rem_phids)); + } + } else if ($add) { + if ($is_feed) { + $parts[] = pht( + '%s added %s reviewer(s) for %s: %s.', + $this->renderAuthor(), + phutil_count($add_phids), + $this->renderObject(), + $this->renderHandleList($add_phids)); + } else { + $parts[] = pht( + '%s added %s reviewer(s): %s.', + $this->renderAuthor(), + phutil_count($add_phids), + $this->renderHandleList($add_phids)); + } + } else if ($rem) { + if ($is_feed) { + $parts[] = pht( + '%s removed %s reviewer(s) for %s: %s.', + $this->renderAuthor(), + phutil_count($rem_phids), + $this->renderObject(), + $this->renderHandleList($rem_phids)); + } else { + $parts[] = pht( + '%s removed %s reviewer(s): %s.', + $this->renderAuthor(), + phutil_count($rem_phids), + $this->renderHandleList($rem_phids)); + } + } + + $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; + $blocks = array(); + $unblocks = array(); + foreach ($new as $phid => $new_status) { + $old_status = idx($old, $phid); + if (!$old_status) { + continue; + } + + $was_blocking = ($old_status == $status_blocking); + $now_blocking = ($new_status == $status_blocking); + + $is_block = ($now_blocking && !$was_blocking); + $is_unblock = (!$now_blocking && $was_blocking); + + if ($is_block) { + $blocks[] = $phid; + } + if ($is_unblock) { + $unblocks[] = $phid; + } + } + + $total_count = count($blocks) + count($unblocks); + + if ($blocks && $unblocks) { + if ($is_feed) { + $parts[] = pht( + '%s changed %s blocking reviewer(s) for %s, added %s: %s; removed '. + '%s: %s.', + $this->renderAuthor(), + new PhutilNumber($total_count), + $this->renderObject(), + phutil_count($blocks), + $this->renderHandleList($blocks), + phutil_count($unblocks), + $this->renderHandleList($unblocks)); + } else { + $parts[] = pht( + '%s changed %s blocking reviewer(s), added %s: %s; removed %s: %s.', + $this->renderAuthor(), + new PhutilNumber($total_count), + phutil_count($blocks), + $this->renderHandleList($blocks), + phutil_count($unblocks), + $this->renderHandleList($unblocks)); + } + } else if ($blocks) { + if ($is_feed) { + $parts[] = pht( + '%s added %s blocking reviewer(s) for %s: %s.', + $this->renderAuthor(), + phutil_count($blocks), + $this->renderObject(), + $this->renderHandleList($blocks)); + } else { + $parts[] = pht( + '%s added %s blocking reviewer(s): %s.', + $this->renderAuthor(), + phutil_count($blocks), + $this->renderHandleList($blocks)); + } + } else if ($unblocks) { + if ($is_feed) { + $parts[] = pht( + '%s removed %s blocking reviewer(s) for %s: %s.', + $this->renderAuthor(), + phutil_count($unblocks), + $this->renderObject(), + $this->renderHandleList($unblocks)); + } else { + $parts[] = pht( + '%s removed %s blocking reviewer(s): %s.', + $this->renderAuthor(), + phutil_count($unblocks), + $this->renderHandleList($unblocks)); + } + } + + if ($this->isTextMode()) { + return implode(' ', $parts); + } else { + return phutil_implode_html(' ', $parts); + } + } + + public function validateTransactions($object, array $xactions) { + $actor = $this->getActor(); + $errors = array(); + + $author_phid = $object->getAuthorPHID(); + $config_self_accept_key = 'differential.allow-self-accept'; + $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key); + + $old = $this->generateOldValue($object); + foreach ($xactions as $xaction) { + $new = $this->generateNewValue($object, $xaction->getNewValue()); + + $add = array_diff_key($new, $old); + if (!$add) { + continue; + } + + $objects = id(new PhabricatorObjectQuery()) + ->setViewer($actor) + ->withPHIDs(array_keys($add)) + ->execute(); + $objects = mpull($objects, null, 'getPHID'); + + foreach ($add as $phid => $status) { + if (!isset($objects[$phid])) { + $errors[] = $this->newInvalidError( + pht( + 'Reviewer "%s" is not a valid object.', + $phid), + $xaction); + continue; + } + + switch (phid_get_type($phid)) { + case PhabricatorPeopleUserPHIDType::TYPECONST: + case PhabricatorOwnersPackagePHIDType::TYPECONST: + case PhabricatorProjectProjectPHIDType::TYPECONST: + break; + default: + $errors[] = $this->newInvalidError( + pht( + 'Reviewer "%s" must be a user, a package, or a project.', + $phid), + $xaction); + continue 2; + } + + // NOTE: This weird behavior around commandeering is a bit unorthodox, + // but this restriction is an unusual one. + + $is_self = ($phid === $author_phid); + if ($is_self && !$allow_self_accept) { + if (!$xaction->getIsCommandeerSideEffect()) { + $errors[] = $this->newInvalidError( + pht('The author of a revision can not be a reviewer.'), + $xaction); + continue; + } + } + } + } + + return $errors; + } + +} diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -277,7 +277,7 @@ return !strlen($value); } - private function isTextMode() { + protected function isTextMode() { $target = $this->getStorage()->getRenderingTarget(); return ($target == PhabricatorApplicationTransaction::TARGET_TEXT); }