From aea46e55dac035e33080016c190c6c0765a84fa1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 05:23:42 -0700 Subject: [PATCH] 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 --- .../20170328.reviewers.01.void.sql | 2 + src/__phutil_library_map__.php | 2 + .../editor/DifferentialTransactionEditor.php | 13 ++-- .../storage/DifferentialReviewer.php | 9 +++ .../DifferentialRevisionReviewTransaction.php | 41 +++++++----- .../DifferentialRevisionVoidTransaction.php | 63 +++++++++++++++++++ 6 files changed, 109 insertions(+), 21 deletions(-) create mode 100644 resources/sql/autopatches/20170328.reviewers.01.void.sql create mode 100644 src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php diff --git a/resources/sql/autopatches/20170328.reviewers.01.void.sql b/resources/sql/autopatches/20170328.reviewers.01.void.sql new file mode 100644 index 0000000000..b46cb9351d --- /dev/null +++ b/resources/sql/autopatches/20170328.reviewers.01.void.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_reviewer + ADD voidedPHID VARBINARY(64); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5d9e3de268..0921fa21e9 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 3f00f4b837..9b837876fb 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/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; } } diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index d43f533b5c..6912e2af1e 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/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; } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index 3db289470a..c0971a5a2b 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/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) { diff --git a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php new file mode 100644 index 0000000000..e40cb8af62 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php @@ -0,0 +1,63 @@ +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; + } + +}