Page MenuHomePhabricator
Files F15278973

D18019.id.diff
No OneTemporary
Actions

D18019.id.diff
View Options

diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
--- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
@@ -50,7 +50,8 @@
protected function getActionOptions(
PhabricatorUser $viewer,
- DifferentialRevision $revision) {
+ DifferentialRevision $revision,
+ $include_accepted = false) {
$reviewers = $revision->getReviewers();
@@ -98,10 +99,13 @@
}
}
- if ($reviewer->isAccepted($diff_phid)) {
- // If a reviewer is already in a full "accepted" state, don't
- // include that reviewer as an option.
- continue;
+ if (!$include_accepted) {
+ if ($reviewer->isAccepted($diff_phid)) {
+ // If a reviewer is already in a full "accepted" state, don't
+ // include that reviewer as an option unless we're listing all
+ // reviwers, including reviewers who have already accepted.
+ continue;
+ }
}
$reviewer_phids[$reviewer_phid] = $reviewer_phid;
@@ -185,7 +189,12 @@
'least one reviewer.'));
}
- list($options) = $this->getActionOptions($actor, $object);
+ // NOTE: We're including reviewers who have already been accepted in this
+ // check. Legitimate users may race one another to accept on behalf of
+ // packages. If we get a form submission which includes a reviewer which
+ // someone has already accepted, that's fine. See T12757.
+
+ list($options) = $this->getActionOptions($actor, $object, true);
foreach ($value as $phid) {
if (!isset($options[$phid])) {
throw new Exception(
diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
--- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
@@ -17,6 +17,16 @@
$viewer = $this->getActor();
list($options, $default) = $this->getActionOptions($viewer, $object);
+ // Remove reviewers which aren't actionable. In the case of "Accept", we
+ // may allow the transaction to proceed with some reviewers who have
+ // already accepted, to avoid race conditions where two reviewers fill
+ // out the form at the same time and accept on behalf of the same package.
+ // It's okay for these reviewers to survive validation, but they should
+ // not survive beyond this point.
+ $value = array_fuse($value);
+ $value = array_intersect($value, array_keys($options));
+ $value = array_values($value);
+
sort($default);
sort($value);

File Metadata

Mime Type
text/plain
Expires
Tue, Mar 4, 1:09 AM (4 h, 50 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7220218
Default Alt Text
D18019.id.diff (2 KB)

Event Timeline

Phabricator · Built by Phacility