From 8aa8ef49dae1db52844bebd57a182dadcb3e29fd Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 6 Oct 2013 17:09:24 -0700 Subject: [PATCH] Provide an "Add blocking reviewer..." Herald action Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that. Test Plan: Added Herald rules and updated revisions; see screenshots. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1279 Differential Revision: https://secure.phabricator.com/D7244 --- .../constants/DifferentialReviewerStatus.php | 1 + .../editor/DifferentialRevisionEditor.php | 30 ++++++++++++++----- .../view/DifferentialReviewersView.php | 4 +++ .../herald/adapter/HeraldAdapter.php | 7 +++++ .../HeraldDifferentialRevisionAdapter.php | 17 +++++++++++ .../herald/storage/HeraldRule.php | 2 +- 6 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/applications/differential/constants/DifferentialReviewerStatus.php b/src/applications/differential/constants/DifferentialReviewerStatus.php index 02b716f28a..a892762914 100644 --- a/src/applications/differential/constants/DifferentialReviewerStatus.php +++ b/src/applications/differential/constants/DifferentialReviewerStatus.php @@ -2,6 +2,7 @@ final class DifferentialReviewerStatus { + const STATUS_BLOCKING = 'blocking'; const STATUS_ADDED = 'added'; const STATUS_ACCEPTED = 'accepted'; const STATUS_REJECTED = 'rejected'; diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index 135aa88e19..cc1cd6c6a6 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -276,11 +276,14 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { 'ccs' => $adapter->getCCsAddedByHerald(), ); $rem_ccs = $adapter->getCCsRemovedByHerald(); + $blocking_reviewers = array_keys( + $adapter->getBlockingReviewersAddedByHerald()); } else { $sub = array( 'rev' => array(), 'ccs' => array(), ); + $blocking_reviewers = array(); } // Remove any CCs which are prevented by Herald rules. @@ -314,7 +317,7 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { $this->getActor(), array_keys($add['rev']), array_keys($rem['rev']), - $this->getActorPHID()); + $blocking_reviewers); // We want to attribute new CCs to a "reasonPHID", representing the reason // they were added. This is either a user (if some user explicitly CCs @@ -602,7 +605,8 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { DifferentialRevision $revision, PhabricatorUser $actor, array $add_phids, - array $remove_phids) { + array $remove_phids, + array $blocking_phids = array()) { $reviewers = $revision->getReviewers(); @@ -618,22 +622,32 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { $editor = id(new PhabricatorEdgeEditor()) ->setActor($actor); - $options = array( - 'data' => array( - 'status' => DifferentialReviewerStatus::STATUS_ADDED - ) - ); - $reviewer_phids_map = array_fill_keys($reviewers, true); + $blocking_phids = array_fuse($blocking_phids); foreach ($add_phids as $phid) { // Adding an already existing edge again would have cause memory loss // That is, the previous state for that reviewer would be lost if (isset($reviewer_phids_map[$phid])) { + // TODO: If we're writing a blocking edge, we should overwrite an + // existing weaker edge (like "added" or "commented"), just not a + // stronger existing edge. continue; } + if (isset($blocking_phids[$phid])) { + $status = DifferentialReviewerStatus::STATUS_BLOCKING; + } else { + $status = DifferentialReviewerStatus::STATUS_ADDED; + } + + $options = array( + 'data' => array( + 'status' => $status, + ) + ); + $editor->addEdge( $revision->getPHID(), PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER, diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index 09179e57bf..3a247ee967 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -82,6 +82,10 @@ final class DifferentialReviewersView extends AphrontView { } break; + case DifferentialReviewerStatus::STATUS_BLOCKING: + $item->setIcon('minus-red', pht('Blocking Review')); + break; + default: $item->setIcon('question-dark', pht('%s?', $reviewer->getStatus())); break; diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 8edc3fe204..ec0485ed21 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -53,6 +53,7 @@ abstract class HeraldAdapter { const ACTION_ASSIGN_TASK = 'assigntask'; const ACTION_ADD_PROJECTS = 'addprojects'; const ACTION_ADD_REVIEWERS = 'addreviewers'; + const ACTION_ADD_BLOCKING_REVIEWERS = 'addblockingreviewers'; const VALUE_TEXT = 'text'; const VALUE_NONE = 'none'; @@ -488,6 +489,7 @@ abstract class HeraldAdapter { self::ACTION_ASSIGN_TASK => pht('Assign task to'), self::ACTION_ADD_PROJECTS => pht('Add projects'), self::ACTION_ADD_REVIEWERS => pht('Add reviewers'), + self::ACTION_ADD_BLOCKING_REVIEWERS => pht('Add blocking reviewers'), ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array( @@ -500,6 +502,8 @@ abstract class HeraldAdapter { self::ACTION_ASSIGN_TASK => pht('Assign task to me'), self::ACTION_ADD_PROJECTS => pht('Add projects'), self::ACTION_ADD_REVIEWERS => pht('Add me as a reviewer'), + self::ACTION_ADD_BLOCKING_REVIEWERS => + pht('Add me as a blocking reviewer'), ); default: throw new Exception("Unknown rule type '{$rule_type}'!"); @@ -525,6 +529,7 @@ abstract class HeraldAdapter { case self::ACTION_REMOVE_CC: case self::ACTION_AUDIT: case self::ACTION_ASSIGN_TASK: + case self::ACTION_ADD_REVIEWERS: case self::ACTION_ADD_REVIEWERS: // For personal rules, force these actions to target the rule owner. $target = array($author_phid); @@ -623,6 +628,7 @@ abstract class HeraldAdapter { case self::ACTION_AUDIT: case self::ACTION_ASSIGN_TASK: case self::ACTION_ADD_REVIEWERS: + case self::ACTION_ADD_BLOCKING_REVIEWERS: return self::VALUE_NONE; case self::ACTION_FLAG: return self::VALUE_FLAG_COLOR; @@ -647,6 +653,7 @@ abstract class HeraldAdapter { case self::ACTION_ASSIGN_TASK: return self::VALUE_USER; case self::ACTION_ADD_REVIEWERS: + case self::ACTION_ADD_BLOCKING_REVIEWERS: return self::VALUE_USER_OR_PROJECT; default: throw new Exception("Unknown or invalid action '{$action}'."); diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 8c0ce70c98..d61bb20b3a 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -115,6 +115,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { return $this->addReviewerPHIDs; } + public function getBlockingReviewersAddedByHerald() { + return $this->blockingReviewerPHIDs; + } + public function getPHID() { return $this->revision->getPHID(); } @@ -346,6 +350,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { self::ACTION_REMOVE_CC, self::ACTION_EMAIL, self::ACTION_ADD_REVIEWERS, + self::ACTION_ADD_BLOCKING_REVIEWERS, self::ACTION_NOTHING, ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: @@ -355,6 +360,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { self::ACTION_EMAIL, self::ACTION_FLAG, self::ACTION_ADD_REVIEWERS, + self::ACTION_ADD_BLOCKING_REVIEWERS, self::ACTION_NOTHING, ); } @@ -453,6 +459,17 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { true, pht('Added reviewers.')); break; + case self::ACTION_ADD_BLOCKING_REVIEWERS: + // This adds reviewers normally, it just also marks them blocking. + foreach ($effect->getTarget() as $phid) { + $this->addReviewerPHIDs[$phid] = true; + $this->blockingReviewerPHIDs[$phid] = true; + } + $result[] = new HeraldApplyTranscript( + $effect, + true, + pht('Added blocking reviewers.')); + break; default: throw new Exception("No rules to handle action '{$action}'."); } diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 57b1974d7f..255df439ef 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -13,7 +13,7 @@ final class HeraldRule extends HeraldDAO protected $repetitionPolicy; protected $ruleType; - protected $configVersion = 13; + protected $configVersion = 14; private $ruleApplied = self::ATTACHABLE; // phids for which this rule has been applied private $validAuthor = self::ATTACHABLE;