diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php index d37754227e..6d83d97a47 100644 --- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php @@ -1,232 +1,261 @@ 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); + $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('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, ); $reviewing = array_fuse($reviewing); $objects = $this->getRevisionsUnderReview($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!$this->hasReviewersWithStatus($object, $phids, $reviewing)) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterShouldLand(array $phids) { $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if ($object->getStatus() != $status_accepted) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterShouldUpdate(array $phids) { $statuses = array( ArcanistDifferentialRevisionStatus::NEEDS_REVISION, ArcanistDifferentialRevisionStatus::CHANGES_PLANNED, ArcanistDifferentialRevisionStatus::IN_PREPARATION, ); $statuses = array_fuse($statuses); $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (empty($statuses[$object->getStatus()])) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterWaitingForReview(array $phids) { $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if ($object->getStatus() != $status_review) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterWaitingOnAuthors(array $phids) { $statuses = array( ArcanistDifferentialRevisionStatus::ACCEPTED, ArcanistDifferentialRevisionStatus::NEEDS_REVISION, ArcanistDifferentialRevisionStatus::CHANGES_PLANNED, ArcanistDifferentialRevisionStatus::IN_PREPARATION, ); $statuses = array_fuse($statuses); $objects = $this->getRevisionsNotAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (empty($statuses[$object->getStatus()])) { continue; } $results[$key] = $object; unset($this->objects[$key]); } return $results; } private function filterWaitingOnOtherReviewers(array $phids) { $statuses = array( ArcanistDifferentialRevisionStatus::NEEDS_REVIEW, ); $statuses = array_fuse($statuses); $objects = $this->getRevisionsNotAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { if (!isset($statuses[$object->getStatus()])) { 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; + } + } diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 08c9707225..836022c882 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -1,45 +1,50 @@ array( 'reviewerStatus' => 'text64', 'lastActionDiffPHID' => 'phid?', 'lastCommentDiffPHID' => 'phid?', ), self::CONFIG_KEY_SCHEMA => array( 'key_revision' => array( 'columns' => array('revisionPHID', 'reviewerPHID'), 'unique' => true, ), ), ) + parent::getConfiguration(); } public function isUser() { $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; return (phid_get_type($this->getReviewerPHID()) == $user_type); } public function attachAuthority(PhabricatorUser $user, $has_authority) { $this->authority[$user->getCacheFragment()] = $has_authority; return $this; } public function hasAuthority(PhabricatorUser $viewer) { $cache_fragment = $viewer->getCacheFragment(); return $this->assertAttachedKey($this->authority, $cache_fragment); } + public function isResigned() { + $status_resigned = DifferentialReviewerStatus::STATUS_RESIGNED; + return ($this->getReviewerStatus() == $status_resigned); + } + } diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index 54b7e35257..291f859d08 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -1,139 +1,160 @@ 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(); - foreach ($this->reviewers as $reviewer) { + + // 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); $comment_phid = $reviewer->getLastCommentDiffPHID(); $is_current_comment = $this->isCurrent($comment_phid); $item = new PHUIStatusItemView(); $item->setHighlighted($reviewer->hasAuthority($viewer)); switch ($reviewer->getReviewerStatus()) { case DifferentialReviewerStatus::STATUS_ADDED: if ($comment_phid) { if ($is_current_comment) { $item->setIcon( 'fa-comment', 'blue', pht('Commented')); } else { $item->setIcon( 'fa-comment-o', 'bluegrey', pht('Commented Previously')); } } else { $item->setIcon( PHUIStatusItemView::ICON_OPEN, 'bluegrey', pht('Review Requested')); } break; case DifferentialReviewerStatus::STATUS_ACCEPTED: if ($is_current_action) { $item->setIcon( PHUIStatusItemView::ICON_ACCEPT, 'green', pht('Accepted')); } else { $item->setIcon( 'fa-check-circle-o', 'bluegrey', pht('Accepted Prior Diff')); } break; case DifferentialReviewerStatus::STATUS_REJECTED: if ($is_current_action) { $item->setIcon( PHUIStatusItemView::ICON_REJECT, 'red', pht('Requested Changes')); } else { $item->setIcon( 'fa-times-circle-o', 'bluegrey', pht('Requested Changes to Prior Diff')); } break; case DifferentialReviewerStatus::STATUS_BLOCKING: $item->setIcon( PHUIStatusItemView::ICON_MINUS, 'red', pht('Blocking Review')); break; + case DifferentialReviewerStatus::STATUS_RESIGNED: + $item->setIcon( + 'fa-times', + 'grey', + pht('Resigned')); + break; + default: $item->setIcon( PHUIStatusItemView::ICON_QUESTION, 'bluegrey', pht('%s?', $reviewer->getReviewerStatus())); break; } $item->setTarget($handle->renderHovercardLink()); $view->addItem($item); } return $view; } private function isCurrent($action_phid) { if (!$this->diff) { echo "A\n"; 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; } } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index c393ee51c5..42f644e8d1 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -1,197 +1,191 @@ getViewerReviewerStatus($revision, $viewer) !== null); } protected function isViewerFullyAccepted( DifferentialRevision $revision, PhabricatorUser $viewer) { return $this->isViewerReviewerStatusFullyAmong( $revision, $viewer, array( DifferentialReviewerStatus::STATUS_ACCEPTED, ), true); } protected function isViewerFullyRejected( DifferentialRevision $revision, PhabricatorUser $viewer) { return $this->isViewerReviewerStatusFullyAmong( $revision, $viewer, array( DifferentialReviewerStatus::STATUS_REJECTED, ), true); } protected function getViewerReviewerStatus( DifferentialRevision $revision, PhabricatorUser $viewer) { if (!$viewer->getPHID()) { return null; } foreach ($revision->getReviewers() as $reviewer) { if ($reviewer->getReviewerPHID() != $viewer->getPHID()) { continue; } return $reviewer->getReviewerStatus(); } return null; } protected function isViewerReviewerStatusFullyAmong( DifferentialRevision $revision, PhabricatorUser $viewer, 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 // personal review has no state. $status = $this->getViewerReviewerStatus($revision, $viewer); if ($status === null) { 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->getReviewers() as $reviewer) { if (!$reviewer->hasAuthority($viewer)) { continue; } $status = $reviewer->getReviewerStatus(); if (!isset($status_map[$status])) { return false; } if ($require_current) { if ($reviewer->getLastActionDiffPHID() != $active_phid) { return false; } } } return true; } protected function applyReviewerEffect( DifferentialRevision $revision, PhabricatorUser $viewer, $value, $status) { $map = array(); // When you accept or reject, you may accept or reject on behalf of all // reviewers you have authority for. When you resign, you only affect // yourself. $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED); if ($with_authority) { foreach ($revision->getReviewers() as $reviewer) { if ($reviewer->hasAuthority($viewer)) { $map[$reviewer->getReviewerPHID()] = $status; } } } // In all cases, you affect yourself. $map[$viewer->getPHID()] = $status; // Convert reviewer statuses into edge data. foreach ($map as $reviewer_phid => $reviewer_status) { $map[$reviewer_phid] = array( 'data' => array( 'status' => $reviewer_status, ), ); } // This is currently double-writing: to the old (edge) store and the new // (reviewer) store. Do the old edge write first. $src_phid = $revision->getPHID(); $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; $editor = new PhabricatorEdgeEditor(); foreach ($map as $dst_phid => $edge_data) { if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { // TODO: For now, we just remove these reviewers. In the future, we will // store resignations explicitly. $editor->removeEdge($src_phid, $edge_type, $dst_phid); } else { $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data); } } $editor->save(); // Now, do the new write. if ($map) { $diff = $this->getEditor()->getActiveDiff($revision); if ($diff) { $diff_phid = $diff->getPHID(); } else { $diff_phid = null; } $table = new DifferentialReviewer(); $reviewers = $table->loadAllWhere( 'revisionPHID = %s AND reviewerPHID IN (%Ls)', $src_phid, array_keys($map)); $reviewers = mpull($reviewers, null, 'getReviewerPHID'); foreach ($map as $dst_phid => $edge_data) { $reviewer = idx($reviewers, $dst_phid); if (!$reviewer) { $reviewer = id(new DifferentialReviewer()) ->setRevisionPHID($src_phid) ->setReviewerPHID($dst_phid); } $reviewer->setReviewerStatus($status); if ($diff_phid) { $reviewer->setLastActionDiffPHID($diff_phid); } - if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { - if ($reviewer->getID()) { - $reviewer->delete(); - } - } else { - try { - $reviewer->save(); - } catch (AphrontDuplicateKeyQueryException $ex) { - // At least for now, just ignore it if we lost a race. - } + try { + $reviewer->save(); + } catch (AphrontDuplicateKeyQueryException $ex) { + // At least for now, just ignore it if we lost a race. } } } } }