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

Fix handling arrays in session data #709

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Fix handling arrays in session data #709

wants to merge 4 commits into from

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Mar 11, 2019

Draft fix for #693.

If user stores data in array format the rest of the data in the surrounding item gets overwritten. This fork happens when we check for checkboxes data. Storing data as objects as we show in our docs works fine.

This PR:

  • Moves the check up in the code so that we catch the checkboxes data separately from other arrays
  • Does some tidying up of the logic to make it easier to follow

We should really have automated tests in place to check if the session data is being stored correctly but as the kit currently has only very simple tests we need to think about how we're going to do the test coverage.

For now, I've added in a really simple manual way of testing whether the storage is working by setting lots of different shapes of data in session-data-defaults.js, amending them in a form and echoing them on the following page to check that they are retained when the values are changed. See test page.

To do:

  • Remove manual test commit
  • Add automated tests to check that user data of various shapes is stored correctly or create a card for adding the tests later - the test commit above shows what these tests could be.

As reported in
#693
the session data can get overwritten. This happened if
there is an array in the session data as the logic takes
this to be checkboxes data and stores it, overwriting
the existing data.
Check for objects first. This makes the relationship of objects and arrays clearer
in case person reading the code doesn’t know that arrays are always objects.

Plus some other tidy up.
Demo for testing that data in various shapes doesn't get overwritten.

Next step: have tests to handle this.
@daviddotto
Copy link
Contributor

This is now working as expected, thanks for your work on this, it's a big help for me and our team's prototype 👍

Co-authored-by: Nick Colley <nick.colley@digital.cabinet-office.gov.uk>
@hannalaakso
Copy link
Member Author

I’ve just pushed up some simple tests that @NickColley and I worked on for this quite a long time ago now. We should add some more to account for all the cases currently described in 399dd8e (and then remove the said commit).

@daviddotto
Copy link
Contributor

daviddotto commented Nov 26, 2019

Where name is example[0][foo] an object with a key of "0" is added to the data object instead of an array with one item. Perhaps there needs to be a check if the key is an int and handle as an array?


// Insert values here

// Example as used in current docs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to leave this file blank for users to fill in if they need to? Can this test data be stored somewhere else?

Copy link
Contributor

@NickColley NickColley Nov 26, 2019

Choose a reason for hiding this comment

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

It's just test code for whoever picks this up next. "Delete this commit. Demo for testing"

Copy link
Contributor

Choose a reason for hiding this comment

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

ah thanks!

describe('The data storage', () => {
// check session-data defaults file exists

it.skip('file should exist', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with Jest, but I think you're trying to access the session defaults file from an http request, which won't work. The file exists for the server to set up the default session data, not for access client-side.

}

const inputData = {
vehicle: {
Copy link
Contributor

Choose a reason for hiding this comment

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

inputData comes from an http request, in which case I think it can only be key/value pairs, not objects. I think to test we need to use dot or square bracket notation in strings, like 'vehicle.registration=OUTATIME'

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I'm wrong, the strings get converted to objects by Express BodyParser here:
https://github.com/alphagov/govuk-prototype-kit/blob/master/server.js#L134-L138

@joelanman
Copy link
Contributor

@daviddotto yes I can replicate that. In this PR there is a default data file that sets up arrays in JSON. In that case the [0] notation appears to work, but in the case where there is no default data, it interprets it as a string property of an object, as you say.

@joelanman
Copy link
Contributor

I don't think this PR currently works to update arrays, I've written up my findings here:
#693 (comment)

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.

5 participants