Fix an issue where "Request Review" of a fully-accepted revision would transition to "Accepted"

Summary:
Ref T10967. This is explained in more detail in T10967#217125

When an author does "Request Review" on an accepted revision, void (in the sense of "cancel out", like a bank check) any "accepted" reviewers on the current diff.

Test Plan:
  - Create a revision with author A and reviewer B.
  - Accept as B.
  - "Request Review" as A.
  - (With sticky accepts enabled.)
  - Before patch: revision swithced back to "accepted".
  - After patch: the earlier review is "voided" by te "Request Review", and the revision switches to "Review Requested".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17566

resources/sql/autopatches/20170328.reviewers.01.void.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_differential.differential_reviewer
+  ADD voidedPHID VARBINARY(64);

src/__phutil_library_map__.php

@@ -571,6 +571,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionTransactionType' => 'applications/differential/xaction/DifferentialRevisionTransactionType.php',
     'DifferentialRevisionUpdateHistoryView' => 'applications/differential/view/DifferentialRevisionUpdateHistoryView.php',
     'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php',
+    'DifferentialRevisionVoidTransaction' => 'applications/differential/xaction/DifferentialRevisionVoidTransaction.php',
     'DifferentialSchemaSpec' => 'applications/differential/storage/DifferentialSchemaSpec.php',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php',
     'DifferentialStoredCustomField' => 'applications/differential/customfield/DifferentialStoredCustomField.php',
@@ -5357,6 +5358,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionTransactionType' => 'PhabricatorModularTransactionType',
     'DifferentialRevisionUpdateHistoryView' => 'AphrontView',
     'DifferentialRevisionViewController' => 'DifferentialController',
+    'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'DifferentialConduitAPIMethod',
     'DifferentialStoredCustomField' => 'DifferentialCustomField',

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -357,6 +357,14 @@ final class DifferentialTransactionEditor
       }
     }
 
+    if ($downgrade_accepts) {
+      $void_type = DifferentialRevisionVoidTransaction::TRANSACTIONTYPE;
+      $results[] = id(new DifferentialTransaction())
+        ->setTransactionType($void_type)
+        ->setIgnoreOnNoEffect(true)
+        ->setNewValue(true);
+    }
+
     $is_commandeer = false;
     switch ($xaction->getTransactionType()) {
       case DifferentialTransaction::TYPE_UPDATE:
@@ -685,11 +693,8 @@ final class DifferentialTransactionEditor
               break;
             case DifferentialReviewerStatus::STATUS_ACCEPTED:
               if ($reviewer->isUser()) {
-                $action_phid = $reviewer->getLastActionDiffPHID();
                 $active_phid = $active_diff->getPHID();
-                $is_current = ($action_phid == $active_phid);
-
-                if ($is_sticky_accept || $is_current) {
+                if ($reviewer->isAccepted($active_phid)) {
                   $has_accepting_user = true;
                 }
               }

src/applications/differential/storage/DifferentialReviewer.php

@@ -9,6 +9,7 @@ final class DifferentialReviewer
   protected $lastActionDiffPHID;
   protected $lastCommentDiffPHID;
   protected $lastActorPHID;
+  protected $voidedPHID;
 
   private $authority = array();
 
@@ -19,6 +20,7 @@ final class DifferentialReviewer
         'lastActionDiffPHID' => 'phid?',
         'lastCommentDiffPHID' => 'phid?',
         'lastActorPHID' => 'phid?',
+        'voidedPHID' => 'phid?',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'key_revision' => array(
@@ -59,6 +61,13 @@ final class DifferentialReviewer
       return false;
     }
 
+    // If this accept has been voided (for example, but a reviewer using
+    // "Request Review"), don't count it as a real "Accept" even if it is
+    // against the current diff PHID.
+    if ($this->getVoidedPHID()) {
+      return false;
+    }
+
     if (!$diff_phid) {
       return true;
     }

src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php

@@ -50,25 +50,19 @@ abstract class DifferentialRevisionReviewTransaction
   protected function isViewerFullyAccepted(
     DifferentialRevision $revision,
     PhabricatorUser $viewer) {
-    return $this->isViewerReviewerStatusFullyAmong(
+    return $this->isViewerReviewerStatusFully(
       $revision,
       $viewer,
-      array(
-        DifferentialReviewerStatus::STATUS_ACCEPTED,
-      ),
-      true);
+      DifferentialReviewerStatus::STATUS_ACCEPTED);
   }
 
   protected function isViewerFullyRejected(
     DifferentialRevision $revision,
     PhabricatorUser $viewer) {
-    return $this->isViewerReviewerStatusFullyAmong(
+    return $this->isViewerReviewerStatusFully(
       $revision,
       $viewer,
-      array(
-        DifferentialReviewerStatus::STATUS_REJECTED,
-      ),
-      true);
+      DifferentialReviewerStatus::STATUS_REJECTED);
   }
 
   protected function getViewerReviewerStatus(
@@ -90,11 +84,10 @@ abstract class DifferentialRevisionReviewTransaction
     return null;
   }
 
-  protected function isViewerReviewerStatusFullyAmong(
+  private function isViewerReviewerStatusFully(
     DifferentialRevision $revision,
     PhabricatorUser $viewer,
-    array $status_list,
-    $require_current) {
+    $require_status) {
 
     // If the user themselves is not a reviewer, the reviews they have
     // authority over can not all be in any set of states since their own
@@ -106,24 +99,33 @@ abstract class DifferentialRevisionReviewTransaction
 
     $active_phid = $this->getActiveDiffPHID($revision);
 
+    $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED;
+    $is_accepted = ($require_status == $status_accepted);
+
     // Otherwise, check that all reviews they have authority over are in
     // the desired set of states.
-    $status_map = array_fuse($status_list);
     foreach ($revision->getReviewers() as $reviewer) {
       if (!$reviewer->hasAuthority($viewer)) {
         continue;
       }
 
       $status = $reviewer->getReviewerStatus();
-      if (!isset($status_map[$status])) {
+      if ($status != $require_status) {
         return false;
       }
 
-      if ($require_current) {
-        if ($reviewer->getLastActionDiffPHID() != $active_phid) {
+      // Here, we're primarily testing if we can remove a void on the review.
+      if ($is_accepted) {
+        if (!$reviewer->isAccepted($active_phid)) {
           return false;
         }
       }
+
+      // This is a broader check to see if we can update the diff where the
+      // last action occurred.
+      if ($reviewer->getLastActionDiffPHID() != $active_phid) {
+        return false;
+      }
     }
 
     return true;
@@ -223,6 +225,11 @@ abstract class DifferentialRevisionReviewTransaction
           $reviewer->setLastActorPHID($this->getActingAsPHID());
         }
 
+        // Clear any outstanding void on this reviewer. A void may be placed
+        // by the author using "Request Review" when a reviewer has already
+        // accepted.
+        $reviewer->setVoidedPHID(null);
+
         try {
           $reviewer->save();
         } catch (AphrontDuplicateKeyQueryException $ex) {

src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php

@@ -0,0 +1,63 @@
+<?php
+
+/**
+ * This is an internal transaction type used to void reviews.
+ *
+ * For example, "Request Review" voids any open accepts, so they no longer
+ * act as current accepts.
+ */
+final class DifferentialRevisionVoidTransaction
+  extends DifferentialRevisionTransactionType {
+
+  const TRANSACTIONTYPE = 'differential.revision.void';
+
+  public function generateOldValue($object) {
+    return false;
+  }
+
+  public function generateNewValue($object, $value) {
+    $table = new DifferentialReviewer();
+    $table_name = $table->getTableName();
+    $conn = $table->establishConnection('w');
+
+    $rows = queryfx_all(
+      $conn,
+      'SELECT reviewerPHID FROM %T
+        WHERE revisionPHID = %s
+          AND voidedPHID IS NULL
+          AND reviewerStatus = %s',
+      $table_name,
+      $object->getPHID(),
+      DifferentialReviewerStatus::STATUS_ACCEPTED);
+
+    return ipull($rows, 'reviewerPHID');
+  }
+
+  public function getTransactionHasEffect($object, $old, $new) {
+    return (bool)$new;
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $table = new DifferentialReviewer();
+    $table_name = $table->getTableName();
+    $conn = $table->establishConnection('w');
+
+    queryfx(
+      $conn,
+      'UPDATE %T SET voidedPHID = %s
+        WHERE revisionPHID = %s
+          AND voidedPHID IS NULL
+          AND reviewerStatus = %s',
+      $table_name,
+      $this->getActingAsPHID(),
+      $object->getPHID(),
+      DifferentialReviewerStatus::STATUS_ACCEPTED);
+  }
+
+  public function shouldHide() {
+    // This is an internal transaction, so don't show it in feeds or
+    // transaction logs.
+    return true;
+  }
+
+}