VACMS-16409: Update project-conventions.md

READMES/testing.md → READMES/automated-tests.md

@@ -1,4 +1,4 @@
-# Testing
+# Automated Tests
 
 The CMS codebase is tested several times in the development lifecycle:
 
@@ -169,7 +169,7 @@ eliminate these issues, we opted instead to generate a _baseline_ and fail only
 builds that introduced new code issues.
 
 A PHPStan baseline is simply a list of existing errors.  We maintain the
-baseline in our codebase (see [phpstan-baseline.neon](../phpstan-baseline.neon)
+baseline in our codebase (see [phpstan-baseline.neon](../phpstan-baseline.neon))
 to prevent these historical errors from interfering with our CI/CD processes.
 
 This does have drawbacks, though; it can be confusing to have the same code in

READMES/codeowners.md

@@ -27,3 +27,9 @@ The important ideas are:
 6. Viewing the CODEOWNERS file in GitHub should provide debugging information.
 
 As with everything else in this project, CODEOWNERS is subject to continual refinement and development. Please raise issues and suggest improvements where appropriate.
+
+For specifics about when and what you should review, see [Project Conventions](./project-conventions.md#code-review)
+
+----
+
+[Table of Contents](../README.md)

READMES/devops/deploy-oob.md

@@ -66,7 +66,7 @@ If the PO is not already aware, we should inform them of:
 
 ### Preparation
 
-Verify that the `testing/cms-post-deploy-tests-staging` job containing the fix commit has completed successfully and that the combined status for the commit in GitHub is passing. 
+Verify that the `testing/cms-post-deploy-tests-staging` job containing the fix commit has completed successfully and that the combined status for the commit in GitHub is passing.
 
 This can be verified from the [commits listing page](https://github.com/department-of-veterans-affairs/va.gov-cms/commits/main) or with an HTTPS API URL like [this](https://api.github.com/repos/department-of-veterans-affairs/va.gov-cms/commits/bbb7e0e809e17766a5df478c95fb1266d1a654b1/status).
 
@@ -91,7 +91,7 @@ First, notify the Sitewide team, Helpdesk, and Editors of an Out-of-Band deploym
 Notify all Drupal engineers in `@cms-engineers-group`  (product teams & CMS Team) in `#sitewide-program` and request that they hold off on merging anything until further notice, as that can delay the testing pipeline.
 
 ```slack
-:alert: @cms-engineers-group We are preparing for an Out-of-Band deployment.  Please hold off on merging anything until further notice, so as not to delay testing and rollout.  
+:alert: @cms-engineers-group We are preparing for an Out-of-Band deployment.  Please hold off on merging anything until further notice, so as not to delay testing and rollout.
 ```
 
 We want to notify the CMS team to minimize surprise and alarm (if they unexpectedly see an unscheduled deployment happen) and for general situational awareness.
@@ -133,23 +133,9 @@ Helpdesk should notify editors and other stakeholders that the issue has been re
 Notify all Drupal engineers `@cms-engineers-group`  (product teams & CMS Team) in `#sitewide-program` and let them know they are clear to resume merging code as needed.
 
 ```slack
-@cms-engineers-group The Out-of-Band deployment is complete.  You may resume merging as needed.  
+@cms-engineers-group The Out-of-Band deployment is complete.  You may resume merging as needed.
 ```
-### Postmortem
-
-Chances are that any situation serious enough to require an out-of-band deploy will warrant a postmortem.
-
-To create the postmortem, follow the procedure [here](https://github.com/department-of-veterans-affairs/va.gov-team-sensitive/tree/master/Postmortems). Note that this involves a pull request and review process. Don't just create it in `master` :slightly_smiling_face:
-
-Remember that the purpose of a postmortem is to determine the root causes – the deficits in processes and tools – that made this situation possible, and reduce the likelihood of it happening again. It is not to assign blame, express guilt, etc.
 
-#### Communicating Impact
-
-Take extra care in how you report the impact. Use actual, quantifiable figures, statistics, and graphs if you have them. Be sure to communicate the normal, background figures for comparison: the background error rate, the number of users total and the users who _weren't_ affected by the issue, etc. It's difficult to understand or convey actual impact without an understanding of the normal situation. 
-
-If you can't readily access this information:
-- acknowledge that problem in the postmortem document
-- open and prioritize tickets to address
-- list these among your followup actions in the postmortem document
+### Postmortem
 
-Ultimately, someone reading your postmortem should be able to come away with a good understanding of the severity of the issue and its impacts on direct users of the CMS, stakeholders within the system, and the Veteran community.
+Chances are that any situation serious enough to require an out-of-band deploy will warrant a postmortem. See [Postmortems](../postmortems.md) for more information about that process.

READMES/postmortems.md

@@ -0,0 +1,23 @@
+# Postmortems
+
+Chances are that any situation serious enough to require an [out-of-band deploy](./devops/deploy-oob.md) will warrant a postmortem.
+
+To create the postmortem, follow the procedure [here](https://github.com/department-of-veterans-affairs/va.gov-team-sensitive/tree/master/Postmortems). Note that this involves a pull request and review process. Don't just create it in `master` :slightly_smiling_face:
+
+Remember that the purpose of a postmortem is to determine the root causes – the deficits in processes and tools – that made this situation possible, and reduce the likelihood of it happening again. It is not to assign blame, express guilt, etc.
+
+## Communicating Impact
+
+Take extra care in how you report the impact. Use actual, quantifiable figures, statistics, and graphs if you have them. Be sure to communicate the normal, background figures for comparison: the background error rate, the number of users total and the users who _weren't_ affected by the issue, etc. It's difficult to understand or convey actual impact without an understanding of the normal situation.
+
+If you can't readily access this information:
+
+- acknowledge that problem in the postmortem document
+- open and prioritize tickets to address
+- list these among your followup actions in the postmortem document
+
+Ultimately, someone reading your postmortem should be able to come away with a good understanding of the severity of the issue and its impacts on direct users of the CMS, stakeholders within the system, and the Veteran community.
+
+----
+
+[Table of Contents](../README.md)

READMES/project-conventions.md

@@ -1,13 +1,142 @@
 # Project Conventions
 
-## Naming Conventions:
-* Modules: `vagov_modulename`
-* Content types: `vagov_contentype`
-* Fields: `field_[contenttypename]_fieldname`
-
-## Theme-ing Conventions
-* [BEM (Block End Modifier)](https://getbem.com/introduction/) syntax when possible
-* CSS custom properties instead of Sass variables
+## Naming
+
+* Modules: `va_gov_<subject>`, e.g. `va_gov_content_release`, `va_gov_user`.
+* Content Types: `<content type name>`, e.g. `page`, `campaign_landing_page`.
+* Fields: `field_<field name>`, e.g. `field_administration`, `field_alert_dismissable`.
+* Services: `<module name>.<service name>`, e.g. `va_gov_content_release.reporter`, `va_gov_benefits.entity_event_subscriber`.
+
+## Tickets
+
+* The title should be clear and descriptive. Keep in mind that this ticket may contain useful information for years, so make it searchable.
+* The description should contain as much detail as possible. It is great to link to a Slack thread, but it is better to summarize the salient points of the thread, copy and paste comments, *et cetera*, to make the ticket the source of truth.
+* Acceptance Criteria
+  * Must be **SMART**:
+    * **Specific**: List the specific conditions that must be met for the ticket to be considered complete.
+    * **Measurable**: Criteria should be quantifiable to avoid ambiguity.
+    * **Achievable**: Ensure that the criteria are realistic and attainable.
+    * **Relevant**: Criteria should be directly related to the ticket's goal.
+    * **Time-Bound**: If applicable, include deadlines or time constraints.
+  * If you don't know the acceptance criteria, or cannot satisfy the above conditions, open a discovery ticket to investigate the problem so that you can draft a meaningful ticket that will be more successful at addressing it.
+  * `Defect` and `Critical Defect` tickets often come in with no other Acceptance Criteria than something along the lines of "this problem is fixed." These should always be something like "A solution has been identified and implemented as part of this ticket or a followup ticket has been opened to implement the solution." This should allow for reasonable estimation while acknowledging the massive risk inherent in such a ticket.
+* A User Story is very helpful for ensuring that we preserve a focus on improving the user experience.
+* Prefer opening multiple small tickets to a single large ticket.
+  * Smaller tickets are easier to write, easier to test, easier to debug, easier to review, easier to close, and easier to report.
+  * The overhead is negligible.
+  * Use epics, GitHub Projects, or links between tickets to make it easier to track down related tickets.
+* Use the `Defect` label to identify tickets for issues actively and meaningfully impacting the user experience.
+* Use the `Critical Defect` label to identify tickets for issues impacting the user experience to such a degree that they need to be resolved within the current sprint.
+  * This will generally also be `Unplanned Work`.
+  * This may involve a [Postmortem](./postmortems.md), if only an informal, internal one, if there was a substantial disruption to user experience and there was a "teachable moment".
+* Use the `Incident` label to identify tickets for issues impacting the user experience to such a degree that they need to be resolved *immediately*.
+  * This will almost always also be `Unplanned Work`.
+  * This will often involve a [Postmortem](./postmortems.md) unless the underlying issue was out of the team's control, e.g. an upstream failure causing a service disruption.
+  * This will often be opened *after* the work is done, as engineers are too busy fighting a fire to draft an informed and well-written ticket.
+* Ticket status should reflect the status of the work on `main`.
+  * In other words, do not close a ticket for a defect that has been resolved in an integration branch. Mark it with a `Done` label.
+* If work can be done on a PR targeting `main`, do it there, *not* on a PR targeting an integration branch.
+
+## Development
+
+* Prefer implementing event subscribers to hook implementations.
+* Do not add new code to `va_gov_backend`.
+* If significantly changing code in `va_gov_backend`, prefer relocating it to a new or existing module.
+* Avoid mingling multiple concerns into a PR. Keep changes small and easily testable and reversible.
+* Practice clean coding principles:
+  * Create new custom modules when appropriate.
+  * Keep functions, class methods, and classes small, concise, single-purpose, and reusable.
+  * Write unit tests for functions and methods.
+  * Use meaningful variable and function names.
+  * Provide clear comments and documentation in code.
+  * Prefer self-documenting code to comments about implementation details (which may drift out of sync).
+  * Use dependency injection wherever possible; do not instantiate services through `\Drupal::<whatever>` or `\Drupal::service(<whatever>)` calls.
+  * Prefer modern object-oriented alternatives like event subscribers and other object-oriented APIs to legacy hook implementations.
+  * Refactor when appropriate. You are empowered to improve the design of existing code.
+
+### Security
+
+* We follow a [Zero Trust Security Model](https://en.wikipedia.org/wiki/Zero_trust_security_model).
+  * Strictly define roles and permissions in Drupal, ensuring users have only the access they need, following the Principle of Least Privilege.
+  * Limit the number of administrative accounts and use them only when necessary.
+  * Regularly update Drupal core and modules to their latest secure versions.
+  * Assess the severity of vulnerabilities regardless of whether it needs to be exploited:
+    * by a user
+    * by a user with a specific role
+    * from within the VA network
+    * from the command line
+
+### Theming
+
+* Prefer [BEM (Block, Element, Modifier)](https://getbem.com/introduction/) syntax when possible.
+* Prefer CSS custom properties instead of Sass variables.
+
+### Contrib Modules
+
+* If your work depends on a contrib module behaving in a specific way, add automated tests to assure consistency across updates moving forward.
+* If functionality for which your team is responsible breaks as a result of a bug added by an update to a contrib module, add automated tests to guard against regressions.
+* Ensure that contrib modules are explicitly listed as dependencies by custom modules that rely on them.
+
+## Automated Testing
+
+* Prefer PHPUnit unit testing for basic functionality of classes, methods, and functions.
+* Prefer PHPUnit "existing site base" (functional) tests for complex functionality interacting with or dependent upon hooks and/or Drupal Core functionality.
+* Add new PHPUnit tests in `tests/phpunit/<module>/(unit|functional)/<namespace>`, e.g. `tests/phpunit/va_gov_magichead/functional/Field/MaxDepthTest.php`.
+* Prefer Cypress tests for complex UI functionality, forms, etc.
+* Add new Cypress tests in:
+  * `tests/cypress/integration/features/content_type` for general functionality of a specific content type.
+  * `tests/cypress/integration/features/<team name>` for team-specific tests
+    * This is intended to protect against unexpected changes to tests that support your team's mission
+* Add new Cypress step definitions in:
+  * `tests/cypress/integration/step_definitions/common` for general purpose step definitions
+  * `tests/cypress/integration/step_definitions/<team name>` for team-specific step definitions
+    * This is intended to protect against unexpected changes to tests that support your team's mission
+  * `tests/cypress/integration/step_definitions/<subject>` for step definitions only relevant to a specific subject or concept
+
+## Pull Requests
+
+* PRs should normally have a single associated ticket.
+  * If there is no ticket for the work you are doing:
+    * Consider whether you should really be doing it.
+    * Consider whether this work is substantial enough that it should qualify as `Unplanned Work`.
+    * Clear it with the tech lead of your team, and escalate to the PM, DM, and PO.
+    * Open a ticket.
+  * If you are opening a PR that will close multiple tickets:
+    * Consider whether this is necessary or advisable.
+    * Consider what repercussions this may have if an issue forces a revert of the entire PR.
+* Keep PRs in Draft until they are ready to be reviewed and merged.
+  * Do not rely on labels, title prefixes, etc. Only Draft mode mechanically prevents a premature merge.
+* Write QA Steps that demonstrate that the PR addresses the Acceptance Criteria of the related ticket.
+* If these QA Steps are automatable (and most should be), then consider adding a Cypress test.
+* Do not merge a PR with unresolved comments.
+* Do not resolve a comment unless you opened it.
+* Resolve a comment promptly when it has received a satisfactory response.
+
+## Code Review
+
+* As a reviewer, use [conventional comments](https://conventionalcomments.org/) to signal your intent, e.g. **Nitpick**, **Suggestion (non-binding)**, **Request**, etc.
+* Do not make unsolicited comments on PRs that do not belong to a member of your team unless you feel it poses a clear and present danger.
+  * If you have a suggestion for an alternative approach, or a concern about the ticket or the approach, escalate it to your PM/DM/PO.
+* Do not review PRs that do not belong to a member of your team unless your review has been requested.
+  * If your review has been requested as a result of code ownership, but the code in question is owned by multiple teams, consider whether your review is necessary or valuable in this context.
+  * If in doubt of when and what to review, consult the badges on individual files:
+    * If you are on a team that has sole ownership of a particular file, treat the review request as explicit.
+    * ![Owned by my team](./images/ownership-badge.png)
+    * If the file is owned only by another team, don't review the file (or PR) unless you see a problem.
+    * ![Owned only by another team](./images/ownership-badge-2.png)
+    * If the file is owned by multiple teams, including yours, don't review the file (or PR) unless you see a problem.
+    * ![Owned by multiple teams](./images/ownership-badge-none.png)
+* Do not make changes to PRs that do not belong to a member of your team without their consent and approval.
+  * Instead, make suggestions, justify them, and allow the owner to approve or reject them.
+* Do not merge new changes into PRs that do not belong to a member of your team (excluding Dependabot, etc) without their consent and approval.
+* Do not merge a PR that does not belong to a member of your team without their consent and approval.
+* Do not review PRs that are in "Draft" status unless you have been specifically requested.
+
+## Communication
+
+* Prefer communication in the open. Ask questions in the open.
+  * If the documentation does not contain the answer to a procedural question, add documentation containing the answer.
+  * If the documentation *does* contain the answer, consider revising/improving it to improve searchability/relevance/readability/anything else.
 
 ----