diff --git a/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php --- a/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php @@ -53,7 +53,7 @@ } $revision = DifferentialRevision::initializeNewRevision($viewer); - $revision->attachReviewerStatus(array()); + $revision->attachReviewers(array()); $result = $this->applyFieldEdit( $request, diff --git a/src/applications/differential/customfield/DifferentialProjectReviewersField.php b/src/applications/differential/customfield/DifferentialProjectReviewersField.php --- a/src/applications/differential/customfield/DifferentialProjectReviewersField.php +++ b/src/applications/differential/customfield/DifferentialProjectReviewersField.php @@ -52,7 +52,7 @@ private function getProjectReviewers() { $reviewers = array(); - foreach ($this->getObject()->getReviewerStatus() as $reviewer) { + foreach ($this->getObject()->getReviewers() as $reviewer) { if (!$reviewer->isUser()) { $reviewers[] = $reviewer; } diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -17,7 +17,7 @@ protected function readValueFromRevision( DifferentialRevision $revision) { - return $revision->getReviewerStatus(); + return $revision->getReviewers(); } public function shouldAppearInPropertyView() { @@ -53,7 +53,7 @@ private function getUserReviewers() { $reviewers = array(); - foreach ($this->getObject()->getReviewerStatus() as $reviewer) { + foreach ($this->getObject()->getReviewers() as $reviewer) { if ($reviewer->isUser()) { $reviewers[] = $reviewer; } 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 @@ -326,9 +326,9 @@ // 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 @@ } 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 @@ ); $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 @@ 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 @@ $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 @@ $has_rejecting_reviewer = false; $has_rejecting_older_reviewer = false; $has_blocking_reviewer = false; - foreach ($object->getReviewerStatus() as $reviewer) { - $reviewer_status = $reviewer->getStatus(); + foreach ($object->getReviewers() as $reviewer) { + $reviewer_status = $reviewer->getReviewerStatus(); switch ($reviewer_status) { case DifferentialReviewerStatus::STATUS_REJECTED: - $has_rejecting_reviewer = true; + $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,7 +685,13 @@ break; case DifferentialReviewerStatus::STATUS_ACCEPTED: if ($reviewer->isUser()) { - $has_accepting_user = true; + $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 @@ 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 @@ // 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 @@ // 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; } diff --git a/src/applications/differential/engineextension/DifferentialHovercardEngineExtension.php b/src/applications/differential/engineextension/DifferentialHovercardEngineExtension.php --- a/src/applications/differential/engineextension/DifferentialHovercardEngineExtension.php +++ b/src/applications/differential/engineextension/DifferentialHovercardEngineExtension.php @@ -54,8 +54,7 @@ pht('Author'), $viewer->renderHandle($revision->getAuthorPHID())); - $reviewer_phids = $revision->getReviewerStatus(); - $reviewer_phids = mpull($reviewer_phids, 'getReviewerPHID'); + $reviewer_phids = $revision->getReviewerPHIDs(); $hovercard->addField( pht('Reviewers'), diff --git a/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php b/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php --- a/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php +++ b/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php @@ -37,10 +37,9 @@ } $phids = array(); - foreach ($revision->getReviewerStatus() as $reviewer) { - switch ($reviewer->getStatus()) { + foreach ($revision->getReviewers() as $reviewer) { + switch ($reviewer->getReviewerStatus()) { case DifferentialReviewerStatus::STATUS_ACCEPTED: - case DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER: $phids[] = $reviewer->getReviewerPHID(); break; } diff --git a/src/applications/differential/field/DifferentialReviewersCommitMessageField.php b/src/applications/differential/field/DifferentialReviewersCommitMessageField.php --- a/src/applications/differential/field/DifferentialReviewersCommitMessageField.php +++ b/src/applications/differential/field/DifferentialReviewersCommitMessageField.php @@ -45,8 +45,8 @@ $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; $results = array(); - foreach ($revision->getReviewerStatus() as $reviewer) { - if ($reviewer->getStatus() == $status_blocking) { + foreach ($revision->getReviewers() as $reviewer) { + if ($reviewer->getReviewerStatus() == $status_blocking) { $suffixes = array('!' => '!'); } else { $suffixes = array(); diff --git a/src/applications/differential/herald/DifferentialReviewersHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersHeraldAction.php --- a/src/applications/differential/herald/DifferentialReviewersHeraldAction.php +++ b/src/applications/differential/herald/DifferentialReviewersHeraldAction.php @@ -37,8 +37,7 @@ } } - $reviewers = $object->getReviewerStatus(); - $reviewers = mpull($reviewers, null, 'getReviewerPHID'); + $reviewers = $object->getReviewers(); if ($is_blocking) { $new_status = DifferentialReviewerStatus::STATUS_BLOCKING; diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php @@ -137,8 +137,7 @@ } public function loadReviewers() { - $reviewers = $this->getObject()->getReviewerStatus(); - return mpull($reviewers, 'getReviewerPHID'); + return $this->getObject()->getReviewerPHIDs(); } diff --git a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php --- a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php +++ b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php @@ -13,7 +13,7 @@ $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. diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -974,7 +974,7 @@ $reviewers[$reviewer_phid] = $reviewer; } - $revision->attachReviewerStatus($reviewers); + $revision->attachReviewers($reviewers); } } @@ -993,7 +993,6 @@ $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) { diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php --- a/src/applications/differential/query/DifferentialRevisionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php @@ -56,13 +56,13 @@ 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; } diff --git a/src/applications/differential/search/DifferentialRevisionFulltextEngine.php b/src/applications/differential/search/DifferentialRevisionFulltextEngine.php --- a/src/applications/differential/search/DifferentialRevisionFulltextEngine.php +++ b/src/applications/differential/search/DifferentialRevisionFulltextEngine.php @@ -14,7 +14,7 @@ ->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 @@ // owner is the author (e.g., accepted, rejected, closed). $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; if ($revision->getStatus() == $status_review) { - $reviewers = $revision->getReviewerStatus(); - $reviewers = mpull($reviewers, 'getReviewerPHID', 'getReviewerPHID'); + $reviewers = $revision->getReviewerPHIDs(); + $reviewers = array_fuse($reviewers); + if ($reviewers) { foreach ($reviewers as $phid) { $document->addRelationship( 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 @@ -27,12 +27,6 @@ ) + 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); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -70,7 +70,7 @@ ->setAuthorPHID($actor->getPHID()) ->attachRepository(null) ->attachActiveDiff(null) - ->attachReviewerStatus(array()) + ->attachReviewers(array()) ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW); } @@ -332,30 +332,31 @@ ); } - 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 @@ ->withPHIDs(array($this->getPHID())) ->needReviewers(true) ->executeOne() - ->getReviewerStatus(); + ->getReviewers(); } else { - $reviewers = $this->getReviewerStatus(); + $reviewers = $this->getReviewers(); } foreach ($reviewers as $reviewer) { 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 @@ -21,7 +21,8 @@ $viewer, array( DifferentialReviewerStatus::STATUS_ACCEPTED, - )); + ), + true); } protected function isViewerFullyRejected( @@ -32,7 +33,8 @@ $viewer, array( DifferentialReviewerStatus::STATUS_REJECTED, - )); + ), + true); } protected function getViewerReviewerStatus( @@ -43,12 +45,12 @@ 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 @@ 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 @@ 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 @@ // 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; } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php @@ -7,8 +7,8 @@ const EDITKEY = 'reviewers'; public function generateOldValue($object) { - $reviewers = $object->getReviewerStatus(); - $reviewers = mpull($reviewers, 'getStatus', 'getReviewerPHID'); + $reviewers = $object->getReviewers(); + $reviewers = mpull($reviewers, 'getReviewerStatus', 'getReviewerPHID'); return $reviewers; } diff --git a/src/applications/differential/xaction/DifferentialRevisionTransactionType.php b/src/applications/differential/xaction/DifferentialRevisionTransactionType.php --- a/src/applications/differential/xaction/DifferentialRevisionTransactionType.php +++ b/src/applications/differential/xaction/DifferentialRevisionTransactionType.php @@ -57,4 +57,13 @@ $xaction); } + protected function getActiveDiffPHID(DifferentialRevision $revision) { + try { + $diff = $revision->getActiveDiff(); + return $diff->getPHID(); + } catch (Exception $ex) { + return null; + } + } + } diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -165,7 +165,7 @@ $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 @@ // 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; }