diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index dec0b9fac..60b0b9d8c 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,11 +1,7 @@ ## Ticket + Resolves #00 - ## Changes @@ -45,82 +41,63 @@ All other changes require just a single approving review.--> - [ ] Met the acceptance criteria, or will meet them in a subsequent PR - [ ] Created/modified automated tests -- [ ] Added at least 2 developers as PR reviewers (only 1 will need to approve) -- [ ] Messaged on Slack or in standup to notify the team that a PR is ready for review -- [ ] Changes to “how we do things” are documented in READMEs and or onboarding guide -- [ ] If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited. +- [ ] Update documentation in READMEs and/or onboarding guide #### Ensured code standards are met (Original Developer) - -- [ ] All new functions and methods are commented using plain language -- [ ] Did dependency updates in Pipfile also get changed in requirements.txt? + +- [ ] If any updated dependencies on Pipfile, also update dependencies in requirements.txt. - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values #### Validated user-facing changes (if applicable) -- [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing +- [ ] Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist +- [ ] Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing - [ ] Checked keyboard navigability - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) -- [ ] Add at least 1 designer as PR reviewer ### As a code reviewer, I have #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Reviewed this code and left comments +- [ ] Verified code meets all checks above. Address any checks that are not satisfied +- [ ] Reviewed this code and left comments. Indicate if comments must be addressed before code is merged - [ ] Checked that all code is adequately covered by tests -- [ ] Made it clear which comments need to be addressed before this work is merged -- [ ] If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited. - -#### Ensured code standards are met (Code reviewer) - -- [ ] All new functions and methods are commented using plain language -- [ ] Interactions with external systems are wrapped in try/except -- [ ] Error handling exists for unusual or missing values -- [ ] (Rarely needed) Did dependency updates in Pipfile also get changed in requirements.txt? +- [ ] Verify migrations are valid and do not conflict with existing migrations #### Validated user-facing changes as a developer +**Note:** Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A. - [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing - [ ] Checked keyboard navigability - [ ] Meets all designs and user flows provided by design/product - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - -- [ ] Tested with multiple browsers, the suggestion is to use ones that the developer didn't (check off which ones were used) - - [ ] Chrome - - [ ] Microsoft Edge - - [ ] FireFox - - [ ] Safari - - [ ] (Rarely needed) Tested as both an analyst and applicant user -**Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist - ### As a designer reviewer, I have #### Verified that the changes match the design intention - [ ] Checked that the design translated visually -- [ ] Checked behavior +- [ ] Checked behavior. Comment any found issues or broken flows. - [ ] Checked different states (empty, one, some, error) - [ ] Checked for landmarks, page heading structure, and links -- [ ] Tried to break the intended flow #### Validated user-facing changes as a designer - [ ] Checked keyboard navigability - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - - [ ] Tested with multiple browsers (check off which ones were used) - [ ] Chrome - [ ] Microsoft Edge - [ ] FireFox - [ ] Safari - - [ ] (Rarely needed) Tested as both an analyst and applicant user +### References +- [Code review best practices](../docs/dev-practices/code_review.md) + ## Screenshots