From 9249e203185e2e6917a589ec2b338f73ab0715e7 Mon Sep 17 00:00:00 2001 From: David Arvelo Date: Sat, 21 Jun 2014 17:59:12 -0400 Subject: [PATCH] Fix promise rejection fixes #3013 - bubble promise rejection on post model validation/save error so that it doesn't bubble as resolved - prefer `.catch()` for promise handlers as per recommendations - check if `.save()` is being called from model.destroyRecord() and forgo validation if so - normalize output for both `.validate()` and `.save()` --- controllers/modals/leave-editor.js | 2 +- controllers/post-settings-menu.js | 9 +++-- controllers/posts/post.js | 1 + mixins/editor-base-controller.js | 6 ++-- mixins/validation-engine.js | 56 +++++++++++++++++++----------- 5 files changed, 47 insertions(+), 27 deletions(-) diff --git a/controllers/modals/leave-editor.js b/controllers/modals/leave-editor.js index 6faee8446..e12d02a64 100644 --- a/controllers/modals/leave-editor.js +++ b/controllers/modals/leave-editor.js @@ -14,8 +14,8 @@ var LeaveEditorController = Ember.Controller.extend({ model = editorController.get('model'); } - // @TODO: throw some kind of error here? return true will send it upward? if (!transition || !editorController) { + this.notifications.showError('Sorry, there was an error in the application. Please let the Ghost team know what happened.'); return true; } diff --git a/controllers/post-settings-menu.js b/controllers/post-settings-menu.js index 845df7216..65b48f71e 100644 --- a/controllers/post-settings-menu.js +++ b/controllers/post-settings-menu.js @@ -23,8 +23,9 @@ var PostSettingsMenuController = Ember.ObjectController.extend({ self.notifications.showSuccess('Successfully converted to ' + (val ? 'static page' : 'post')); return self.get('page'); - }, function (errors) { + }).catch(function (errors) { self.notifications.showErrors(errors); + return Ember.RSVP.reject(errors); }); } @@ -137,8 +138,9 @@ var PostSettingsMenuController = Ember.ObjectController.extend({ return self.get('model').save().then(function () { self.notifications.showSuccess('Permalink successfully changed to ' + self.get('slug') + '.'); - }, function (errors) { + }).catch(function (errors) { self.notifications.showErrors(errors); + return Ember.RSVP.reject(errors); }); }); }, @@ -190,8 +192,9 @@ var PostSettingsMenuController = Ember.ObjectController.extend({ this.get('model').save().then(function () { self.notifications.showSuccess('Publish date successfully changed to ' + formatDate(self.get('published_at')) + '.'); - }, function (errors) { + }).catch(function (errors) { self.notifications.showErrors(errors); + return Ember.RSVP.reject(errors); }); } } diff --git a/controllers/posts/post.js b/controllers/posts/post.js index ac30b72fd..4e9a708a5 100644 --- a/controllers/posts/post.js +++ b/controllers/posts/post.js @@ -11,6 +11,7 @@ var PostController = Ember.ObjectController.extend({ self.notifications.showSuccess('Post successfully marked as ' + (featured ? 'featured' : 'not featured') + '.'); }).catch(function (errors) { self.notifications.showErrors(errors); + return Ember.RSVP.reject(errors); }); } } diff --git a/mixins/editor-base-controller.js b/mixins/editor-base-controller.js index f6d664ae9..1456a92c7 100644 --- a/mixins/editor-base-controller.js +++ b/mixins/editor-base-controller.js @@ -109,7 +109,6 @@ var EditorControllerMixin = Ember.Mixin.create(MarkerManager, { actions: { save: function () { var status = this.get('willPublish') ? 'published' : 'draft', - model = this.get('model'), self = this; // set markdown equal to what's in the editor, minus the image markers. @@ -117,7 +116,7 @@ var EditorControllerMixin = Ember.Mixin.create(MarkerManager, { this.set('status', status); - return model.save().then(function () { + return this.get('model').save().then(function (model) { model.updateTags(); // `updateTags` triggers `isDirty => true`. // for a saved model it would otherwise be false. @@ -126,8 +125,9 @@ var EditorControllerMixin = Ember.Mixin.create(MarkerManager, { self.notifications.showSuccess('Post status saved as ' + model.get('status') + '.'); return model; - }, function (errors) { + }).catch(function (errors) { self.notifications.showErrors(errors); + return Ember.RSVP.reject(errors); }); }, diff --git a/mixins/validation-engine.js b/mixins/validation-engine.js index 611e89c3f..263a77f73 100644 --- a/mixins/validation-engine.js +++ b/mixins/validation-engine.js @@ -17,7 +17,7 @@ var ValidationEngine = Ember.Mixin.create({ return new Ember.RSVP.Promise(function (resolve, reject) { if (!type || !validator) { - return reject('The validator specified, "' + type + '", did not exist!'); + return reject(self.formatErrors('The validator specified, "' + type + '", did not exist!')); } var validationErrors = validator.validate(self); @@ -26,10 +26,33 @@ var ValidationEngine = Ember.Mixin.create({ return resolve(); } - return reject(validationErrors); + return reject(self.formatErrors(validationErrors)); }); }, + // format errors to be used in `notifications.showErrors`. + // format is [{ message: 'concatenated error messages' }] + formatErrors: function (errors) { + var message = 'There was an error saving this ' + this.get('validationType'); + + if (Ember.isArray(errors)) { + // get validation error messages + message += ': ' + errors.mapBy('message').join(' '); + } else if (typeof errors === 'object') { + // Get messages from server response + message += ': ' + getRequestErrorMessage(errors); + } else if (typeof errors === 'string') { + message += ': ' + errors; + } else { + message += '.'; + } + + // set format for notifications.showErrors + message = [{ message: message }]; + + return message; + }, + // override save to do validation first save: function () { var self = this, @@ -37,31 +60,24 @@ var ValidationEngine = Ember.Mixin.create({ // ref: https://github.com/emberjs/ember.js/pull/4301 _super = this.__nextSuper; + // model.destroyRecord() calls model.save() behind the scenes. + // in that case, we don't need validation checks or error propagation. + if (this.get('isDeleted')) { + return this._super(); + } + // If validation fails, reject with validation errors. // If save to the server fails, reject with server response. return this.validate().then(function () { return _super.call(self); }).catch(function (result) { - var message = 'There was an error saving this ' + self.get('validationType'); - - if (Ember.isArray(result)) { - // get validation error messages - message += ': ' + result.mapBy('message').join(' '); - } else if (typeof result === 'object') { - // Get messages from server response - message += ': ' + getRequestErrorMessage(result); - } else if (typeof result === 'string') { - message += ': ' + result; - } else { - message += '.'; + // server save failed, format the errors and reject the promise. + // if validations failed, the errors will already be formatted for us. + if (! Ember.isArray(result)) { + result = self.formatErrors(result); } - // set format for notifications.showErrors - message = [{ message: message }]; - - return new Ember.RSVP.Promise(function (resolve, reject) { - reject(message); - }); + return Ember.RSVP.reject(result); }); } });