Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a request spec for the "Add placement" journey #825

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

ollietreend
Copy link
Contributor

Context

Request specs are useful for covering behaviours and side effects which aren't strictly user facing.

They can be used when the scenario is difficult to reproduce in a system spec (for example, requesting paths which can't be navigated to using the UI), or where the expected behaviour can't be asserted directly from the UI (for example, to check that email or Slack notifications are triggered in response to a user's actions).

Changes proposed in this pull request

I've added request specs to cover the following "Add placement" journeys:

  • School → Placements → Add placement
  • Support console → School → Placements → Add placement

I've also moved the system spec for adding a placement in the support console, so the spec file sits alongside other related specs. It wasn't easy to find in its original location.

Guidance to review

No specific guidance, but...

A note on "DRY"

I recognise that the two request specs added here are very similar. I've chosen not to try and DRY-up the test into a shared example.

My reasoning behind that decision:

  • There are only seven examples in common between the two specs.
  • Some parts of the test are different – for example, I've used different *_path route helpers because the requests go to different controllers.
  • There's potential for the two to diverge a bit – especially in the way they handle 'unhappy' paths.
    • For example, there's currently a question over how the support console should handle schools which don't have placement contact details yet.
    • Support users have access to every school, whereas regular users need to be on the users list for the school before they're allowed access. I've included test coverage of this.
  • Generally speaking, I think tests work best when they're explicit and easy to read, without too much indirection or abstraction. Some people call this writing "WET" tests. But it's an art, not a science.

If anybody has strong opinions about whether I should DRY-up these tests or not, I'm happy to hear them!

Link to Trello card

https://trello.com/c/nvYNw8IB/543-add-test-coverage-for-add-placement-journey-edge-cases-and-side-effects

This covers the non-user facing outputs of the "Add placement" journey:

- a Placement record gets created
- a Slack message gets sent to the team
- unauthorized users can't create a Placement

I've also relocated the "placements" request spec because it maps to the
placements controller which exists in the directory above.
This adds a request spec for the support console implementation of the
"Add placement" journey.
I've moved the "Add placement" system spec for support users so it's
located in the same place as other related specs.

It now sits alongside the following specs:

- support user adds a placement
- support user removes a placement
- support user views a placement
- support user views placements
@ollietreend ollietreend requested a review from a team as a code owner July 5, 2024 14:37
@ollietreend ollietreend merged commit 02f4ee1 into main Jul 5, 2024
8 checks passed
@ollietreend ollietreend deleted the add-placement-test-coverage branch July 5, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants