Skip to content

Reviewing Pull Requests

Contents:

Review Template

In the main comment for your review, summarize your review using the following template.

# Pass [N] Complete - APPROVED ✅ [🟡 🟢]

# Pass [N] Complete - CHANGES REQUESTED ⚠️ [🔴 🟠 🟡 🟢]

## Summary

- [ ] Reviewed the accompanying Jira ticket to fully understand the scope of work being completed in this PR
- [ ] Thoroughly reviewed the code changes to the system and provided commentary where needed
- [ ] Completed thorough functional testing of the PR to ensure these changes satisfy the specifications in the accompanying Jira ticket
- [ ] Documented all of my findings in this PR review

## Functional, Code Quality, and Related Issues

### Must be addressed (🔴, 🟠) with Linked Github Issues

- [ ] [GITHUB_ISSUE_LINK]

### Can be deferred (🟡 , 🟢 ) with Linked Jira or Github issues

- [ ] [JIRA_OR_GITHUB_ISSUE_LINK]

3 Notes on the Review Template Header

  1. If the review is APPROVED, remove the CHANGES REQUESTED line, and vice versa. Less typing, more better.

  2. Fill in the Pass number, e.g. # Pass [1] - .... This is the number of the times you have reviewed this PR, not the total number of reviews

  3. Remove the dots that don't apply to the PR review. E.g. if changes are requested but there are only 🔴 and 🟢 items, modify the header to indicate this:

# Pass [1] Complete - CHANGES REQUESTED ⚠️ [🔴 🟢]

Example PR Review Summary

Example PR Review

Comment / Issue Classification, a.k.a. "Dot Notation"

Categorize each of your comments with a prefix:

  • 🔴 (:red_circle:) - Functional Issue, needs fixed.
  • 🟠 (:orange_circle:) - Non-functional issue that needs to be fixed with this
  • 🟡 (:yellow_circle:) - Non-functional issue. This should be fixed if time permits, but can be deferred to a tech debt ticket with authorization.
  • 🟢 (:green_circle:) - Question. The comment doesn't represent a problem, and no change is needed. You've just seen something you have questions about. This may lead to other comments or may just be a learning opportunity.

Functional Testing

When functionally testing a PR, be brutal. Try and break it. Put numbers where letters should be. Use Postman to bypass clientside security. Your goal is to break the software without altering the code.

Inline Code Suggestions

Be careful when making or applying automatic code suggestions in GitHub. It's a very useful feature, but a typo can have significant consequences. If code is more complex, consider pairing with the engineer to craft the solution in an IDE.

Reassign & Notify

Explicitly ask for changes or approve

After completing the review in Github, explicitly "Request Changes" or "Approve" the PR. I.e. don't post lone comments isolated from an "actual" review.

Start a review

Why?

  1. Lone comments could be easily missed
  2. The PR cannot be merged without X explicit approvals
  3. If comments require addressing but changes weren't explicitly requested, the PR might accidentally be merged prior to addressing them.

Once your review has been finalized, assign it back to the PR's author and notify them in slack with a ping.

Don't forget this step!

PR Assignment is critical! If a PR is not assigned to the right person, it can and will be missed.

Q&A

Q: Who decides what can be deferred as tech debt vs. should be resolved in this PR?

The reviewer is typically in a position to make this judgement call based on the specific situation, but ultimately it's a discussion.

Q: I made a comment on a line / block of code that's 🔴 or 🟠. Do I still need to create a GitHub Issue?

Probably not, but it can't hurt. If you feel like there's likely going to be a lot of back and forth it can't hurt, but ultimately this is up to reviewer discretion.

Q: I found something that could be deferred (🟡), but it's a quick fix and there are other changes to address already. Should I open a Jira ticket?

Not yet. If the dev is already having to make other changes and this is something minor, it's far more efficient to have them resolve this then. Reserve tech debt tickets for larger / more complex refactoring efforts that jeopardize the turnaround time on the PR.

Q: While testing a PR, I found a bug that is related to the scope of this PR, but was not introduced in this PR. What do I do?

Open a Jira ticket and link the ticket in the review under the "deferred issues" section.

Q: As a reviewer, should I ever just make a change myself rather than returning it with Changes Requested?

This is up to reviewer discretion.

If the only feedback on a PR is a typo, grammar change, or removing a lone piece of commented code, it's usually a better use of everyone's time to make note of it (so the opening dev is aware of the change) and then add a commit to fix it. If the PR is literred with typos, commented code, console.log()s, etc., return it to the opening dev to resolve.