Pull Requests

What are the goals of a PR? 🔗

Goals in order of importance:

1. Ensure correctness
2. Spread and share knowledge
3. Improve the design
4. Ensure consistency

Guide for authors 🔗

Who do I assign as a reviewer? 🔗

• Default to a single reviewer
• Reviewer should be selected based on:
  • Knowledge of reviewer (for example, if changing code related to events, @johngallagher has done work in this area)
  • Skills of reviewer (for example, @karlentwistle is skilled with performance testing)
  • Use git blame and/or ask the #engineering Slack channel, if you’re unsure
• If other developers have more knowledge than the reviewer, the reviewer can pull them into the conversation
• For PRs in patterns, always assign the whole of Engineering

Timescales 🔗

• The reviewer should give you a timescale when you can expect your PR to be reviewed.
• This means you can plan your day
• Look for a comment in the PR

Scope creep 🔗

If you get feedback that’s outside the scope of the ticket:

1. Create another ticket. Kindly ask the reviewers to help add more details to the ticket if they are more familiar with the requirements.
2. Link to the ticket in a comment then resolve the comment

What should be included in a PR? 🔗

PRs should be small and focussed.

Make sure you include everything on the PR Checklist

Guide for reviewers 🔗

What’s the mindset? 🔗

1. To check the author’s work for correctness
2. To prompt for better ways at approaching the problem
3. To look at the problem through fresh eyes
4. To encourage good design and progress of the codebase
5. To ask for the PR to be small enough so they can productively review it
6. To look for potentially missing automated tests

Timescale 🔗

• When asked to review a PR, please let the author know when you’ll be able to review it
• Send a Slack message to the author with the expectation, for example:

@authorname I’ll review this at 10am GMT Friday 13th March

Scope creep 🔗

• If you give feedback that’s outside the scope, bear in mind it won’t necessarily be addressed within this PR
• The author should create a ticket for this follow-up work, post a link, and resolve the comment. You might be requested to add more details to the ticket in case you’re more familiar with its requirements.

What should be included in a PR? 🔗

PRs should be small and focussed.

It’s your right to ask for the PR to be broken down.

Check everything on the PR Checklist

PR Checklist 🔗

Title 🔗

1. Clear and detailed title

2. Title and body includes Jira ticket number e.g. [BIG-6523] or [no-ticket]

Description 🔗

1. Contains link to Jira ticket

2. The why will help us understand the change 6 months from now

3. The how is detailed and includes links to the relevant source code

4. Manual test results included

Code 🔗

1. Tested on a review app or locally

2. Adequate automated test coverage of changes made

3. Author has reviewed their own PR

4. Live review scheduled if change is non trivial

For UI changes 🔗

1. PR includes before and after videos or screenshots

2. Tested on the supported browsers, videos or screenshots included in the PR

For defects 🔗

1. Regression test has been written that replicates the bug

For Segment tracking 🔗

1. Tested events coming through via the Segment debugger

2. Videos or screenshots taken of the events asserting what you expect - see example

During Review 🔗

1. Author responses to reviewer comments are clear

2. When feedback has been actioned, the commit sha of the fix is in the comment and the comment has been resolved

3. Every reviewer comment has been responded to and resolved

Example of a great PR 🔗

[BIG-123] Add authentication support for dashboard 🔗

What 🔗

I’ve added support for authentication to implement the dashboard. It includes model, table, controller and test.

Why 🔗

These changes complete the user login and account creation experience. See #JIRA-123 for more information.

How 🔗

This includes a migration, model and controller for user authentication. I’m using Devise to do the heavy lifting. I ran Devise migrations and those are included here.

Testing 🔗

🟢 When user logs in they are shown the dashboard 🔗

✅ User is shown dashboard ✅ Logs indicate success

🟢 When a guest user visits the dashboard they are redirected to the home page 🔗

✅ User is shown home page ✅ Warning shown in logs

Browser Testing 🔗

Mobile 🔗

Desktop 🔗

Screenshots 🔗

<screenshots of the user logged in>

Anything Else? 🔗

Let’s consider using a 3rd party authentication provider for this, to offload MFA and other considerations as they arise and as the privacy landscape evolves.

AWS Cognito is a good option, so is Firebase. I’m happy to start researching this path.

Let’s also consider breaking this out into its own service. We can then re-use it or share the accounts with other apps in the future.