From 71e062a0ac208837278120c0b813e3ec9d780617 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 10 Oct 2017 13:35:53 +0100
Subject: [PATCH] Add "Review Standards"

---
 docs/standards/review.md | 67 ++++++++++++++++++++++++++++++++++++++++
 mkdocs.yml               |  1 +
 2 files changed, 68 insertions(+)
 create mode 100644 docs/standards/review.md

diff --git a/docs/standards/review.md b/docs/standards/review.md
new file mode 100644
index 00000000..c0f7bc04
--- /dev/null
+++ b/docs/standards/review.md
@@ -0,0 +1,67 @@
+The Review Standards provide a name and description for common review tasks.
+
+## Usage
+
+When reviewing a pull-request, you may consult this list for ideas/inspiration on things to check.  If you find a problem or feel that some QA task remains to
+be done, then it can help to post a link to the relevant guideline.  This practice allows newcomers to understand the critique, but it doesn't require you to
+write a long, bespoke blurb.
+
+## Common
+
+### Ensure that the PR links to a JIRA issue. {:#r-jira}
+
+For most bug-fixes and improvements, there needs to be a JIRA issue. However, small NFC PRs and some WIP PRs may not need that. (***R-JIRA***)
+
+### Examine test results. {:#r-test}
+
+If the automated test comes back with any failures, look into why. Hopefully, Jenkins provides an itemized list of failures. If not, dig further into the "Console" output for PHP-fatals or build-failures. (***R-TEST***)
+
+### Read the code. {:#r-read}
+
+Is it understandable? Does it follow common conventions? Does it fit in context? If it changes a difficult section of code -- does it tend to make that section better or worse? (***R-READ***)
+
+### Try it out. {:#r-run}
+
+Use the code somehow. You don’t need to attack every imaginable scenario in every PR, but you should do something to try it out. Be proportionate. (***R-RUN***)
+
+### Assess impact on users. {:#r-users}
+
+If a user was comfortable using the old revision, would they upgrade and assimilate naturally and unthinkingly to the new revision? If not, has there been commensurate effort to provide a fair transition-path and communication? (***R-USERS***)
+
+### Assess impact on extensions/integrations. {:#r-ext}
+
+Would the proposal change the behaviors/inputs/outputs of an API, hook, or widely-used function? If an existing extension uses it, would it continue to work the same way? If you're unsure, consider grepping universe for inspiration. If there is a foreseeable problem, has there been commensurate effort to communicate change and provide a fair transition path? (***R-EXT***)
+
+### Assess impact on core. {:#r-core}
+
+Would the patch change the contract for a PHP function or JS widget or CSS class? If so, have you verified that there are no stale/dangling references based on the old contract? Look for unexpected side-effects. (***R-CORE***)
+
+### Check for tests or maintainability. {:#r-maint}
+
+Many changes should introduce some kind of automated test or protective measure to ensure maintainability. However, there can be tricky cost/benefit issues, and the author and reviewer must exercise balanced judgment. (***R-MAINT***)
+
+## Gotchas
+
+### Packaging {:#rg-pkg}
+
+If the PR adds a new top-level file, new top-level folder, or novel file-type, consider whether "distmaker" will properly convey the file in *.zip/*.tar.gz builds. (***RG-PKG***)
+
+### Permissions {:#rg-perm}
+
+If the PR changes the permissions model, are we sure that demo/test builds and existing installations will continue to work the same? (***RG-PERM***)
+
+### Security {:#rg-sec}
+
+If the PR passes data between different tiers (such as SQL/PHP/HTML/CLI), is this data escaped and validated correctly? Or would it be vulnerable to SQL-injections, cross-site scripting, or similar? (***RG-SEC***)
+
+### Settings {:#rg-setting}
+
+If the PR adds or removes a setting, will existing deployments or build-scripts which reference the setting continue to work as expected? (***RG-SETTING***)
+
+### Schema {:#rg-schema}
+
+If the PR changes the DB, ensure new installations and upgraded installations will end up with consistent schema. (Extra: If it's a backport, take extra care to consider all upgrade paths.) (***RG-SCHEMA***)
+
+### Hook signature {:#rg-hook}
+
+Adding a new parameter to an existing hook may be syntactically safe, but is it semantically safe? (***RG-HOOK***)
diff --git a/mkdocs.yml b/mkdocs.yml
index 318acfb0..6513ed1a 100644
--- a/mkdocs.yml
+++ b/mkdocs.yml
@@ -237,6 +237,7 @@ pages:
   - PHP Standards: standards/php.md
   - Javascript Reference: standards/javascript.md
   - Database Standards: standards/database.md
+  - Review Standards: standards/review.md
 - Documentation:
   - Writing Documentation: documentation/index.md
   - Documenting Extensions: documentation/extensions.md
-- 
GitLab