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);