diff --git a/src/docs/user/userguide/audit.diviner b/src/docs/user/userguide/audit.diviner index 2ebc26a687..de6878ed53 100644 --- a/src/docs/user/userguide/audit.diviner +++ b/src/docs/user/userguide/audit.diviner @@ -1,104 +1,162 @@ @title Audit User Guide @group userguide -Guide to the Audit (post-push code review) tool and workflow. +Guide to using Phabricator to audit published commits. -= Overview = -Phabricator supports two code review workflows, "review" (pre-push) and -"audit" (post-push). To understand the differences between the two, see +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}. -This document summarizes the post-push "audit" workflow implemented by the -creatively-named //Audit// tool. -= How Audit Works = +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. -Using auditing allows you to push and deploy code without waiting for code -review, while still doing code review eventually. The Audit tool primarily keeps -track of two things: +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) to audit a - commit. These can be triggered in a number of ways (see below). - -In the Audit tool's home screen and on the homepage you can see commits and -requests that require your action: - - - **Required Audits** are open audit requests that require you, a project - you are a member of, or a package you own to audit a commit. An audit - request is closed when you approve the associated commit. - - **Problem Commits** are commits you authored which someone has raised a - concern about in audit. Problem commits go away when you satisfy all the - auditors and get them to "Approve" the commit. - -For example: - - - Evan creates commit `abcdef1234` and pushes it to the remote. - - This triggers an audit request to Bob through some mechanism (see below for - a description of trigger mechanisms). - - Later, Bob logs into Phabricator and sees the audit request on his homepage. - - Bob clicks through and examines the commit. He notices a problem, so he - selects "Raise Concern" and describes the issue in a comment. - - Evan receives an email that Bob has raised a concern about his commit. He - opts not to deal with it immediately. - - Later, Evan logs into Phabricator and sees the commit on his homepage - under "Problem Commits". - - Evan resolves the issue somehow (e.g., by discussing it with Bob, or fixing - it in another commit). - - Now satisfied, Bob "Accepts" the original commit. - - This causes the request to disappear from Bob's queue, and the commit to - disappear from Evan's queue. - -= Audit Triggers = + - **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 Phabrictor 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 audit request for yourself by commenting on any commit. - - You can create an Owners package and select "Enable Auditing" (this is an - advanced feature which is only likely to be useful for very large teams). + - You can create an Owners package and enable automatic auditing for the + package. -= Audits in Small Teams = + +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 = + +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. -= Next Steps = + +Next Steps +========== - Learn more about Herald at @{article:Herald User Guide}. diff --git a/src/docs/user/userguide/reviews_vs_audit.diviner b/src/docs/user/userguide/reviews_vs_audit.diviner index 0ccdef787f..642683840c 100644 --- a/src/docs/user/userguide/reviews_vs_audit.diviner +++ b/src/docs/user/userguide/reviews_vs_audit.diviner @@ -1,123 +1,143 @@ @title User Guide: Review vs Audit @group userguide -Discusses the differences between Review and Audit workflows. +Discusses the differences between "review" and "audit" workflows. -= Overview = +Overview +======== -Phabricator supports two similar but separate code review workflows: +Phabricator supports two similar but separate code review workflows: "review" +and "audit". - - **Differential** is used for pre-push code review, called "reviews" - elsewhere in the documentation. You can learn more in - @{article:Differential User Guide}. - - **Audit** is used for post-push code reviews, called "audits" elsewhere in - the documentation. You can learn more in @{article:Audit User Guide}. +Review occurs in **Differential**, before changes are published. You can learn +more in @{article:Differential User Guide}. -(By "pre-push", this document means review which blocks deployment of changes, -while "post-push" means review which happens after changes are deployed or -en route to deployment.) +Audit occurs in **Diffusion**, after changes are published. You can learn more +in @{article:Audit User Guide}. -Both are lightweight, asynchronous web-based workflows where reviewers/auditors -inspect code independently, from their own machines -- not synchronous review -sessions where authors and reviewers meet in person to discuss changes. +When this documentation discusses "unpublished changes", it refers to changes +which are still subject to being reworked in response to feedback. In many +workflows, these changes will only exist locally on the developer's machine, +but some workflows push tentative or temporary changes into remotes. The step +that "publishes" changes might be either pushing or merging them, depending on +your workflow. -= Advantages of Review = +Both the audit and review workflows are lightweight, asynchronous web-based +workflows where reviewers or auditors inspect code independently, from their +own machines -- not synchronous review sessions where authors and reviewers +meet in person to discuss changes. -Pre-push review is significantly more powerful than post-push auditing. You -gain these advantages by requiring review //before// changes may be pushed: +Broadly, review is normally a //blocking// workflow: in review workflows, +authors usually can not publish changes until review completes and reviewers +are satisfied. + +In contrast, audit is normally a //nonblocking// workflow: in audit workflows, +changes usually move forward by default. + +Advantages of Review +==================== + +Pre-publish review is significantly more powerful than post-publish auditing. +You gain these advantages by requiring review //before// changes may be +published: - Authors have a strong incentive to craft small, well-formed changes that will be readily understood, to explain them adequately, and to provide appropriate test plans, test coverage and context. - Reviewers have a real opportunity to make significant suggestions about architecture or approach in review. These suggestions are less attractive to adopt from audit, and may be much more difficult to adopt if significant - time has passed between push and audit. + time has passed between publish and audit. - Authors have a strong incentive to fix problems and respond to feedback - received during review, because it blocks them. Authors have a much weaker - incentive to address problems raised during audit. - - Authors can ask reviewers to apply and verify fixes before they are pushed. + received during review because it blocks them. Authors have a much weaker + incentive to promptly address problems raised during audit. + - Authors can ask reviewers to apply and verify fixes before they are + published. - Authors can easily pursue feedback early, and get course corrections on approach or direction. - Reviewers are better prepared to support a given change once it is in production, having already had a chance to become familiar with and reason through the code. - Reviewers are able to catch problems which automated tests may have difficulty detecting. For example, human reviewers are able to reason about performance problems that tests can easily miss because they run on small datasets and stub out service calls. - Communicating about changes //before// they happen generally leads to better preparation for their effects. The theoretical cost of review is that it slows down development by introducing a blocking step into the process and generally wastes developer time that could be better spent developing. This is less true than it appears, because the costs are low and pay for themselves in other ways: - - Differential is fast and provides a very lightweight process for submitting + - Differential is fast and provides a lightweight process for submitting code for review and for performing review. - Authors are free to pursue other changes while code is being reviewed. With appropriate change management (like local branching in Git) they can even pursue dependent changes easily. Authors should rarely if ever be blocked on review, even though an individual change is blocked until it is approved. - The workflow as a whole is lightweight and, with skillful reviewers, effective at identifying bugs. It is generally faster to fix bugs in review than in production. - More importantly, it is effective at identifying problems with architecture and approach. These are free to fix in review ("don't do this, it is a bad idea") and may be very time consuming to fix in production. No matter how good your test suite is, it can't identify solutions which are poor because of missing context, or miscommunication, or which are simply bad ideas. - Changes which are too large or too complicated to be reviewed quickly are often //too large and too complicated, period//. Nearly all large changes can be split apart into small, independent pieces which are easier to understand and test. Review tends to encourage smaller and better-factored changes. - Review can be integrated with static analysis which can detect (and, in many cases, correct) mechanical problems with code like syntax, formatting, naming conventions, style problems, misspellings, and some program errors. This reduces the amount of time it takes to review code, and means reviewers can focus on actual problems with the code rather than minor stylistic issues. - Review creates a permanent record of context and intent which explains why a change was made, generally with much more information than commit messages alone (authors have an incentive to properly explain a change when sending it for review). This makes it easier to understand code later, and to respond appropriately when it breaks. - With `arc patch`, it is roughly as easy to pull a change out of Differential as it is to pull it out of the remote. -= Advantages of Audit = +Advantages of Audit +=================== -Post-push review is significantly better than nothing. If you are unpersuaded +Post-publish audit is a less powerful workflow than pre-publish review, but can +supplement review and is better than nothing on its own. If you are unpersuaded by the arguments above (or work on a team that is unswayed), audits provide some of the benefits of review with less friction: - - Audits are driven entirely by Phabricator, users do not need to install + - Audits are driven entirely by Phabricator: users do not need to install `arc`. - Audits require little adjustment to existing workflows and little training. - Audits are completely nonblocking, and send fewer notifications than review. - Even if you have review, audits can be useful as a supplement to keep tabs on lower-importance changes or raise issues that are discovered after review. -= Recommendations = +Recommendations +=============== Here are super biased recommendations from developers of code review software: - If you can do review, do it. Supplement it with audits for less important changes as your organization scales. - If you can't do review immediately, set up audits and try to transition toward review. Some types of changes (like tentative changes or requests for feedback about code) are a naturally good fit for review and can serve as a stepping stone toward broader acceptance. Greater familiarity with the toolset may also foster more acceptance toward review, and the value of review may become more obvious as the organization scales (e.g., once you get interns). - If you aren't interested in review, just do audits. You can always change your mind later. But consider review! It's really good, we promise! -= Next Steps = +Next Steps +========== - Learn more about reviews in @{article:Differential User Guide}; or - learn more about audits in @{article:Audit User Guide}.