diff --git a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php index 23583422fb..7d14df7239 100644 --- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php @@ -1,285 +1,321 @@ setName('delete') ->setExamples('**delete** [--dry-run] ...') ->setSynopsis(pht('Delete audit requests matching parameters.')) ->setArguments( array( array( 'name' => 'dry-run', 'help' => pht( 'Show what would be deleted, but do not actually delete '. 'anything.'), ), array( 'name' => 'users', 'param' => 'names', 'help' => pht('Select only audits by a given list of users.'), ), array( 'name' => 'repositories', 'param' => 'repos', 'help' => pht( 'Select only audits in a given list of repositories.'), ), array( 'name' => 'commits', 'param' => 'commits', 'help' => pht('Select only audits for the given commits.'), ), array( 'name' => 'min-commit-date', 'param' => 'date', 'help' => pht( 'Select only audits for commits on or after the given date.'), ), array( 'name' => 'max-commit-date', 'param' => 'date', 'help' => pht( 'Select only audits for commits on or before the given date.'), ), array( 'name' => 'status', 'param' => 'status', 'help' => pht( 'Select only audits in the given status. By default, '. 'only open audits are selected.'), ), array( 'name' => 'ids', 'param' => 'ids', 'help' => pht('Select only audits with the given IDs.'), ), )); } public function execute(PhutilArgumentParser $args) { $viewer = $this->getViewer(); $users = $this->loadUsers($args->getArg('users')); $repos = $this->loadRepos($args->getArg('repositories')); $commits = $this->loadCommits($args->getArg('commits')); $ids = $this->parseList($args->getArg('ids')); $status = $args->getArg('status'); $min_date = $this->loadDate($args->getArg('min-commit-date')); $max_date = $this->loadDate($args->getArg('max-commit-date')); if ($min_date && $max_date && ($min_date > $max_date)) { throw new PhutilArgumentUsageException( pht('Specified maximum date must come after specified minimum date.')); } $is_dry_run = $args->getArg('dry-run'); $query = id(new DiffusionCommitQuery()) ->setViewer($this->getViewer()) ->needAuditRequests(true); if ($status) { $query->withStatuses(array($status)); } $id_map = array(); if ($ids) { $id_map = array_fuse($ids); $query->withAuditIDs($ids); } if ($repos) { $query->withRepositoryIDs(mpull($repos, 'getID')); } $auditor_map = array(); if ($users) { $auditor_map = array_fuse(mpull($users, 'getPHID')); $query->withAuditorPHIDs($auditor_map); } if ($commits) { $query->withPHIDs(mpull($commits, 'getPHID')); } - $commits = $query->execute(); - $commits = mpull($commits, null, 'getPHID'); + $commit_iterator = new PhabricatorQueryIterator($query); + $audits = array(); - foreach ($commits as $commit) { + foreach ($commit_iterator as $commit) { $commit_audits = $commit->getAudits(); foreach ($commit_audits as $key => $audit) { if ($id_map && empty($id_map[$audit->getID()])) { unset($commit_audits[$key]); continue; } if ($auditor_map && empty($auditor_map[$audit->getAuditorPHID()])) { unset($commit_audits[$key]); continue; } if ($min_date && $commit->getEpoch() < $min_date) { unset($commit_audits[$key]); continue; } if ($max_date && $commit->getEpoch() > $max_date) { unset($commit_audits[$key]); continue; } } - $audits[] = $commit_audits; - } - $audits = array_mergev($audits); - $console = PhutilConsole::getConsole(); - - if (!$audits) { - $console->writeErr("%s\n", pht('No audits match the query.')); - return 0; - } - - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs(mpull($audits, 'getAuditorPHID')) - ->execute(); + if (!$commit_audits) { + continue; + } + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(mpull($commit_audits, 'getAuditorPHID')) + ->execute(); - foreach ($audits as $audit) { - $commit = $commits[$audit->getCommitPHID()]; + foreach ($commit_audits as $audit) { + $audit_id = $audit->getID(); - $console->writeOut( - "%s\n", - sprintf( + $description = sprintf( '%10d %-16s %-16s %s: %s', - $audit->getID(), + $audit_id, $handles[$audit->getAuditorPHID()]->getName(), PhabricatorAuditStatusConstants::getStatusName( $audit->getAuditStatus()), $commit->getRepository()->formatCommitName( $commit->getCommitIdentifier()), - trim($commit->getSummary()))); + trim($commit->getSummary())); + + $audits[] = array( + 'auditID' => $audit_id, + 'commitPHID' => $commit->getPHID(), + 'description' => $description, + ); + } } - if (!$is_dry_run) { - $message = pht( - 'Really delete these %d audit(s)? They will be permanently deleted '. - 'and can not be recovered.', - count($audits)); - if ($console->confirm($message)) { - foreach ($audits as $audit) { - $id = $audit->getID(); - $console->writeOut("%s\n", pht('Deleting audit %d...', $id)); - $audit->delete(); - } + if (!$audits) { + echo tsprintf( + "%s\n", + pht('No audits match the query.')); + return 0; + } + + foreach ($audits as $audit_spec) { + echo tsprintf( + "%s\n", + $audit_spec['description']); + } + + if ($is_dry_run) { + echo tsprintf( + "%s\n", + pht('This is a dry run, so no changes will be made.')); + return 0; + } + + $message = pht( + 'Really delete these %s audit(s)? They will be permanently deleted '. + 'and can not be recovered.', + phutil_count($audits)); + if (!phutil_console_confirm($message)) { + echo tsprintf( + "%s\n", + pht('User aborted the workflow.')); + return 1; + } + + $audits_by_commit = igroup($audits, 'commitPHID'); + foreach ($audits_by_commit as $commit_phid => $audit_specs) { + $audit_ids = ipull($audit_specs, 'auditID'); + + $audits = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere( + 'id IN (%Ld)', + $audit_ids); + + foreach ($audits as $audit) { + $id = $audit->getID(); + + echo tsprintf( + "%s\n", + pht('Deleting audit %d...', $id)); + + $audit->delete(); } + + $this->synchronizeCommitAuditState($commit_phid); } return 0; } private function loadUsers($users) { $users = $this->parseList($users); if (!$users) { return null; } $objects = id(new PhabricatorPeopleQuery()) ->setViewer($this->getViewer()) ->withUsernames($users) ->execute(); $objects = mpull($objects, null, 'getUsername'); foreach ($users as $name) { if (empty($objects[$name])) { throw new PhutilArgumentUsageException( pht('No such user with username "%s"!', $name)); } } return $objects; } private function parseList($list) { $list = preg_split('/\s*,\s*/', $list); foreach ($list as $key => $item) { $list[$key] = trim($item); } foreach ($list as $key => $item) { if (!strlen($item)) { unset($list[$key]); } } return $list; } private function loadRepos($identifiers) { $identifiers = $this->parseList($identifiers); if (!$identifiers) { return null; } $query = id(new PhabricatorRepositoryQuery()) ->setViewer($this->getViewer()) ->withIdentifiers($identifiers); $repos = $query->execute(); $map = $query->getIdentifierMap(); foreach ($identifiers as $identifier) { if (empty($map[$identifier])) { throw new PhutilArgumentUsageException( pht('No repository "%s" exists!', $identifier)); } } return $repos; } private function loadDate($date) { if (!$date) { return null; } $epoch = strtotime($date); if (!$epoch || $epoch < 1) { throw new PhutilArgumentUsageException( pht( 'Unable to parse date "%s". Use a format like "%s".', $date, '2000-01-01')); } return $epoch; } private function loadCommits($commits) { $names = $this->parseList($commits); if (!$names) { return null; } $query = id(new DiffusionCommitQuery()) ->setViewer($this->getViewer()) ->withIdentifiers($names); $commits = $query->execute(); $map = $query->getIdentifierMap(); foreach ($names as $name) { if (empty($map[$name])) { throw new PhutilArgumentUsageException( pht('No such commit "%s"!', $name)); } } return $commits; } } diff --git a/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php index 6112a38e1d..b9d90bddc8 100644 --- a/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php @@ -1,90 +1,125 @@ 'all', 'help' => pht('Update all commits in all repositories.'), ), array( 'name' => 'objects', 'wildcard' => true, 'help' => pht('Update named commits and repositories.'), ), ); } protected function loadCommitsWithConstraints(PhutilArgumentParser $args) { $viewer = $this->getViewer(); $all = $args->getArg('all'); $names = $args->getArg('objects'); if (!$names && !$all) { throw new PhutilArgumentUsageException( pht( 'Specify "--all" to affect everything, or a list of specific '. 'commits or repositories to affect.')); } else if ($names && $all) { throw new PhutilArgumentUsageException( pht( 'Specify either a list of objects to affect or "--all", but not '. 'both.')); } if ($all) { $objects = new LiskMigrationIterator(new PhabricatorRepository()); } else { $query = id(new PhabricatorObjectQuery()) ->setViewer($viewer) ->withNames($names); $query->execute(); $objects = array(); $results = $query->getNamedResults(); foreach ($names as $name) { if (!isset($results[$name])) { throw new PhutilArgumentUsageException( pht( 'Object "%s" is not a valid object.', $name)); } $object = $results[$name]; if (!($object instanceof PhabricatorRepository) && !($object instanceof PhabricatorRepositoryCommit)) { throw new PhutilArgumentUsageException( pht( 'Object "%s" is not a valid repository or commit.', $name)); } $objects[] = $object; } } return $objects; } protected function loadCommitsForConstraintObject($object) { $viewer = $this->getViewer(); if ($object instanceof PhabricatorRepository) { $commits = id(new DiffusionCommitQuery()) ->setViewer($viewer) ->withRepository($object) ->execute(); } else { $commits = array($object); } return $commits; } + protected function synchronizeCommitAuditState($commit_phid) { + $viewer = $this->getViewer(); + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withPHIDs(array($commit_phid)) + ->needAuditRequests(true) + ->executeOne(); + if (!$commit) { + return; + } + + $old_status = $commit->getAuditStatusObject(); + $commit->updateAuditStatus($commit->getAudits()); + $new_status = $commit->getAuditStatusObject(); + + if ($old_status->getKey() == $new_status->getKey()) { + echo tsprintf( + "%s\n", + pht( + 'No synchronization changes for "%s".', + $commit->getDisplayName())); + } else { + echo tsprintf( + "%s\n", + pht( + 'Synchronizing "%s": "%s" -> "%s".', + $commit->getDisplayName(), + $old_status->getName(), + $new_status->getName())); + + $commit->save(); + } + } + } diff --git a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php index 96d06e65c2..abd0a3c637 100644 --- a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php @@ -1,58 +1,37 @@ setName('synchronize') - ->setExamples('**synchronize** ...') - ->setSynopsis(pht('Update audit status for commits.')) + ->setExamples( + "**synchronize** __repository__ ...\n". + "**synchronize** __commit__ ...\n". + "**synchronize** --all") + ->setSynopsis( + pht( + 'Update commits to make their summary audit state reflect the '. + 'state of their actual audit requests. This can fix inconsistencies '. + 'in database state if audit requests have been mangled '. + 'accidentally (or on purpose).')) ->setArguments( array_merge( $this->getCommitConstraintArguments(), array())); } public function execute(PhutilArgumentParser $args) { $viewer = $this->getViewer(); $objects = $this->loadCommitsWithConstraints($args); foreach ($objects as $object) { $commits = $this->loadCommitsForConstraintObject($object); foreach ($commits as $commit) { - $commit = id(new DiffusionCommitQuery()) - ->setViewer($viewer) - ->withPHIDs(array($commit->getPHID())) - ->needAuditRequests(true) - ->executeOne(); - if (!$commit) { - continue; - } - - $old_status = $commit->getAuditStatusObject(); - $commit->updateAuditStatus($commit->getAudits()); - $new_status = $commit->getAuditStatusObject(); - - if ($old_status->getKey() == $new_status->getKey()) { - echo tsprintf( - "%s\n", - pht( - 'No changes for "%s".', - $commit->getDisplayName())); - } else { - echo tsprintf( - "%s\n", - pht( - 'Updating "%s": "%s" -> "%s".', - $commit->getDisplayName(), - $old_status->getName(), - $new_status->getName())); - - $commit->save(); - } + $this->synchronizeCommitAuditState($commit->getPHID()); } } } } diff --git a/src/docs/user/userguide/audit.diviner b/src/docs/user/userguide/audit.diviner index 0d10867906..5223f6c969 100644 --- a/src/docs/user/userguide/audit.diviner +++ b/src/docs/user/userguide/audit.diviner @@ -1,202 +1,199 @@ @title Audit User Guide @group userguide Guide to using Phabricator to audit published commits. Overview ======== Phabricator supports two code review workflows, "review" (pre-publish) and "audit" (post-publish). To understand the differences between the two, see @{article:User Guide: Review vs Audit}. How Audit Works =============== The audit workflow occurs after changes have been published. It provides ways to track, discuss, and resolve issues with commits that are discovered after they go through whatever review process you have in place (if you have one). Two examples of how you might use audit are: **Fix Issues**: If a problem is discovered after a change has already been published, users can find the commit which introduced the problem and raise a concern on it. This notifies the author of the commit and prompts them to remedy the issue. **Watch Changes**: In some cases, you may want to passively look over changes that satisfy some criteria as they are published. For example, you may want to review all Javascript changes at the end of the week to keep an eye on things, or make sure that code which impacts a subsystem is looked at by someone on that team, eventually. Developers may also want other developers to take a second look at things if they realize they aren't sure about something after a change has been published, or just want to provide a heads-up. You can configure Herald rules and Owners packages to automatically trigger audits of commits that satisfy particular criteria. Audit States and Actions ======================== The audit workflow primarily keeps track of two things: - **Commits** and their audit state (like "Not Audited", "Approved", or "Concern Raised"). - **Audit Requests** which ask a user (or some other entity, like a project or package) to audit a commit. These can be triggered in a number of ways (see below). Users interact with commits by leaving comments and applying actions, like accepting the changes or raising a concern. These actions change the state of their own audit and the overall audit state of the commit. Here's an example of a typical audit workflow: - Alice publishes a commit containing some Javascript. - This triggers an audit request to Bailey, the Javascript technical lead on the project (see below for a description of trigger mechanisms). - Later, Bailey logs into Phabricator and sees the audit request. She ignores it for the moment, since it isn't blocking anything. At the end of the week she looks through her open requests to see what the team has been up to. - Bailey notices a few minor problems with Alice's commit. She leaves comments describing improvements and uses "Raise Concern" to send the commit back into Alice's queue. - Later, Alice logs into Phabricator and sees that Bailey has raised a concern (usually, Alice will also get an email). She resolves the issue somehow, maybe by making a followup commit with fixes. - After the issues have been dealt with, she uses "Request Verification" to return the change to Bailey so Bailey can verify that the concerns have been addressed. - Bailey uses "Accept Commit" to close the audit. In {nav Diffusion > Browse Commits}, you can review commits and query for commits with certain audit states. The default "Active Audits" view shows all of the commits which are relevant to you given their audit state, divided into buckets: - **Needs Attention**: These are commits which you authored that another user has raised a concern about: for example, maybe they believe they have found a bug or some other problem. You should address the concerns. - **Needs Verification**: These are commits which someone else authored that you previously raised a concern about. The author has indicated that they believe the concern has been addressed. You should verify that the remedy is satisfactory and accept the change, or raise a further concern. - **Ready to Audit**: These are commits which someone else authored that you have been asked to audit, either by a user or by a system rule. You should look over the changes and either accept them or raise concerns. - **Waiting on Authors**: These are commits which someone else authored that you previously raised a concern about. The author has not responded to the concern yet. You may want to follow up. - **Waiting on Auditors**: These are commits which you authored that someone else needs to audit. You can use the query constraints to filter this list or find commits that match certain criteria. Audit Triggers ============== Audit requests can be triggered in a number of ways: - You can add auditors explicitly from the web UI, using either "Edit Commit" or the "Change Auditors" action. You might do this if you realize you are not sure about something that you recently published and want a second opinion. - If you put `Auditors: username1, username2` in your commit message, it will trigger an audit request to those users when you push it to a tracked branch. - You can create rules in Herald that trigger audits based on properties of the commit -- like the files it touches, the text of the change, the author, etc. - You can create an Owners package and enable automatic auditing for the package. Audits in Small Teams ===================== If you have a small team and don't need complicated trigger rules, you can set up a simple audit workflow like this: - Create a new Project, "Code Audits". - Create a new global Herald rule for Commits, which triggers an audit by the "Code Audits" project for every commit where "Differential Revision" "does not exist" (this will allow you to transition partly or fully to review later if you want). - Have every engineer join the "Code Audits" project. This way, everyone will see an audit request for every commit, but it will be dismissed if anyone approves it. Effectively, this enforces the rule "every commit should have //someone// look at it". Once your team gets bigger, you can refine this ruleset so that developers see only changes that are relevant to them. Audit Tips ========== - When viewing a commit, audit requests you are responsible for are highlighted. You are responsible for a request if it's a user request and you're that user, or if it's a project request and you're a member of the project, or if it's a package request and you're a package owner. Any action you take will update the state of all the requests you're responsible for. - You can leave inline comments by clicking the line numbers in the diff. - You can leave a comment across multiple lines by dragging across the line numbers. - Inline comments are initially saved as drafts. They are not submitted until you submit a comment at the bottom of the page. - Press "?" to view keyboard shortcuts. Audit Maintenance ================= The `bin/audit` command allows you to perform several maintenance operations. Get more information about a command by running: ``` phabricator/ $ ./bin/audit help ``` Supported operations are: **Delete Audits**: Delete audits that match certain parameters with `bin/audit delete`. You can use this command to forcibly delete requests which may have triggered incorrectly (for example, because a package or Herald rule was configured in an overbroad way). -After deleting audits, you may want to run `bin/audit synchronize` to -synchronize audit state. - **Synchronize Audit State**: Synchronize the audit state of commits to the current open audit requests with `bin/audit synchronize`. Normally, overall audit state is automatically kept up to date as changes are -made to an audit. However, if you delete audits or manually update the database -to make changes to audit request state, the state of corresponding commits may -no longer be correct. +made to an audit. However, if you manually update the database to make changes +to audit request state, the state of corresponding commits may no longer be +consistent. This command will update commits so their overall audit state reflects the cumulative state of their actual audit requests. **Update Owners Package Membership**: Update which Owners packages commits belong to with `bin/audit update-owners`. Normally, commits are automatically associated with packages when they are imported. You can use this command to manually rebuild this association if you run into problems with it. Next Steps ========== - Learn more about Herald at @{article:Herald User Guide}.