From b9f9c576edf3e10a3e14b65451d064ac16cca475 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Tue, 24 Jun 2014 06:33:24 +0000 Subject: [PATCH] Enable validation for settings/general screen Closes #3036 Refs #3012 -Enable validation for settings/general -Turn on functional tests for the validations -Move notification closeAll calls so that notifications are cleared on attempted saves instead of just on successful saves --- core/client/controllers/settings/general.js | 13 +- core/client/controllers/settings/user.js | 5 +- core/client/mixins/editor-base-controller.js | 6 +- core/client/mixins/validation-engine.js | 4 +- core/client/models/setting.js | 6 +- core/client/validators/setting.js | 33 ++++++ core/test/functional/client/settings_test.js | 118 +++++++++---------- 7 files changed, 115 insertions(+), 70 deletions(-) create mode 100644 core/client/validators/setting.js diff --git a/core/client/controllers/settings/general.js b/core/client/controllers/settings/general.js index c3486a5a46..56c33f89fb 100644 --- a/core/client/controllers/settings/general.js +++ b/core/client/controllers/settings/general.js @@ -30,13 +30,18 @@ var SettingsGeneralController = Ember.ObjectController.extend({ save: function () { var self = this; + // @TODO This should call closePassive() to only close passive notifications + self.notifications.closeAll(); + return this.get('model').save().then(function (model) { - // @TODO This should call closePassive() to only close passive notifications - self.notifications.closeAll(); - self.notifications.showSuccess('Settings successfully saved.'); + return model; - }).catch(this.notifications.showErrors); + }).catch(function (errors) { + self.notifications.showErrors(errors); + + return Ember.RSVP.reject(errors); + }); }, } }); diff --git a/core/client/controllers/settings/user.js b/core/client/controllers/settings/user.js index 4c80a1af52..54c4a3a495 100644 --- a/core/client/controllers/settings/user.js +++ b/core/client/controllers/settings/user.js @@ -23,13 +23,14 @@ var SettingsUserController = Ember.Controller.extend({ save: function () { var self = this; + // @TODO This should call closePassive() to only close passive notifications + self.notifications.closeAll(); + alert('@TODO: Saving user...'); if (this.user.validate().get('isValid')) { this.user.save().then(function (response) { - // @TODO This should call closePassive() to only close passive notifications - self.notifications.closeAll(); alert('Done saving' + JSON.stringify(response)); }, function () { alert('Error saving.'); diff --git a/core/client/mixins/editor-base-controller.js b/core/client/mixins/editor-base-controller.js index 98f9e61471..37f256f79e 100644 --- a/core/client/mixins/editor-base-controller.js +++ b/core/client/mixins/editor-base-controller.js @@ -124,6 +124,9 @@ var EditorControllerMixin = Ember.Mixin.create(MarkerManager, { isNew = this.get('isNew'), self = this; + // @TODO This should call closePassive() to only close passive notifications + self.notifications.closeAll(); + // ensure an incomplete tag is finalised before save this.get('controllers.post-tags-input').send('addNewTag'); @@ -137,9 +140,6 @@ var EditorControllerMixin = Ember.Mixin.create(MarkerManager, { // for a saved model it would otherwise be false. self.set('isDirty', false); - // @TODO This should call closePassive() to only close passive notifications - self.notifications.closeAll(); - self.notifications.showSuccess('Post status saved as ' + model.get('status') + '.', isNew ? true : false); return model; diff --git a/core/client/mixins/validation-engine.js b/core/client/mixins/validation-engine.js index f573d20909..b5f0afbd70 100644 --- a/core/client/mixins/validation-engine.js +++ b/core/client/mixins/validation-engine.js @@ -5,6 +5,7 @@ import PostValidator from 'ghost/validators/post'; import SignupValidator from 'ghost/validators/signup'; import SigninValidator from 'ghost/validators/signin'; import ForgotValidator from 'ghost/validators/forgotten'; +import SettingValidator from 'ghost/validators/setting'; ValidatorExtensions.init(); @@ -13,7 +14,8 @@ var ValidationEngine = Ember.Mixin.create({ post: PostValidator, signup: SignupValidator, signin: SigninValidator, - forgotten: ForgotValidator + forgotten: ForgotValidator, + setting: SettingValidator }, validate: function (opts) { diff --git a/core/client/models/setting.js b/core/client/models/setting.js index d77930c302..a3857c8708 100644 --- a/core/client/models/setting.js +++ b/core/client/models/setting.js @@ -1,4 +1,8 @@ -var Setting = DS.Model.extend({ +import ValidationEngine from 'ghost/mixins/validation-engine'; + +var Setting = DS.Model.extend(ValidationEngine, { + validationType: 'setting', + title: DS.attr('string'), description: DS.attr('string'), email: DS.attr('string'), diff --git a/core/client/validators/setting.js b/core/client/validators/setting.js new file mode 100644 index 0000000000..9a61946c64 --- /dev/null +++ b/core/client/validators/setting.js @@ -0,0 +1,33 @@ +var SettingValidator = Ember.Object.create({ + validate: function (model) { + var validationErrors = [], + title = model.get('title'), + description = model.get('description'), + email = model.get('email'), + postsPerPage = model.get('postsPerPage'); + + if (!validator.isLength(title, 0, 150)) { + validationErrors.push({ message: 'Title is too long' }); + } + + if (!validator.isLength(description, 0, 200)) { + validationErrors.push({ message: 'Description is too long' }); + } + + if (!validator.isEmail(email) || !validator.isLength(email, 0, 254)) { + validationErrors.push({ message: 'Please supply a valid email address' }); + } + + if (!validator.isInt(postsPerPage) || postsPerPage > 1000) { + validationErrors.push({ message: 'Please use a number less than 1000' }); + } + + if (!validator.isInt(postsPerPage) || postsPerPage < 0) { + validationErrors.push({ message: 'Please use a number greater than 0' }); + } + + return validationErrors; + } +}); + +export default SettingValidator; diff --git a/core/test/functional/client/settings_test.js b/core/test/functional/client/settings_test.js index ede21442a5..217d11d9b9 100644 --- a/core/test/functional/client/settings_test.js +++ b/core/test/functional/client/settings_test.js @@ -110,65 +110,65 @@ CasperTest.emberBegin('General settings pane is correct', 8, function suite(test }); //// ## General settings validations tests -//CasperTest.emberBegin('General settings validation is correct', 7, function suite(test) { -// casper.thenOpenAndWaitForPageLoad('settings.general', function testTitleAndUrl() { -// test.assertTitle('Ghost Admin', 'Ghost admin has no title'); -// test.assertUrlMatch(/ghost\/ember\/settings\/general\/$/, 'Landed on the correct URL'); -// }); -// -// // Ensure general blog title field length validation -// casper.fillAndSave('form#settings-general', { -// 'general[title]': new Array(152).join('a') -// }); -// -// casper.waitForSelectorTextChange('.notification-error', function onSuccess() { -// test.assertSelectorHasText('.notification-error', 'too long'); -// }, casper.failOnTimeout(test, 'Blog title length error did not appear'), 2000); -// -// casper.thenClick('.js-bb-notification .close'); -// -// // Ensure general blog description field length validation -// casper.fillAndSave('form#settings-general', { -// 'general[description]': new Array(202).join('a') -// }); -// -// casper.waitForSelectorTextChange('.notification-error', function onSuccess() { -// test.assertSelectorHasText('.notification-error', 'too long'); -// }, casper.failOnTimeout(test, 'Blog description length error did not appear')); -// -// casper.thenClick('.js-bb-notification .close'); -// -// // Ensure postsPerPage number field form validation -// casper.fillAndSave('form#settings-general', { -// 'general[postsPerPage]': 'notaninteger' -// }); -// -// casper.waitForSelectorTextChange('.notification-error', function onSuccess() { -// test.assertSelectorHasText('.notification-error', 'use a number'); -// }, casper.failOnTimeout(test, 'postsPerPage error did not appear'), 2000); -// -// casper.thenClick('.js-bb-notification .close'); -// -// // Ensure postsPerPage max of 1000 -// casper.fillAndSave('form#settings-general', { -// 'general[postsPerPage]': '1001' -// }); -// -// casper.waitForSelectorTextChange('.notification-error', function onSuccess() { -// test.assertSelectorHasText('.notification-error', 'use a number less than 1000'); -// }, casper.failOnTimeout(test, 'postsPerPage max error did not appear', 2000)); -// -// casper.thenClick('.js-bb-notification .close'); -// -// // Ensure postsPerPage min of 0 -// casper.fillAndSave('form#settings-general', { -// 'general[postsPerPage]': '-1' -// }); -// -// casper.waitForSelectorTextChange('.notification-error', function onSuccess() { -// test.assertSelectorHasText('.notification-error', 'use a number greater than 0'); -// }, casper.failOnTimeout(test, 'postsPerPage min error did not appear', 2000)); -//}); +CasperTest.emberBegin('General settings validation is correct', 7, function suite(test) { + casper.thenOpenAndWaitForPageLoad('settings.general', function testTitleAndUrl() { + test.assertTitle('Ghost Admin', 'Ghost admin has no title'); + test.assertUrlMatch(/ghost\/ember\/settings\/general\/$/, 'Landed on the correct URL'); + }); + + // Ensure general blog title field length validation + casper.fillAndSave('form#settings-general', { + 'general[title]': new Array(152).join('a') + }); + + casper.waitForSelectorTextChange('.notification-error', function onSuccess() { + test.assertSelectorHasText('.notification-error', 'too long'); + }, casper.failOnTimeout(test, 'Blog title length error did not appear'), 2000); + + casper.thenClick('.js-bb-notification .close'); + + // Ensure general blog description field length validation + casper.fillAndSave('form#settings-general', { + 'general[description]': new Array(202).join('a') + }); + + casper.waitForSelectorTextChange('.notification-error', function onSuccess() { + test.assertSelectorHasText('.notification-error', 'too long'); + }, casper.failOnTimeout(test, 'Blog description length error did not appear')); + + casper.thenClick('.js-bb-notification .close'); + + // Ensure postsPerPage number field form validation + casper.fillAndSave('form#settings-general', { + 'general[postsPerPage]': 'notaninteger' + }); + + casper.waitForSelectorTextChange('.notification-error', function onSuccess() { + test.assertSelectorHasText('.notification-error', 'use a number'); + }, casper.failOnTimeout(test, 'postsPerPage error did not appear'), 2000); + + casper.thenClick('.js-bb-notification .close'); + + // Ensure postsPerPage max of 1000 + casper.fillAndSave('form#settings-general', { + 'general[postsPerPage]': '1001' + }); + + casper.waitForSelectorTextChange('.notification-error', function onSuccess() { + test.assertSelectorHasText('.notification-error', 'use a number less than 1000'); + }, casper.failOnTimeout(test, 'postsPerPage max error did not appear', 2000)); + + casper.thenClick('.js-bb-notification .close'); + + // Ensure postsPerPage min of 0 + casper.fillAndSave('form#settings-general', { + 'general[postsPerPage]': '-1' + }); + + casper.waitForSelectorTextChange('.notification-error', function onSuccess() { + test.assertSelectorHasText('.notification-error', 'use a number greater than 0'); + }, casper.failOnTimeout(test, 'postsPerPage min error did not appear', 2000)); +}); // ### User settings tests // Please uncomment and fix these as the functionality is implemented