From f10b4d82855ba961ae7ba773f39ee7945bed669b Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 10:33:45 -0700 Subject: [PATCH 01/16] Create code review guide --- .github/pull_request_template.md | 2 +- docs/dev-practices/code_review.md | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 docs/dev-practices/code_review.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index dec0b9fac..4f2349204 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,6 +1,6 @@ ## Ticket -Resolves #00 +Resolves #001 ## Changes @@ -45,15 +40,10 @@ 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? - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values @@ -62,24 +52,16 @@ All other changes require just a single approving review.--> - [ ] 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 code standards and comments if any standards above 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? +- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations`. #### Validated user-facing changes as a developer @@ -88,12 +70,6 @@ All other changes require just a single approving review.--> - [ ] 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 @@ -103,10 +79,9 @@ All other changes require just a single approving review.--> #### 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 diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 56d4db394..38ed83232 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -1,6 +1,21 @@ -# Code Review +## Code Review After creating a pull request, pull request submitters should: -- Add at least 2 developers as PR reviewers (only 1 will need to approve) -- Message on Slack or in standup to notify the team that a PR is ready for review -- If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. \ No newline at end of file +- Add at least 2 developers as PR reviewers (only 1 will need to approve). +- Message on Slack or in standup to notify the team that a PR is ready for review. +- If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. +- If any updated dependencies on Pipfile, also update dependencies in requirements.txt. + +## Pull Requests for User-facing changes +Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. + +When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. + +## Coding standards +(The Coding standards section may be moved to a new code standards file in a future ticket. +For now we're simply moving PR template content into the code review document for consolidation) + +### Plain language +All functions and methods should use plain language. + +TODO: Description and examples in code standards ticket. \ No newline at end of file From 93ee3b0b8f3901bb14889585ef90906adac83692 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:46:13 -0700 Subject: [PATCH 03/16] Refactor spacing --- .github/pull_request_template.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index e493e0a92..e2340bebe 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -43,7 +43,8 @@ Resolves #001 - [ ] Update documentation in READMEs and/or onboarding guide #### Ensured code standards are met (Original Developer) - + +N/A - no external systems or errors, this is just docs refactoring. - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values @@ -58,7 +59,7 @@ Resolves #001 #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Verified code meets code standards and comments if any standards above are not satisfied +- [ ] Verified code meets above code standards and user-facing checks. Addresses 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 - [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations`. @@ -85,7 +86,7 @@ Resolves #001 #### Validated user-facing changes as a designer -- [ ] Checked keyboard navigability +- [ ] Checked different states (empty, one, some, error) - [ ] 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) From 2bdef1e01ff802855fcd92ade0a6bd0cd705764f Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:47:13 -0700 Subject: [PATCH 04/16] Fix spacing --- .github/pull_request_template.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index e2340bebe..351ce579b 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -70,7 +70,6 @@ N/A - no external systems or errors, this is just docs refactoring. - [ ] 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) - - [ ] (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 From de4161dde44f3dce9d96329540e82d8be1d14285 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:47:49 -0700 Subject: [PATCH 05/16] Remove unused content --- .github/pull_request_template.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 351ce579b..4d3b76746 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -44,7 +44,6 @@ Resolves #001 #### Ensured code standards are met (Original Developer) -N/A - no external systems or errors, this is just docs refactoring. - [ ] Interactions with external systems are wrapped in try/except - [ ] Error handling exists for unusual or missing values From 3997251eb19c9c77748f1afbe3b875ecfe92329e Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:49:19 -0700 Subject: [PATCH 06/16] Add browser section --- .github/pull_request_template.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 4d3b76746..a3646c40a 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -87,11 +87,11 @@ Resolves #001 - [ ] Checked different states (empty, one, some, error) - [ ] 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 +#### Test support on multiple browsers. Check the browser(s) tested. +- [ ] Chrome +- [ ] Microsoft Edge +- [ ] FireFox +- [ ] Safari - [ ] (Rarely needed) Tested as both an analyst and applicant user From fc421ce0578621e47bd34f33c38913ddcae7fcd7 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:56:38 -0700 Subject: [PATCH 07/16] Update code review doc --- docs/dev-practices/code_review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 38ed83232..aa3d13404 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -18,4 +18,4 @@ For now we're simply moving PR template content into the code review document fo ### Plain language All functions and methods should use plain language. -TODO: Description and examples in code standards ticket. \ No newline at end of file +TODO: Plain language description and examples in code standards ticket. \ No newline at end of file From c553ebb773df1e1544f406f9004fe89fd6ba9732 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:57:57 -0700 Subject: [PATCH 08/16] Fix punctuation --- .github/pull_request_template.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index a3646c40a..ecf117f15 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -59,9 +59,9 @@ Resolves #001 - [ ] Pulled this branch locally and tested it - [ ] Verified code meets above code standards and user-facing checks. Addresses any checks that are not satisfied -- [ ] Reviewed this code and left comments. Indicate if comments must be addressed before code is merged. +- [ ] 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 -- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations`. +- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations` #### Validated user-facing changes as a developer From 02839026153c57f7688daf242c4c959a155a2427 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 4 Oct 2024 10:17:39 -0700 Subject: [PATCH 09/16] Add more copy changes --- .github/pull_request_template.md | 4 ++-- docs/dev-practices/code_review.md | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index ecf117f15..c4e63bccd 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -65,14 +65,14 @@ Resolves #001 #### Validated user-facing changes as a developer +**Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist + - [ ] 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) - [ ] (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 diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index aa3d13404..f30eec64e 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -6,9 +6,10 @@ After creating a pull request, pull request submitters should: - If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. - If any updated dependencies on Pipfile, also update dependencies in requirements.txt. -## Pull Requests for User-facing changes Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. +All other changes require just a single approving review. +## Pull Requests for User-facing changes When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. ## Coding standards From 980f997dbcea3cf32ba4ccf68b9880552db3b83d Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 4 Oct 2024 10:29:10 -0700 Subject: [PATCH 10/16] Add PR naming conventions --- .github/pull_request_template.md | 13 +++++-------- docs/dev-practices/code_review.md | 7 ++++++- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index c4e63bccd..20571b305 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -64,7 +64,6 @@ Resolves #001 - [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations` #### Validated user-facing changes as a developer - **Note:** Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist - [ ] New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing @@ -86,13 +85,11 @@ Resolves #001 - [ ] Checked different states (empty, one, some, error) - [ ] Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI) - -#### Test support on multiple browsers. Check the browser(s) tested. -- [ ] Chrome -- [ ] Microsoft Edge -- [ ] FireFox -- [ ] Safari - +- [ ] 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 ## Screenshots diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index f30eec64e..09e6e0c1c 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -1,5 +1,8 @@ ## Code Review +Pull requests should be titled in the format of `#issue_number: Descriptive name ideally matching ticket name - [sandbox]` +Any pull requests including a migration should be suffixed with ` - MIGRATION` + After creating a pull request, pull request submitters should: - Add at least 2 developers as PR reviewers (only 1 will need to approve). - Message on Slack or in standup to notify the team that a PR is ready for review. @@ -7,11 +10,13 @@ After creating a pull request, pull request submitters should: - If any updated dependencies on Pipfile, also update dependencies in requirements.txt. Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. -All other changes require just a single approving review. +All other changes require a single approving review. ## Pull Requests for User-facing changes When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. +Add new pages to the .pa11yci file so they are included in our automated accessibility testing. + ## Coding standards (The Coding standards section may be moved to a new code standards file in a future ticket. For now we're simply moving PR template content into the code review document for consolidation) From c20f7e66791906b88a30e19130efe44642108400 Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Fri, 4 Oct 2024 10:49:27 -0700 Subject: [PATCH 11/16] Updating branch naming standards in contributing.md --- CONTRIBUTING.md | 21 +-------------------- docs/dev-practices/code_review.md | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ab15c660f..5e1c01be9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,17 +14,6 @@ There are a handful of things we do not commit to the repository: For developers, you can auto-deploy your code to your sandbox (if applicable) by naming your branch thusly: jsd/123-feature-description Where 'jsd' stands for your initials and sandbox environment name (if you were called John Smith Doe), and 123 matches the ticket number if applicable. -## Approvals - -When a code change is made that is not user facing, then the following is required: -- a developer approves the PR - -When a code change is made that is user facing, beyond content updates, then the following are required: -- a developer approves the PR -- a designer approves the PR or checks off all relevant items in this checklist - -Content or document updates require a single person to approve. - ## Project Management We use [Github Projects](https://docs.github.com/en/issues/planning-and-tracking-with-projects/learning-about-projects/about-projects) for project management and tracking. @@ -39,14 +28,6 @@ Every issue in this respository and on the project board should be appropriately We also have labels for each discipline and for research and project management related tasks. While this repository and project board track development work, we try to document all work related to the project here as well. -## Pull request etiquette - -- The submitter is in charge of merging their PRs unless the approver is given explicit permission. -- Do not commit to another person's branch unless given explicit permission. -- Keep pull requests as small as possible. This makes them easier to review and track changes. -- Bias towards approving i.e. "good to merge once X is fixed" rather than blocking until X is fixed, requiring an additional review. -- Write descriptive pull requests. This is not only something that makes it easier to review, but is a great source of documentation. - ## Branch Naming -Our branch naming convention is `name/topic-or-feature`, for example: `lmm/add-contributing-doc`. +Our branch naming convention is `name/issue_no-description`, for example: `lmm/0000-add-contributing-doc`. diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 09e6e0c1c..1cea4aa04 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -1,7 +1,7 @@ ## Code Review Pull requests should be titled in the format of `#issue_number: Descriptive name ideally matching ticket name - [sandbox]` -Any pull requests including a migration should be suffixed with ` - MIGRATION` +Pull requests including a migration should be suffixed with ` - MIGRATION` After creating a pull request, pull request submitters should: - Add at least 2 developers as PR reviewers (only 1 will need to approve). @@ -9,19 +9,27 @@ After creating a pull request, pull request submitters should: - If any model was updated to modify/add/delete columns, run makemigrations and commit the associated migrations file. - If any updated dependencies on Pipfile, also update dependencies in requirements.txt. +## Pull request approvals Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. All other changes require a single approving review. +The submitter is responsible for merging their PR unless the approver is given explcit permission. Similarly, do not commit to another person's branch unless given explicit permission. + +Bias towards approving i.e. "good to merge once X is fixed" rather than blocking until X is fixed, requiring an additional review. + ## Pull Requests for User-facing changes When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. Add new pages to the .pa11yci file so they are included in our automated accessibility testing. +## Other Pull request norms +- Keep pull requests as small as possible. This makes them easier to review and track changes. +- Write descriptive pull requests. This is not only something that makes it easier to review, but is a great source of documentation. + +[comment]: The Coding standards section will be moved to a new code standards file in #2898. For now we're simply moving PR template content into the code review document for consolidation ## Coding standards -(The Coding standards section may be moved to a new code standards file in a future ticket. -For now we're simply moving PR template content into the code review document for consolidation) ### Plain language All functions and methods should use plain language. -TODO: Plain language description and examples in code standards ticket. \ No newline at end of file +TODO: Plain language description and examples in code standards ticket. From ec74cfb24d316a152e18343ac309942283407d5d Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Mon, 7 Oct 2024 11:30:31 -0700 Subject: [PATCH 12/16] Incorporate feedback --- .github/pull_request_template.md | 13 +++++++++---- docs/dev-practices/code_review.md | 1 - 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 20571b305..09f966148 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,6 +1,6 @@ ## Ticket -Resolves #001 +Resolves #00 ## Changes @@ -44,12 +44,14 @@ Resolves #001 #### Ensured code standards are met (Original Developer) +- [ ] 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 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) @@ -58,10 +60,10 @@ Resolves #001 #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Verified code meets above code standards and user-facing checks. Addresses any checks that are not satisfied +- [ ] Verified code meets above code standards and user-facing checklist. 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 -- [ ] If any model was updated to modify/add/delete columns, verified migrations can be run with `makemigrations` +- [ ] 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 reviewers should not make a duplicate checklist @@ -92,6 +94,9 @@ Resolves #001 - [ ] Safari - [ ] (Rarely needed) Tested as both an analyst and applicant user +### References +- [Code review best practices](../docs/dev-practices/code_review.md) + ## Screenshots Resolves #00 ## Changes From 2dd8d53ec9e7c7099701e33bdde5eb5ffb96b30c Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:50:43 -0700 Subject: [PATCH 15/16] Updated with review comments --- .github/pull_request_template.md | 9 +++++---- docs/dev-practices/code_review.md | 8 +++----- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 934c95ab8..e457d7a63 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -44,14 +44,14 @@ Resolves #00 - [ ] Update documentation in READMEs and/or onboarding guide #### Ensured code standards are met (Original Developer) - + - [ ] 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) -- [ ] Tag @dotgov-designers for design review. If code is not user-facing, delete design reviewer checklist +- [ ] 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) @@ -61,13 +61,13 @@ Resolves #00 #### Reviewed, tested, and left feedback about the changes - [ ] Pulled this branch locally and tested it -- [ ] Verified code meets above code standards and user-facing checklist. Address any checks that are not satisfied +- [ ] 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 - [ ] 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 reviewers should not make a duplicate checklist +**Note:** Multiple code reviewers can share the checklists above, a second reviewers 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 @@ -81,6 +81,7 @@ Resolves #00 - [ ] Checked that the design translated visually - [ ] Checked behavior. Comment any found issues or broken flows. +- [ ] Checked keyboard navigability - [ ] Checked different states (empty, one, some, error) - [ ] Checked for landmarks, page heading structure, and links diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 4a27d71d6..5a8849754 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -12,12 +12,12 @@ After creating a pull request, pull request submitters should: Code changes on user-facing features (excluding content updates) require approval from at least one developer and one designer. All other changes require a single approving review. -The submitter is responsible for merging their PR unless the approver is given explcit permission. Similarly, do not commit to another person's branch unless given explicit permission. +The submitter is responsible for merging their PR unless the approver is given explicit permission. Similarly, do not commit to another person's branch unless given explicit permission. Bias towards approving i.e. "good to merge once X is fixed" rather than blocking until X is fixed, requiring an additional review. ## Pull Requests for User-facing changes -When making user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. +When making or reviewing user-facing changes, test that your changes work on multiple browsers including Chrome, Microsoft Edge, Firefox, and Safari. Add new pages to the .pa11yci file so they are included in our automated accessibility testing. @@ -29,6 +29,4 @@ Add new pages to the .pa11yci file so they are included in our automated accessi ## Coding standards ### Plain language -All functions and methods should use plain language. - -TODO: Plain language description and examples in code standards ticket. +All functions and methods should use plain language. \ No newline at end of file From 7885f389212a6635f78d9b74b59ec2a3ce00aa2f Mon Sep 17 00:00:00 2001 From: Erin Song <121973038+erinysong@users.noreply.github.com> Date: Thu, 17 Oct 2024 13:49:06 -0700 Subject: [PATCH 16/16] Remove code standards comment --- docs/dev-practices/code_review.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/dev-practices/code_review.md b/docs/dev-practices/code_review.md index 5a8849754..7b054cad5 100644 --- a/docs/dev-practices/code_review.md +++ b/docs/dev-practices/code_review.md @@ -25,7 +25,6 @@ Add new pages to the .pa11yci file so they are included in our automated accessi - Keep pull requests as small as possible. This makes them easier to review and track changes. - Write descriptive pull requests. This is not only something that makes it easier to review, but is a great source of documentation. -[comment]: The Coding standards section will be moved to a new code standards file in #2898. For now we're simply moving PR template content into the code review document for consolidation ## Coding standards ### Plain language