revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00
Code Issues Releases Wiki Activity

Allow custom actions in Differential to explicitly override "accept" stickiness

Browse source 
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
This commit is contained in:
epriestley 2018-03-12 16:39:29 -07:00
parent df8d4dff67
commit 1e93b49b1b
6 changed files with 78 additions and 48 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
resources/sql/autopatches/20180312.reviewers.01.options.sql Normal file
View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_differential.differential_reviewer
ADD options LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT};

2
resources/sql/autopatches/20180312.reviewers.02.optionsdefault.sql Normal file
View file

@ -0,0 +1,2 @@
UPDATE {$NAMESPACE}_differential.differential_reviewer
SET options = '{}' WHERE options = '';

52
src/applications/differential/editor/DifferentialTransactionEditor.php
View file

@ -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;

13
src/applications/differential/storage/DifferentialReviewer.php
View file

@ -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);

11
src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
View file

@ -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) {

46
src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php
View file

@ -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) {