Removed unused jsonErrorRenderer + renamed V2

- As of Ghost 5.0 we only use the V2 version of jsonErrorRenderer
- Removed the old one, and renamed the V2 to not have a suffix any more
- Added 100% coverage to tests whilst here

ghost/mw-error-handler/lib/mw-error-handler.js

@@ -105,32 +105,19 @@ module.exports.prepareStack = (err, req, res, next) => { // eslint-disable-line
 };
 
 const jsonErrorRenderer = (err, req, res, next) => { // eslint-disable-line no-unused-vars
-    res.json({
-        errors: [{
-            message: err.message,
-            context: err.context,
-            help: err.help,
-            errorType: err.errorType,
-            errorDetails: err.errorDetails,
-            ghostErrorCode: err.ghostErrorCode
-        }]
-    });
-};
-
-const jsonErrorRendererV2 = (err, req, res, next) => { // eslint-disable-line no-unused-vars
     const userError = prepareUserMessage(err, req);
 
     res.json({
         errors: [{
-            message: userError.message || null,
+            message: userError.message,
             context: userError.context || null,
             type: err.errorType || null,
             details: err.errorDetails || null,
             property: err.property || null,
             help: err.help || null,
             code: err.code || null,
-            id: err.id || null
-            // @TODO: add ghostErrorCode here in a major (I thought V2 was for the V2 api, but it's actually the newer/correct handler)
+            id: err.id || null,
+            ghostErrorCode: err.ghostErrorCode || null
         }]
     });
 };
@@ -231,9 +218,6 @@ module.exports.pageNotFound = (req, res, next) => {
     next(new errors.NotFoundError({message: tpl(messages.pageNotFound)}));
 };
 
-/**
- * @deprecated: not used as of Ghost v5 and should be removed in a major bump
- */
 module.exports.handleJSONResponse = sentry => [
     // Make sure the error can be served
     module.exports.prepareError,
@@ -245,21 +229,6 @@ module.exports.handleJSONResponse = sentry => [
     jsonErrorRenderer
 ];
 
-/**
- * @deprecated with above
- * @TODO: rename this without the v2 in a major bump
- */
-module.exports.handleJSONResponseV2 = sentry => [
-    // Make sure the error can be served
-    module.exports.prepareError,
-    // Handle the error in Sentry
-    sentry.errorHandler,
-    // Format the stack for the user
-    module.exports.prepareStack,
-    // Render the error using JSON format
-    jsonErrorRendererV2
-];
-
 module.exports.handleHTMLResponse = sentry => [
     // Make sure the error can be served
     module.exports.prepareError,

ghost/mw-error-handler/package.json

@@ -7,7 +7,7 @@
   "main": "index.js",
   "scripts": {
     "dev": "echo \"Implement me!\"",
-    "test": "NODE_ENV=testing c8 --all --check-coverage --reporter text --reporter cobertura mocha './test/**/*.test.js'",
+    "test": "NODE_ENV=testing c8 --all --check-coverage --100 --reporter text --reporter cobertura mocha './test/**/*.test.js'",
     "lint": "eslint . --ext .js --cache",
     "posttest": "yarn lint"
   },

ghost/mw-error-handler/test/error-handler.test.js

@@ -6,10 +6,10 @@ const {InternalServerError} = require('@tryghost/errors');
 const {
     prepareError,
     handleJSONResponse,
-    handleJSONResponseV2,
     handleHTMLResponse,
     prepareStack,
-    resourceNotFound
+    resourceNotFound,
+    pageNotFound
 } = require('../');
 
 describe('Prepare Error', function () {
@@ -23,6 +23,45 @@ describe('Prepare Error', function () {
             done();
         });
     });
+
+    it('Correctly prepares a 404 error', function (done) {
+        let error = {message: 'Oh dear', statusCode: 404};
+
+        prepareError(error, {}, {
+            set: () => {}
+        }, (err) => {
+            err.statusCode.should.eql(404);
+            err.name.should.eql('NotFoundError');
+            err.stack.should.startWith('NotFoundError: Resource could not be found');
+            err.hideStack.should.eql(true);
+            done();
+        });
+    });
+
+    it('Correctly prepares an error array', function (done) {
+        prepareError([new Error('test!')], {}, {
+            set: () => {}
+        }, (err) => {
+            err.statusCode.should.eql(500);
+            err.name.should.eql('InternalServerError');
+            err.stack.should.startWith('Error: test!');
+            done();
+        });
+    });
+
+    it('Correctly prepares a handlebars errpr', function (done) {
+        let error = new Error('obscure handlebars message!');
+        error.stack += '\nnode_modules/handlebars/something';
+
+        prepareError(error, {}, {
+            set: () => {}
+        }, (err) => {
+            err.statusCode.should.eql(400);
+            err.name.should.eql('IncorrectUsageError');
+            err.stack.should.startWith('Error: obscure handlebars message!');
+            done();
+        });
+    });
 });
 
 describe('Prepare Stack', function () {
@@ -50,22 +89,8 @@ describe('Error renderers', function () {
         }, () => {});
     });
 
-    it('Renders JSON for v2', function (done) {
-        const errorRenderer = handleJSONResponseV2({
-            errorHandler: () => {}
-        })[3];
-
-        errorRenderer(new Error('test!'), {}, {
-            json: (data) => {
-                data.errors.length.should.eql(1);
-                data.errors[0].message.should.eql('test!');
-                done();
-            }
-        }, () => {});
-    });
-
     it('Handles unknown errors when preparing user message', function (done) {
-        const errorRenderer = handleJSONResponseV2({
+        const errorRenderer = handleJSONResponse({
             errorHandler: () => {}
         })[3];
 
@@ -85,7 +110,7 @@ describe('Error renderers', function () {
     });
 
     it('Uses templates when required', function (done) {
-        const errorRenderer = handleJSONResponseV2({
+        const errorRenderer = handleJSONResponse({
             errorHandler: () => {}
         })[3];
 
@@ -100,6 +125,30 @@ describe('Error renderers', function () {
             json: (data) => {
                 data.errors.length.should.eql(1);
                 data.errors[0].message.should.eql('Internal server error, cannot list blog.');
+                data.errors[0].context.should.eql('test!');
+                done();
+            }
+        }, () => {});
+    });
+
+    it('Uses defined message + context when available', function (done) {
+        const errorRenderer = handleJSONResponse({
+            errorHandler: () => {}
+        })[3];
+
+        errorRenderer(new InternalServerError({
+            message: 'test!',
+            context: 'Image was too large.'
+        }), {
+            frameOptions: {
+                docName: 'images',
+                method: 'upload'
+            }
+        }, {
+            json: (data) => {
+                data.errors.length.should.eql(1);
+                data.errors[0].message.should.eql('Internal server error, cannot upload image.');
+                data.errors[0].context.should.eql('test! Image was too large.');
                 done();
             }
         }, () => {});
@@ -186,4 +235,22 @@ describe('Resource Not Found', function () {
             done();
         });
     });
+
+    describe('pageNotFound', function () {
+        it('returns 404 with special message when message not set', function (done) {
+            pageNotFound({}, {}, (error) => {
+                should.equal(error.statusCode, 404);
+                should.equal(error.message, 'Page not found');
+                done();
+            });
+        });
+
+        it('returns 404 with special message even if message is set', function (done) {
+            pageNotFound({message: 'uh oh'}, {}, (error) => {
+                should.equal(error.statusCode, 404);
+                should.equal(error.message, 'Page not found');
+                done();
+            });
+        });
+    });
 });