recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Refine bucketing and display rules for voided "Accepts" in Differential

Summary:
See PHI190. This clarifies the ruleset a bit:

- If you accepted, then the author used "Request Review" explicitly, we now show "Accepted Earlier" instead of "Accepted" in the "Reviewers" list on the main revision page. This makes it sligthly more clear why the revision is back in your review queue without picking through the transaction log.
- Instead of moving all non-current accepts into "Ready to Review", move only voided accepts into "Ready to Review". This stops us from pulling older accepts which haven't been voided (which could have been incorrectly pulled) and correctly pulls older, voided accepts from before an update (for example: accept, then request review, then update) and generally aligns better with intent/expectation.

Test Plan:
- Accepted, requested review.
- Saw reviewer as "Accepted Earlier".
- Saw review in "Ready to Review" bucket.
- Accepted, updated (with sticky accept).
- Saw reviewer as "Accepted Prior Diff".
- Saw review as "Waiting on Authors".

Reviewers: amckinley

Reviewed By: amckinley

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

epriestley 12e6106a 587faa6f

+13 -8
+1 -1
src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
··· 138 138 139 139 $results = array(); 140 140 foreach ($objects as $key => $object) { 141 - if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, false)) { 141 + if (!$this->hasReviewersWithStatus($object, $phids, $reviewing, true)) { 142 142 continue; 143 143 } 144 144
+4 -5
src/applications/differential/query/DifferentialRevisionResultBucket.php
··· 54 54 DifferentialRevision $revision, 55 55 array $phids, 56 56 array $statuses, 57 - $current = null) { 57 + $include_voided = null) { 58 58 59 59 foreach ($revision->getReviewers() as $reviewer) { 60 60 $reviewer_phid = $reviewer->getReviewerPHID(); ··· 67 67 continue; 68 68 } 69 69 70 - if ($current !== null) { 70 + if ($include_voided !== null) { 71 71 if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) { 72 - $diff_phid = $revision->getActiveDiffPHID(); 73 - $is_current = $reviewer->isAccepted($diff_phid); 74 - if ($is_current !== $current) { 72 + $is_voided = (bool)$reviewer->getVoidedPHID(); 73 + if ($is_voided !== $include_voided) { 75 74 continue; 76 75 } 77 76 }
+8 -2
src/applications/differential/view/DifferentialReviewersView.php
··· 47 47 48 48 $action_phid = $reviewer->getLastActionDiffPHID(); 49 49 $is_current_action = $this->isCurrent($action_phid); 50 + $is_voided = (bool)$reviewer->getVoidedPHID(); 50 51 51 52 $comment_phid = $reviewer->getLastCommentDiffPHID(); 52 53 $is_current_comment = $this->isCurrent($comment_phid); ··· 86 87 break; 87 88 88 89 case DifferentialReviewerStatus::STATUS_ACCEPTED: 89 - if ($is_current_action) { 90 + if ($is_current_action && !$is_voided) { 90 91 $icon = PHUIStatusItemView::ICON_ACCEPT; 91 92 $color = 'green'; 92 93 if ($authority_name !== null) { ··· 97 98 } else { 98 99 $icon = 'fa-check-circle-o'; 99 100 $color = 'bluegrey'; 100 - if ($authority_name !== null) { 101 + 102 + if (!$is_current_action && $is_voided) { 103 + // The reviewer accepted the revision, but later the author 104 + // used "Request Review" to request an updated review. 105 + $label = pht('Accepted Earlier'); 106 + } else if ($authority_name !== null) { 101 107 $label = pht('Accepted Prior Diff (by %s)', $authority_name); 102 108 } else { 103 109 $label = pht('Accepted Prior Diff');