diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php @@ -38,123 +38,6 @@ $commit->getPHID()); } - // When an actor submits an audit comment, we update all the audit requests - // they have authority over to reflect the most recent status. The general - // idea here is that if audit has triggered for, e.g., several packages, but - // a user owns all of them, they can clear the audit requirement in one go - // without auditing the commit for each trigger. - - $audit_phids = self::loadAuditPHIDsForUser($actor); - $audit_phids = array_fill_keys($audit_phids, true); - - $requests = $commit->getAudits(); - - // TODO: We should validate the action, currently we allow anyone to, e.g., - // close an audit if they muck with form parameters. I'll followup with this - // and handle the no-effect cases (e.g., closing and already-closed audit). - - $actor_is_author = ($actor->getPHID() == $commit->getAuthorPHID()); - - // Pick a meaningful action, if we have one. - $action = PhabricatorAuditActionConstants::COMMENT; - foreach ($comments as $comment) { - switch ($comment->getAction()) { - case PhabricatorAuditActionConstants::CLOSE: - case PhabricatorAuditActionConstants::RESIGN: - case PhabricatorAuditActionConstants::ACCEPT: - case PhabricatorAuditActionConstants::CONCERN: - $action = $comment->getAction(); - break; - } - } - - if ($action == PhabricatorAuditActionConstants::CLOSE) { - - // This is now applied by the transaction Editor. - - } else if ($action == PhabricatorAuditActionConstants::RESIGN) { - - // This is now applied by the transaction Editor. - - } else { - $have_any_requests = false; - foreach ($requests as $request) { - if (empty($audit_phids[$request->getAuditorPHID()])) { - continue; - } - - $request_is_for_actor = - ($request->getAuditorPHID() == $actor->getPHID()); - - $have_any_requests = true; - $new_status = null; - switch ($action) { - case PhabricatorAuditActionConstants::COMMENT: - case PhabricatorAuditActionConstants::ADD_CCS: - case PhabricatorAuditActionConstants::ADD_AUDITORS: - // Commenting or adding cc's/auditors doesn't change status. - break; - case PhabricatorAuditActionConstants::ACCEPT: - if (!$actor_is_author || $request_is_for_actor) { - // When modifying your own commits, you act only on behalf of - // yourself, not your packages/projects -- the idea being that - // you can't accept your own commits. - $new_status = PhabricatorAuditStatusConstants::ACCEPTED; - } - break; - case PhabricatorAuditActionConstants::CONCERN: - if (!$actor_is_author || $request_is_for_actor) { - // See above. - $new_status = PhabricatorAuditStatusConstants::CONCERNED; - } - break; - default: - throw new Exception("Unknown action '{$action}'!"); - } - if ($new_status !== null) { - $request->setAuditStatus($new_status); - $request->save(); - } - } - - // If the actor has no current authority over any audit trigger, make a - // new one to represent their audit state. - if (!$have_any_requests) { - $new_status = null; - switch ($action) { - case PhabricatorAuditActionConstants::COMMENT: - case PhabricatorAuditActionConstants::ADD_AUDITORS: - case PhabricatorAuditActionConstants::ADD_CCS: - break; - case PhabricatorAuditActionConstants::ACCEPT: - $new_status = PhabricatorAuditStatusConstants::ACCEPTED; - break; - case PhabricatorAuditActionConstants::CONCERN: - $new_status = PhabricatorAuditStatusConstants::CONCERNED; - break; - case PhabricatorAuditActionConstants::CLOSE: - // Impossible to reach this block with 'close'. - default: - throw new Exception("Unknown or invalid action '{$action}'!"); - } - - if ($new_status !== null) { - $request = id(new PhabricatorRepositoryAuditRequest()) - ->setCommitPHID($commit->getPHID()) - ->setAuditorPHID($actor->getPHID()) - ->setAuditStatus($new_status) - ->setAuditReasons(array('Voluntary Participant')) - ->save(); - $requests[] = $request; - } - } - } - - $commit->updateAuditStatus($requests); - $commit->save(); - - $commit->attachAudits($requests); - // Convert old comments into real transactions and apply them with a // normal editor. diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -133,13 +133,18 @@ $status_concerned = PhabricatorAuditStatusConstants::CONCERNED; $status_closed = PhabricatorAuditStatusConstants::CLOSED; $status_resigned = PhabricatorAuditStatusConstants::RESIGNED; + $status_accepted = PhabricatorAuditStatusConstants::ACCEPTED; + $status_concerned = PhabricatorAuditStatusConstants::CONCERNED; $actor_phid = $this->requireActor()->getPHID(); + $actor_is_author = ($object->getAuthorPHID()) && + ($actor_phid == $object->getAuthorPHID()); foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { case PhabricatorAuditActionConstants::ACTION: - switch ($xaction->getNewValue()) { + $new = $xaction->getNewValue(); + switch ($new) { case PhabricatorAuditActionConstants::CLOSE: // "Close" means wipe out all the concerns. $requests = $object->getAudits(); @@ -162,6 +167,58 @@ ->save(); } break; + case PhabricatorAuditActionConstants::ACCEPT: + case PhabricatorAuditActionConstants::CONCERN: + if ($new == PhabricatorAuditActionConstants::ACCEPT) { + $new_status = $status_accepted; + } else { + $new_status = $status_concerned; + } + + $requests = $object->getAudits(); + $requests = mpull($requests, null, 'getAuditorPHID'); + + // Figure out which requests the actor has authority over: these + // are user requests where they are the auditor, and packages + // and projects they are a member of. + + if ($actor_is_author) { + // When modifying your own commits, you act only on behalf of + // yourself, not your packages/projects -- the idea being that + // you can't accept your own commits. + $authority_phids = array($actor_phid); + } else { + $authority_phids = + PhabricatorAuditCommentEditor::loadAuditPHIDsForUser( + $this->requireActor()); + } + + $authority = array_select_keys( + $requests, + $authority_phids); + + if (!$authority) { + // If the actor has no authority over any existing requests, + // create a new request for them. + + $actor_request = id(new PhabricatorRepositoryAuditRequest()) + ->setCommitPHID($object->getPHID()) + ->setAuditorPHID($actor_phid) + ->setAuditStatus($new_status) + ->save(); + + $requests[$actor_phid] = $actor_request; + $object->attachAudits($requests); + } else { + // Otherwise, update the audit status of the existing requests. + foreach ($authority as $request) { + $request + ->setAuditStatus($new_status) + ->save(); + } + } + break; + } break; }