diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php index 5f5f4008db..711f70afb3 100644 --- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php @@ -1,298 +1,298 @@ objects = $objects; $phids = $query->getEvaluatedParameter('responsiblePHIDs'); if (!$phids) { throw new Exception( pht( 'You can not bucket results by required action without '. 'specifying "Responsible Users".')); } $phids = array_fuse($phids); // Before continuing, throw away any revisions which responsible users // have explicitly resigned from. // The goal is to allow users to resign from revisions they don't want to // review to get these revisions off their dashboard, even if there are // other project or package reviewers which they have authority over. $this->filterResigned($phids); // We also throw away draft revisions which you aren't the author of. $this->filterOtherDrafts($phids); $groups = array(); $groups[] = $this->newGroup() ->setName(pht('Must Review')) ->setKey(self::KEY_MUSTREVIEW) ->setNoDataString(pht('No revisions are blocked on your review.')) ->setObjects($this->filterMustReview($phids)); $groups[] = $this->newGroup() ->setName(pht('Ready to Review')) ->setKey(self::KEY_SHOULDREVIEW) ->setNoDataString(pht('No revisions are waiting on you to review them.')) ->setObjects($this->filterShouldReview($phids)); $groups[] = $this->newGroup() ->setName(pht('Ready to Land')) ->setNoDataString(pht('No revisions are ready to land.')) ->setObjects($this->filterShouldLand($phids)); $groups[] = $this->newGroup() ->setName(pht('Ready to Update')) ->setNoDataString(pht('No revisions are waiting for updates.')) ->setObjects($this->filterShouldUpdate($phids)); $groups[] = $this->newGroup() ->setName(pht('Drafts')) ->setNoDataString(pht('You have no draft revisions.')) ->setObjects($this->filterDrafts($phids)); $groups[] = $this->newGroup() ->setName(pht('Waiting on Review')) ->setNoDataString(pht('None of your revisions are waiting on review.')) ->setObjects($this->filterWaitingForReview($phids)); $groups[] = $this->newGroup() ->setName(pht('Waiting on Authors')) ->setNoDataString(pht('No revisions are waiting on author action.')) ->setObjects($this->filterWaitingOnAuthors($phids)); $groups[] = $this->newGroup() ->setName(pht('Waiting on Other Reviewers')) ->setNoDataString(pht('No revisions are waiting for other reviewers.')) ->setObjects($this->filterWaitingOnOtherReviewers($phids)); // Because you can apply these buckets to queries which include revisions // that have been closed, add an "Other" bucket if we still have stuff // that didn't get filtered into any of the previous buckets. if ($this->objects) { $groups[] = $this->newGroup() ->setName(pht('Other Revisions')) ->setObjects($this->objects); } return $groups; } private function filterMustReview(array $phids) { $blocking = array( DifferentialReviewerStatus::STATUS_BLOCKING, DifferentialReviewerStatus::STATUS_REJECTED, DifferentialReviewerStatus::STATUS_REJECTED_OLDER, ); $blocking = array_fuse($blocking); $objects = $this->getRevisionsUnderReview($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$this->hasReviewersWithStatus($object, $phids, $blocking)) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterShouldReview(array $phids) { $reviewing = array( DifferentialReviewerStatus::STATUS_ADDED, DifferentialReviewerStatus::STATUS_COMMENTED, // If an author has used "Request Review" to put an accepted revision // back into the "Needs Review" state, include "Accepted" reviewers // whose reviews have been voided in the "Should Review" bucket. // If we don't do this, they end up in "Waiting on Other Reviewers", // even if there are no other reviewers. DifferentialReviewerStatus::STATUS_ACCEPTED, ); $reviewing = array_fuse($reviewing); $objects = $this->getRevisionsUnderReview($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { - if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, false)) { + if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, true)) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterShouldLand(array $phids) { $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$object->isAccepted()) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterShouldUpdate(array $phids) { $statuses = array( DifferentialRevisionStatus::NEEDS_REVISION, DifferentialRevisionStatus::CHANGES_PLANNED, ); $statuses = array_fuse($statuses); $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (empty($statuses[$object->getModernRevisionStatus()])) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterWaitingForReview(array $phids) { $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$object->isNeedsReview()) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterWaitingOnAuthors(array $phids) { $statuses = array( DifferentialRevisionStatus::ACCEPTED, DifferentialRevisionStatus::NEEDS_REVISION, DifferentialRevisionStatus::CHANGES_PLANNED, ); $statuses = array_fuse($statuses); $objects = $this->getRevisionsNotAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (empty($statuses[$object->getModernRevisionStatus()])) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterWaitingOnOtherReviewers(array $phids) { $objects = $this->getRevisionsNotAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$object->isNeedsReview()) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterResigned(array $phids) { $resigned = array( DifferentialReviewerStatus::STATUS_RESIGNED, ); $resigned = array_fuse($resigned); $objects = $this->getRevisionsNotAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$this->hasReviewersWithStatus($object, $phids, $resigned)) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterOtherDrafts(array $phids) { $objects = $this->getRevisionsNotAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$object->isDraft()) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterDrafts(array $phids) { $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$object->isDraft()) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } } diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php index a0769ac349..c5cc5c0e6c 100644 --- a/src/applications/differential/query/DifferentialRevisionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php @@ -1,87 +1,86 @@ setAncestorClass(__CLASS__) ->setUniqueMethod('getResultBucketKey') ->execute(); } protected function getRevisionsUnderReview(array $objects, array $phids) { $results = array(); $objects = $this->getRevisionsNotAuthored($objects, $phids); foreach ($objects as $key => $object) { if (!$object->isNeedsReview()) { continue; } $results[$key] = $object; } return $results; } protected function getRevisionsAuthored(array $objects, array $phids) { $results = array(); foreach ($objects as $key => $object) { if (isset($phids[$object->getAuthorPHID()])) { $results[$key] = $object; } } return $results; } protected function getRevisionsNotAuthored(array $objects, array $phids) { $results = array(); foreach ($objects as $key => $object) { if (empty($phids[$object->getAuthorPHID()])) { $results[$key] = $object; } } return $results; } protected function hasReviewersWithStatus( DifferentialRevision $revision, array $phids, array $statuses, - $current = null) { + $include_voided = null) { foreach ($revision->getReviewers() as $reviewer) { $reviewer_phid = $reviewer->getReviewerPHID(); if (empty($phids[$reviewer_phid])) { continue; } $status = $reviewer->getReviewerStatus(); if (empty($statuses[$status])) { continue; } - if ($current !== null) { + if ($include_voided !== null) { if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) { - $diff_phid = $revision->getActiveDiffPHID(); - $is_current = $reviewer->isAccepted($diff_phid); - if ($is_current !== $current) { + $is_voided = (bool)$reviewer->getVoidedPHID(); + if ($is_voided !== $include_voided) { continue; } } } return true; } return false; } } diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index 33aad25289..f88669e539 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -1,186 +1,192 @@ reviewers = $reviewers; return $this; } public function setHandles(array $handles) { assert_instances_of($handles, 'PhabricatorObjectHandle'); $this->handles = $handles; return $this; } public function setActiveDiff(DifferentialDiff $diff) { $this->diff = $diff; return $this; } public function render() { $viewer = $this->getUser(); $reviewers = $this->reviewers; $view = new PHUIStatusListView(); // Move resigned reviewers to the bottom. $head = array(); $tail = array(); foreach ($reviewers as $key => $reviewer) { if ($reviewer->isResigned()) { $tail[$key] = $reviewer; } else { $head[$key] = $reviewer; } } $reviewers = $head + $tail; foreach ($reviewers as $reviewer) { $phid = $reviewer->getReviewerPHID(); $handle = $this->handles[$phid]; $action_phid = $reviewer->getLastActionDiffPHID(); $is_current_action = $this->isCurrent($action_phid); + $is_voided = (bool)$reviewer->getVoidedPHID(); $comment_phid = $reviewer->getLastCommentDiffPHID(); $is_current_comment = $this->isCurrent($comment_phid); $item = new PHUIStatusItemView(); $item->setHighlighted($reviewer->hasAuthority($viewer)); // If someone other than the reviewer acted on the reviewer's behalf, // show who is responsible for the current state. This is usually a // user accepting for a package or project. $authority_phid = $reviewer->getLastActorPHID(); if ($authority_phid && ($authority_phid !== $phid)) { $authority_name = $viewer->renderHandle($authority_phid) ->setAsText(true); } else { $authority_name = null; } switch ($reviewer->getReviewerStatus()) { case DifferentialReviewerStatus::STATUS_ADDED: if ($comment_phid) { if ($is_current_comment) { $icon = 'fa-comment'; $color = 'blue'; $label = pht('Commented'); } else { $icon = 'fa-comment-o'; $color = 'bluegrey'; $label = pht('Commented Previously'); } } else { $icon = PHUIStatusItemView::ICON_OPEN; $color = 'bluegrey'; $label = pht('Review Requested'); } break; case DifferentialReviewerStatus::STATUS_ACCEPTED: - if ($is_current_action) { + if ($is_current_action && !$is_voided) { $icon = PHUIStatusItemView::ICON_ACCEPT; $color = 'green'; if ($authority_name !== null) { $label = pht('Accepted (by %s)', $authority_name); } else { $label = pht('Accepted'); } } else { $icon = 'fa-check-circle-o'; $color = 'bluegrey'; - if ($authority_name !== null) { + + if (!$is_current_action && $is_voided) { + // The reviewer accepted the revision, but later the author + // used "Request Review" to request an updated review. + $label = pht('Accepted Earlier'); + } else if ($authority_name !== null) { $label = pht('Accepted Prior Diff (by %s)', $authority_name); } else { $label = pht('Accepted Prior Diff'); } } break; case DifferentialReviewerStatus::STATUS_REJECTED: if ($is_current_action) { $icon = PHUIStatusItemView::ICON_REJECT; $color = 'red'; if ($authority_name !== null) { $label = pht('Requested Changes (by %s)', $authority_name); } else { $label = pht('Requested Changes'); } } else { $icon = 'fa-times-circle-o'; $color = 'red'; if ($authority_name !== null) { $label = pht( 'Requested Changes to Prior Diff (by %s)', $authority_name); } else { $label = pht('Requested Changes to Prior Diff'); } } break; case DifferentialReviewerStatus::STATUS_BLOCKING: $icon = PHUIStatusItemView::ICON_MINUS; $color = 'red'; $label = pht('Blocking Review'); break; case DifferentialReviewerStatus::STATUS_RESIGNED: $icon = 'fa-times'; $color = 'grey'; $label = pht('Resigned'); break; default: $icon = PHUIStatusItemView::ICON_QUESTION; $color = 'bluegrey'; $label = pht('Unknown ("%s")', $reviewer->getReviewerStatus()); break; } $item->setIcon($icon, $color, $label); $item->setTarget($handle->renderHovercardLink()); if ($reviewer->isPackage()) { if (!$reviewer->getChangesets()) { $item->setNote(pht('(Owns No Changed Paths)')); } } $view->addItem($item); } return $view; } private function isCurrent($action_phid) { if (!$this->diff) { return true; } if (!$action_phid) { return true; } $diff_phid = $this->diff->getPHID(); if (!$diff_phid) { return true; } if ($diff_phid == $action_phid) { return true; } return false; } }