From 664985ebe0be6fe3e0444142d340fd72d72b843f Mon Sep 17 00:00:00 2001 From: Hanna Laakso Date: Mon, 11 Mar 2019 14:33:22 +0000 Subject: [PATCH 1/4] Check if session data array is for checkboxes As reported in https://github.com/alphagov/govuk-prototype-kit/issues/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. --- lib/utils.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index b3adf58100..e6e231a006 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -236,11 +236,8 @@ var storeData = function (input, data) { } // Remove _unchecked from arrays of checkboxes - if (Array.isArray(val)) { - var index = val.indexOf('_unchecked') - if (index !== -1) { - val.splice(index, 1) - } + if (Array.isArray(val) && val.indexOf('_unchecked') !== -1) { + val.splice(val.indexOf('_unchecked'), 1) } else if (typeof val === 'object') { // Store nested objects that aren't arrays if (typeof data[i] !== 'object') { From 83efefe744b06e7d161ac8e326df8a6762c03c0d Mon Sep 17 00:00:00 2001 From: Hanna Laakso Date: Mon, 11 Mar 2019 14:33:39 +0000 Subject: [PATCH 2/4] Refactor storing arrays and objects code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/utils.js | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index e6e231a006..068fddf2c4 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -221,6 +221,9 @@ exports.matchMdRoutes = function (req, res) { // Store data from POST body or GET query in session var storeData = function (input, data) { + // Value we store to indicate unchecked checkboxes. See `auto-store-data.js` + var uncheckedValue = '_unchecked' + for (var i in input) { // any input where the name starts with _ is ignored if (i.indexOf('_') === 0) { @@ -230,25 +233,26 @@ var storeData = function (input, data) { var val = input[i] // Delete values when users unselect checkboxes - if (val === '_unchecked' || val === ['_unchecked']) { + if (val === uncheckedValue || val === [uncheckedValue]) { delete data[i] continue } - // Remove _unchecked from arrays of checkboxes - if (Array.isArray(val) && val.indexOf('_unchecked') !== -1) { - val.splice(val.indexOf('_unchecked'), 1) - } else if (typeof val === 'object') { - // Store nested objects that aren't arrays - if (typeof data[i] !== 'object') { - data[i] = {} - } + if (typeof val === 'object') { + // If array contains `uncheckedValue`, it's array of checkboxes + if (Array.isArray(val) && val.indexOf(uncheckedValue) !== -1) { + // Remove `uncheckedValue` from arrays of checkboxes + val.splice(val.indexOf(uncheckedValue), 1) + } else { // Store arrays that don't contain "_unchecked", and nested objects + if (typeof data[i] !== 'object') { + data[i] = {} + } - // Add nested values - storeData(val, data[i]) - continue + // Add nested values + storeData(val, data[i]) + continue + } } - data[i] = val } } From 399dd8efdfc2de33dd8b3d97abdd6dd62ab9a35a Mon Sep 17 00:00:00 2001 From: Hanna Laakso Date: Mon, 11 Mar 2019 17:32:27 +0000 Subject: [PATCH 3/4] Delete this commit. Demo for testing Demo for testing that data in various shapes doesn't get overwritten. Next step: have tests to handle this. --- app/data/session-data-defaults.js | 46 +++++++++++++++++++++-- docs/views/examples/pass-data/test1.html | 47 ++++++++++++++++++++++++ docs/views/examples/pass-data/test2.html | 42 +++++++++++++++++++++ 3 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 docs/views/examples/pass-data/test1.html create mode 100644 docs/views/examples/pass-data/test2.html diff --git a/app/data/session-data-defaults.js b/app/data/session-data-defaults.js index 9e4921b0ab..3b5158abb9 100644 --- a/app/data/session-data-defaults.js +++ b/app/data/session-data-defaults.js @@ -16,9 +16,47 @@ Example usage: ============================================================================ */ - module.exports = { - - // Insert values here - + // Example as used in current docs + claimant: { + field1: 'Example 1', + field2: 'Example 2', + field3: 'Example 3' + }, + partner: { + field1: 'Example 1', + field2: 'Example 2', + field3: 'Example 3' + }, + myObject: { + // Array of objects within object + myArrayOfObjects: [ + { + name: 'test1', + address: 'test2' + }, + { + name: 'test3' + } + ], + // Simple array within object + mySimpleArray: [ + 'test4', + 'test5' + ], + // Multi-level array within object + myMultiLevelArray: [ + [ + ['test6', 'test7'], + 'test8' + ] + ] + }, + // Arrays within array + myArray: [ + [ + 'test9', + ['test10', 'test11'] + ] + ] } diff --git a/docs/views/examples/pass-data/test1.html b/docs/views/examples/pass-data/test1.html new file mode 100644 index 0000000000..ac507c3399 --- /dev/null +++ b/docs/views/examples/pass-data/test1.html @@ -0,0 +1,47 @@ +{% extends "layout.html" %} + +{% block pageTitle %} + Example - Passing data +{% endblock %} + +{% block beforeContent %} + {% include "includes/breadcrumb_examples.html" %} +{% endblock %} + +{% block content %} + +
+
+ +
+ +
+

+ Example as used in current docs +

+ +

+ Array of objects within object +

+ +

+ Simple array within object +

+ +

+ Multi-level array within object +

+ +

+ Arrays within array +

+ +
+ + + +
+ +
+
+{% endblock %} diff --git a/docs/views/examples/pass-data/test2.html b/docs/views/examples/pass-data/test2.html new file mode 100644 index 0000000000..55b407b153 --- /dev/null +++ b/docs/views/examples/pass-data/test2.html @@ -0,0 +1,42 @@ +{% extends "layout.html" %} + +{% block pageTitle %} +Check your answers +{% endblock %} + +{% block content %} + +
+
+ +

Example as used in current docs

+ {{ data['claimant']['field1'] }} + {{ data['claimant']['field2'] }} +
+
+

Array of objects within object

+ {{ data['myObject']['myArrayOfObjects'][0]['name'] }} + {{ data['myObject']['myArrayOfObjects'][0]['address'] }} + {{ data['myObject']['myArrayOfObjects'][1]['name'] }} +
+
+

Simple array within object

+ {{ data['myObject']['mySimpleArray'][0] }} + {{ data['myObject']['mySimpleArray'][1] }} +
+
+

Multi-level array within object

+ {{ data['myObject']['myMultiLevelArray'][0][0][0] }} + {{ data['myObject']['myMultiLevelArray'][0][0][1] }} + {{ data['myObject']['myMultiLevelArray'][0][1] }} +
+
+

Arrays within array

+ {{ data['myArray'][0][0] }} + {{ data['myArray'][0][1][0] }} + {{ data['myArray'][0][1][1] }} +
+ +
+ +{% endblock %} From fb8a221e11c8994a1a981a1eae208cc4716aa418 Mon Sep 17 00:00:00 2001 From: Hanna Laakso Date: Fri, 5 Apr 2019 14:28:59 +0100 Subject: [PATCH 4/4] Add some simple tests Co-authored-by: Nick Colley --- __tests__/spec/data-storage.js | 76 ++++++++++++++++++++++++++++++++++ lib/utils.js | 4 +- 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 __tests__/spec/data-storage.js diff --git a/__tests__/spec/data-storage.js b/__tests__/spec/data-storage.js new file mode 100644 index 0000000000..e140e385f2 --- /dev/null +++ b/__tests__/spec/data-storage.js @@ -0,0 +1,76 @@ +/* eslint-env jest */ +const request = require('supertest') +const app = require('../../server.js') +const path = require('path') +const fs = require('fs') +const utils = require('../../lib/utils.js') + +jest.setTimeout(30000) + +function readFile (pathFromRoot) { + return fs.readFileSync(path.join(__dirname, '../../' + pathFromRoot), 'utf8') +} +/** + * + */ +describe('The data storage', () => { + // check session-data defaults file exists + + it.skip('file should exist', (done) => { + const sessionDataDefaultsFile = readFile('app/data/session-data-defaults.js') + + request(app) + .get(sessionDataDefaultsFile) + .expect('Content-Type', /application\/javascript; charset=UTF-8/) + // .expect(200) + .end(function (err, res) { + if (err) { + done(err) + } else { + done() + } + }) + }) + + describe('storeData function', () => { + it('should add input data into session data', async () => { + let initialSessionData = { + 'driver-name': 'Dr Emmett Brown' + } + + const inputData = { + 'vehicle-registration': 'OUTATIME' + } + + utils.storeData(inputData, initialSessionData) + + expect(initialSessionData).toEqual({ + 'driver-name': 'Dr Emmett Brown', + 'vehicle-registration': 'OUTATIME' + }) + }) + + it('should merge object into object', async () => { + let initialSessionData = { + vehicle: { + 'driver-name': 'Dr Emmett Brown' + } + } + + const inputData = { + vehicle: { + registration: 'OUTATIME' + } + } + + utils.storeData(inputData, initialSessionData) + + expect(initialSessionData).toEqual({ + vehicle: { + 'driver-name': 'Dr Emmett Brown', + registration: 'OUTATIME' + } + }) + }) + }) +}) diff --git a/lib/utils.js b/lib/utils.js index 068fddf2c4..ac76bd25a0 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -220,7 +220,7 @@ exports.matchMdRoutes = function (req, res) { } // Store data from POST body or GET query in session -var storeData = function (input, data) { +function storeData (input, data) { // Value we store to indicate unchecked checkboxes. See `auto-store-data.js` var uncheckedValue = '_unchecked' @@ -257,6 +257,8 @@ var storeData = function (input, data) { } } +exports.storeData = storeData + // Get session default data from file let sessionDataDefaults = {}