@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
Implement Accept/Reject in ApplicationTransactions, approximately
Summary: Ref T2222. This mostly makes Accept/Reject work. The big missing piece is that overall revision status does not yet update properly. I need to think about how I want that to work a little bit more.
Test Plan: Accepted and rejected some stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8333
+138
-20
+1
src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
+1
src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
+126
-19
src/applications/differential/editor/DifferentialTransactionEditor.php
+126
-19
src/applications/differential/editor/DifferentialTransactionEditor.php
···
69
69
$status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
70
70
$status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
71
71
72
-
switch ($xaction->getNewValue()) {
72
+
$action_type = $xaction->getNewValue();
73
+
switch ($action_type) {
74
+
case DifferentialAction::ACTION_ACCEPT:
75
+
case DifferentialAction::ACTION_REJECT:
76
+
if ($action_type == DifferentialAction::ACTION_ACCEPT) {
77
+
$new_status = DifferentialReviewerStatus::STATUS_ACCEPTED;
78
+
} else {
79
+
$new_status = DifferentialReviewerStatus::STATUS_REJECTED;
80
+
}
81
+
82
+
$actor = $this->getActor();
83
+
$actor_phid = $actor->getPHID();
84
+
85
+
// These transactions can cause effects in to ways: by altering the
86
+
// status of an existing reviewer; or by adding the actor as a new
87
+
// reviewer.
88
+
89
+
$will_add_reviewer = true;
90
+
foreach ($object->getReviewerStatus() as $reviewer) {
91
+
if ($reviewer->hasAuthority($actor)) {
92
+
if ($reviewer->getStatus() != $new_status) {
93
+
return true;
94
+
}
95
+
}
96
+
if ($reviewer->getReviewerPHID() == $actor_phid) {
97
+
$will_add_reviwer = false;
98
+
}
99
+
}
100
+
101
+
return $will_add_reviewer;
73
102
case DifferentialAction::ACTION_CLOSE:
74
103
return ($object->getStatus() != $status_closed);
75
104
case DifferentialAction::ACTION_ABANDON:
···
116
145
case DifferentialTransaction::TYPE_INLINE:
117
146
return;
118
147
case PhabricatorTransactions::TYPE_EDGE:
119
-
// TODO: When removing reviewers, we may be able to move the revision
120
-
// to "Accepted".
148
+
// TODO: Update review status.
121
149
return;
122
150
case DifferentialTransaction::TYPE_ACTION:
123
151
$status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
···
125
153
126
154
switch ($xaction->getNewValue()) {
127
155
case DifferentialAction::ACTION_RESIGN:
128
-
// TODO: Update review status?
129
-
break;
156
+
case DifferentialAction::ACTION_ACCEPT:
157
+
case DifferentialAction::ACTION_REJECT:
158
+
// These have no direct effects, and affect review status only
159
+
// indirectly by altering reviewers with TYPE_EDGE transactions.
160
+
return;
130
161
case DifferentialAction::ACTION_ABANDON:
131
162
$object->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
132
-
break;
163
+
return;
133
164
case DifferentialAction::ACTION_RETHINK:
134
165
$object->setStatus($status_revision);
135
-
break;
166
+
return;
136
167
case DifferentialAction::ACTION_RECLAIM:
137
168
$object->setStatus($status_review);
138
169
// TODO: Update review status?
139
-
break;
170
+
return;
140
171
case DifferentialAction::ACTION_REOPEN:
141
172
$object->setStatus($status_review);
142
173
// TODO: Update review status?
143
-
break;
174
+
return;
144
175
case DifferentialAction::ACTION_REQUEST:
145
176
$object->setStatus($status_review);
146
177
// TODO: Update review status?
147
-
break;
178
+
return;
148
179
case DifferentialAction::ACTION_CLOSE:
149
180
$object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
150
-
break;
181
+
return;
151
182
case DifferentialAction::ACTION_CLAIM:
152
183
$object->setAuthorPHID($this->getActor()->getPHID());
153
-
break;
154
-
default:
155
-
// TODO: For now, we're just shipping the rest of these through
156
-
// without acting on them.
157
-
break;
184
+
return;
158
185
}
159
-
return null;
186
+
break;
160
187
}
161
188
162
189
return parent::applyCustomInternalTransaction($object, $xaction);
···
168
195
169
196
$results = parent::expandTransaction($object, $xaction);
170
197
switch ($xaction->getTransactionType()) {
198
+
// TODO: If the user comments and is a reviewer, we should upgrade their
199
+
// edge metadata to STATUS_COMMENTED.
200
+
171
201
case DifferentialTransaction::TYPE_ACTION:
172
202
173
-
$actor_phid = $this->getActor()->getPHID();
203
+
$actor = $this->getActor();
204
+
$actor_phid = $actor->getPHID();
174
205
$type_edge = PhabricatorTransactions::TYPE_EDGE;
175
206
$edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
207
+
$action_type = $xaction->getNewValue();
176
208
177
-
switch ($xaction->getNewValue()) {
209
+
switch ($action_type) {
210
+
case DifferentialAction::ACTION_ACCEPT:
211
+
case DifferentialAction::ACTION_REJECT:
212
+
if ($action_type == DifferentialAction::ACTION_ACCEPT) {
213
+
$data = array(
214
+
'status' => DifferentialReviewerStatus::STATUS_ACCEPTED,
215
+
);
216
+
} else {
217
+
$data = array(
218
+
'status' => DifferentialReviewerStatus::STATUS_REJECTED,
219
+
);
220
+
}
221
+
222
+
$edits = array();
223
+
224
+
foreach ($object->getReviewerStatus() as $reviewer) {
225
+
if ($reviewer->hasAuthority($actor)) {
226
+
$edits[$reviewer->getReviewerPHID()] = array(
227
+
'data' => $data,
228
+
);
229
+
}
230
+
}
231
+
232
+
// Also either update or add the actor themselves as a reviewer.
233
+
$edits[$actor_phid] = array(
234
+
'data' => $data,
235
+
);
236
+
237
+
$results[] = id(new DifferentialTransaction())
238
+
->setTransactionType($type_edge)
239
+
->setMetadataValue('edge:type', $edge_reviewer)
240
+
->setIgnoreOnNoEffect(true)
241
+
->setNewValue(array('+' => $edits));
242
+
break;
243
+
178
244
case DifferentialAction::ACTION_CLAIM:
179
245
// If the user is commandeering, add the previous owner as a
180
246
// reviewer and remove the actor.
···
294
360
$config_reopen_key = 'differential.allow-reopen';
295
361
$allow_reopen = PhabricatorEnv::getEnvConfig($config_reopen_key);
296
362
363
+
$config_self_accept_key = 'differential.allow-self-accept';
364
+
$allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key);
365
+
297
366
$revision_status = $revision->getStatus();
298
367
299
368
$status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
···
301
370
$status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
302
371
303
372
switch ($action) {
373
+
case DifferentialAction::ACTION_ACCEPT:
374
+
if ($actor_is_author && !$allow_self_accept) {
375
+
return pht(
376
+
'You can not accept this revision because you are the owner.');
377
+
}
378
+
379
+
if ($revision_status == $status_abandoned) {
380
+
return pht(
381
+
'You can not accept this revision because it has been '.
382
+
'abandoned.');
383
+
}
384
+
385
+
if ($revision_status == $status_closed) {
386
+
return pht(
387
+
'You can not accept this revision because it has already been '.
388
+
'closed.');
389
+
}
390
+
break;
391
+
392
+
case DifferentialAction::ACTION_REJECT:
393
+
if ($actor_is_author) {
394
+
return pht(
395
+
'You can not request changes to your own revision.');
396
+
}
397
+
398
+
if ($revision_status == $status_abandoned) {
399
+
return pht(
400
+
'You can not request changes to this revision because it has been '.
401
+
'abandoned.');
402
+
}
403
+
404
+
if ($revision_status == $status_closed) {
405
+
return pht(
406
+
'You can not request changes to this revision because it has '.
407
+
'already been closed.');
408
+
}
409
+
break;
410
+
304
411
case DifferentialAction::ACTION_RESIGN:
305
412
// You can always resign from a revision if you're a reviewer. If you
306
413
// aren't, this is a no-op rather than invalid.
+6
src/applications/differential/storage/DifferentialTransaction.php
+6
src/applications/differential/storage/DifferentialTransaction.php
···
151
151
return pht(
152
152
'You can not commandeer this revision because you already own '.
153
153
'it.');
154
+
case DifferentialAction::ACTION_ACCEPT:
155
+
return pht(
156
+
'You have already accepted this revision.');
157
+
case DifferentialAction::ACTION_REJECT:
158
+
return pht(
159
+
'You have already requested changes to this revision.');
154
160
}
155
161
break;
156
162
}
+5
-1
src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+5
-1
src/applications/transactions/storage/PhabricatorApplicationTransaction.php
···
438
438
$this->renderHandleLink($author_phid),
439
439
count($add),
440
440
$this->renderHandleList($add));
441
-
} else {
441
+
} else if ($rem) {
442
442
$string = PhabricatorEdgeConfig::getRemoveStringForEdgeType($type);
443
443
return pht(
444
444
$string,
445
445
$this->renderHandleLink($author_phid),
446
446
count($rem),
447
447
$this->renderHandleList($rem));
448
+
} else {
449
+
return pht(
450
+
'%s edited edge metadata.',
451
+
$this->renderHandleLink($author_phid));
448
452
}
449
453
450
454
case PhabricatorTransactions::TYPE_CUSTOMFIELD: