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 @@ -506,6 +506,7 @@ 'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php', 'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php', 'DifferentialRevisionAbandonTransaction' => 'applications/differential/xaction/DifferentialRevisionAbandonTransaction.php', + 'DifferentialRevisionAcceptTransaction' => 'applications/differential/xaction/DifferentialRevisionAcceptTransaction.php', 'DifferentialRevisionActionTransaction' => 'applications/differential/xaction/DifferentialRevisionActionTransaction.php', 'DifferentialRevisionAffectedFilesHeraldField' => 'applications/differential/herald/DifferentialRevisionAffectedFilesHeraldField.php', 'DifferentialRevisionAuthorHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorHeraldField.php', @@ -545,6 +546,7 @@ 'DifferentialRevisionPlanChangesTransaction' => 'applications/differential/xaction/DifferentialRevisionPlanChangesTransaction.php', 'DifferentialRevisionQuery' => 'applications/differential/query/DifferentialRevisionQuery.php', 'DifferentialRevisionReclaimTransaction' => 'applications/differential/xaction/DifferentialRevisionReclaimTransaction.php', + 'DifferentialRevisionRejectTransaction' => 'applications/differential/xaction/DifferentialRevisionRejectTransaction.php', 'DifferentialRevisionRelationship' => 'applications/differential/relationships/DifferentialRevisionRelationship.php', 'DifferentialRevisionRelationshipSource' => 'applications/search/relationship/DifferentialRevisionRelationshipSource.php', 'DifferentialRevisionReopenTransaction' => 'applications/differential/xaction/DifferentialRevisionReopenTransaction.php', @@ -553,6 +555,7 @@ 'DifferentialRevisionRepositoryTransaction' => 'applications/differential/xaction/DifferentialRevisionRepositoryTransaction.php', 'DifferentialRevisionRequestReviewTransaction' => 'applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php', 'DifferentialRevisionRequiredActionResultBucket' => 'applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php', + 'DifferentialRevisionResignTransaction' => 'applications/differential/xaction/DifferentialRevisionResignTransaction.php', 'DifferentialRevisionResultBucket' => 'applications/differential/query/DifferentialRevisionResultBucket.php', 'DifferentialRevisionReviewersHeraldField' => 'applications/differential/herald/DifferentialRevisionReviewersHeraldField.php', 'DifferentialRevisionReviewersTransaction' => 'applications/differential/xaction/DifferentialRevisionReviewersTransaction.php', @@ -5180,6 +5183,7 @@ 'PhabricatorConduitResultInterface', ), 'DifferentialRevisionAbandonTransaction' => 'DifferentialRevisionActionTransaction', + 'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionActionTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionAffectedFilesHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionAuthorHeraldField' => 'DifferentialRevisionHeraldField', @@ -5219,6 +5223,7 @@ 'DifferentialRevisionPlanChangesTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialRevisionReclaimTransaction' => 'DifferentialRevisionActionTransaction', + 'DifferentialRevisionRejectTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionRelationship' => 'PhabricatorObjectRelationship', 'DifferentialRevisionRelationshipSource' => 'PhabricatorObjectRelationshipSource', 'DifferentialRevisionReopenTransaction' => 'DifferentialRevisionActionTransaction', @@ -5227,6 +5232,7 @@ 'DifferentialRevisionRepositoryTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionRequestReviewTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionRequiredActionResultBucket' => 'DifferentialRevisionResultBucket', + 'DifferentialRevisionResignTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionResultBucket' => 'PhabricatorSearchResultBucket', 'DifferentialRevisionReviewersHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionReviewersTransaction' => 'DifferentialRevisionTransactionType', diff --git a/src/applications/differential/constants/DifferentialReviewerStatus.php b/src/applications/differential/constants/DifferentialReviewerStatus.php --- a/src/applications/differential/constants/DifferentialReviewerStatus.php +++ b/src/applications/differential/constants/DifferentialReviewerStatus.php @@ -9,6 +9,7 @@ const STATUS_COMMENTED = 'commented'; const STATUS_ACCEPTED_OLDER = 'accepted-older'; const STATUS_REJECTED_OLDER = 'rejected-older'; + const STATUS_RESIGNED = 'resigned'; /** * Returns the relative strength of a status, used to pick a winner when a @@ -34,6 +35,7 @@ self::STATUS_ACCEPTED => 5, self::STATUS_REJECTED => 5, + self::STATUS_RESIGNED => 5, ); return idx($map, $constant, 0); 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 @@ -38,7 +38,8 @@ protected function newObjectQuery() { return id(new DifferentialRevisionQuery()) ->needActiveDiffs(true) - ->needReviewerStatus(true); + ->needReviewerStatus(true) + ->needReviewerAuthority(true); } protected function getObjectCreateTitleText($object) { diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -0,0 +1,77 @@ +getActor(); + return $this->isViewerAcceptingReviewer($object, $actor); + } + + public function applyExternalEffects($object, $value) { + $status = DifferentialReviewerStatus::STATUS_ACCEPTED; + $actor = $this->getActor(); + $this->applyReviewerEffect($object, $actor, $value, $status); + } + + protected function validateAction($object, PhabricatorUser $viewer) { + if ($object->isClosed()) { + throw new Exception( + pht( + 'You can not accept this revision because it has already been '. + 'closed. Only open revisions can be accepted.')); + } + + $config_key = 'differential.allow-self-accept'; + if (!PhabricatorEnv::getEnvConfig($config_key)) { + if ($this->isViewerRevisionAuthor($object, $viewer)) { + throw new Exception( + pht( + 'You can not accept this revision because you are the revision '. + 'author. You can only accept revisions you do not own. You can '. + 'change this behavior by adjusting the "%s" setting in Config.', + $config_key)); + } + } + + if ($this->isViewerAcceptingReviewer($object, $viewer)) { + throw new Exception( + pht( + 'You can not accept this revision because you have already '. + 'accepted it.')); + } + } + + public function getTitle() { + return pht( + '%s accepted this revision.', + $this->renderAuthor()); + } + + public function getTitleForFeed() { + return pht( + '%s accepted %s.', + $this->renderAuthor(), + $this->renderObject()); + } + +} diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -41,6 +41,116 @@ return ($viewer->getPHID() === $revision->getAuthorPHID()); } + protected function isViewerAnyReviewer( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + return ($this->getViewerReviewerStatus($revision, $viewer) !== null); + } + + protected function isViewerAcceptingReviewer( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + return $this->isViewerReviewerStatusAmong( + $revision, + $viewer, + array( + DifferentialReviewerStatus::STATUS_ACCEPTED, + )); + } + + protected function isViewerRejectingReviewer( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + return $this->isViewerReviewerStatusAmong( + $revision, + $viewer, + array( + DifferentialReviewerStatus::STATUS_REJECTED, + )); + } + + protected function getViewerReviewerStatus( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + + if (!$viewer->getPHID()) { + return null; + } + + foreach ($revision->getReviewerStatus() as $reviewer) { + if ($reviewer->getReviewerPHID() != $viewer->getPHID()) { + continue; + } + + return $reviewer->getStatus(); + } + + return null; + } + + protected function isViewerReviewerStatusAmong( + DifferentialRevision $revision, + PhabricatorUser $viewer, + array $status_list) { + + $status = $this->getViewerReviewerStatus($revision, $viewer); + if ($status === null) { + return false; + } + + $status_map = array_fuse($status_list); + return isset($status_map[$status]); + } + + protected function applyReviewerEffect( + DifferentialRevision $revision, + PhabricatorUser $viewer, + $value, + $status) { + + $map = array(); + + // When you accept or reject, you may accept or reject on behalf of all + // reviewers you have authority for. When you resign, you only affect + // yourself. + $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED); + if ($with_authority) { + foreach ($revision->getReviewerStatus() as $reviewer) { + if ($reviewer->hasAuthority($viewer)) { + $map[$reviewer->getReviewerPHID()] = $status; + } + } + } + + // In all cases, you affect yourself. + $map[$viewer->getPHID()] = $status; + + // Convert reviewer statuses into edge data. + foreach ($map as $reviewer_phid => $reviewer_status) { + $map[$reviewer_phid] = array( + 'data' => array( + 'status' => $reviewer_status, + ), + ); + } + + $src_phid = $revision->getPHID(); + $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; + + $editor = new PhabricatorEdgeEditor(); + foreach ($map as $dst_phid => $edge_data) { + if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { + // TODO: For now, we just remove these reviewers. In the future, we will + // store resignations explicitly. + $editor->removeEdge($src_phid, $edge_type, $dst_phid); + } else { + $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data); + } + } + + $editor->save(); + } + public function newEditField( DifferentialRevision $revision, PhabricatorUser $viewer) { diff --git a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php @@ -0,0 +1,74 @@ +getActor(); + return $this->isViewerRejectingReviewer($object, $actor); + } + + public function applyExternalEffects($object, $value) { + $status = DifferentialReviewerStatus::STATUS_REJECTED; + $actor = $this->getActor(); + $this->applyReviewerEffect($object, $actor, $value, $status); + } + + protected function validateAction($object, PhabricatorUser $viewer) { + if ($object->isClosed()) { + throw new Exception( + pht( + 'You can not request changes to this revision because it has '. + 'already been closed. You can only request changes to open '. + 'revisions.')); + } + + if ($this->isViewerRevisionAuthor($object, $viewer)) { + throw new Exception( + pht( + 'You can not request changes to this revision because you are the '. + 'revision author. You can only request changes to revisions you do '. + 'not own.')); + } + + if ($this->isViewerRejectingReviewer($object, $viewer)) { + throw new Exception( + pht( + 'You can not request changes to this revision because you have '. + 'already requested changes.')); + } + } + + public function getTitle() { + return pht( + '%s requested changes to this revision.', + $this->renderAuthor()); + } + + public function getTitleForFeed() { + return pht( + '%s requested changes to %s.', + $this->renderAuthor(), + $this->renderObject()); + } + +} diff --git a/src/applications/differential/xaction/DifferentialRevisionResignTransaction.php b/src/applications/differential/xaction/DifferentialRevisionResignTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionResignTransaction.php @@ -0,0 +1,66 @@ +getActor(); + return !$this->isViewerAnyReviewer($object, $actor); + } + + public function applyExternalEffects($object, $value) { + $status = DifferentialReviewerStatus::STATUS_RESIGNED; + $actor = $this->getActor(); + $this->applyReviewerEffect($object, $actor, $value, $status); + } + + protected function validateAction($object, PhabricatorUser $viewer) { + if ($object->isClosed()) { + throw new Exception( + pht( + 'You can not resign from this revision because it has already '. + 'been closed. You can only resign from open revisions.')); + } + + if (!$this->isViewerAnyReviewer($object, $viewer)) { + throw new Exception( + pht( + 'You can not resign from this revision because you are not a '. + 'reviewer. You can only resign from revisions where you are a '. + 'reviewer.')); + } + } + + public function getTitle() { + return pht( + '%s resigned from this revision.', + $this->renderAuthor()); + } + + public function getTitleForFeed() { + return pht( + '%s resigned from %s.', + $this->renderAuthor(), + $this->renderObject()); + } + +}