Downgrade accepts on "request changes", and make sticky accepts optional

Summary:
Fixes T3202. This fixes a couple of workflow issues:

  - Accepted Revision -> Request Review. Currently this stays "accepted" due to sticky rules being too aggressive, but should transition to "needs review".
  - Accepted Revision -> Plan Changes -> Request Review. Currently this stays "accepted". I think this behavior is correct, and have retained it. (In this case, you don't update the revision, you just "undo" your plan changes.) You can "Request Review" again to get back to "Needs Review".

Then implements a "sticky accept" switch:

  - When off, updates downgrade accepts.
  - When off, "request review" always downgrades accepts.

Test Plan:
  - Went through all (I think?) of the plan changes / request review / accept / update workflows, with sticky accept on and off.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3202

Differential Revision: https://secure.phabricator.com/D8614

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php

@@ -114,6 +114,25 @@ final class PhabricatorDifferentialConfigOptions
             "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(

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -238,10 +238,16 @@ final class DifferentialTransactionEditor
     $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 @@ final class DifferentialTransactionEditor
       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 @@ final class DifferentialTransactionEditor
     $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 @@ final class DifferentialTransactionEditor
 
       $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) {

src/docs/user/userguide/differential_faq.diviner

@@ -5,22 +5,25 @@ Common questions about Differential.
 
 = 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 @@ get an accept), solve the problem socially by telling them to stop being jerks.
 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 @@ Most of these are pretty straightforward, but "Ignore Most" is not:
     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}.