From 1e93b49b1bf23b8d63bc984b6c1d0ab37c1d3c20 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Mar 2018 16:39:29 -0700 Subject: [PATCH] Allow custom actions in Differential to explicitly override "accept" stickiness Summary: See PHI431. Ref T13102. An install is interested in a custom "non-sticky" accept action, roughly. On the one hand, this is a pretty hacky patch. However, I suspect it inches us closer to T731, and I'm generally comfortable with exploring the realms of "Accept Next Update", "Unblock Without Accepting", etc., as long as most of it doesn't end up enabled by default in the upstream. Test Plan: - Accepted and updated revisions normally, saw accepts respect global stickiness. - Modified the "Accept" action to explicitly be unsticky, saw nonsticky accept behavior after update. Maniphest Tasks: T13102 Differential Revision: https://secure.phabricator.com/D19211 --- .../20180312.reviewers.01.options.sql | 2 + .../20180312.reviewers.02.optionsdefault.sql | 2 + .../editor/DifferentialTransactionEditor.php | 52 ++++++------------- .../storage/DifferentialReviewer.php | 13 +++++ .../DifferentialRevisionReviewTransaction.php | 11 +++- .../DifferentialRevisionVoidTransaction.php | 46 +++++++++++----- 6 files changed, 78 insertions(+), 48 deletions(-) create mode 100644 resources/sql/autopatches/20180312.reviewers.01.options.sql create mode 100644 resources/sql/autopatches/20180312.reviewers.02.optionsdefault.sql 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) {