diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php index 7d1f1a92ef..9b6aad3ad1 100644 --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -1,222 +1,231 @@ getReviewers(); $options = array(); $value = array(); // Put the viewer's user reviewer first, if it exists, so that "Accept as // yourself" is always at the top. $head = array(); $tail = array(); foreach ($reviewers as $key => $reviewer) { if ($reviewer->isUser()) { $head[$key] = $reviewer; } else { $tail[$key] = $reviewer; } } $reviewers = $head + $tail; $diff_phid = $this->getActiveDiffPHID($revision); $reviewer_phids = array(); // If the viewer isn't a reviewer, add them to the list of options first. // This happens when you navigate to some revision you aren't involved in: // you can accept and become a reviewer. $viewer_phid = $viewer->getPHID(); if ($viewer_phid) { if (!isset($reviewers[$viewer_phid])) { $reviewer_phids[$viewer_phid] = $viewer_phid; } } $default_unchecked = array(); foreach ($reviewers as $reviewer) { $reviewer_phid = $reviewer->getReviewerPHID(); if (!$reviewer->hasAuthority($viewer)) { // If the viewer doesn't have authority to act on behalf of a reviewer, // we check if they can accept by force. if ($revision->canReviewerForceAccept($viewer, $reviewer)) { $default_unchecked[$reviewer_phid] = true; } else { continue; } } - 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; } $handles = $viewer->loadHandles($reviewer_phids); $head = array(); $tail = array(); foreach ($reviewer_phids as $reviewer_phid) { $is_force = isset($default_unchecked[$reviewer_phid]); if ($is_force) { $tail[] = $reviewer_phid; $options[$reviewer_phid] = pht( 'Force accept as %s', $viewer->renderHandle($reviewer_phid)); } else { $head[] = $reviewer_phid; $value[] = $reviewer_phid; $options[$reviewer_phid] = pht( 'Accept as %s', $viewer->renderHandle($reviewer_phid)); } } // Reorder reviewers so "force accept" reviewers come at the end. $options = array_select_keys($options, $head) + array_select_keys($options, $tail); return array($options, $value); } public function generateOldValue($object) { $actor = $this->getActor(); return $this->isViewerFullyAccepted($object, $actor); } public function applyExternalEffects($object, $value) { $status = DifferentialReviewerStatus::STATUS_ACCEPTED; $actor = $this->getActor(); $this->applyReviewerEffect($object, $actor, $value, $status); } protected function validateAction($object, PhabricatorUser $viewer) { if ($object->isClosed()) { throw new Exception( pht( 'You can not accept this revision because it has already been '. 'closed. Only open revisions can be accepted.')); } $config_key = 'differential.allow-self-accept'; if (!PhabricatorEnv::getEnvConfig($config_key)) { if ($this->isViewerRevisionAuthor($object, $viewer)) { throw new Exception( pht( 'You can not accept this revision because you are the revision '. 'author. You can only accept revisions you do not own. You can '. 'change this behavior by adjusting the "%s" setting in Config.', $config_key)); } } if ($this->isViewerFullyAccepted($object, $viewer)) { throw new Exception( pht( 'You can not accept this revision because you have already '. 'accepted it.')); } } protected function validateOptionValue($object, $actor, array $value) { if (!$value) { throw new Exception( pht( 'When accepting a revision, you must accept on behalf of at '. '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( pht( 'Reviewer "%s" is not a valid reviewer which you have authority '. 'to accept on behalf of.', $phid)); } } } public function getTitle() { $new = $this->getNewValue(); if (is_array($new) && $new) { return pht( '%s accepted this revision as %s reviewer(s): %s.', $this->renderAuthor(), phutil_count($new), $this->renderHandleList($new)); } else { return pht( '%s accepted this revision.', $this->renderAuthor()); } } public function getTitleForFeed() { return pht( '%s accepted %s.', $this->renderAuthor(), $this->renderObject()); } } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index d44de8706c..c30514b9ab 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -1,283 +1,293 @@ 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); if ($default === $value) { return true; } return $value; } protected function isViewerAnyReviewer( DifferentialRevision $revision, PhabricatorUser $viewer) { return ($this->getViewerReviewerStatus($revision, $viewer) !== null); } protected function isViewerAnyAuthority( DifferentialRevision $revision, PhabricatorUser $viewer) { $reviewers = $revision->getReviewers(); foreach ($revision->getReviewers() as $reviewer) { if ($reviewer->hasAuthority($viewer)) { return true; } } return false; } protected function isViewerFullyAccepted( DifferentialRevision $revision, PhabricatorUser $viewer) { return $this->isViewerReviewerStatusFully( $revision, $viewer, DifferentialReviewerStatus::STATUS_ACCEPTED); } protected function isViewerFullyRejected( DifferentialRevision $revision, PhabricatorUser $viewer) { return $this->isViewerReviewerStatusFully( $revision, $viewer, DifferentialReviewerStatus::STATUS_REJECTED); } protected function getViewerReviewerStatus( DifferentialRevision $revision, PhabricatorUser $viewer) { if (!$viewer->getPHID()) { return null; } foreach ($revision->getReviewers() as $reviewer) { if ($reviewer->getReviewerPHID() != $viewer->getPHID()) { continue; } return $reviewer->getReviewerStatus(); } return null; } private function isViewerReviewerStatusFully( DifferentialRevision $revision, PhabricatorUser $viewer, $require_status) { // If the user themselves is not a reviewer, the reviews they have // authority over can not all be in any set of states since their own // personal review has no state. $status = $this->getViewerReviewerStatus($revision, $viewer); if ($status === null) { return false; } $active_phid = $this->getActiveDiffPHID($revision); $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; $status_rejected = DifferentialReviewerStatus::STATUS_REJECTED; $is_accepted = ($require_status == $status_accepted); $is_rejected = ($require_status == $status_rejected); // Otherwise, check that all reviews they have authority over are in // the desired set of states. foreach ($revision->getReviewers() as $reviewer) { if (!$reviewer->hasAuthority($viewer)) { $can_force = false; if ($is_accepted) { if ($revision->canReviewerForceAccept($viewer, $reviewer)) { $can_force = true; } } if (!$can_force) { continue; } } $status = $reviewer->getReviewerStatus(); if ($status != $require_status) { return false; } // Here, we're primarily testing if we can remove a void on the review. if ($is_accepted) { if (!$reviewer->isAccepted($active_phid)) { return false; } } if ($is_rejected) { if (!$reviewer->isRejected($active_phid)) { return false; } } // This is a broader check to see if we can update the diff where the // last action occurred. if ($reviewer->getLastActionDiffPHID() != $active_phid) { return false; } } return true; } protected function applyReviewerEffect( DifferentialRevision $revision, PhabricatorUser $viewer, $value, $status) { $map = array(); // When you accept or reject, you may accept or reject on behalf of all // reviewers you have authority for. When you resign, you only affect // yourself. $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED); $with_force = ($status == DifferentialReviewerStatus::STATUS_ACCEPTED); if ($with_authority) { foreach ($revision->getReviewers() as $reviewer) { if (!$reviewer->hasAuthority($viewer)) { if (!$with_force) { continue; } if (!$revision->canReviewerForceAccept($viewer, $reviewer)) { continue; } } $map[$reviewer->getReviewerPHID()] = $status; } } // In all cases, you affect yourself. $map[$viewer->getPHID()] = $status; // If we're applying an "accept the defaults" transaction, and this // transaction type uses checkboxes, replace the value with the list of // defaults. if (!is_array($value)) { list($options, $default) = $this->getActionOptions($viewer, $revision); if ($options) { $value = $default; } } // If we have a specific list of reviewers to act on, usually because the // user has submitted a specific list of reviewers to act as by // unchecking some checkboxes under "Accept", only affect those reviewers. if (is_array($value)) { $map = array_select_keys($map, $value); } // Convert reviewer statuses into edge data. foreach ($map as $reviewer_phid => $reviewer_status) { $map[$reviewer_phid] = array( 'data' => array( 'status' => $reviewer_status, ), ); } // This is currently double-writing: to the old (edge) store and the new // (reviewer) store. Do the old edge write first. $src_phid = $revision->getPHID(); $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; $editor = new PhabricatorEdgeEditor(); foreach ($map as $dst_phid => $edge_data) { if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { // TODO: For now, we just remove these reviewers. In the future, we will // store resignations explicitly. $editor->removeEdge($src_phid, $edge_type, $dst_phid); } else { $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data); } } $editor->save(); // Now, do the new write. if ($map) { $diff = $this->getEditor()->getActiveDiff($revision); if ($diff) { $diff_phid = $diff->getPHID(); } else { $diff_phid = null; } $table = new DifferentialReviewer(); $reviewers = $table->loadAllWhere( 'revisionPHID = %s AND reviewerPHID IN (%Ls)', $src_phid, array_keys($map)); $reviewers = mpull($reviewers, null, 'getReviewerPHID'); foreach ($map as $dst_phid => $edge_data) { $reviewer = idx($reviewers, $dst_phid); if (!$reviewer) { $reviewer = id(new DifferentialReviewer()) ->setRevisionPHID($src_phid) ->setReviewerPHID($dst_phid); } $old_status = $reviewer->getReviewerStatus(); $reviewer->setReviewerStatus($status); if ($diff_phid) { $reviewer->setLastActionDiffPHID($diff_phid); } if ($old_status !== $status) { $reviewer->setLastActorPHID($this->getActingAsPHID()); } // Clear any outstanding void on this reviewer. A void may be placed // by the author using "Request Review" when a reviewer has already // accepted. $reviewer->setVoidedPHID(null); try { $reviewer->save(); } catch (AphrontDuplicateKeyQueryException $ex) { // At least for now, just ignore it if we lost a race. } } } } }