Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F14448957
D17566.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
9 KB
Referenced Files
None
Subscribers
None
D17566.diff
View Options
diff --git a/resources/sql/autopatches/20170328.reviewers.01.void.sql b/resources/sql/autopatches/20170328.reviewers.01.void.sql
new file mode 100644
--- /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
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -571,6 +571,7 @@
'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 @@
'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
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -357,6 +357,14 @@
}
}
+ 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 @@
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
--- a/src/applications/differential/storage/DifferentialReviewer.php
+++ b/src/applications/differential/storage/DifferentialReviewer.php
@@ -9,6 +9,7 @@
protected $lastActionDiffPHID;
protected $lastCommentDiffPHID;
protected $lastActorPHID;
+ protected $voidedPHID;
private $authority = array();
@@ -19,6 +20,7 @@
'lastActionDiffPHID' => 'phid?',
'lastCommentDiffPHID' => 'phid?',
'lastActorPHID' => 'phid?',
+ 'voidedPHID' => 'phid?',
),
self::CONFIG_KEY_SCHEMA => array(
'key_revision' => array(
@@ -59,6 +61,13 @@
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
--- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
@@ -50,25 +50,19 @@
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 @@
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 @@
$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 @@
$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
--- /dev/null
+++ b/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;
+ }
+
+}
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Fri, Dec 27, 7:09 PM (2 h, 18 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6933016
Default Alt Text
D17566.diff (9 KB)
Attached To
Mode
D17566: Fix an issue where "Request Review" of a fully-accepted revision would transition to "Accepted"
Attached
Detach File
Event Timeline
Log In to Comment