Page MenuHomePhabricator
Files F14619871

D19211.id.diff
No OneTemporary
Actions

D19211.id.diff
View Options

diff --git a/resources/sql/autopatches/20180312.reviewers.01.options.sql b/resources/sql/autopatches/20180312.reviewers.01.options.sql
new file mode 100644
--- /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
--- /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
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -154,59 +154,41 @@
$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
--- a/src/applications/differential/storage/DifferentialReviewer.php
+++ b/src/applications/differential/storage/DifferentialReviewer.php
@@ -10,12 +10,16 @@
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 @@
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
--- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
@@ -164,7 +164,14 @@
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 @@
// 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
--- a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php
@@ -16,21 +16,43 @@
}
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) {

File Metadata

Mime Type
text/plain
Expires
Fri, Jan 10, 9:17 PM (13 m, 26 s)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6984096
Default Alt Text
D19211.id.diff (8 KB)

Event Timeline

Phabricator · Built by Phacility