mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 05:50:55 +01:00
Don't consider accepting on behalf of valid-but-accepted reviewers to be a validation error
Summary: Fixes T12757. Here's a simple repro for this: - Add a package you own as a reviewer to a revision you're reviewing. - Open two windows, select "Accept", don't submit the form. - Submit the form in window A. - Submit the fomr in window B. Previously, window B would show an error, because we considered accepting on behalf of the package invalid, as the package had already accepted. Instead, let repeat-accepts through without complaint. Some product stuff: - We could roadblock users with a more narrow validation error message here instead, like "Package X has already been accepted.", but I think this would be more annoying than helpful. - If your accept has no effect (i.e., everything you're accepting for has already accepted) we currently just let it through. I think this is fine -- and a bit tricky to tailor -- but the ideal/consistent beavior is to do a "no effect" warning like "All the reviewers you're accepting for have already accepted.". This is sufficiently finnicky/rare (and probably not terribly useful/desiable in this specific case)that I'm just punting. Test Plan: Did the flow above, got an "Accept" instead of a validation error. Reviewers: chad, lvital Reviewed By: chad, lvital Subscribers: lvital Maniphest Tasks: T12757 Differential Revision: https://secure.phabricator.com/D18019
This commit is contained in:
parent
84742a94db
commit
19572f53fd
2 changed files with 25 additions and 6 deletions
|
@ -50,7 +50,8 @@ final class DifferentialRevisionAcceptTransaction
|
||||||
|
|
||||||
protected function getActionOptions(
|
protected function getActionOptions(
|
||||||
PhabricatorUser $viewer,
|
PhabricatorUser $viewer,
|
||||||
DifferentialRevision $revision) {
|
DifferentialRevision $revision,
|
||||||
|
$include_accepted = false) {
|
||||||
|
|
||||||
$reviewers = $revision->getReviewers();
|
$reviewers = $revision->getReviewers();
|
||||||
|
|
||||||
|
@ -98,11 +99,14 @@ final class DifferentialRevisionAcceptTransaction
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!$include_accepted) {
|
||||||
if ($reviewer->isAccepted($diff_phid)) {
|
if ($reviewer->isAccepted($diff_phid)) {
|
||||||
// If a reviewer is already in a full "accepted" state, don't
|
// If a reviewer is already in a full "accepted" state, don't
|
||||||
// include that reviewer as an option.
|
// include that reviewer as an option unless we're listing all
|
||||||
|
// reviwers, including reviewers who have already accepted.
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
$reviewer_phids[$reviewer_phid] = $reviewer_phid;
|
$reviewer_phids[$reviewer_phid] = $reviewer_phid;
|
||||||
}
|
}
|
||||||
|
@ -185,7 +189,12 @@ final class DifferentialRevisionAcceptTransaction
|
||||||
'least one reviewer.'));
|
'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) {
|
foreach ($value as $phid) {
|
||||||
if (!isset($options[$phid])) {
|
if (!isset($options[$phid])) {
|
||||||
throw new Exception(
|
throw new Exception(
|
||||||
|
|
|
@ -17,6 +17,16 @@ abstract class DifferentialRevisionReviewTransaction
|
||||||
$viewer = $this->getActor();
|
$viewer = $this->getActor();
|
||||||
list($options, $default) = $this->getActionOptions($viewer, $object);
|
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($default);
|
||||||
sort($value);
|
sort($value);
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue