From 4abc79c0b560cdc53058697329ce212730f869ca Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 24 Aug 2016 13:45:54 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20Improve=20theme=20validation=20e?= =?UTF-8?q?rror=20messaging=20(#7253)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs #7204 - Adds a new ThemeValidationError class - This error has a top level message, but will also contain all the individual errors within the `errorDetails` property - Updated the API error handling to return `errorDetails` if it is present --- core/client | 2 +- core/server/api/themes.js | 20 ++++--------------- core/server/errors/index.js | 7 +++++++ core/server/errors/theme-validation-error.js | 18 +++++++++++++++++ core/server/translations/en.json | 2 +- .../test/functional/routes/api/themes_spec.js | 4 +++- 6 files changed, 34 insertions(+), 19 deletions(-) create mode 100644 core/server/errors/theme-validation-error.js diff --git a/core/client b/core/client index a14ac50b6c3a..eb0db3fafe15 160000 --- a/core/client +++ b/core/client @@ -1 +1 @@ -Subproject commit a14ac50b6c3afd96cdf6e176f61aadfd00407492 +Subproject commit eb0db3fafe15dfc1b246a955811b61fac231c288 diff --git a/core/server/api/themes.js b/core/server/api/themes.js index b3f86223365a..993e9af37041 100644 --- a/core/server/api/themes.js +++ b/core/server/api/themes.js @@ -47,22 +47,10 @@ themes = { return; } - var validationErrors = []; - _.each(theme.results.error, function (error) { - if (error.failures) { - _.each(error.failures, function (childError) { - validationErrors.push(new errors.ValidationError(i18n.t('errors.api.themes.invalidTheme', { - reason: childError.ref - }))); - }); - } - - validationErrors.push(new errors.ValidationError(i18n.t('errors.api.themes.invalidTheme', { - reason: error.rule - }))); - }); - - throw validationErrors; + throw new errors.ThemeValidationError( + i18n.t('errors.api.themes.invalidTheme'), + theme.results.error + ); }) .then(function () { return storageAdapter.exists(config.paths.themePath + '/' + zip.shortName); diff --git a/core/server/errors/index.js b/core/server/errors/index.js index f89b906ed531..48efa66fdec8 100644 --- a/core/server/errors/index.js +++ b/core/server/errors/index.js @@ -13,6 +13,7 @@ var _ = require('lodash'), RequestEntityTooLargeError = require('./request-too-large-error'), UnauthorizedError = require('./unauthorized-error'), ValidationError = require('./validation-error'), + ThemeValidationError = require('./theme-validation-error'), UnsupportedMediaTypeError = require('./unsupported-media-type-error'), EmailError = require('./email-error'), DataImportError = require('./data-import-error'), @@ -245,6 +246,11 @@ errors = { errorContent.message = _.isString(errorItem) ? errorItem : (_.isObject(errorItem) ? errorItem.message : i18n.t('errors.errors.unknownApiError')); errorContent.errorType = errorItem.errorType || 'InternalServerError'; + + if (errorItem.errorType === 'ThemeValidationError' && errorItem.errorDetails) { + errorContent.errorDetails = errorItem.errorDetails; + } + errors.push(errorContent); }); @@ -447,6 +453,7 @@ module.exports.InternalServerError = InternalServerError; module.exports.NoPermissionError = NoPermissionError; module.exports.UnauthorizedError = UnauthorizedError; module.exports.ValidationError = ValidationError; +module.exports.ThemeValidationError = ThemeValidationError; module.exports.RequestEntityTooLargeError = RequestEntityTooLargeError; module.exports.UnsupportedMediaTypeError = UnsupportedMediaTypeError; module.exports.EmailError = EmailError; diff --git a/core/server/errors/theme-validation-error.js b/core/server/errors/theme-validation-error.js new file mode 100644 index 000000000000..93ddd7c820d9 --- /dev/null +++ b/core/server/errors/theme-validation-error.js @@ -0,0 +1,18 @@ +// # Theme Validation Error +// Custom error class with status code and type prefilled. + +function ThemeValidationError(message, details) { + this.message = message; + this.stack = new Error().stack; + this.statusCode = 422; + if (details) { + this.errorDetails = details; + } + + this.errorType = this.name; +} + +ThemeValidationError.prototype = Object.create(Error.prototype); +ThemeValidationError.prototype.name = 'ThemeValidationError'; + +module.exports = ThemeValidationError; diff --git a/core/server/translations/en.json b/core/server/translations/en.json index 129426d608ed..7b5c0c55ff3a 100644 --- a/core/server/translations/en.json +++ b/core/server/translations/en.json @@ -358,7 +358,7 @@ "noPermissionToBrowseThemes": "You do not have permission to browse themes.", "noPermissionToEditThemes": "You do not have permission to edit themes.", "themeDoesNotExist": "Theme does not exist.", - "invalidTheme": "Theme is invalid: {reason}", + "invalidTheme": "Theme is not compatible or contains errors.", "missingFile": "Please select a theme.", "invalidFile": "Please select a valid zip file.", "overrideCasper": "Please rename your zip, it's not allowed to override the default casper theme.", diff --git a/core/test/functional/routes/api/themes_spec.js b/core/test/functional/routes/api/themes_spec.js index b69bbd86ac43..9c786a801036 100644 --- a/core/test/functional/routes/api/themes_spec.js +++ b/core/test/functional/routes/api/themes_spec.js @@ -156,7 +156,8 @@ describe('Themes API', function () { res.statusCode.should.eql(422); res.body.errors.length.should.eql(1); - res.body.errors[0].message.should.eql('Theme is invalid: A template file called post.hbs must be present.'); + res.body.errors[0].errorType.should.eql('ThemeValidationError'); + res.body.errors[0].message.should.eql('Theme is not compatible or contains errors.'); done(); }); }); @@ -170,6 +171,7 @@ describe('Themes API', function () { res.statusCode.should.eql(422); res.body.errors.length.should.eql(1); + res.body.errors[0].errorType.should.eql('ValidationError'); res.body.errors[0].message.should.eql('Please rename your zip, it\'s not allowed to override the default casper theme.'); done(); });