@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
Make updates of rejected revisions behave correctly again
Summary:
Ref T2222. Ref T4481. Specifically:
- When a revision is updated, change all "Reject" reviewers to "Reject Prior".
- Change status to "Needs Review".
- Update the state logic to account for this properly.
Test Plan:
- Created a revision as user A, with B as a reviewer.
- Rejected as B.
- Updated the revision as A.
- Saw revision in "needs review" state, with B as a "Rejected Prior" reviewer.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4481, T2222
Differential Revision: https://secure.phabricator.com/D8402
+61
-5
+7
-2
src/applications/differential/constants/DifferentialReviewerStatus.php
+7
-2
src/applications/differential/constants/DifferentialReviewerStatus.php
···
7
7
const STATUS_ACCEPTED = 'accepted';
8
8
const STATUS_REJECTED = 'rejected';
9
9
const STATUS_COMMENTED = 'commented';
10
+
const STATUS_ACCEPTED_OLDER = 'accepted-older';
11
+
const STATUS_REJECTED_OLDER = 'rejected-older';
10
12
11
13
/**
12
14
* Returns the relative strength of a status, used to pick a winner when a
···
27
29
28
30
self::STATUS_BLOCKING => 3,
29
31
30
-
self::STATUS_ACCEPTED => 4,
31
-
self::STATUS_REJECTED => 4,
32
+
self::STATUS_ACCEPTED_OLDER => 4,
33
+
self::STATUS_REJECTED_OLDER => 4,
34
+
35
+
self::STATUS_ACCEPTED => 5,
36
+
self::STATUS_REJECTED => 5,
32
37
);
33
38
34
39
return idx($map, $constant, 0);
+42
-3
src/applications/differential/editor/DifferentialTransactionEditor.php
+42
-3
src/applications/differential/editor/DifferentialTransactionEditor.php
···
137
137
PhabricatorLiskDAO $object,
138
138
PhabricatorApplicationTransaction $xaction) {
139
139
140
+
$status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
141
+
$status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
142
+
140
143
switch ($xaction->getTransactionType()) {
141
144
case PhabricatorTransactions::TYPE_VIEW_POLICY:
142
145
$object->setViewPolicy($xaction->getNewValue());
···
151
154
case PhabricatorTransactions::TYPE_EDGE:
152
155
return;
153
156
case DifferentialTransaction::TYPE_UPDATE:
157
+
$object->setStatus($status_review);
154
158
// TODO: Update the `diffPHID` once we add that.
155
159
return;
156
160
case DifferentialTransaction::TYPE_ACTION:
157
-
$status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
158
-
$status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
159
-
160
161
switch ($xaction->getNewValue()) {
161
162
case DifferentialAction::ACTION_RESIGN:
162
163
case DifferentialAction::ACTION_ACCEPT:
···
203
204
204
205
$results = parent::expandTransaction($object, $xaction);
205
206
switch ($xaction->getTransactionType()) {
207
+
case DifferentialTransaction::TYPE_UPDATE:
208
+
$new_accept = DifferentialReviewerStatus::STATUS_ACCEPTED;
209
+
$new_reject = DifferentialReviewerStatus::STATUS_REJECTED;
210
+
$old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER;
211
+
$old_reject = DifferentialReviewerStatus::STATUS_REJECTED_OLDER;
212
+
213
+
// When a revision is updated, change all "reject" to "rejected older
214
+
// revision". This means we won't immediately push the update back into
215
+
// "needs review", but outstanding rejects will still block it from
216
+
// moving to "accepted".
217
+
$edits = array();
218
+
foreach ($object->getReviewerStatus() as $reviewer) {
219
+
if ($reviewer->getStatus() == $new_reject) {
220
+
$edits[$reviewer->getReviewerPHID()] = array(
221
+
'data' => array(
222
+
'status' => $old_reject,
223
+
),
224
+
);
225
+
}
226
+
227
+
// TODO: If sticky accept is off, do a similar update for accepts.
228
+
}
229
+
230
+
if ($edits) {
231
+
$results[] = id(new DifferentialTransaction())
232
+
->setTransactionType($type_edge)
233
+
->setMetadataValue('edge:type', $edge_reviewer)
234
+
->setIgnoreOnNoEffect(true)
235
+
->setNewValue(array('+' => $edits));
236
+
}
237
+
break;
238
+
206
239
case PhabricatorTransactions::TYPE_COMMENT:
207
240
// When a user leaves a comment, upgrade their reviewer status from
208
241
// "added" to "commented" if they're also a reviewer. We may further
···
433
466
//
434
467
// - at least one accepting reviewer who is a user; and
435
468
// - no rejects; and
469
+
// - no rejects of older diffs; and
436
470
// - no blocking reviewers.
437
471
438
472
$has_accepting_user = false;
439
473
$has_rejecting_reviewer = false;
474
+
$has_rejecting_older_reviewer = false;
440
475
$has_blocking_reviewer = false;
441
476
foreach ($new_revision->getReviewerStatus() as $reviewer) {
442
477
$reviewer_status = $reviewer->getStatus();
···
444
479
case DifferentialReviewerStatus::STATUS_REJECTED:
445
480
$has_rejecting_reviewer = true;
446
481
break;
482
+
case DifferentialReviewerStatus::STATUS_REJECTED_OLDER:
483
+
$has_rejecting_older_reviewer = true;
484
+
break;
447
485
case DifferentialReviewerStatus::STATUS_BLOCKING:
448
486
$has_blocking_reviewer = true;
449
487
break;
···
458
496
$new_status = null;
459
497
if ($has_accepting_user &&
460
498
!$has_rejecting_reviewer &&
499
+
!$has_rejecting_older_reviewer &&
461
500
!$has_blocking_reviewer) {
462
501
$new_status = $status_accepted;
463
502
} else if ($has_rejecting_reviewer) {
+12
src/applications/differential/view/DifferentialReviewersView.php
+12
src/applications/differential/view/DifferentialReviewersView.php
···
58
58
}
59
59
break;
60
60
61
+
case DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER:
62
+
$item->setIcon(
63
+
'accept-dark',
64
+
pht('Accepted Prior Diff'));
65
+
break;
66
+
61
67
case DifferentialReviewerStatus::STATUS_REJECTED:
62
68
if ($is_current) {
63
69
$item->setIcon(
···
68
74
'reject-dark',
69
75
pht('Requested Changes to Prior Diff'));
70
76
}
77
+
break;
78
+
79
+
case DifferentialReviewerStatus::STATUS_REJECTED_OLDER:
80
+
$item->setIcon(
81
+
'reject-dark',
82
+
pht('Rejected Prior Diff'));
71
83
break;
72
84
73
85
case DifferentialReviewerStatus::STATUS_COMMENTED: