diff --git a/src/applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php index d9b0779211..25d30dc0b1 100644 --- a/src/applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php @@ -27,6 +27,7 @@ protected function defineParamTypes() { 'diffid' => 'required diffid', 'fields' => 'required dict', 'message' => 'required string', + 'needs-review' => 'optional bool', ); } @@ -45,9 +46,11 @@ protected function defineErrorTypes() { protected function execute(ConduitAPIRequest $request) { $viewer = $request->getUser(); + $needs_review = $request->getValue('needs-review', false); $diff = id(new DifferentialDiffQuery()) ->setViewer($viewer) + ->needReview($needs_review) ->withIDs(array($request->getValue('diffid'))) ->executeOne(); if (!$diff) { diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index b935b09b2d..f9b9af9e58 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -404,9 +404,29 @@ private function updateReviewStatus( DifferentialRevision $revision, array $xactions) { + $was_change_planned = $revision->isChangePlanned(); $was_accepted = $revision->isAccepted(); $was_revision = $revision->isNeedsRevision(); $was_review = $revision->isNeedsReview(); + + // // Revisions should not transition out of "Request Changes" + // // or "Changes Planned" state unless the "needs-review" flag + // // has been passed from arcanist + // if ($was_change_planned || $was_revision) { + // $needs_review = false; + // // Check if any transaction has needs-review flag + // foreach ($xactions as $xaction) { + // if ($xaction->getMetadataValue('needs-review')) { + // $needs_review = true; + // break; + // } + // } + + // if (!$needs_review) { + // return $xactions; + // } + // } + if (!$was_accepted && !$was_revision && !$was_review) { // Revisions can't transition out of other statuses (like closed or // abandoned) as a side effect of reviewer status changes. @@ -471,10 +491,10 @@ private function updateReviewStatus( // reviewer resigns or is removed. $new_status = DifferentialRevisionStatus::NEEDS_REVIEW; } else if ($was_revision) { - // This revision was "Needs Revision", but no longer has any rejecting + // This revision was "Needs Revision", and no longer has any rejecting // reviewers. This usually happens after the last rejecting reviewer - // resigns or is removed. Put the revision back in "Needs Review". - $new_status = DifferentialRevisionStatus::NEEDS_REVIEW; + // resigns or is removed. Put the revision in "Changes Planned". + $new_status = DifferentialRevisionStatus::CHANGES_PLANNED; } if ($new_status === null) { @@ -536,6 +556,19 @@ protected function shouldPublishFeedStory( protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { + + // Check if any transaction has needs-review flag + foreach ($xactions as $xaction) { + if ($xaction->getMetadataValue('needs-review')) { + return true; + } + } + + // Don't notify reviewers for revisions in "changes planned" / "request changes" state + if ($object->isChangePlanned() || $object->isNeedsRevision()) { + return false; + } + return true; } diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php index 04019df1e0..32f80f69b3 100644 --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/src/applications/differential/query/DifferentialDiffQuery.php @@ -12,6 +12,7 @@ final class DifferentialDiffQuery private $needChangesets = false; private $needProperties; + private $requestReview = false; public function withIDs(array $ids) { $this->ids = $ids; @@ -53,6 +54,22 @@ public function needProperties($need_properties) { return $this; } + /** + * Set whether or not the user has passed --needs-review flag, + * indicating query is ready for review and + * should send notifications to associated reviewers. + * + * @param bool True to update revision to "Needs Review" and + * send notifications to reviewers. + * + * @return this + * @task config + */ + public function requestReview($requestReview) { + $this->requestReview = $requestReview; + return $this; + } + public function newResultObject() { return new DifferentialDiff(); } diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 75d3e052e4..11af4af4b8 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -34,7 +34,7 @@ final class DifferentialRevisionQuery private $needDiffIDs = false; private $needCommitPHIDs = false; private $needHashes = false; - private $needReviewers = false; + private $needReviewers = false; private $needReviewerAuthority; private $needDrafts; private $needFlags; diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index a33a13f0b6..f0686a6eff 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -39,6 +39,8 @@ final class DifferentialDiff protected $viewPolicy; + protected $requestReview; + private $unsavedChangesets = array(); private $changesets = self::ATTACHABLE; private $revision = self::ATTACHABLE; @@ -66,6 +68,7 @@ protected function getConfiguration() { 'bookmark' => 'text255?', 'repositoryUUID' => 'text64?', 'commitPHID' => 'phid?', + 'requestReview' => 'bool?', // T6203/NULLABILITY // These should be non-null; all diffs should have a creation method @@ -363,6 +366,16 @@ public function getDiffProperties() { return $this->assertAttached($this->properties); } + public function getRequestReview() { + return $this->getProperty(self::PROPERTY_REQUEST_REVIEW, false); + } + + public function setRequestReview($requestReview) { + return $this->setProperty( + self::PROPERTY_REQUEST_REVIEW, + $requestReview); + } + public function attachBuildable(HarbormasterBuildable $buildable = null) { $this->buildable = $buildable; return $this;