Put revision back in "Changes Planned" when revision in "Request Changes" state is updated

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) {

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;
   }
 

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();
   }

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;

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;