Merge pull request #441 from totten/master-revtpl

standards/review.md - Add templates. Tweak codes.

docs/core/pr-review.md

@@ -87,14 +87,10 @@ An easy way to do this is:
 1. Run `drush civicrm-upgrade-db` to perform database upgrades.
 
 
-## Form an opinion about the fix
-
-* The change should make sense for *all users*.
-* The change should not take users by surprise.
-* Significant changes should add functionality in a generalized way that is configurable.
-
-
 ## Write a review as a comment
 
-Summarize your actions and findings, and recommend specific next steps (e.g. merging or otherwise). In your comment, tag [one of the active contributors](https://github.com/civicrm/civicrm-core/graphs/contributors) (e.g. `@eileenmcnaughton`) so they will see that the PR is ready for further action.
+1. Evaluate the change against each of our [review standards](/standards/review.md) criteria.
+1. If you like, copy-paste one of the [review templates](/standards/review.md#templates) into your comment and fill out the template. 
+    * If you choose not to use a template, then summarize your actions and findings, and recommend specific next steps (e.g. merging or otherwise).
+1. In your comment, tag [one of the active contributors](https://github.com/civicrm/civicrm-core/graphs/contributors) (e.g. `@eileenmcnaughton`) so they will see that the PR is ready for further action.
 

docs/standards/review.md

@@ -6,8 +6,15 @@ When [reviewing a pull-request](/core/pr-review.md), you may consult this list f
 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.
 
-!!! tip
-    The codes below (e.g. `r-jira`) are here to make it easier to reference these standards when chatting with others about PR review.
+!!! tip "Standard codes"
+    Each standard has a code name (e.g. `r-jira`). These make it easier to reference the standards when chatting with others about PR review.
+
+## Templates
+
+You may conduct a structured review, checking each standard in turn. Doing this  will be easier if you copy a template and paste it into your Github comment.
+
+* When conducting your first or second structured review, copy [template-mc-1.0.md](https://raw.githubusercontent.com/civicrm/civicrm-dev-docs/master/docs/standards/review/template-mc-1.0.md). It provides several examples.
+* Once you're familiar with the criteria, copy [template-word-1.0.md](https://raw.githubusercontent.com/civicrm/civicrm-dev-docs/master/docs/standards/review/template-word-1.0.md). It's a bit shorter and quicker.
 
 ## Common standards
 
@@ -37,13 +44,13 @@ Use the code somehow. You don’t need to attack every imaginable scenario in ev
 
 ### User impact {:#r-user}
 
-_Standard code: `r-users`_
+_Standard code: `r-user`_
 
 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?
 
-### Technical impact {:#r-technical}
+### Technical impact {:#r-tech}
 
-_Standard code: `r-technical`_
+_Standard code: `r-tech`_
 
 * Would the patch materially change the contract (signature/pre-condition/post-condition) for APIv3, a hook, a PHP function, a PHP class, a JS widget, or a CSS class?
 * Would you consider the changed element to be an officially supported contract? A de-facto important contract? An obscure internal detail?

docs/standards/review/template-del-1.0.md

+(*CiviCRM Review Template DEL-1.0*)
+
+<!-- In each category, choose the option that most applies. Delete the others. Optionally, provide more details or explanation in the "Comments". -->
+
+* JIRA ([`r-jira`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-jira))
+    * __UNREVIEWED__
+    * __PASS__ : The PR has a JIRA reference. (Or: it does not need one.)
+    * __ISSUE__: Please file a ticket in [JIRA](http://issues.civicrm.org/) and place it in the subject
+    * __COMMENTS__: *(optional)*
+* Test results ([`r-test`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-test))
+    * __UNREVIEWED__
+    * __PASS__: The test results are all-clear.
+    * __PASS__: The test results have failures, but these have been individually inspected and found to be irrelevant.
+    * __ISSUE__: The test failures need to be resolved.
+    * __COMMENTS__: *(optional)*
+* Code quality ([`r-code`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-code))
+    * __UNREVIEWED__
+    * __PASS__: The functionality, purpose, and style of the code seems clear+sensible.
+    * __ISSUE__: Something was unclear to me.
+    * __ISSUE__: The approach should be different.
+    * __COMMENTS__: *(optional)*
+* Run it ([`r-run`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-run))
+    * __UNREVIEWED__
+    * __PASS__: I executed the code in a few plausible ways, and it behaved as expected.
+    * __ISSUE__: I executed the code in a few plausible ways, and it had a problem.
+    * __COMMENTS__: *(optional)*
+* User impact ([`r-user`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-user))
+    * __UNREVIEWED__
+    * __PASS__: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
+    * __ISSUE__: The change would noticeably impact the user-experience (eg requiring retraining).
+    * __PASS__: The change would noticeably impact the user-experience (eg requiring retraining), but this has been addressed with a suitable transition/communication plan.
+    * __COMMENTS__: *(optional)*
+* Technical impact ([`r-tech`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech))
+    * __UNREVIEWED__
+    * __PASS__: The change preserves compatibility with existing callers/code/downstream.
+    * __PASS__: The change potentially affects compatibility, but the risks have been sufficiently managed.
+    * __ISSUE__: The change potentially affects compatibility, and the risks have **not** been sufficiently managed.
+    * __COMMENTS__: *(optional)*
+* Maintainability ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint))
+    * __UNREVIEWED__
+    * __PASS__: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
+    * __PASS__: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
+    * __ISSUE__: The change does not sufficiently improve test coverage. 
+    * __COMMENTS__: *(optional)*
+* Documentation ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc))
+    * __UNREVIEWED__
+    * __PASS__: There are relevant updates for the documentation.
+    * __PASS__: The changes do not require documentation.
+    * __ISSUE__: The user documentation should be updated.
+    * __ISSUE__: The administrator documentation should be updated.
+    * __ISSUE__: The developer documentation should be updated.
+    * __COMMENTS__: *(optional)*

docs/standards/review/template-mc-1.0.md

+(*CiviCRM Review Template MC-1.0*)
+
+<!-- In each category, choose the option that most applies. Optionally, provide more details or explanation in the "Comments". -->
+
+* JIRA ([`r-jira`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-jira))
+    * [ ] __PASS__ : The PR has a JIRA reference. (Or: it does not need one.)
+    * [ ] __ISSUE__: Please file a ticket in [JIRA](http://issues.civicrm.org/) and place it in the subject
+    * [ ] __COMMENTS__: *(optional)*
+* Test results ([`r-test`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-test))
+    * [ ] __PASS__: The test results are all-clear.
+    * [ ] __PASS__: The test results have failures, but these have been individually inspected and found to be irrelevant.
+    * [ ] __ISSUE__: The test failures need to be resolved.
+    * [ ] __COMMENTS__: *(optional)*
+* Code quality ([`r-code`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-code))
+    * [ ] __PASS__: The functionality, purpose, and style of the code seems clear+sensible.
+    * [ ] __ISSUE__: Something was unclear to me.
+    * [ ] __ISSUE__: The approach should be different.
+    * [ ] __COMMENTS__: *(optional)*
+* Run it ([`r-run`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-run))
+    * [ ] __PASS__: I executed the code in a few plausible ways, and it behaved as expected.
+    * [ ] __ISSUE__: I executed the code in a few plausible ways, and it had a problem.
+    * [ ] __COMMENTS__: *(optional)*
+* User impact ([`r-user`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-user))
+    * [ ] __PASS__: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
+    * [ ] __ISSUE__: The change would noticeably impact the user-experience (eg requiring retraining).
+    * [ ] __PASS__: The change would noticeably impact the user-experience (eg requiring retraining), but this has been addressed with a suitable transition/communication plan.
+    * [ ] __COMMENTS__: *(optional)*
+* Technical impact ([`r-tech`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech))
+    * [ ] __PASS__: The change preserves compatibility with existing callers/code/downstream.
+    * [ ] __PASS__: The change potentially affects compatibility, but the risks have been sufficiently managed.
+    * [ ] __ISSUE__: The change potentially affects compatibility, and the risks have **not** been sufficiently managed.
+    * [ ] __COMMENTS__: *(optional)*
+* Maintainability ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint))
+    * [ ] __PASS__: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
+    * [ ] __PASS__: The change does not sufficiently improve test coverage, but special circumstances make it important to accept the change anyway.
+    * [ ] __ISSUE__: The change does not sufficiently improve test coverage. 
+    * [ ] __COMMENTS__: *(optional)*
+* Documentation ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc))
+    * [ ] __PASS__: There are relevant updates for the documentation, or the changes do not require documentation.
+    * [ ] __ISSUE__: The user documentation should be updated.
+    * [ ] __ISSUE__: The administrator documentation should be updated.
+    * [ ] __ISSUE__: The developer documentation should be updated.
+    * [ ] __COMMENTS__: *(optional)*

docs/standards/review/template-word-1.0.md

+(*CiviCRM Review Template WORD-1.0*)
+
+<!-- In each category, change the word "Undecided" to "Pass" or "Issue". Add explanatory comments if prompted or desired. -->
+
+* ([`r-jira`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-jira)) __Undecided__
+* ([`r-test`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-test)) __Undecided__
+* ([`r-code`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-code)) __Undecided__
+* ([`r-doc`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-doc)) __Undecided__
+* ([`r-maint`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-maint)) __Undecided__
+* ([`r-run`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-run)) __Undecided__: (*Describe...*)
+* ([`r-user`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-user)) __Undecided__: (*Describe...*)
+* ([`r-tech`](https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech)) __Undecided__: (*Describe...*)