The following are expectations for all development work. Make sure you understand and fulfil these expectations when marking a pull request as ready for review.

Basics

Pull request

Your branch should generally follow the format spr-123-story-title, but the story-title portion does not need to be exact and can be abbreviated to make it easy to identify and switch to your branch.

The pull request title should always be formatted as SPR-123 Story Title. The description should include:

• A brief description of the changes made

• DOD checklist (part of template)

• List of test cases to review

• Release notes

  • CS-facing description of what changed

  • Before/after screenshots/recording if applicable depending on the nature of the story

• A link to the story

Acceptance criteria

Scope

Make sure to go through every detail of the description prior to starting development and during development to make sure all specified criteria and potentially unrealized criteria have been understood and met. The story description should be updated with any missing details.

If additional requirements are uncovered or find related issues during development, they should be included if it meets the originally intended scope of the story. Otherwise it may be wiser to create a new story for future planning to avoid stalling development. This is case-dependent and not easy to generalize.

Design

The story should provide general direction of how it should look and function, but may be missing specific details.

Code

Code quality

eslint

eslint is configured to keep syntax unified and avoid common issues. This is essentially a basic code quality bar to meet to avoid the time spent commenting about these things in reviews, but you should be mindful that it doesn’t cover everything and it doesn’t prevent all ugly code.

Any newly ignored eslint rules should have a reasonable explanation. e.g., any newly ignored console.logs are for production logging.

🤖 In the future, we can configure GitHub actions to automatically flag eslint issues in pull requests.

Improvement

If you’re updating an existing file, try to leave it better than you found it where possible but you don’t have to go too far if you cause scope creep and/or introduce a regression.

Comprehensible

Code should be written in a way that you won’t regret it tomorrow, next sprint, or next year. The developer reviewing your code should be able to understand it without it being explained to them, and so should the person reading your code next year, as it may or may not be you.

Even if it is you, you can’t be expected to remember everything about what you just wrote a year from now, so make sure you give whoever it is the biggest leg up possible.

Well designed

Design for change and extensibility. At some point or another, it’s always probable that we’ll need to extend the functionality of what you just added or changed, even if we don’t know it yet. Or maybe we need to just change some logic down the road either entirely or based on certain additional conditions.

It would be pretty easy to just say adhere to “SOLID” or “clean code” principles or whatever the common design pattern du jour is, but one size doesn’t necessarily fit all and chasing something specific often loses sight of the objective just so you can meet that standard.

Documented

Code should be self-documenting. If that’s not possible, refactor or document it. Documenting doesn’t mean literally explaining what it’s doing line-by-line, documenting involves explaining the purpose of what you wrote and often also why it’s written that way.

Security

This varies depending on the story. If writing a new API, make sure you understand what roles/scopes are expected to be authorized to access the API and what scope of data.

Defense in depth

Every layer of the application should be mindful of security in one way or another. You can’t trust anything on the client, so API security is critical, but the client should still be as secure as possible. The API also has multiple layers where security can be implemented.

Authorization

Keep in mind that authorization not only involves “who” has access (roles, scopes, users), but there’s also “what” they have access to (scoped data filtering), and sometimes “when” (expiry dates).

Beyond that, we need to ensure that we don’t introduce any security vulnerabilities by considering how changes could be vulnerable to be abused, either through attack by malicious parties or by authorized users attempting to access unauthorized data.

While the majority of Play data is for public consumption, we do still have sensitive that we’re trusted to protect and we need to be aware of this fact.

Third party libraries

The Javascript ecosystem is built on top of a lot of third party libraries, which means that we are potentially vulnerable to “supply chain attacks” through compromised libraries, or simply unknown vulnerabilities introduced in code developed by third parties.

Be mindful of this fact when introducing new dependencies and examine the library in your process. npm libraries and Github repositories cannot be fully trusted to be security audited.

🤖 In the future, we should consider automated dependency scanning and tools like Dependabot to automatically upgrade dependencies. This requires significant testing infrastructure to limit regressions.

Testing

Regression testing

Changes can sometimes cause unexpected issues. Think carefully about the side effects that your changes could have and what else uses the affected logic.

Depending on the change, you may want to step back a degree to make sure the user workflow hasn’t regressed at any point, sometimes this isn’t obvious. Even if it’s too obvious, validate your assumptions.

It may be helpful to provide a checklist of things to check in the pull request and comment on what has been tested.

Unit testing

Our goal is to move as much of our business logic (as feasible) into our libraries and leave the api/app directories for integration logic with third-party libraries (API/UI/ORM/etc).

Libraries should aim to be agnostic of the consumer (data source) and tested as such. This would make it easier to swap out a framework down the road.

🤖 In the future, we can configure GitHub actions to automatically flag failing tests and show code coverage changes (+/-%) in pull requests.

Integration testing

🛠️ To be implemented in the future.

Shipping

Code review

Code reviews are a critical opportunity for peer review and act as a test to ensure the definition of done is met.

Remember the goal is intended to improve the product and developer, not to criticize the changes like it’s open season. Questions and challenges should always be welcome and changes should either be reasonably defensible or open to change.

Production ready

Pull requests are merged to master, which is always expected to be deployable to production. If a pull request cannot confidently meet this standard, it’s not ready to be merged.

This requirement can be met if the change is feature flagged allowing it’s UI and behaviour to be entirely disabled in production until a later time.

Documentation

The Play Documentation Confluence space is written to enable the CS and Academy teams to answer tickets and create user-facing documentation and training. Developer documentation should explain functionality in full detail without needing to be supported by sources outside of the space.

This documentation space is a good opportunity to explain implementation details and reasoning/limitations to provide additional context for support teams.

Release notes and documentation can be updated at any point during development, and should be fully completed prior to merging the pull request.