diff --git a/resources/sql/autopatches/20180312.reviewers.01.options.sql b/resources/sql/autopatches/20180312.reviewers.01.options.sql new file mode 100644 index 0000000000..159426614d --- /dev/null +++ b/resources/sql/autopatches/20180312.reviewers.01.options.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_reviewer + ADD options LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180312.reviewers.02.optionsdefault.sql b/resources/sql/autopatches/20180312.reviewers.02.optionsdefault.sql new file mode 100644 index 0000000000..d509011f73 --- /dev/null +++ b/resources/sql/autopatches/20180312.reviewers.02.optionsdefault.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_differential.differential_reviewer + SET options = '{}' WHERE options = ''; diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index d04b0d684a..9882246608 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -154,59 +154,41 @@ final class DifferentialTransactionEditor $edge_ref_task = DifferentialRevisionHasTaskEdgeType::EDGECONST; - $is_sticky_accept = PhabricatorEnv::getEnvConfig( - 'differential.sticky-accept'); - - $downgrade_rejects = false; - $downgrade_accepts = false; + $want_downgrade = array(); + $must_downgrade = array(); if ($this->getIsCloseByCommit()) { // Never downgrade reviewers when we're closing a revision after a // commit. } else { switch ($xaction->getTransactionType()) { case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: - $downgrade_rejects = true; - if (!$is_sticky_accept) { - // If "sticky accept" is disabled, also downgrade the accepts. - $downgrade_accepts = true; - } + $want_downgrade[] = DifferentialReviewerStatus::STATUS_ACCEPTED; + $want_downgrade[] = DifferentialReviewerStatus::STATUS_REJECTED; break; case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE: - $downgrade_rejects = true; - if ((!$is_sticky_accept) || - (!$object->isChangePlanned())) { - // If the old state isn't "changes planned", downgrade the accepts. - // This exception allows an accepted revision to go through - // "Plan Changes" -> "Request Review" and return to "accepted" if - // the author didn't update the revision, essentially undoing the - // "Plan Changes". - $downgrade_accepts = true; + if (!$object->isChangePlanned()) { + // If the old state isn't "Changes Planned", downgrade the accepts + // even if they're sticky. + + // We don't downgrade for "Changes Planned" to allow an author to + // undo a "Plan Changes" by immediately following it up with a + // "Request Review". + $want_downgrade[] = DifferentialReviewerStatus::STATUS_ACCEPTED; + $must_downgrade[] = DifferentialReviewerStatus::STATUS_ACCEPTED; } + $want_downgrade[] = DifferentialReviewerStatus::STATUS_REJECTED; break; } } - $new_accept = DifferentialReviewerStatus::STATUS_ACCEPTED; - $new_reject = DifferentialReviewerStatus::STATUS_REJECTED; - $old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER; - $old_reject = DifferentialReviewerStatus::STATUS_REJECTED_OLDER; - - $downgrade = array(); - if ($downgrade_accepts) { - $downgrade[] = DifferentialReviewerStatus::STATUS_ACCEPTED; - } - - if ($downgrade_rejects) { - $downgrade[] = DifferentialReviewerStatus::STATUS_REJECTED; - } - - if ($downgrade) { + if ($want_downgrade) { $void_type = DifferentialRevisionVoidTransaction::TRANSACTIONTYPE; $results[] = id(new DifferentialTransaction()) ->setTransactionType($void_type) ->setIgnoreOnNoEffect(true) - ->setNewValue($downgrade); + ->setMetadataValue('void.force', $must_downgrade) + ->setNewValue($want_downgrade); } $is_commandeer = false; diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index e3f9bdaf8d..823047f218 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -10,12 +10,16 @@ final class DifferentialReviewer protected $lastCommentDiffPHID; protected $lastActorPHID; protected $voidedPHID; + protected $options = array(); private $authority = array(); private $changesets = self::ATTACHABLE; protected function getConfiguration() { return array( + self::CONFIG_SERIALIZATION => array( + 'options' => self::SERIALIZATION_JSON, + ), self::CONFIG_COLUMN_SCHEMA => array( 'reviewerStatus' => 'text64', 'lastActionDiffPHID' => 'phid?', @@ -64,6 +68,15 @@ final class DifferentialReviewer return $this->assertAttached($this->changesets); } + public function setOption($key, $value) { + $this->options[$key] = $value; + return $this; + } + + public function getOption($key, $default = null) { + return idx($this->options, $key, $default); + } + public function isResigned() { $status_resigned = DifferentialReviewerStatus::STATUS_RESIGNED; return ($this->getReviewerStatus() == $status_resigned); diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index b112eb638f..8aec75d2fb 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -164,7 +164,14 @@ abstract class DifferentialRevisionReviewTransaction DifferentialRevision $revision, PhabricatorUser $viewer, $value, - $status) { + $status, + array $reviewer_options = array()) { + + PhutilTypeSpec::checkMap( + $reviewer_options, + array( + 'sticky' => 'optional bool', + )); $map = array(); @@ -253,6 +260,8 @@ abstract class DifferentialRevisionReviewTransaction // accepted. $reviewer->setVoidedPHID(null); + $reviewer->setOption('sticky', idx($reviewer_options, 'sticky')); + try { $reviewer->save(); } catch (AphrontDuplicateKeyQueryException $ex) { diff --git a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php index 5073a9c464..331c637aad 100644 --- a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php @@ -16,21 +16,43 @@ final class DifferentialRevisionVoidTransaction } 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 IN (%Ls)', - $table_name, + $reviewers = id(new DifferentialReviewer())->loadAllWhere( + 'revisionPHID = %s + AND voidedPHID IS NULL + AND reviewerStatus IN (%Ls)', $object->getPHID(), $value); - return ipull($rows, 'reviewerPHID'); + $must_downgrade = $this->getMetadataValue('void.force', array()); + $must_downgrade = array_fuse($must_downgrade); + + $default = PhabricatorEnv::getEnvConfig('differential.sticky-accept'); + foreach ($reviewers as $key => $reviewer) { + $status = $reviewer->getReviewerStatus(); + + // If this void is forced, always downgrade. For example, this happens + // when an author chooses "Request Review": existing reviews are always + // voided, even if they're sticky. + if (isset($must_downgrade[$status])) { + continue; + } + + // Otherwise, if this is a sticky accept, don't void it. Accepts may be + // explicitly sticky or unsticky, or they'll use the default value if + // no value is specified. + $is_sticky = $reviewer->getOption('sticky'); + $is_sticky = coalesce($is_sticky, $default); + + if ($status === DifferentialReviewerStatus::STATUS_ACCEPTED) { + if ($is_sticky) { + unset($reviewers[$key]); + continue; + } + } + } + + + return mpull($reviewers, 'getReviewerPHID'); } public function getTransactionHasEffect($object, $old, $new) {