No OneTemporary
Actions

Size

25 KB

Referenced Files

None

Subscribers

None

View Options

	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 @@
	<?php

	final class PhabricatorAuditManagementDeleteWorkflow
	extends PhabricatorAuditManagementWorkflow {

	protected function didConstruct() {
	$this
	->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 @@
	<?php

	abstract class PhabricatorAuditManagementWorkflow
	extends PhabricatorManagementWorkflow {


	protected function getCommitConstraintArguments() {
	return array(
	array(
	'name' => '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 @@
	<?php

	final class PhabricatorAuditSynchronizeManagementWorkflow
	extends PhabricatorAuditManagementWorkflow {

	protected function didConstruct() {
	$this
	->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 <command>
	```

	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}.

File Metadata

Mime Type: text/x-diff
Expires: Tue, Dec 2, 5:34 PM (12 h, 50 m)
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 432293
Default Alt Text: (25 KB)

No OneTemporaryActions

View Options

File Metadata

Event Timeline

No OneTemporary
Actions