Add defaults file for auto store #475

mikeshawdev · 2018-01-08T16:48:24Z

Add the ability to provide default values to be added to session data. Key/value pairs that already exist in the session will not be overwritten.

Fixes #474

If provided, the app will try to merge data from a defaults file with data from the session. Defaults will only apply if there isn't a corresponding value already in the session, so data can be overridden within the prototype if necessary.

edwardhorsford

Thanks for working on this @mikeshawdev!

What do you think about having the kit create the autoDataDefaults file from a template if it doesn't already exist? We did this with the .env so that there's no risk of us overwriting it, but users upgrading to the kit still get the file.

edwardhorsford · 2018-01-08T17:41:40Z

app/defaults.js

+ * Provide default values for the user session. These are automatically added
+ * via the `autoStoreData` middleware. These values will only be added to the
+ * session, if a value doesn't already exist.
+ * @type {Object}


Could this comment include an example for how to use to set some data?

Added this comment

edwardhorsford · 2018-01-08T17:42:08Z

lib/utils.js

@@ -258,6 +258,19 @@ exports.autoStoreData = function (req, res, next) {
    req.session.data = {}
  }

+  try {
+    let defaultsFilePath = path.join(__dirname, '/../app/defaults.js')


Is this expected to set other defaults or just defaults for the autoDataStore? If the latter, I wonder if the name could be more specific.

It's just for the session data defaults, I've renamed the file to be more explicit

Moved `app/defaults.js` to `app/data/session-defaults.js`, which is a more obvious name and structure

If the session data defaults file doesn't exist on start, we create it from a template file

mikeshawdev · 2018-01-08T20:24:07Z

@edwardhorsford I think that's a great idea, my latest commit adds this behaviour to the kit

edwardhorsford · 2018-01-19T12:53:36Z

lib/utils.js

+      storeData(defaults, req.session)
+    }
+  } catch (e) {
+    console.error('Could not load defaults file')


Could this be more specific about what the defaults file is? Default user data file?

It could also specify the location it's looking for.

Removed this file as it would be generated by the app

Make comments on how to work with the file clearer

edwardhorsford · 2018-01-23T14:06:46Z

Thanks for all these changes @mikeshawdev.

I get an error when running it - I think related to the non existent app/data folder. Can you take a look? I guess you could either create the folder or put the file in app/.

Other than that, I've tested and it seems to work well. I added a few defaults to the file and they appear as normal. GETing new values then overwrites them.

One final thing to do would be to squash relevant commits.

joelanman · 2018-01-23T14:19:33Z

This all runs on every request, wonder if that's the best approach

joelanman · 2018-01-23T14:39:33Z

lib/utils.js

@@ -258,6 +258,19 @@ exports.autoStoreData = function (req, res, next) {
    req.session.data = {}
  }

+  try {
+    let defaultsFilePath = path.join(__dirname, '/../app/data/session-defaults.js')


Requiring the file can come out of the middleware, it only needs to happen once right?

Previously, the creation of the session defaults file failed if the `app/data` directory didn't exist. This commit adds a check for directory existence, and creates it if not already there

joelanman · 2018-01-23T19:07:33Z

Hope you don't mind, had a bash at a small refactor of the utils.js code:

// Get session default data from file
let sessionDefaults = {}

const defaultsFilePath = path.join(__dirname, '/../app/data/session-defaults.js')

try {
  sessionDefaults = require(defaultsFilePath)
} catch (e) {
  console.error('Could not load the session defaults from app/data/session-defaults.js. Might be a syntax error?')
}

// Middleware - store any data sent in session, and pass it to all views
exports.autoStoreData = function (req, res, next) {
  if (!req.session.data) {
    req.session.data = {}
  }

  req.session.data = Object.assign({}, sessionDefaults, req.session.data)

  storeData(req.body, req.session)
  storeData(req.query, req.session)

I don't think you need to send the defaults to storeData? That function does things like checking for _unchecked values which we wouldn't get in the defaults

mikeshawdev · 2018-01-24T09:26:17Z

@edwardhorsford cheers for looking it over! Once everyone is happy, I'll squash the commits

@joelanman That change looks good to me! You're right, the file code only needs to happen once! I'll add your code to the PR now

Previously, the session defaults file was required as part of the middleware. However, running this on every request is inefficient. This commit moves the require outside the middleware, so it's only required once when the app starts.

mikeshawdev · 2018-01-24T09:55:52Z

@edwardhorsford @joelanman changes added!

joelanman · 2018-08-15T09:37:32Z

Rebased and merged here: #501

Thanks @mikeshawdev !

Mike Shaw added 4 commits January 8, 2018 16:36

Add explanatory comment to defaults file

330abba

Add try/catch around defaults loading code

c756dc3

Fix linting error

0e04802

edwardhorsford suggested changes Jan 8, 2018

View reviewed changes

Mike Shaw added 2 commits January 8, 2018 20:09

Rename defaults file for clarity

69c7012

Moved `app/defaults.js` to `app/data/session-defaults.js`, which is a more obvious name and structure

Create session defaults file if it doesn't exist

2444521

If the session data defaults file doesn't exist on start, we create it from a template file

mikeshawdev changed the title ~~Feature/add defaults file for auto store~~ Add defaults file for auto store Jan 9, 2018

edwardhorsford reviewed Jan 19, 2018

View reviewed changes

Mike Shaw added 5 commits January 19, 2018 14:56

Add examples to session defaults file

4781257

Remove app session defaults file

43dc55c

Removed this file as it would be generated by the app

Update examples in session defaults template file

2cd5216

Provide clearer error message for session defaults error

ceaf811

Amend template session defaults file

1be2e8d

Make comments on how to work with the file clearer

joelanman suggested changes Jan 23, 2018

View reviewed changes

Fix issue when creating session defaults file

e525357

Previously, the creation of the session defaults file failed if the `app/data` directory didn't exist. This commit adds a check for directory existence, and creates it if not already there

Move session defaults require outside middleware

01dee9c

Previously, the session defaults file was required as part of the middleware. However, running this on every request is inefficient. This commit moves the require outside the middleware, so it's only required once when the app starts.

Merge branch 'master' into feature/add-defaults-file-for-auto-store

0141a88

joelanman approved these changes May 8, 2018

View reviewed changes

edwardhorsford approved these changes May 8, 2018

View reviewed changes

joelanman closed this Aug 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add defaults file for auto store #475

Add defaults file for auto store #475

mikeshawdev commented Jan 8, 2018

edwardhorsford left a comment

edwardhorsford Jan 8, 2018

mikeshawdev Jan 8, 2018

edwardhorsford Jan 8, 2018

mikeshawdev Jan 8, 2018

mikeshawdev commented Jan 8, 2018 •

edited

Loading

edwardhorsford Jan 19, 2018

edwardhorsford Jan 19, 2018

edwardhorsford commented Jan 23, 2018

joelanman commented Jan 23, 2018

joelanman Jan 23, 2018

joelanman commented Jan 23, 2018

mikeshawdev commented Jan 24, 2018 •

edited

Loading

mikeshawdev commented Jan 24, 2018

joelanman commented Aug 15, 2018

Add defaults file for auto store #475

Add defaults file for auto store #475

Conversation

mikeshawdev commented Jan 8, 2018

edwardhorsford left a comment

Choose a reason for hiding this comment

edwardhorsford Jan 8, 2018

Choose a reason for hiding this comment

mikeshawdev Jan 8, 2018

Choose a reason for hiding this comment

edwardhorsford Jan 8, 2018

Choose a reason for hiding this comment

mikeshawdev Jan 8, 2018

Choose a reason for hiding this comment

mikeshawdev commented Jan 8, 2018 • edited Loading

edwardhorsford Jan 19, 2018

Choose a reason for hiding this comment

edwardhorsford Jan 19, 2018

Choose a reason for hiding this comment

edwardhorsford commented Jan 23, 2018

joelanman commented Jan 23, 2018

joelanman Jan 23, 2018

Choose a reason for hiding this comment

joelanman commented Jan 23, 2018

mikeshawdev commented Jan 24, 2018 • edited Loading

mikeshawdev commented Jan 24, 2018

joelanman commented Aug 15, 2018

mikeshawdev commented Jan 8, 2018 •

edited

Loading

mikeshawdev commented Jan 24, 2018 •

edited

Loading