🎨 Improve theme validation error messaging (TryGhost#7253)

refs TryGhost#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
mixonic · Oct 28, 2016 · 4abc79c · 4abc79c
1 parent f4828c1
commit 4abc79c
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 19 deletions.
diff --git a/core/client b/core/client
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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();
                 });