Rename "getReviewerStatus()" to "getReviewers()"

Summary:
Ref T10967. Improves some method names:

  - `Revision->getReviewerStatus()` -> `Revision->getReviewers()`
  - `Revision->attachReviewerStatus()` -> `Revision->attachReviewers()`
  - `Reviewer->getStatus()` -> `Reviewer->getReviewerStatus()` (this is mostly to make this more greppable)

Test Plan:
  - bunch o' `grep`
  - Browsed around.
  - If I missed anything, it should fatal in an obvious way. We have a lot of other `getStatus()` calls and it's hard to be sure I got them all.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

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

src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php

@@ -53,7 +53,7 @@ final class DifferentialCreateRevisionConduitAPIMethod
     }
 
     $revision = DifferentialRevision::initializeNewRevision($viewer);
-    $revision->attachReviewerStatus(array());
+    $revision->attachReviewers(array());
 
     $result = $this->applyFieldEdit(
       $request,

src/applications/differential/customfield/DifferentialProjectReviewersField.php

@@ -52,7 +52,7 @@ final class DifferentialProjectReviewersField
 
   private function getProjectReviewers() {
     $reviewers = array();
-    foreach ($this->getObject()->getReviewerStatus() as $reviewer) {
+    foreach ($this->getObject()->getReviewers() as $reviewer) {
       if (!$reviewer->isUser()) {
         $reviewers[] = $reviewer;
       }

src/applications/differential/customfield/DifferentialReviewersField.php

@@ -17,7 +17,7 @@ final class DifferentialReviewersField
 
   protected function readValueFromRevision(
     DifferentialRevision $revision) {
-    return $revision->getReviewerStatus();
+    return $revision->getReviewers();
   }
 
   public function shouldAppearInPropertyView() {
@@ -53,7 +53,7 @@ final class DifferentialReviewersField
 
   private function getUserReviewers() {
     $reviewers = array();
-    foreach ($this->getObject()->getReviewerStatus() as $reviewer) {
+    foreach ($this->getObject()->getReviewers() as $reviewer) {
       if ($reviewer->isUser()) {
         $reviewers[] = $reviewer;
       }

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -326,9 +326,9 @@ final class DifferentialTransactionEditor
       // actually change the diff text.
 
       $edits = array();
-      foreach ($object->getReviewerStatus() as $reviewer) {
+      foreach ($object->getReviewers() as $reviewer) {
         if ($downgrade_rejects) {
-          if ($reviewer->getStatus() == $new_reject) {
+          if ($reviewer->getReviewerStatus() == $new_reject) {
             $edits[$reviewer->getReviewerPHID()] = array(
               'data' => array(
                 'status' => $old_reject,
@@ -338,7 +338,7 @@ final class DifferentialTransactionEditor
         }
 
         if ($downgrade_accepts) {
-          if ($reviewer->getStatus() == $new_accept) {
+          if ($reviewer->getReviewerStatus() == $new_accept) {
             $edits[$reviewer->getReviewerPHID()] = array(
               'data' => array(
                 'status' => $old_accept,
@@ -415,9 +415,9 @@ final class DifferentialTransactionEditor
         );
 
         $edits = array();
-        foreach ($object->getReviewerStatus() as $reviewer) {
+        foreach ($object->getReviewers() as $reviewer) {
           if ($reviewer->getReviewerPHID() == $actor_phid) {
-            if ($reviewer->getStatus() == $status_added) {
+            if ($reviewer->getReviewerStatus() == $status_added) {
               $edits[$actor_phid] = array(
                 'data' => $data,
               );
@@ -623,7 +623,7 @@ final class DifferentialTransactionEditor
         pht('Failed to load revision from transaction finalization.'));
     }
 
-    $object->attachReviewerStatus($new_revision->getReviewerStatus());
+    $object->attachReviewers($new_revision->getReviewers());
     $object->attachActiveDiff($new_revision->getActiveDiff());
     $object->attachRepository($new_revision->getRepository());
 
@@ -645,7 +645,11 @@ final class DifferentialTransactionEditor
     $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
     $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
 
+    $is_sticky_accept = PhabricatorEnv::getEnvConfig(
+      'differential.sticky-accept');
+
     $old_status = $object->getStatus();
+    $active_diff = $object->getActiveDiff();
     switch ($old_status) {
       case $status_accepted:
       case $status_revision:
@@ -661,11 +665,17 @@ final class DifferentialTransactionEditor
         $has_rejecting_reviewer = false;
         $has_rejecting_older_reviewer = false;
         $has_blocking_reviewer = false;
-        foreach ($object->getReviewerStatus() as $reviewer) {
+        foreach ($object->getReviewers() as $reviewer) {
-          $reviewer_status = $reviewer->getStatus();
+          $reviewer_status = $reviewer->getReviewerStatus();
           switch ($reviewer_status) {
             case DifferentialReviewerStatus::STATUS_REJECTED:
+              $action_phid = $reviewer->getLastActionDiffPHID();
+              $active_phid = $active_diff->getPHID();
+              $is_current = ($action_phid == $active_phid);
+
+              if ($is_current) {
                 $has_rejecting_reviewer = true;
+              }
               break;
             case DifferentialReviewerStatus::STATUS_REJECTED_OLDER:
               $has_rejecting_older_reviewer = true;
@@ -675,8 +685,14 @@ final class DifferentialTransactionEditor
               break;
             case DifferentialReviewerStatus::STATUS_ACCEPTED:
               if ($reviewer->isUser()) {
+                $action_phid = $reviewer->getLastActionDiffPHID();
+                $active_phid = $active_diff->getPHID();
+                $is_current = ($action_phid == $active_phid);
+
+                if ($is_sticky_accept || $is_current) {
                   $has_accepting_user = true;
                 }
+              }
               break;
           }
         }
@@ -1032,7 +1048,7 @@ final class DifferentialTransactionEditor
   protected function getMailTo(PhabricatorLiskDAO $object) {
     $phids = array();
     $phids[] = $object->getAuthorPHID();
-    foreach ($object->getReviewerStatus() as $reviewer) {
+    foreach ($object->getReviewers() as $reviewer) {
       $phids[] = $reviewer->getReviewerPHID();
     }
     return $phids;
@@ -1507,7 +1523,7 @@ final class DifferentialTransactionEditor
     // and both are needlessly complex. This logic should live in the normal
     // transaction application pipeline. See T10967.
 
-    $reviewers = $object->getReviewerStatus();
+    $reviewers = $object->getReviewers();
     $reviewers = mpull($reviewers, null, 'getReviewerPHID');
 
     if ($is_blocking) {
@@ -1528,7 +1544,7 @@ final class DifferentialTransactionEditor
       // If we're applying a stronger status (usually, upgrading a reviewer
       // into a blocking reviewer), skip this check so we apply the change.
       $old_strength = DifferentialReviewerStatus::getStatusStrength(
-        $reviewers[$phid]->getStatus());
+        $reviewers[$phid]->getReviewerStatus());
       if ($old_strength <= $new_strength) {
         continue;
       }

src/applications/differential/engineextension/DifferentialHovercardEngineExtension.php

@@ -54,8 +54,7 @@ final class DifferentialHovercardEngineExtension
       pht('Author'),
       $viewer->renderHandle($revision->getAuthorPHID()));
 
-    $reviewer_phids = $revision->getReviewerStatus();
+    $reviewer_phids = $revision->getReviewerPHIDs();
-    $reviewer_phids = mpull($reviewer_phids, 'getReviewerPHID');
 
     $hovercard->addField(
       pht('Reviewers'),

src/applications/differential/field/DifferentialReviewedByCommitMessageField.php

@@ -37,10 +37,9 @@ final class DifferentialReviewedByCommitMessageField
     }
 
     $phids = array();
-    foreach ($revision->getReviewerStatus() as $reviewer) {
+    foreach ($revision->getReviewers() as $reviewer) {
-      switch ($reviewer->getStatus()) {
+      switch ($reviewer->getReviewerStatus()) {
         case DifferentialReviewerStatus::STATUS_ACCEPTED:
-        case DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER:
           $phids[] = $reviewer->getReviewerPHID();
           break;
       }

src/applications/differential/field/DifferentialReviewersCommitMessageField.php

@@ -45,8 +45,8 @@ final class DifferentialReviewersCommitMessageField
     $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING;
 
     $results = array();
-    foreach ($revision->getReviewerStatus() as $reviewer) {
+    foreach ($revision->getReviewers() as $reviewer) {
-      if ($reviewer->getStatus() == $status_blocking) {
+      if ($reviewer->getReviewerStatus() == $status_blocking) {
         $suffixes = array('!' => '!');
       } else {
         $suffixes = array();

src/applications/differential/herald/DifferentialReviewersHeraldAction.php

@@ -37,8 +37,7 @@ abstract class DifferentialReviewersHeraldAction
       }
     }
 
-    $reviewers = $object->getReviewerStatus();
+    $reviewers = $object->getReviewers();
-    $reviewers = mpull($reviewers, null, 'getReviewerPHID');
 
     if ($is_blocking) {
       $new_status = DifferentialReviewerStatus::STATUS_BLOCKING;

src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php

@@ -137,8 +137,7 @@ final class HeraldDifferentialRevisionAdapter
   }
 
   public function loadReviewers() {
-    $reviewers = $this->getObject()->getReviewerStatus();
+    return $this->getObject()->getReviewerPHIDs();
-    return mpull($reviewers, 'getReviewerPHID');
   }
 
 

src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php

@@ -13,7 +13,7 @@ final class PhabricatorDifferentialRevisionTestDataGenerator
     $author = $this->loadPhabricatorUser();
 
     $revision = DifferentialRevision::initializeNewRevision($author);
-    $revision->attachReviewerStatus(array());
+    $revision->attachReviewers(array());
     $revision->attachActiveDiff(null);
 
     // This could be a bit richer and more formal than it is.

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -974,7 +974,7 @@ final class DifferentialRevisionQuery
         $reviewers[$reviewer_phid] = $reviewer;
       }
 
-      $revision->attachReviewerStatus($reviewers);
+      $revision->attachReviewers($reviewers);
     }
   }
 
@@ -993,7 +993,6 @@ final class DifferentialRevisionQuery
     $project_type = PhabricatorProjectProjectPHIDType::TYPECONST;
     $package_type = PhabricatorOwnersPackagePHIDType::TYPECONST;
 
-    $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
     foreach ($reviewers as $revision_phid => $reviewer_list) {
       if (!$allow_self) {
         if ($revision_map[$revision_phid]->getAuthorPHID() == $viewer_phid) {

src/applications/differential/query/DifferentialRevisionResultBucket.php

@@ -56,13 +56,13 @@ abstract class DifferentialRevisionResultBucket
     array $phids,
     array $statuses) {
 
-    foreach ($revision->getReviewerStatus() as $reviewer) {
+    foreach ($revision->getReviewers() as $reviewer) {
       $reviewer_phid = $reviewer->getReviewerPHID();
       if (empty($phids[$reviewer_phid])) {
         continue;
       }
 
-      $status = $reviewer->getStatus();
+      $status = $reviewer->getReviewerStatus();
       if (empty($statuses[$status])) {
         continue;
       }

src/applications/differential/search/DifferentialRevisionFulltextEngine.php

@@ -14,7 +14,7 @@ final class DifferentialRevisionFulltextEngine
       ->executeOne();
 
     // TODO: This isn't very clean, but custom fields currently rely on it.
-    $object->attachReviewerStatus($revision->getReviewerStatus());
+    $object->attachReviewers($revision->getReviewers());
 
     $document->setDocumentTitle($revision->getTitle());
 
@@ -36,8 +36,9 @@ final class DifferentialRevisionFulltextEngine
     // owner is the author (e.g., accepted, rejected, closed).
     $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
     if ($revision->getStatus() == $status_review) {
-      $reviewers = $revision->getReviewerStatus();
+      $reviewers = $revision->getReviewerPHIDs();
-      $reviewers = mpull($reviewers, 'getReviewerPHID', 'getReviewerPHID');
+      $reviewers = array_fuse($reviewers);
+
       if ($reviewers) {
         foreach ($reviewers as $phid) {
           $document->addRelationship(

src/applications/differential/storage/DifferentialReviewer.php

@@ -27,12 +27,6 @@ final class DifferentialReviewer
     ) + parent::getConfiguration();
   }
 
-  public function getStatus() {
-    // TODO: This is an older method for compatibility with some callers
-    // which have not yet been cleaned up.
-    return $this->getReviewerStatus();
-  }
-
   public function isUser() {
     $user_type = PhabricatorPeopleUserPHIDType::TYPECONST;
     return (phid_get_type($this->getReviewerPHID()) == $user_type);

src/applications/differential/storage/DifferentialRevision.php

@@ -70,7 +70,7 @@ final class DifferentialRevision extends DifferentialDAO
       ->setAuthorPHID($actor->getPHID())
       ->attachRepository(null)
       ->attachActiveDiff(null)
-      ->attachReviewerStatus(array())
+      ->attachReviewers(array())
       ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
   }
 
@@ -332,30 +332,31 @@ final class DifferentialRevision extends DifferentialDAO
     );
   }
 
-  public function getReviewerStatus() {
+  public function getReviewers() {
     return $this->assertAttached($this->reviewerStatus);
   }
 
-  public function attachReviewerStatus(array $reviewers) {
+  public function attachReviewers(array $reviewers) {
     assert_instances_of($reviewers, 'DifferentialReviewer');
+    $reviewers = mpull($reviewers, null, 'getReviewerPHID');
     $this->reviewerStatus = $reviewers;
     return $this;
   }
 
   public function getReviewerPHIDs() {
-    $reviewers = $this->getReviewerStatus();
+    $reviewers = $this->getReviewers();
     return mpull($reviewers, 'getReviewerPHID');
   }
 
   public function getReviewerPHIDsForEdit() {
-    $reviewers = $this->getReviewerStatus();
+    $reviewers = $this->getReviewers();
 
     $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING;
 
     $value = array();
     foreach ($reviewers as $reviewer) {
       $phid = $reviewer->getReviewerPHID();
-      if ($reviewer->getStatus() == $status_blocking) {
+      if ($reviewer->getReviewerStatus() == $status_blocking) {
         $value[] = 'blocking('.$phid.')';
       } else {
         $value[] = $phid;
@@ -480,9 +481,9 @@ final class DifferentialRevision extends DifferentialDAO
         ->withPHIDs(array($this->getPHID()))
         ->needReviewers(true)
         ->executeOne()
-        ->getReviewerStatus();
+        ->getReviewers();
     } else {
-      $reviewers = $this->getReviewerStatus();
+      $reviewers = $this->getReviewers();
     }
 
     foreach ($reviewers as $reviewer) {

src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php

@@ -21,7 +21,8 @@ abstract class DifferentialRevisionReviewTransaction
       $viewer,
       array(
         DifferentialReviewerStatus::STATUS_ACCEPTED,
-      ));
+      ),
+      true);
   }
 
   protected function isViewerFullyRejected(
@@ -32,7 +33,8 @@ abstract class DifferentialRevisionReviewTransaction
       $viewer,
       array(
         DifferentialReviewerStatus::STATUS_REJECTED,
-      ));
+      ),
+      true);
   }
 
   protected function getViewerReviewerStatus(
@@ -43,12 +45,12 @@ abstract class DifferentialRevisionReviewTransaction
       return null;
     }
 
-    foreach ($revision->getReviewerStatus() as $reviewer) {
+    foreach ($revision->getReviewers() as $reviewer) {
       if ($reviewer->getReviewerPHID() != $viewer->getPHID()) {
         continue;
       }
 
-      return $reviewer->getStatus();
+      return $reviewer->getReviewerStatus();
     }
 
     return null;
@@ -57,7 +59,8 @@ abstract class DifferentialRevisionReviewTransaction
   protected function isViewerReviewerStatusFullyAmong(
     DifferentialRevision $revision,
     PhabricatorUser $viewer,
-    array $status_list) {
+    array $status_list,
+    $require_current) {
 
     // If the user themselves is not a reviewer, the reviews they have
     // authority over can not all be in any set of states since their own
@@ -67,18 +70,26 @@ abstract class DifferentialRevisionReviewTransaction
       return false;
     }
 
+    $active_phid = $this->getActiveDiffPHID($revision);
+
     // Otherwise, check that all reviews they have authority over are in
     // the desired set of states.
     $status_map = array_fuse($status_list);
-    foreach ($revision->getReviewerStatus() as $reviewer) {
+    foreach ($revision->getReviewers() as $reviewer) {
       if (!$reviewer->hasAuthority($viewer)) {
         continue;
       }
 
-      $status = $reviewer->getStatus();
+      $status = $reviewer->getReviewerStatus();
       if (!isset($status_map[$status])) {
         return false;
       }
+
+      if ($require_current) {
+        if ($reviewer->getLastActionDiffPHID() != $active_phid) {
+          return false;
+        }
+      }
     }
 
     return true;
@@ -97,7 +108,7 @@ abstract class DifferentialRevisionReviewTransaction
     // yourself.
     $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED);
     if ($with_authority) {
-      foreach ($revision->getReviewerStatus() as $reviewer) {
+      foreach ($revision->getReviewers() as $reviewer) {
         if ($reviewer->hasAuthority($viewer)) {
           $map[$reviewer->getReviewerPHID()] = $status;
         }

src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php

@@ -7,8 +7,8 @@ final class DifferentialRevisionReviewersTransaction
   const EDITKEY = 'reviewers';
 
   public function generateOldValue($object) {
-    $reviewers = $object->getReviewerStatus();
+    $reviewers = $object->getReviewers();
-    $reviewers = mpull($reviewers, 'getStatus', 'getReviewerPHID');
+    $reviewers = mpull($reviewers, 'getReviewerStatus', 'getReviewerPHID');
     return $reviewers;
   }
 

src/applications/differential/xaction/DifferentialRevisionTransactionType.php

@@ -57,4 +57,13 @@ abstract class DifferentialRevisionTransactionType
       $xaction);
   }
 
+  protected function getActiveDiffPHID(DifferentialRevision $revision) {
+    try {
+      $diff = $revision->getActiveDiff();
+      return $diff->getPHID();
+    } catch (Exception $ex) {
+      return null;
+    }
+  }
+
 }

src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php

@@ -165,7 +165,7 @@ final class PhabricatorRepositoryCommitOwnersWorker
     $accepted_statuses = array_fuse($accepted_statuses);
 
     $found_accept = false;
-    foreach ($revision->getReviewerStatus() as $reviewer) {
+    foreach ($revision->getReviewers() as $reviewer) {
       $reviewer_phid = $reviewer->getReviewerPHID();
 
       // If this reviewer isn't a package owner, just ignore them.
@@ -175,7 +175,7 @@ final class PhabricatorRepositoryCommitOwnersWorker
 
       // If this reviewer accepted the revision and owns the package, we're
       // all clear and do not need to trigger an audit.
-      if (isset($accepted_statuses[$reviewer->getStatus()])) {
+      if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) {
         $found_accept = true;
         break;
       }