diff --git a/docs/core/pr-review.md b/docs/core/pr-review.md index 56e340b1fc54987cfa4998fdf7b36f709c633772..14a41785bc54e8b7c341dee2f92fe02c9867c1fa 100644 --- a/docs/core/pr-review.md +++ b/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. diff --git a/docs/standards/review.md b/docs/standards/review.md index 0354e0ee5ca098be7e6e14382a2af6f13ee43c7a..33ad5811870d400d83f99a99312597181071c1e5 100644 --- a/docs/standards/review.md +++ b/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? diff --git a/docs/standards/review/template-del-1.0.md b/docs/standards/review/template-del-1.0.md new file mode 100644 index 0000000000000000000000000000000000000000..56a6c0df22da00027480143fee959c177ef98ba7 --- /dev/null +++ b/docs/standards/review/template-del-1.0.md @@ -0,0 +1,52 @@ +(*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)* diff --git a/docs/standards/review/template-mc-1.0.md b/docs/standards/review/template-mc-1.0.md new file mode 100644 index 0000000000000000000000000000000000000000..21c345ba652ed959f1c5a2447dc69de14307d359 --- /dev/null +++ b/docs/standards/review/template-mc-1.0.md @@ -0,0 +1,43 @@ +(*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)* diff --git a/docs/standards/review/template-word-1.0.md b/docs/standards/review/template-word-1.0.md new file mode 100644 index 0000000000000000000000000000000000000000..f36711cf6cd8927c299e5cb72ee73396333b7968 --- /dev/null +++ b/docs/standards/review/template-word-1.0.md @@ -0,0 +1,12 @@ +(*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...*)