Page MenuHomePhabricator
Files F13973181

D17050.diff
No OneTemporary
Actions

D17050.diff
View Options

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 @@
+<?php
+
+final class DifferentialRevisionReviewersTransaction
+ extends DifferentialRevisionTransactionType {
+
+ const TRANSACTIONTYPE = 'differential.revision.reviewers';
+
+ public function generateOldValue($object) {
+ $reviewers = $object->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);
}

File Metadata

Mime Type
text/plain
Expires
Oct 18 2024, 11:34 PM (4 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6729193
Default Alt Text
D17050.diff (15 KB)

Event Timeline

Phabricator · Built by Phacility