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

Set up session table (return requirements) #594

Merged
merged 10 commits into from
Dec 15, 2023
Merged

Conversation

robertparkinson
Copy link
Collaborator

https://eaflood.atlassian.net/browse/WATER-4257

We need to store user journeys as people progress through the screens and we need a table to store these in.

@robertparkinson robertparkinson marked this pull request as ready for review December 15, 2023 15:47
@robertparkinson robertparkinson self-assigned this Dec 15, 2023
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the tweak-y-est of tweaks! 😁

// Thing under test
const SessionModel = require('../../app/models/session.model.js')

describe('Sessions model', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('Sessions model', () => {
describe('Session model', () => {

Comment on lines 39 to 42
const defaults = {
id: generateUUID(),
data: {}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where a table automatically generates an ID we don't tend to set one in the helper. If you've seen us do this it should normally come with a note saying the previous team failed to implement an automatic UUID on insert in the underlying table, hence the need to do it.

So, we just focus on setting meaningful defaults to reflect what an instantiated record would typically look like.

In this particular case data: {} could be what we often find. So, I'd suggest one of the following, I'll leave you to decide which one to go with!

  const defaults = {
    data: { licenceId: '01/128' }
  }

Or whatever pretend test values you'd like to set in data:

or

  const defaults = {}

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertparkinson robertparkinson merged commit 4ab9567 into main Dec 15, 2023
5 checks passed
@robertparkinson robertparkinson deleted the setup-session-table branch December 15, 2023 16:34
@Cruikshanks Cruikshanks added the enhancement New feature or request label Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants