Review Guide

Guide and checklist for those reviewing a Pull Request

Make sure to set yourself as "Busy" on GitHub when you are away for the day so you don't get assigned PRs

Checklist

• Ensure ticket has been added to the PR

• Ensure the ticket is in the "In Review" column

• Check the PR has a list of changes made and screenshots (frontend only)

• Ensure the CI checks have passed

• Make sure the PR is complete

• Check the code for any common issues

• Run the changes locally and compare with the requirements in the ticket, user stories, and design (frontend only)

• Jump on a call with the Author if you are unsure of any part of the PR, or if it is large

If changes are needed

• Add all changes as comments, preferably next to the relevant code

• GitHub allows you to suggest changes, this can help demonstrate the required changes more clearly

Suggesting changes allows you to provide an alternative solution for requested changes

• When you've added all your comments, use the "Request Changes" feature in GitHub to send those comments to the author

• Move the ticket to "Changes Needed" in Asana if requesting changes

Make sure to check back once the changes have been made and rereview. The GitHub App for Slack is good for getting updates on PRs.

Once the PR is good to go

• Approve the PR only once you are happy with it to be merged

• Tag the secondary reviewer in an LGTM (looks good to me) comment

Don't approve a PR unless you are happy for that code to be released to production (except in an emergency hotfix)

Pull Request Flow

1. Author opens a Pull Request to merge their branch into the development branch

2. Author completes the PR checklist:

   1. Add PR to Asana ticket

   2. Add changes to PR

   3. Self review the code

3. GitHub will automatically assign 2-3 people as reviewers

   1. 1-2 Primary reviewers

   2. 1 Secondary reviewer

4. The Primary reviewer(s) will review according to the Checklist

   1. Once happy, the Primary reviewers will approve the PR and tag the Secondary reviewer

5. The Secondary reviewer will quickly skim over the code and any comments, and will approve & merge the PR

Merging PRs

It is up to the Secondary reviewer to merge a PR once it has had at least 2 approvals (including 1 from themself).

Merges should be made using the "Squash commits" strategy to squash all development commits into a single commit.

That single commit must follow the Conventional Commits standard:

The Conventional Commits specification is a lightweight convention on top of commit messages. It provides an easy set of rules for creating an explicit commit history; which makes it easier to write automated tools on top of. This convention dovetails with SemVer, by describing the features, fixes, and breaking changes made in commit messages.

The commit message should be structured as follows:

---

<type>[optional scope]: <description>

[optional body]

[optional footer(s)]

---

The commit contains the following structural elements, to communicate intent to the consumers of your library:

1. fix: a commit of the type fix patches a bug in your codebase (this correlates with PATCH in Semantic Versioning).

2. feat: a commit of the type feat introduces a new feature to the codebase (this correlates with MINOR in Semantic Versioning).

3. BREAKING CHANGE: a commit that has a footer BREAKING CHANGE:, or appends a ! after the type/scope, introduces a breaking API change (correlating with MAJOR in Semantic Versioning). A BREAKING CHANGE can be part of commits of any type.

4. types other than fix: and feat: are allowed, for example @commitlint/config-conventional (based on the the Angular convention) recommends build:, chore:, ci:, docs:, style:, refactor:, perf:, test:, and others.

5. footers other than BREAKING CHANGE: <description> may be provided and follow a convention similar to git trailer format.

Additional types are not mandated by the Conventional Commits specification, and have no implicit effect in Semantic Versioning (unless they include a BREAKING CHANGE). A scope may be provided to a commit’s type, to provide additional contextual information and is contained within parenthesis, e.g., feat(parser): add ability to parse arrays.

Common Issues

Below are some common PR issues. Reviewers should look out for these issues, and request they are fixed unless the Author has a good reason to keep it as is.

1. Code does not comply with the styleguide

2. Code is hard to understand

3. Debugging logs are left in

4. Commented out code has been left in

5. any has been used

6. Logic is duplicated

7. A function has been marked as asynchronous when it does not need to be

Frontend

1. The spacing does not match the design

2. The font size does not match the design or project standard

3. The colour is not correct

4. Navigation or Route props/hooks have not been typed

5. Screens have been placed in the OutsideNavigator for no clear reason

6. Inline styles are used excessively

7. Components are too large

8. Components contain too much business logic

9. API errors are not handled

10. Loading states are missing

Backend

1. An entity has been changed but a migration has not been generated

2. A migration includes changes that are not relevant to this change

3. Tests have not been added/updated

4. Resolvers contain too much business logic

5. A service is interacting directly with another service's entity

6. Fields are missing descriptions