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

Allow nested field values in session #573

Merged
merged 1 commit into from
Sep 7, 2018

Conversation

colinrotherham
Copy link
Contributor

Hello!

We've got a service where we use the same nunjucks views to collect data for both claimants and optionally their partners.

E.g.

<!-- Claimant fields -->
<input name="claimant[field1]" value="Example 1">
<input name="claimant[field2]" value="Example 2">
<input name="claimant[field3]" value="Example 3">

<!-- Partner fields -->
<input name="partner[field1]" value="Example 1">
<input name="partner[field2]" value="Example 2">
<input name="partner[field3]" value="Example 3">

With this pull request useAutoStoreData will store the data correctly 👍

session: {
  data: {
    claimant: {
      field1: 'Example 1',
      field2: 'Example 2',
      field3: 'Example 3'
    },
    partner: {
      field1: 'Example 1',
      field2: 'Example 2',
      field3: 'Example 3'
    }
  }
}

i.e. Nicely grouped as claimant and partner as specified in the HTML.

We also regularly switch express-session with client-sessions (cookie vs. memory store) but it doesn't have the session.destroy() method. I've added a little compatibility change for this too.

It'll help other teams also wanting to use another session store that survives Node.js restarts.

@colinrotherham
Copy link
Contributor Author

Useful for other fields too…

<input name="address[line1]">
<input name="address[line2]">
<input name="address[town]">
<input name="address[postcode]">

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Aug 2, 2018

Pushed another change up as this affects the nunjucks checked() function.

It now supports dot or bracket notation:

// Flat name/value pairs
checked("example1", "true")
checked("example2", "true")

// Nested name/value pairs (mixed notation)
checked("[parent1].example1", "true")
checked("[parent1].example2", "true")
checked("[parent2].example1", "true")
checked("[parent2].example2", "true")

// Nested name/value pairs (bracket notation, quoted)
checked("['parent-hyphenated-1']['example-1']", "true")
checked("['parent-hyphenated-1']['example-2']", "true")
checked("['parent-hyphenated-2']['example-1']", "true")
checked("['parent-hyphenated-2']['example-2']", "true")

Object notation as strings is hard to parse, so I've used keypather here.

@NickColley
Copy link
Contributor

Hey Colin,

This looks like a useful addition, my only thought is:

How will users know about this feature? Is there any guidance we need to add to the documentation?

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Aug 21, 2018

Hi @NickColley,

Glad you like it. I've gone ahead and added documented in /docs/session:

accessing fields

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Aug 21, 2018

Example added for usage of nunjucks checked() with nested fields in /docs/examples/pass-data:

Usage in views

screen shot 2018-08-21 at 16 00 14

Usage in checked()

screen shot 2018-08-21 at 16 00 23

Usage in checked() (full macro example)

screen shot 2018-08-21 at 16 00 34

Usage in route functions

screen shot 2018-08-21 at 16 00 43

@colinrotherham
Copy link
Contributor Author

Add new entry to changelog

@joelanman
Copy link
Contributor

One thing I'm thinking about - we generally tell people to use [''] notation (for example data['first-name'], as that supports hyphens and possibly even spaces in name, instead of teaching people to use camelCase in some situations.

Could this work using [''] notation?

@colinrotherham
Copy link
Contributor Author

Hi @joelanman,

I've changed all the examples to use [''] notation.

Some screenshots from the documentation have been edited above.

Also #573 (comment) has been updated to show both dot and bracket notation examples.

@colinrotherham
Copy link
Contributor Author

Rebased with latest from master

@colinrotherham
Copy link
Contributor Author

Rebased with latest from master (more!)

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Hi @colinrotherham

This looks really useful, thanks for picking it up 👍 I've left some comments.

lib/utils.js Outdated Show resolved Hide resolved
docs/documentation/session.md Show resolved Hide resolved
server.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
@colinrotherham
Copy link
Contributor Author

Hi @hannalaakso, cheers again for the review.

I've replied to all your points and pushed a new version up 😊

E.g.

<input name="example1[name]" value="Hello 123">
<input name="example2[name]" value="Hello 456">
<input name="example3[name]" value="Hello 789">
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Hi @colinrotherham

Thanks for addressing my comments. I've tested the new version and it's looking good 🙂

Just need another person in the team to review this as well before we can merge it.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

4 participants