diff --git a/docs/core/pr-review.md b/docs/core/pr-review.md new file mode 100644 index 0000000000000000000000000000000000000000..650ecfa34c7680b4b27c5a4e358a6646c96dfaf7 --- /dev/null +++ b/docs/core/pr-review.md @@ -0,0 +1,72 @@ +# Reviewing a PR + +You Too, can be a CiviCRM PR reviewer! +You do not need special access or merge rights on the CiviCRM gitHub organization. +What you do need, is… +* GitHub Account +* A CiviCRM Development Environment (this might be optional, but good to have). One benefit is the ability to check out the PR in your environment. +* Comfort reading code and patches + +## Pick a PR +You should see if there has already been comments on the PR and if the PR looks ready to be reviewed? Have suggested changes already been made? + +Be aware of flags such as “WIP†- work in progress. + +Look for the green check-mark or the red-X. If the automated build tests have failed, then the PR is not ready for review. The PR submitter should have received a notification of this. + +Merge-conflicts also indicate the PR is not ready for review. + +**Claim the PR by commenting on it that you are reviewing it.** + +## Read about the Issue +Every PR *should* have an issue ID for [https://issues.civicrm.org](https://issues.civicrm.org). +Read the original issue and understand how to reproduce the problem and what the solution looks like as well. + +## Read the code changes +On the PR, click over to “Files Changed†and understand what the code is doing. + +## Does this change make sense for EVERYONE? +Is this really a bug? Or is it just outside of the submitter’s use-case? +Will this change take users by surprise? Does it alter defaults? Does it add functionality in a generalized way that is configurable? + + +## Quality +* Is the code readable, and therefore maintainable by the next developer that has to work with it? +* Does it follow best practices? *(whose best practice?)* +* Are there tests needed for this change? +* If you think this is an appropriate change to core, then you can proceed to validate the PR. + + +## Reproduce the problem +Confirm which branch the PR was created against. +This is probably either Master or the LTS. +Setup an instance from that branch. Buildkit (civibuild) is probably how you want to do this. +Repeat the steps to reproduce described in the Jira Issue. + + +... + +rough notes follow +Patches Welcome. + + +Navigate to the build + +Confirm that the issue was a problem and a problem “worth solving†+… generally worthy of being in core… applies to all users + +Review the Jira Issue. + +Confirm that the PR works as advertized by observing the result in the build. + +Confirm that the issue was reproducible in a build of master. + + +Review the code … + makes sense + red flags + + + +“@†mention a core team members and report the findings of the review and recommend action… + diff --git a/mkdocs.yml b/mkdocs.yml index 3baa8d2e2cd83663e4c243581c9a9d5e02cd8ebb..e75e479c6810952bc78f56859bd796ab38276668 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -35,6 +35,7 @@ pages: - Core Development: - When to Edit Core: core/hacking.md # page-tree = NEED_PAGE_MOVE to /core/deciding.md - How to Contribute: core/contributing.md # page-tree = DONE + - Reviewing PR's: core/pr-review.md - Codebase & Architecture: core/architecture.md # page-tree = DONE # Reporting Bugs & Issues: /core/reporting.md # page-tree = NEED_NEW_PAGE # Submitting a Patch: /core/patches.md # page-tree = NEED_NEW_PAGE