How to prepare and review pull requests
Pull requests (PRs) happen when we want to merge a working branch into another branch. Many of our pull requests merge new features or fixes into dev. We also use pull requests to release changes by merging to staging (for final checks), then master (the live site).
We use PRs to review, validate, and discuss work. We sometimes use work-in-progress PRs, denoted by WIP, to share and collaborate on ongoing work before we're ready to merge.
For PRs that are potentially ready for merging, there are some steps we generally follow to make sure all goes well.
- Set the PR up to go from your current branch into
dev. Ensure that you have selected the branch that you intend to merge under the “compare” dropdown. Thedevbranch is the base default, so you don’t need to change that.
- Following the PR template, fill in the PR with the GitHub issue(s) it addresses, a preview link, and a description of the changes.
- Assign at least one other team member to review that PR. You should assign the team member(s) with the most relevant knowledge and skill set:
- Code related changes should have at least either the developer or content strategist review them.
- Content related changes should have at least the content strategist, project manager, or program analysts review them.
- PRs related to a new design should have the user experience designer review them.
- Visual design related PRs should have at least the content strategist or user experience designer review them.
- Did CircleCI tests pass? Did the site build in Federalist? (These can take 2-10 minutes, so don't panic if they're not immediate.) If either failed, proceed with extra caution and pull in a technical reviewer.
- Click on the preview link (either in the PR summary or by clicking "details" next to the Federalist build tracker below). (If there’s no preview link, use the format
https://federalist-proxy.app.cloud.gov/preview/onrr/doi-extractives-data/BRANCH-NAME/to find the preview.) Does everything look as you expect? If you expect widespread changes (or are worried there may be unintentionally widespread changes), here's a list of what to check. - Navigate to any pages in the preview you expect to be different because of the changes, and verify that the changes look right. Also check any pages that might be incidentally affected, and make sure they look as you expect (especially for style or template changes).
- Look at the "Files changed" tab in the pull request. Are these the files you expect to have changed? Do the changes make sense and seem intentional? If there is an unexpectedly long list of files changed (for instance, 259 files changed for a typo fix), pull in a technical reviewer and check with the person who opened the PR. If the change includes a data update, make sure the appropriate yml files have changed.
- If any data has changed, check both national and state-level pages. If templates have changed, check several pages that rely on the templates. For instance, if data or explore-data templates have changed, check states with very different data (i.e. KS, ME, TX, and AK) to make sure they all look right.
- Sometimes, I find it helpful to open up both the current site (either the live version or on dev), and compare the preview side-by-side. This is especially useful for style changes, to ensure the styles cascade appropriately.
If you're comfortable the changes are ready to merge:
- click "Add your review" and approve the changes
- or comment to say it looks good
After the PR has been reviewed and approved, the product manager or the team member acting in that capacity will review and merge the PRs to the 'dev' branch.
- After the PR is merged, delete the temporary/working branch (unless it’s
devorstaging, which don’t get deleted)
If you see something that needs work, or you think it needs additional review, you might:
- use "Add your review" to request changes
- add a reviewer
- comment with a note about what should change
- comment to tag an additional reviewer