amitk/PR Etiquette

## PR Etiquette
The purpose of this document is to record an etiquette guide for all pull requests in the any repository. Like all etiquette, it is designed to make life easier for everyone, and if a guideline ever becomes restrictive, we should reevaluate it.

This can generally be applied to other repos managed by Team 105.

# Contents

1. [Size](#size)
2. [Sequential PRs](#sequential-prs)
3. [Labels](#labels)
4. [Branch Names](#branch-names)
5. [Before opening or updating a PR](#before-opening-or-updating-a-pr)
6. [Opening a new PR](#opening-a-new-pr)
7. [Reviewing](#reviewing)
8. [Before Merging](#before-merging)
9. [Checklist](#checklist)

# Size
Please keep pull requests as small as is reasonably possible. Size is a measure of number of files change, number of lines changes per file, and complexity.

In this case, complexity refers to how difficult it is to understand a the changes in the PR (as opposed to more qualitative measurements like cyclomatic complexity). For example, adding a new attribute to an existing resource touches the following places:

1. API Documents
2. Controllers/Params files
3. Serializers
4. Controller specs
5. Params specs
6. Migrations
7. Schema file
8. Models
9. Model specs
10. OpenAPI YAML file
11. Scenarios
12. Step definitions

Adding an attribute touches roughly 12 files. Yet, from a complexity standpoint, it is fairly simple. It does one thing. It could be made simpler by extracting the openapi.yaml and Cucumber-related changes to their own PR. It could be further simplified by extracting model- and migration-layer code to its own PR, too.

Keeping complexity low is an important part of making a PR easy to review. Remember that a reviewer can look at your code many times before the PR is merged and does not have the benefit of being immersed in the code the entire time.

If any housekeeping changes are being made that affect more than 1 or 2 specific spots in the code, they are better done in a separate PR. For example, if I notice that a path through the controllers has been ignored in the tests that's unrelated to my current PR, those tests should be added as a separate PR.

## Sequential PRs
To keep pull request size small, they can be arranged in a sequence. In the above example about adding an attribute, if the model and factory are added in PR 1 and the controller and params code is added in PR 2, then PR 2 must include the code in PR 1. When looking at the diff of merging PR 2 into develop, reviewers will see the changes from PR 1.

The solution is to create PR to as dependant on PR 1. This means the following should be true:
1. Both PR 1 and PR 2 should contain a number following the Jira story number in the title. (e.g. "MCC-610578 10 - Fixes Publish and Push Bugs" from PR #1230)
2. PR 2 should have the label "Has PR Dependency" (see below) until PR 1 is merged.
3. PR 2 should include a link to PR 1 in the description. This can be done by using the number sign followed by the PR number (e.g. #499). GitHub will take care of the link.
4. When PR 1 is merged, you must merge the develop branch into PR 2's branch and then push the PR 2 branch to GitHub. This will update the PR diff to only show new changes from PR 2.
5. PR 2 should NOT have the label "Pending Review" (see below) until PR 1 is merged. When PR 1 is merged, update the labels by removing the "Has PR Dependency" label and adding the "Pending Review" label.

This process can be continued as many times as necessary with as many concurrently open PRs as needed.


# Labels
Please use labels for pull requests. Some labels describe PR status, while others highlight the target of the PR. Other labels can be added on a per-project basis as needed.

The following status labels are standard:
- **Pending Review** - This PR has had no activity on it and is waiting for review. This is also known as "Feedback requested" in Broadacre and "Waiting for Review" in Usonia.
- **Open Comments to Address** - This PR has been reviewed by at least one person who has made comments that have yet to be addressed by the PR owner. This is also known as "Open Questions to Address" in Broadacre
- **All Comments Addressed** - All of the reviewer comments in this PR have been addressed by the PR owner. It is ready to be further reviewed or merged. This is also known as "Comments addressed" in Broadacre
- **Has PR Dependency** - This PR includes the code from another and can be ignored until that PR is merged. The PR description should include the number of the PR on which this PR depends.
- **DO NOT MERGE** - This PR is not suitable for merging. An explanation should be posted in the PR summary. These are generally ignored by reviewers, but if you want someone to review your PR, please reach out to the reviewer directly outside of GitHub.

Combinations of labels can be used. For example, using "Pending Review" and "Open Comments to Address" means that a PR does have some comments that need to be addressed, but the owner is still hoping for more comments to come in.

The following target labels are standard:
- **Target: Release Branch** - The target of this PR is the release branch. Only suitable after a release branch has been created and before the release branch has been merged into master.
- **Target: Master** - The target of this PR is the master branch. This should only happen after a release to production, once all smoke tests are passing.


# Branch Names
All branches should go in one of the following "folders" (i.e., prepend the name and a slash to the front of the branch name):
* `feature` - Code that addresses stories and technical tasks
* `fix` - Code that addresses bugs
* `experimental` - Code that is being experimented with, that may or may not be intended for merging into develop
* `release` - Only for use when partitioning off code for release.

As a standard procedure, please include the Jira MCC number of the branch name as the first thing after the "folder". Optionally, place any sequence numbers after the MCC number. Place any branch description after the MCC number or sequence number (if included). The descriptions can be whatever you want. For example, branch `feature/MCC-392137_hard_deletes_to_soft_deletes` is the branch that addresses Jira story MCC-392137 by turning hard deletes into soft deletes.

Multiple branches that address the same story is fine. Just make sure that the "folder" and Jira number are the same.


# Before opening or updating a PR

The goal should always be to reduce the number of review iterations, so the fewer times the code is submitted for review the better. Thus, it's always better for the PR owner to catch simple changes. We all miss simple changes. This document likely has some typos, but it's good to catch as many as you can.

* Proofread your code diff. Look for things like debugging comments or breakpoints left in, misspelled words, code that could be refactored to make it clearer or more concise, etc.
  * Proofread EVERY TIME you submit code to be reviewed or re-reviewed. If this is too cumbersome or takes too long, it's a good indication that your PR is too long and you should consider breaking it up.
* Run CI locally using `COVERAGE=true bundle exec rake ci`. Fix any changes before pushing.
* Run Rubocop locally using `bundle exec rake rubocop`. Fix anything it catches or reach out to the team for group consensus changing the rules.
* Run Hammertime linter locally using `bundle exec hammertime lint` if you made any changes to openapi.yaml.
* Don't run CI unnecessarily. If you are pushing an update to an PR and know that you'll be pushing again shortly, put `[skip ci]` at the beginning of the commit comment. That will tell Travis not to run.
* When updating a PR due to a reviewer comment, reply to the reviewer's comment with the commit hash where the comment was addressed.
* When all comments have been addressed, please update the label to **All Comments Addressed**
* As the PR author, do not resolve comments yourself. Leave that to the commenter.


# Opening a new PR
When opening a new PR, please follow the following steps:

* Make sure the first part of the title includes the JIRA number, either alone or in square brackets
* Assign it to yourself
* Give a meaningful summary of the goals of the code changes. Note any future work that you know will need to be done. For example, if you're going to be doing a separate PR for the contract, please say so in the summary.
* Complete the PR checklist in the description. Not every item will be completed for every PR. If they are all complete in a single PR, probably the PR is too big.
* Optional: Request a review from one or more of the team members.
* Tag (@username) any non-standard reviewer who may be interested. For example:
  * In a PR that includes a deploy guide update, please tag our Service Delivery engineer.
  * In a PR that include a contract update, please tag the front-end team.
  * If a change was requested by a particular person, please tag that person specifically.
* Label the PR **Pending Review**.
* If your branch target is the release branch, please tag it accordingly


# Reviewing
Check regularly for new PRs and for status updates to existing PRs, ideally two to three times a day.

Try to review as many PRs as possible. While not everyone is expected to review all PRs, exposing yourself to other PRs is a great way to improve your own coding skills and help your teammates in the process.

For small issues, try to keep comments in PRs, not Slack or email. However, if a discussion becomes too big or cumbersome for PR comments, feel free to discuss it in Slack or a meeting. Once the conversation is done, the PR owner should summarize the outcome of the conversation so that there's a record of the decision in the PR.

The process I follow is this:
* Read the description to find out what the PR is trying to accomplish.
* Read through the app code changes to see how those changes accomplish what the description says.
* Check each method to make sure the method does what the method name says or implies it does.
* Once the app code has been seen, pull up a separate browser window and look side-by-side at the app code and the unit test for the app code. Make sure each code path is covered. Remember that in-line `if`s and `or`s look like a single path but are each at least 2 paths.
* Always be checking for style, file organization, variable/method names, etc.
* Call out any minor issues as being minor. This is code review. It's okay to nitpick other people's work, but it's often nice to let other people know that you understand that you're nitpicking.
* If a suggestion is optional, note it as "non-blocking", meaning the change won't hold up the PR. The owner is free to make the change or not.
* If a suggestion can be done as a separate PR -- for example, if documentation is missing -- feel free to say so.
* If a PR author has satisfactorily responded to your comment, either by changing the code or by convincingly arguing that your change is not needed, resolve the comment. That will tell other reviewers that you are satisfied with the resolution.
* Above all else, be "kind, but honest". It's a phrase used the Medidata architecture review boards, and it's a great philosophy to have in PR reviews.


# Before Merging
* Check all comments, even the ones that are buried by GitHub to make sure they are addressed.
* In order to merge, every team member who commented must **explicitly** sign off on the code. "Just fix ... and it'll be good" is not enough.
  * If for whatever reason a reviewer on the team is later unavailable to re-review (e.g. PTO), please send that reviewer an email with a link to the PR so that they may examine it when they return. Please attempt to see that the original reviewer's request is addressed, even if that means rejecting the reviewer's suggested changes.
  * If the reviewer is on a team outside of eConsent, please give them a chance to review the changes, but the explicit sign-off is not required to merge.
* Unless the changes are trivial (e.g. a small documentation change), the approval of at least two reviewers is required to merge.
  * In cases of trivial changes, only one reviewer is necessary.
  * In all cases, 100% of reviewers must agree to merge the PR.
* NEVER MERGE YOUR OWN PULL REQUEST.


# Checklist
The following comments are relatively simple to fix and frequently made, so please look into these both when writing your own code and when reviewing others' code:

* When testing controller routes that write to the database, make sure that conditions that return error codes do not lead to changes in the data. (e.g. a 403 on a create route should not create anything in the database)
* If code changes, some test should be failing to make that change happen.
* Update scenario tags (i.e. add MCC tag and Validation -> Review[SQA]). If you have any questions about the process, please ask.
* Don't use `let!` when `let` will work.
* Make sure there are no keys or passwords checked into the repository. This refers to anywhere in the commit history, not just at the Git HEAD.
  * If any keys or passwords have been checked into the commit history anywhere, they must be expunged as soon as possible and new keys or passwords generated. This means anywhere in the history, not just the head.
	The purpose of this document is to record an etiquette guide for all pull requests in the any repository. Like all etiquette, it is designed to make life easier for everyone, and if a guideline ever becomes restrictive, we should reevaluate it.

	This can generally be applied to other repos managed by Team 105.

	# Contents

	1. [Size](#size)
	2. [Sequential PRs](#sequential-prs)
	3. [Labels](#labels)
	4. [Branch Names](#branch-names)
	5. [Before opening or updating a PR](#before-opening-or-updating-a-pr)
	6. [Opening a new PR](#opening-a-new-pr)
	7. [Reviewing](#reviewing)
	8. [Before Merging](#before-merging)
	9. [Checklist](#checklist)

	# Size
	Please keep pull requests as small as is reasonably possible. Size is a measure of number of files change, number of lines changes per file, and complexity.

	In this case, complexity refers to how difficult it is to understand a the changes in the PR (as opposed to more qualitative measurements like cyclomatic complexity). For example, adding a new attribute to an existing resource touches the following places:

	1. API Documents
	2. Controllers/Params files
	3. Serializers
	4. Controller specs
	5. Params specs
	6. Migrations
	7. Schema file
	8. Models
	9. Model specs
	10. OpenAPI YAML file
	11. Scenarios
	12. Step definitions

	Adding an attribute touches roughly 12 files. Yet, from a complexity standpoint, it is fairly simple. It does one thing. It could be made simpler by extracting the openapi.yaml and Cucumber-related changes to their own PR. It could be further simplified by extracting model- and migration-layer code to its own PR, too.

	Keeping complexity low is an important part of making a PR easy to review. Remember that a reviewer can look at your code many times before the PR is merged and does not have the benefit of being immersed in the code the entire time.

	If any housekeeping changes are being made that affect more than 1 or 2 specific spots in the code, they are better done in a separate PR. For example, if I notice that a path through the controllers has been ignored in the tests that's unrelated to my current PR, those tests should be added as a separate PR.

	## Sequential PRs
	To keep pull request size small, they can be arranged in a sequence. In the above example about adding an attribute, if the model and factory are added in PR 1 and the controller and params code is added in PR 2, then PR 2 must include the code in PR 1. When looking at the diff of merging PR 2 into develop, reviewers will see the changes from PR 1.

	The solution is to create PR to as dependant on PR 1. This means the following should be true:
	1. Both PR 1 and PR 2 should contain a number following the Jira story number in the title. (e.g. "MCC-610578 10 - Fixes Publish and Push Bugs" from PR #1230)
	2. PR 2 should have the label "Has PR Dependency" (see below) until PR 1 is merged.
	3. PR 2 should include a link to PR 1 in the description. This can be done by using the number sign followed by the PR number (e.g. #499). GitHub will take care of the link.
	4. When PR 1 is merged, you must merge the develop branch into PR 2's branch and then push the PR 2 branch to GitHub. This will update the PR diff to only show new changes from PR 2.
	5. PR 2 should NOT have the label "Pending Review" (see below) until PR 1 is merged. When PR 1 is merged, update the labels by removing the "Has PR Dependency" label and adding the "Pending Review" label.

	This process can be continued as many times as necessary with as many concurrently open PRs as needed.


	# Labels
	Please use labels for pull requests. Some labels describe PR status, while others highlight the target of the PR. Other labels can be added on a per-project basis as needed.

	The following status labels are standard:
	- Pending Review - This PR has had no activity on it and is waiting for review. This is also known as "Feedback requested" in Broadacre and "Waiting for Review" in Usonia.
	- Open Comments to Address - This PR has been reviewed by at least one person who has made comments that have yet to be addressed by the PR owner. This is also known as "Open Questions to Address" in Broadacre
	- All Comments Addressed - All of the reviewer comments in this PR have been addressed by the PR owner. It is ready to be further reviewed or merged. This is also known as "Comments addressed" in Broadacre
	- Has PR Dependency - This PR includes the code from another and can be ignored until that PR is merged. The PR description should include the number of the PR on which this PR depends.
	- DO NOT MERGE - This PR is not suitable for merging. An explanation should be posted in the PR summary. These are generally ignored by reviewers, but if you want someone to review your PR, please reach out to the reviewer directly outside of GitHub.

	Combinations of labels can be used. For example, using "Pending Review" and "Open Comments to Address" means that a PR does have some comments that need to be addressed, but the owner is still hoping for more comments to come in.

	The following target labels are standard:
	- Target: Release Branch - The target of this PR is the release branch. Only suitable after a release branch has been created and before the release branch has been merged into master.
	- Target: Master - The target of this PR is the master branch. This should only happen after a release to production, once all smoke tests are passing.


	# Branch Names
	All branches should go in one of the following "folders" (i.e., prepend the name and a slash to the front of the branch name):
	* `feature` - Code that addresses stories and technical tasks
	* `fix` - Code that addresses bugs
	* `experimental` - Code that is being experimented with, that may or may not be intended for merging into develop
	* `release` - Only for use when partitioning off code for release.

	As a standard procedure, please include the Jira MCC number of the branch name as the first thing after the "folder". Optionally, place any sequence numbers after the MCC number. Place any branch description after the MCC number or sequence number (if included). The descriptions can be whatever you want. For example, branch `feature/MCC-392137_hard_deletes_to_soft_deletes` is the branch that addresses Jira story MCC-392137 by turning hard deletes into soft deletes.

	Multiple branches that address the same story is fine. Just make sure that the "folder" and Jira number are the same.


	# Before opening or updating a PR

	The goal should always be to reduce the number of review iterations, so the fewer times the code is submitted for review the better. Thus, it's always better for the PR owner to catch simple changes. We all miss simple changes. This document likely has some typos, but it's good to catch as many as you can.

	* Proofread your code diff. Look for things like debugging comments or breakpoints left in, misspelled words, code that could be refactored to make it clearer or more concise, etc.
	* Proofread EVERY TIME you submit code to be reviewed or re-reviewed. If this is too cumbersome or takes too long, it's a good indication that your PR is too long and you should consider breaking it up.
	* Run CI locally using `COVERAGE=true bundle exec rake ci`. Fix any changes before pushing.
	* Run Rubocop locally using `bundle exec rake rubocop`. Fix anything it catches or reach out to the team for group consensus changing the rules.
	* Run Hammertime linter locally using `bundle exec hammertime lint` if you made any changes to openapi.yaml.
	* Don't run CI unnecessarily. If you are pushing an update to an PR and know that you'll be pushing again shortly, put `[skip ci]` at the beginning of the commit comment. That will tell Travis not to run.
	* When updating a PR due to a reviewer comment, reply to the reviewer's comment with the commit hash where the comment was addressed.
	* When all comments have been addressed, please update the label to All Comments Addressed
	* As the PR author, do not resolve comments yourself. Leave that to the commenter.


	# Opening a new PR
	When opening a new PR, please follow the following steps:

	* Make sure the first part of the title includes the JIRA number, either alone or in square brackets
	* Assign it to yourself
	* Give a meaningful summary of the goals of the code changes. Note any future work that you know will need to be done. For example, if you're going to be doing a separate PR for the contract, please say so in the summary.
	* Complete the PR checklist in the description. Not every item will be completed for every PR. If they are all complete in a single PR, probably the PR is too big.
	* Optional: Request a review from one or more of the team members.
	* Tag (@username) any non-standard reviewer who may be interested. For example:
	* In a PR that includes a deploy guide update, please tag our Service Delivery engineer.
	* In a PR that include a contract update, please tag the front-end team.
	* If a change was requested by a particular person, please tag that person specifically.
	* Label the PR Pending Review.
	* If your branch target is the release branch, please tag it accordingly


	# Reviewing
	Check regularly for new PRs and for status updates to existing PRs, ideally two to three times a day.

	Try to review as many PRs as possible. While not everyone is expected to review all PRs, exposing yourself to other PRs is a great way to improve your own coding skills and help your teammates in the process.

	For small issues, try to keep comments in PRs, not Slack or email. However, if a discussion becomes too big or cumbersome for PR comments, feel free to discuss it in Slack or a meeting. Once the conversation is done, the PR owner should summarize the outcome of the conversation so that there's a record of the decision in the PR.

	The process I follow is this:
	* Read the description to find out what the PR is trying to accomplish.
	* Read through the app code changes to see how those changes accomplish what the description says.
	* Check each method to make sure the method does what the method name says or implies it does.
	* Once the app code has been seen, pull up a separate browser window and look side-by-side at the app code and the unit test for the app code. Make sure each code path is covered. Remember that in-line `if`s and `or`s look like a single path but are each at least 2 paths.
	* Always be checking for style, file organization, variable/method names, etc.
	* Call out any minor issues as being minor. This is code review. It's okay to nitpick other people's work, but it's often nice to let other people know that you understand that you're nitpicking.
	* If a suggestion is optional, note it as "non-blocking", meaning the change won't hold up the PR. The owner is free to make the change or not.
	* If a suggestion can be done as a separate PR -- for example, if documentation is missing -- feel free to say so.
	* If a PR author has satisfactorily responded to your comment, either by changing the code or by convincingly arguing that your change is not needed, resolve the comment. That will tell other reviewers that you are satisfied with the resolution.
	* Above all else, be "kind, but honest". It's a phrase used the Medidata architecture review boards, and it's a great philosophy to have in PR reviews.


	# Before Merging
	* Check all comments, even the ones that are buried by GitHub to make sure they are addressed.
	* In order to merge, every team member who commented must explicitly sign off on the code. "Just fix ... and it'll be good" is not enough.
	* If for whatever reason a reviewer on the team is later unavailable to re-review (e.g. PTO), please send that reviewer an email with a link to the PR so that they may examine it when they return. Please attempt to see that the original reviewer's request is addressed, even if that means rejecting the reviewer's suggested changes.
	* If the reviewer is on a team outside of eConsent, please give them a chance to review the changes, but the explicit sign-off is not required to merge.
	* Unless the changes are trivial (e.g. a small documentation change), the approval of at least two reviewers is required to merge.
	* In cases of trivial changes, only one reviewer is necessary.
	* In all cases, 100% of reviewers must agree to merge the PR.
	* NEVER MERGE YOUR OWN PULL REQUEST.


	# Checklist
	The following comments are relatively simple to fix and frequently made, so please look into these both when writing your own code and when reviewing others' code:

	* When testing controller routes that write to the database, make sure that conditions that return error codes do not lead to changes in the data. (e.g. a 403 on a create route should not create anything in the database)
	* If code changes, some test should be failing to make that change happen.
	* Update scenario tags (i.e. add MCC tag and Validation -> Review[SQA]). If you have any questions about the process, please ask.
	* Don't use `let!` when `let` will work.
	* Make sure there are no keys or passwords checked into the repository. This refers to anywhere in the commit history, not just at the Git HEAD.
	* If any keys or passwords have been checked into the commit history anywhere, they must be expunged as soon as possible and new keys or passwords generated. This means anywhere in the history, not just the head.