Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F14376541
D8614.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
8 KB
Referenced Files
None
Subscribers
None
D8614.diff
View Options
diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
--- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
+++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
@@ -114,6 +114,25 @@
"generated by an automatic process, and thus get hidden by ".
"default in differential."))
->addExample("/config\.h$/\n#/autobuilt/#", pht("Valid Setting")),
+ $this->newOption('differential.sticky-accept', 'bool', true)
+ ->setBoolOptions(
+ array(
+ pht("Accepts persist across updates"),
+ pht("Accepts are reset by updates"),
+ ))
+ ->setSummary(
+ pht("Should updating an accepted revision require re-review?"))
+ ->setDescription(
+ pht(
+ 'Normally, when revisions that have been "Accepted" are updated, '.
+ 'they remain "Accepted". This allows reviewers to suggest minor '.
+ 'alterations when accepting, and encourages authors to update '.
+ 'if they make minor changes in response to this feedback.'.
+ "\n\n".
+ 'If you want updates to always require re-review, you can disable '.
+ 'the "stickiness" of the "Accepted" status with this option. '.
+ 'This may make the process for minor changes much more burdensome '.
+ 'to both authors and reviewers.')),
$this->newOption('differential.allow-self-accept', 'bool', false)
->setBoolOptions(
array(
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
@@ -238,10 +238,16 @@
$actor_phid = $actor->getPHID();
$type_edge = PhabricatorTransactions::TYPE_EDGE;
+ $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
+
$edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
$edge_ref_task = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK;
+ $is_sticky_accept = PhabricatorEnv::getEnvConfig(
+ 'differential.sticky-accept');
+
$downgrade_rejects = false;
+ $downgrade_accepts = false;
if ($this->getIsCloseByCommit()) {
// Never downgrade reviewers when we're closing a revision after a
// commit.
@@ -249,11 +255,23 @@
switch ($xaction->getTransactionType()) {
case DifferentialTransaction::TYPE_UPDATE:
$downgrade_rejects = true;
+ if (!$is_sticky_accept) {
+ // If "sticky accept" is disabled, also downgrade the accepts.
+ $downgrade_accepts = true;
+ }
break;
case DifferentialTransaction::TYPE_ACTION:
switch ($xaction->getNewValue()) {
case DifferentialAction::ACTION_REQUEST:
$downgrade_rejects = true;
+ if ((!$is_sticky_accept) ||
+ ($object->getStatus() != $status_plan)) {
+ // If the old state isn't "changes planned", downgrade the
+ // accepts. This exception allows an accepted revision to
+ // go through Plan Changes -> Request Review to return to
+ // "accepted" if the author didn't update the revision.
+ $downgrade_accepts = true;
+ }
break;
}
break;
@@ -265,7 +283,7 @@
$old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER;
$old_reject = DifferentialReviewerStatus::STATUS_REJECTED_OLDER;
- if ($downgrade_rejects) {
+ if ($downgrade_rejects || $downgrade_accepts) {
// When a revision is updated, change all "reject" to "rejected older
// revision". This means we won't immediately push the update back into
// "needs review", but outstanding rejects will still block it from
@@ -277,15 +295,25 @@
$edits = array();
foreach ($object->getReviewerStatus() as $reviewer) {
- if ($reviewer->getStatus() == $new_reject) {
- $edits[$reviewer->getReviewerPHID()] = array(
- 'data' => array(
- 'status' => $old_reject,
- ),
- );
+ if ($downgrade_rejects) {
+ if ($reviewer->getStatus() == $new_reject) {
+ $edits[$reviewer->getReviewerPHID()] = array(
+ 'data' => array(
+ 'status' => $old_reject,
+ ),
+ );
+ }
}
- // TODO: If sticky accept is off, do a similar update for accepts.
+ if ($downgrade_accepts) {
+ if ($reviewer->getStatus() == $new_accept) {
+ $edits[$reviewer->getReviewerPHID()] = array(
+ 'data' => array(
+ 'status' => $old_accept,
+ ),
+ );
+ }
+ }
}
if ($edits) {
diff --git a/src/docs/user/userguide/differential_faq.diviner b/src/docs/user/userguide/differential_faq.diviner
--- a/src/docs/user/userguide/differential_faq.diviner
+++ b/src/docs/user/userguide/differential_faq.diviner
@@ -5,22 +5,25 @@
= Why does an "accepted" revision remain accepted when it is updated? =
-When a revision author updates an "accepted" revision in Differential, the
-state remains "accepted". This can be confusing if you expect the revision to
-change to "needs review" when it is updated.
+You can configure this behavior with `differential.sticky-accept`.
-This behavior is intentional, to encourage authors to update revisions when they
-make minor changes after a revision is accepted. For example, a reviewer may
-accept a change with a comment like this:
+When a revision author updates an "Accepted" revision in Differential, the
+state remains "Accepted". This can be confusing if you expect the revision to
+change to "Needs Review" when it is updated.
+
+Although this behavior is configurable, we think stickiness is a good behavior:
+stickiness encourage authors to update revisions when they make minor changes
+after a revision is accepted. For example, a reviewer may accept a change with a
+comment like this:
> Looks great, but can you add some documentation for the foo() function
> before you land it? I also caught a couple typos, see inlines.
-If updating the revision reverted the status to "needs review", the author
+If updating the revision reverted the status to "Needs Review", the author
is discouraged from updating the revision when they make minor changes because
they'll have to wait for their reviewer to have a chance to look at it again.
-Instead, the "accept" state is sticky to encourage them to update the revision
+Instead, the "Accepted" state is sticky to encourage them to update the revision
with a comment like:
> - Added docs.
@@ -41,10 +44,12 @@
Unless you've configured additional layers of enforcement, there's nothing
stopping them from silently changing the code before pushing it, anyway.
+
= How can I enable syntax highlighting? =
You need to install and configure **Pygments** to highlight anything else than
-PHP. Consult the configuration file for instructions.
+PHP. See the `pygments.enabled` configuration setting.
+
= What do the whitespace options mean? =
@@ -61,10 +66,15 @@
setting.
- **Ignore All**: Ignore all whitespace changes in all files.
-= What does the very light green and red backgrounds mean? =
-Differential uses these colors to mark changes coming from rebase - they are
+= What do the very light green and red backgrounds mean? =
+
+Differential uses these colors to mark changes coming from rebase: they are
part of the diff but they were not added or removed by the author. They can
-appear in diff of diffs against different bases. See
-[[ https://secure.phabricator.com/D3324?vs=6468&id=6513#toc | D3324 ]] for
-example.
+appear in diff of diffs against different bases.
+
+= Next Steps =
+
+Continue by:
+
+ - returning to the @{article:Differential User Guide}.
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Sat, Dec 21, 11:56 PM (21 h, 15 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6916620
Default Alt Text
D8614.diff (8 KB)
Attached To
Mode
D8614: Downgrade accepts on "request changes", and make sticky accepts optional
Attached
Detach File
Event Timeline
Log In to Comment