Kit should allow array values to be 'set' instead of overwriting entire array

[daviddotto]

Currently if a user of the kit tries to set a vlue in a data object (for example a complex object set in session-data-defaults.js) by using an input like <input name="defaults.accounts[3].name" value="Ellie Sattler"/> the defaults object will be overwritten with a new object including only the new data.

The expected behavior would be that the name for 4th account in defaults will equal Ellie Sattler and all other data would remain unchanged.

After some investigation it looks like the way the req.query handles incoming keys it converts any string to an object so it flattens into a single object and uses this to replace the root object. I have added a workaround which involves sending and 'selector' and the 'value' inside the value of the input (to preserve its 'string-y-ness) and using special characters in the name or key to handle it seperatly in the storeData() method. For example name=":set:" value="defaults.accounts[3].name=Ellie Sattler"

And then:

const set = require('lodash.set')
// Store data from POST body or GET query in session
var storeData = function(input, data) {
    for (var i in input) {
        ...
        if (i.indexOf(':set:') === 0) {
        const trimLocation = val.indexOf('=')
        const path = val.slice(0, trimLocation)
        const value = val.slice(trimLocation + 1, val.length)
        set(data, path, value)
        continue
        }
        ...
    }
})

This implementation has the added benefit of allowing whole object replacement if required and maintains backwards compatibility.

[joelanman]

I'm fairly sure this is a bug, since we introduced support for nested values here:

https://github.com/alphagov/govuk-prototype-kit/pull/573

If we support nested values, I would expect to be able to overwrite one without losing any others previously stored.

[hannalaakso]

@daviddotto I've raised a PR to address this issue: https://github.com/alphagov/govuk-prototype-kit/pull/709. Could you check if this fixes your problem?

[joelanman]

Hi @daviddotto just reopening this as the fix has not been released - just checking, did @hannalaakso 's code fix it for you?

[daviddotto]

Sorry, yes it did, I hadn't realised I neglected to reply! I needed up rolling my own solution but this solves the original problem

[joelanman]

Having investigated, I think this may be more of a feature request than a bug (which is fine).

When we receive object notation:

name="user[first-name]" value="Joe"

This is converted to an object in Express (via the qs library):

{
  'user': {
    'first-name': 'Joe'
  }
}

and then receive an update later on:

name="user[status]" value="Pending"

becomes

{
  'user': {
    'status': 'Pending'
  }
}

we merge the two objects to get the expected result:

{
  user: {
    'first-name': 'Joe',
    'status': 'Pending'
  }
}

However when we receive array notation, the situation is not clear:

name="users[0]" value="Joe"

in Express this becomes:

{
  'users': ['Joe']
}

but if we try to update the array with this syntax:

name="users[1]" value="David"

it becomes:

{
  'users': ['David']
}

The [1] in the name does not appear to do anything, and I'm not sure what it should be expected to do. Receiving only another definition of the array 'users', the expected thing would be to overwrite the previous array, not merge the two.

Sorry this is long winded but I'm trying to show some special syntax might be required to make this work.

[NickColley]

@joelanman does it make sense for [1] to put 'David' into the array at position 1?

{
  'users': ['Joe', 'David']
}

[NickColley]

Joe has let me know this is not under our control since bodyParser determines how this data turns from a string. So we'd need to adjust bodyParser or use special syntax.

[daviddotto]

I beleive in a implementation I was working with on a prevous project I can across the same issue. I ended up getting around it by using a lodash method to set the data object by feeding it the path, the new value and the existing data from the session. Not an ideal solution as it means adding a new dependency.

[joelanman]

I think it would help to have a documented example of adding to a list in the Prototype Kit.

[JonJMagee]

Following review the Prototype Team are closing this ticket. If the original author wishes to raise this issue again please feel free.