From c7fc565b241e5288e7df25ad53d72a142af4894e Mon Sep 17 00:00:00 2001 From: cliftonc Date: Thu, 12 Apr 2012 14:33:42 +0100 Subject: [PATCH] Major refactor of form parsing, submitted form data can now be validated back against the original form object (drupal style) --- app.js | 13 +- lib/calipso.js | 21 +-- lib/core/Form.js | 324 ++++++++++++++++++++++---------- lib/core/Module.js | 72 +++++-- modules/core/admin/admin.js | 5 +- modules/core/content/content.js | 2 +- modules/core/user/user.js | 16 +- package.json | 1 - 8 files changed, 305 insertions(+), 149 deletions(-) diff --git a/app.js b/app.js index 252e29904..f2059c4d5 100644 --- a/app.js +++ b/app.js @@ -15,7 +15,6 @@ var rootpath = process.cwd() + '/', express = require('express'), mongoose = require('mongoose'), nodepath = require('path'), - form = require('connect-form'), stylus = require('stylus'), colors = require('colors'), calipso = require(path.join(rootpath, 'lib/calipso')), @@ -52,19 +51,16 @@ function bootApplication(next) { app.config = new Config(); app.config.init(); + app.use(express.bodyParser()); app.use(express.methodOverride()); app.use(express.cookieParser()); app.use(express.responseTime()); // Create dummy session middleware - tag it so we can later replace - var temporarySession = function(req, res, next) { - req.session = {}; - next(); - }; + var temporarySession = express.session({ secret: "keyboard cat" }); temporarySession.tag = "session"; app.use(temporarySession); - // Default Theme calipso.defaultTheme = app.config.get('themes:default'); @@ -105,11 +101,6 @@ function bootApplication(next) { app.use(express["static"](path + '/media', {maxAge: 86400000})); app.use(express["static"](path + '/lib/client/js', {maxAge: 86400000})); - // connect-form - app.use(form({ - keepExtensions: app.config.get('libraries:formidable:keepExtensions') - })); - // Translation - after static, set to add mode if appropriate app.use(translate.translate(app.config.get('i18n:language'), app.config.get('i18n:languages'), app.config.get('i18n:additive'))); diff --git a/lib/calipso.js b/lib/calipso.js index ddaef7792..a733d829c 100644 --- a/lib/calipso.js +++ b/lib/calipso.js @@ -119,25 +119,8 @@ function router(app, initCallback) { // Initialise helpers - first pass calipso.getDynamicHelpers(req, res); - // Deal with any form content - // This is due to node-formidable / connect / express - // https://github.com/felixge/node-formidable/issues/30 - // Best to parse the form very early in the chain - if(req.form) { - - calipso.form.process(req, function() { - - // Route the modules - calipso.module.eventRouteModules(req, res, next); - - }); - - } else { - - // Route the modules - calipso.module.eventRouteModules(req, res, next); - - } + // Route the modules + calipso.module.eventRouteModules(req, res, next); }; } diff --git a/lib/core/Form.js b/lib/core/Form.js index d6b5649a0..a825a67a5 100644 --- a/lib/core/Form.js +++ b/lib/core/Form.js @@ -228,7 +228,7 @@ me.defaultTagRenderer = function(field, value, bare){ } return bare ? tagOutput : me.decorateField(field, tagOutput); - + }; me.decorateField = function(field, tagHTML){ @@ -710,16 +710,29 @@ me.render = function(formJson, values, req, next) { self.end_form(formJson) ); - if(typeof next === "function") { - next(form); // intention is to deprecate the asynch version - } else { - return form; - } + saveFormInSession(formJson, req, function(err) { + if(err) calipso.error(err.message); + next(form); + }) }); }; +// Helper to save a form in the session to be used later when processing. +function saveFormInSession(form, req, next) { + + // If we have a form id and a session, save it + if(form.id && calipso.lib._.keys(req.session).length > 0) { + calipso.silly("Saving form " + form.id + " in session."); + req.session.forms = req.session.forms || {}; + req.session.forms[form.id] = form; + req.session.save(next); + } else { + next(); + } +} + /** * Deal with form tabs in jQuery UI style if required. */ @@ -756,6 +769,7 @@ me.start_form = function(form) { return ( '
' + + '' + '
' + '

' + t(form.title) + '

' + '
' + @@ -868,55 +882,7 @@ me.render_fields = function(fieldContainer, values) { }; /** - * Process the values submitted by a form and return a JSON - * object representation (makes it simpler to then process a form submission - * from within a module. - */ -me.process = function(req, next) { - - // Form already processed - // Fix until all modules refactored to use formData - if(req.formProcessed) { - next(req.formData, req.uploadedFiles); - return; - } - - // Process form - if (req.form) { - - req.formData = {}; - req.uploadedFiles = {}; - - req.form - .on('field', function(field, value) { - // Fix for checkboxes - if(value === "true" || value === "on") value = true; - if(value === "false" || value === "off") value = false; - // Copy values to output object - copyFormToObject(field, value, req.formData); - }) - .on('file', function(field, file) { - req.uploadedFiles[field] = req.uploadedFiles[field] || []; - req.uploadedFiles[field].push(file); - }) - .on('end', function() { - replaceDates(req.formData); - req.formProcessed = true; - next(); - }); - - req.form.parse(req); - - } else { - - next(); - - } -}; - - -/** - * Recursive copy of object + * Get the value for a form field from the values object * @param from * @param to */ @@ -940,7 +906,8 @@ function getValueForField(field, values) { if(values && (typeof values.get === "function")) { values = values.get(key); } else { - return ''; + // May be a flat form (e.g. the config form) + return values[field] || ''; } } else { values = values[key]; @@ -954,6 +921,43 @@ function getValueForField(field, values) { } + + +/** + * Get the value for a form field from the values object + * @param from + * @param to + */ +function setValueForField(field, values, value) { + + if(!values) return ''; + + // First of all, split the field name into keys + var path = [] + if(field.match(/.*\]$/)) { + path = field.replace(/\]/g,"").split("["); + } else { + path = [field]; + } + + // + // Scope into the object to get the appropriate nested context + // + while (path.length > 1) { + key = path.shift(); + if (!values[key] || typeof values[key] !== 'object') { + values[key] = {}; + } + values = values[key]; + } + + // Set the specified value in the nested JSON structure + key = path.shift(); + values[key] = value; + return true; + +} + /** * Recursive copy of object * @param from @@ -994,7 +998,7 @@ function copyFormToObject(field, value, target) { * Process a field / section array from a contentType * And modify the form */ -me.processFields = function(form,fields) { +me.processFields = function(form, fields) { // Process fields if(fields.fields) { @@ -1093,71 +1097,197 @@ me.mapFields = function(fields, record) { }; +/** + * Process the values submitted by a form and return a JSON + * object representation (makes it simpler to then process a form submission + * from within a module. + */ +me.process = function(req, next) { + + // Fix until all modules refactored to use formData + if(req.formProcessed) { + + next(req.formData, req.uploadedFiles); + return; + + } else { + + // Data parsed based on original form structure + processFormData(req, function(err, formData) { + + if(err) calipso.error(err); + + req.formData = formData; + req.formProcessed = true; + + return next(req.formData, req.files); + + }); + + } + +}; + /** * Quick scan to replace any date structures * with actual dates so that each module doesn't need to. - * TODO - this all needs to be locale driven. - * TODO - this fairly assumes that all form fields will be within `sections:[]` - * - * This allows for dates to come back in a combination of forms: - * - * 1; All as properties {year,month,day,hours,minutes} - * 2; Date with time as properties {date,hours,minutes} - * 3; Date & time as properties {date,time} - * - * If none of the above (e.g. it is a full date time string), then it just goes - * straight to mongoose and gets dealt with there. */ -function replaceDates(output) { +function processFormData(req, next) { - for(var sectionName in output) { + if(calipso.lib._.keys(req.body).length === 0) { + // No data + return next(); + } - for(var fieldName in output[sectionName]) { + // Get the form id and then remove from the response + var formId = req.body.form ? req.body.form.id : ''; + delete req.body.form; - var field = output[sectionName][fieldName]; + // Get the form and then delete the form from the user session to clean up + var form = req.session.forms ? req.session.forms[formId] : null; + var formData = req.body; - // Full object year,month,day,hours,minutes - if(field.hasOwnProperty('year') && field.hasOwnProperty('hours')) { + if(formId && form) { - var now = new Date(); + processSectionData(form, formData, function(err, formData) { - output[sectionName][fieldName] = new Date( - (field.year || now.getFullYear()), - (field.month || now.getMonth()), - (field.day || now.getDate()), - (field.hours || now.getHours()), - (field.minutes || now.getMinutes()), - (field.seconds || now.getSeconds()) - ); + delete req.session.forms[formId]; - } + req.session.save(function(err) { + if(err) calipso.error(err.message); + }); // Doesn't matter that this is async, can happen in background + + return next(err, formData); + + }); - if(field.hasOwnProperty('date') && field.hasOwnProperty('hours')) { + } else { + + // No form in session, do not process + next(null, formData); - // TODO - LOCALE! - output[sectionName][fieldName] = new Date( - field.date + " " + field.hours + ":" + field.minutes + ":00" - ); + } - } +} - // Date entry - if(field.hasOwnProperty('date') && field.hasOwnProperty('time')) { +function processSectionData(form, formData, next) { - // TODO - LOCALE! - output[sectionName][fieldName] = new Date( - field.date + " " + field.time - ); + // Create a single array of all the form and section fields + var fields = []; + + if(form.sections) { + form.sections.forEach(function(section) { + fields.push(section.fields); + }); + } + if(form.fields) { + fields.push(form.fields); + } + calipso.lib.async.map(fields, function(section, cb) { + processFieldData(section, formData, cb); + }, function(err, result) { + next(err, formData); + }) + +} + +function processFieldData(fields, formData, next) { + + // First create an array of all the fields (and field sets) to allow us to do an async.map + var formFields = []; + + for(var fieldName in fields) { + // It is a section that contains fields + var field = fields[fieldName]; + if(field.fields) { + // This is a field set + for(var subFieldName in field.fields) { + formFields.push(field.fields[subFieldName]); } + } else { + // Just push the field + formFields.push(field) + } + } + + var iteratorFn = function(field, cb) { + + var value = getValueForField(field.name, formData); + + processFieldValue(field, value, function(err, processedValue) { + + if(value !== processedValue) setValueForField(field.name, formData, processedValue); + + cb(err, true); + + }); + } + + calipso.lib.async.map(formFields, iteratorFn, next); + +} + +function processFieldValue(field, value, next) { + + // Process each field + if(field.type === 'checkbox') { + + if(typeof value === 'object') { + // The value has come in as ['off','on'] or [false,true] + // So we always take the last value + value = value[value.length - 1]; } + // Deal with on off + if(value === 'on') value = true; + if(value === 'off') value = false; + } - return output; + if(field.type === 'select') { + // Deal with Yes / No > Boolean + if(value === 'Yes') value = true; + if(value === 'No') value = false; + + } + + if(field.type === 'datetime') { + + if(value.hasOwnProperty('date') && value.hasOwnProperty('time')) { + value = new Date( + value.date + " " + value.time + ); + } + + if(value.hasOwnProperty('date') && value.hasOwnProperty('hours')) { + value = new Date( + value.date + " " + value.hours + ":" + value.minutes + ":00" + ); + } + + if(value.hasOwnProperty('year') && value.hasOwnProperty('hours')) { + + var now = new Date(); + + value = new Date( + (value.year || now.getFullYear()), + (value.month || now.getMonth()), + (value.day || now.getDate()), + (value.hours || now.getHours()), + (value.minutes || now.getMinutes()), + (value.seconds || now.getSeconds()) + ); + + } + + } + + return next(null, value); + } /** diff --git a/lib/core/Module.js b/lib/core/Module.js index 2ea4903dc..a654eeb6c 100644 --- a/lib/core/Module.js +++ b/lib/core/Module.js @@ -31,10 +31,16 @@ function eventRouteModules(req, res, next) { // Later used to decide to show 404 res.routeMatched = false; - // Now, route each of the modules - for(var module in calipso.modules) { - routeModule(req, res, module, false, false, next); - } + // Route 'first' modules that fire before all others + // These first modules can stop the routing of all others + doFirstModules(req, res, function(err) { + + // Now, route each of the other modules + for(var module in calipso.modules) { + routeModule(req, res, module, false, false, next); + } + + }); } @@ -72,7 +78,7 @@ function routeModule(req, res, moduleName, depends, last, next) { // If module is enabled and has no dependencies, or if we are explicitly triggering this via depends // Ignore modules that are specified as post route only - if(module.enabled && (depends || !module.fn.depends) && (last || !module.fn.last)) { + if(module.enabled && (depends || !module.fn.depends) && (last || !module.fn.last) && !module.fn.first) { // Fire event to start req.event.modules[moduleName].route_start(); @@ -87,11 +93,11 @@ function routeModule(req, res, moduleName, depends, last, next) { res.errorMessage = "Module " + moduleName + ", error: " + err.message + err.stack; } - // Expose configuration if module has it - if(calipso.modules[moduleName].fn.config) { - var modulePermit = calipso.permission.Helper.hasPermission("admin:module:configuration"); - res.menu.admin.addMenuItem(req, {name:moduleName,path:'admin/modules/' + moduleName,url:'/admin/modules?module=' + moduleName,description:'Manage ' + moduleName + ' settings ...',permit:modulePermit}); - } + // Expose configuration if module has it + if(module.fn && module.fn.config) { + var modulePermit = calipso.permission.Helper.hasPermission("admin:module:configuration"); + res.menu.admin.addMenuItem(req, {name:moduleName,path:'admin/modules/' + moduleName,url:'/admin/modules?module=' + moduleName,description:'Manage ' + moduleName + ' settings ...',permit:modulePermit}); + } // Finish event req.event.modules[moduleName].route_finish(); @@ -103,6 +109,10 @@ function routeModule(req, res, moduleName, depends, last, next) { }); + } else { + + checkAllModulesRouted(req, res, next); + } } @@ -116,7 +126,7 @@ function checkAllModulesRouted(req, res, next) { var allRouted = true; for(var module in req.event.modules) { - var moduleRouted = (req.event.modules[module].routed || (calipso.modules[module].enabled && calipso.modules[module].fn.last) || !calipso.modules[module].enabled); + var moduleRouted = (req.event.modules[module].routed || (calipso.modules[module].enabled && (calipso.modules[module].fn.last || calipso.modules[module].fn.first)) || !calipso.modules[module].enabled); allRouted = moduleRouted && allRouted; } @@ -132,6 +142,46 @@ function checkAllModulesRouted(req, res, next) { } + +/** + * RUn any modules that are defined as first routing modules + * via first: true, dependencies are ignored for these. + */ +function doFirstModules(req, res, next) { + + // Get all the postRoute modules + var firstModules = []; + for(var moduleName in calipso.modules) { + if(calipso.modules[moduleName].enabled && calipso.modules[moduleName].fn.first) { + firstModules.push(calipso.modules[moduleName]); + } + } + + // Execute their routing functions + calipso.lib.step( + function doFirstModules() { + var group = this.group(); + firstModules.forEach(function(module) { + module.fn.route(req, res, module, calipso.app, group()); + }); + }, + function done(err) { + + // Gracefully deal with errors + if(err) { + res.statusCode = 500; + console.log(err.message); + res.errorMessage = err.message + err.stack; + } + + next(); + + } + ); + +} + + /** * RUn any modules that are defined as last routing modules * via last: true, dependencies are ignored for these atm. diff --git a/modules/core/admin/admin.js b/modules/core/admin/admin.js index c6c4df8a2..da59caf27 100644 --- a/modules/core/admin/admin.js +++ b/modules/core/admin/admin.js @@ -7,7 +7,8 @@ var rootpath = process.cwd() + '/', exports = module.exports = { init: init, - route: route + route: route, + first: true // Admin must run before all else }; /* @@ -214,6 +215,7 @@ function install(req, res, template, block, next) { calipso.form.process(req, function(form) { if (form) { + if(form.userStep) { // Store the user for later calipso.data.adminUser = form.user; @@ -583,7 +585,6 @@ function coreConfig(req, res, template, block, next) { } } - var adminForm = { id:'admin-form', title:'Administration', diff --git a/modules/core/content/content.js b/modules/core/content/content.js index 0adbe5a8b..8842b22bf 100644 --- a/modules/core/content/content.js +++ b/modules/core/content/content.js @@ -258,7 +258,7 @@ function homePage(req,res,template,block,next) { */ function createContent(req,res,template,block,next) { - calipso.form.process(req,function(form) { + calipso.form.process(req, function(form) { if(form) { diff --git a/modules/core/user/user.js b/modules/core/user/user.js index 8ed0e4fb5..9ca4c15a8 100644 --- a/modules/core/user/user.js +++ b/modules/core/user/user.js @@ -103,17 +103,19 @@ function init(module, app, next) { * personalized with javascript - a common strategy for aggressive page caching * TODO - fix a bug wherein the cookie gets set twice *if* on an admin page * TODO - make sure this is the appropriate place for setting this cookie + * TODO - need to fix overall what modules do when an earlier one has already issued a 302 ... */ function setCookie(req, res, template, block, next) { - if(req.session.user){ - if(!req.cookies.userData){ - res.cookie('userData', JSON.stringify(req.session.user)); + if(res.statusCode != 302) { + if(req.session.user) { + if(!req.cookies.userData){ + res.cookie('userData', JSON.stringify(req.session.user)); + } + } else { + res.clearCookie('userData'); } - } else { - res.clearCookie('userData'); } - next(); } @@ -627,7 +629,7 @@ function isUserAdmin(user) { // Set admin var isAdmin = false; user.roles.forEach(function(role) { - if(calipso.data.roles[role].isAdmin){ + if(calipso.data.roles[role] && calipso.data.roles[role].isAdmin){ isAdmin = true; } }) diff --git a/package.json b/package.json index 154b9992f..a9cac60d9 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,6 @@ "mongoose": "2.5.x", "winston": "0.5.x", "mongodb": "0.9.x", - "connect-form": "0.2.x", "qs": "0.4.x", "request":"2.9.x", "pool":"0.4.x",