Merge pull request #2071 from alphagov/tidier-more-informative-errors

Tidier more informative errors
alphagov · Mar 30, 2023 · 7583cef · 7583cef
2 parents ed4c765 + 2917c64
commit 7583cef
Show file tree

Hide file tree

Showing 11 changed files with 207 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,11 @@
 # Changelog
 
 ## Unreleased
+- [#2039: Design error handling](https://github.com/alphagov/govuk-prototype-kit/issues/2039)
+  - Implement new Error pages for 404 and 500 error's.
+
+- [#2039: Display stack trace in browser when there's an error](https://github.com/alphagov/govuk-prototype-kit/issues/2038)
+  - Display error stack for on Server Error page.
 
 ### New features
 

diff --git a/__tests__/fixtures/mockNunjucksIncludes/govuk/template.njk b/__tests__/fixtures/mockNunjucksIncludes/govuk/template.njk
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<html>
+<head><title>{% block pageTitle %}{% endblock %}</title></head>
+<body><main{% if mainClasses %} class="{{ mainClasses }}"{% endif %}>{% block content %}{% endblock %}</main></body>
+</html>
+
+
diff --git a/__tests__/spec/errors.js b/__tests__/spec/errors.js
@@ -2,6 +2,7 @@
 
 // npm dependencies
 const request = require('supertest')
+const path = require('path')
 
 // local dependencies
 const { sleep } = require('../../lib/utils')
@@ -12,17 +13,43 @@ process.env.IS_INTEGRATION_TEST = 'true'
 
 let kitRoutesApi
 
+const getPageTitle = html => {
+  const match = html.match(/<title>((.|\n|\r)*)<\/title>/)
+  if (match) return match[1].trim()
+}
+const getH1 = html => {
+  const match = html.match(/<h1.*>((.|\n|\r)*)<\/h1>/)
+  if (match) return match[1].trim()
+}
+const getFirstParagraph = html => {
+  const match = html.match(/<p .*>((.|\n|\r)*)<\/p>/)
+  if (match) return match[1].trim()
+}
+
 describe('error handling', () => {
   let testRouter
 
   beforeEach(() => {
     jest.resetModules()
 
+    const pluginsApi = require('../../lib/plugins/plugins')
+    const sessionApi = require('../../lib/session')
+    const originalGetAppViews = pluginsApi.getAppViews
+
     kitRoutesApi = require('../../lib/routes/api')
     kitRoutesApi.resetState()
     testRouter = kitRoutesApi.external.setupRouter()
 
     jest.spyOn(global.console, 'error').mockImplementation()
+    jest.spyOn(sessionApi, 'getSessionMiddleware').mockReturnValue((req, res, next) => {
+      req.session = {}
+      next()
+    })
+    jest.spyOn(pluginsApi, 'getAppViews').mockImplementation(() => [
+      path.join(__dirname, '..', 'fixtures', 'mockNunjucksIncludes'),
+      path.join(__dirname, '..', '..', 'lib', 'nunjucks'),
+      ...originalGetAppViews()
+    ])
   })
 
   afterEach(() => {
@@ -42,7 +69,9 @@ describe('error handling', () => {
     expect(console.error).toHaveBeenCalledWith('test error')
 
     expect(response.status).toBe(500)
-    expect(response.text).toEqual('test error')
+    expect(getPageTitle(response.text)).toEqual('Error – GOV.UK Prototype Kit – GOV.UK Prototype Kit')
+    expect(getH1(response.text)).toEqual('There is an error in your code')
+    expect(getFirstParagraph(response.text)).toMatch(/^You can try and fix this yourself or/)
 
     app.close()
   })
@@ -59,7 +88,21 @@ describe('error handling', () => {
     expect(console.error).toHaveBeenCalledWith('template not found: test-page.html')
 
     expect(response.status).toBe(500)
-    expect(response.text).toEqual('template not found: test-page.html')
+    expect(getPageTitle(response.text)).toEqual('Error – GOV.UK Prototype Kit – GOV.UK Prototype Kit')
+    expect(getH1(response.text)).toEqual('There is an error in your code')
+    expect(getFirstParagraph(response.text)).toMatch(/^You can try and fix this yourself or/)
+  })
+
+  it('shows a not found page', async () => {
+    const app = require('../../server.js')
+    const response = await request(app).get('/this-does-not-exist')
+
+    expect(console.error).not.toHaveBeenCalled()
+
+    expect(response.status).toBe(404)
+    expect(getPageTitle(response.text)).toEqual('Page not found – GOV.UK Prototype Kit – GOV.UK Prototype Kit')
+    expect(getH1(response.text)).toEqual('Page not found')
+    expect(getFirstParagraph(response.text)).toMatch(/^There is no page at/)
   })
 
   it('non-fatal errors are not shown in the browser', async () => {

diff --git a/cypress/e2e/dev/1-watch-files/watch-routes.cypress.js b/cypress/e2e/dev/1-watch-files/watch-routes.cypress.js
@@ -23,7 +23,7 @@ describe('watch route file', () => {
     cy.task('log', 'The cypress test page should not be found')
     cy.visit(pagePath, { failOnStatusCode: false })
     cy.get('body')
-      .should('contains.text', `Page not found: ${pagePath}`)
+      .should('contains.text', `There is no page at ${pagePath}`)
 
     cy.task('log', `Replace ${appRoutes} with Cypress routes`)
     cy.task('copyFile', { source: routesFixture, target: appRoutes })
@@ -43,6 +43,6 @@ describe('watch route file', () => {
     cy.task('log', 'The cypress test page should not be found')
     cy.visit(pagePath, { failOnStatusCode: false })
     cy.get('body')
-      .should('contains.text', `Page not found: ${pagePath}`)
+      .should('contains.text', `There is no page at ${pagePath}`)
   })
 })
diff --git a/cypress/e2e/dev/1-watch-files/watch-views.cypress.js b/cypress/e2e/dev/1-watch-files/watch-views.cypress.js
@@ -20,7 +20,7 @@ describe('watching start page', () => {
     cy.task('log', 'The start page should not be found')
     cy.visit(pagePath, { failOnStatusCode: false })
     cy.get('body')
-      .should('contains.text', `Page not found: ${pagePath}`)
+      .should('contains.text', `There is no page at ${pagePath}`)
 
     cy.task('log', `Copy ${templatesView} to ${appView}`)
     cy.task('copyFile', { source: templatesView, target: appView })

diff --git a/cypress/e2e/dev/7-error-page-tests/page-not-found-error.cypress.js b/cypress/e2e/dev/7-error-page-tests/page-not-found-error.cypress.js
@@ -0,0 +1,24 @@
+const { waitForApplication } = require('../../utils')
+
+const pageName = 'Page not found'
+const checkPageNotFoundText = 'There is no page at /p4ge-n0t-f0und'
+const helpText = 'This may be because:'
+const helpList = [
+  'you typed or pasted the web address incorrectly',
+  'a link in your code is wrong',
+  'a form in your code is wrong',
+  'you have not created the page yet']
+const contactSupportText = 'You can try and fix this yourself or contact the GOV.UK Prototype Kit team if you need help.'
+
+it('internal server error results in 500 page being displayed correctly', () => {
+  waitForApplication()
+
+  cy.visit('/p4ge-n0t-f0und', { failOnStatusCode: false })
+  cy.get('.govuk-heading-l').contains(pageName)
+  cy.get('.govuk-body').contains(checkPageNotFoundText)
+  cy.get('.govuk-body').contains(helpText)
+  for (const hint of helpList) {
+    cy.get('li').contains(hint)
+  }
+  cy.get('.govuk-body').contains(contactSupportText)
+})
diff --git a/cypress/e2e/dev/7-error-page-tests/server-error.cypress.js b/cypress/e2e/dev/7-error-page-tests/server-error.cypress.js
@@ -0,0 +1,40 @@
+const path = require('path')
+const { waitForApplication } = require('../../utils')
+const routesFixture = path.join(Cypress.config('fixturesFolder'), 'routes.js')
+const appRoutesPath = path.join('app', 'routes.js')
+const appRoutes = path.join(Cypress.env('projectFolder'), appRoutesPath)
+
+const pageName = 'There is an error in your code'
+const contactSupportText = 'You can try and fix this yourself or contact the GOV.UK Prototype Kit team if you need help.'
+
+const templateNotFoundText = 'template not found: test-page.html'
+
+describe('Server Error Test', () => {
+  before(() => {
+    cy.task('log', `Replace ${appRoutes} with Cypress routes`)
+    cy.task('copyFile', { source: routesFixture, target: appRoutes })
+  })
+
+  after(() => {
+    cy.task('log', `Restore ${appRoutesPath}`)
+    cy.task('copyFromStarterFiles', { filename: appRoutesPath })
+  })
+
+  it('internal server error results in 500 page being displayed correctly', () => {
+    waitForApplication()
+
+    cy.visit('/error', { failOnStatusCode: false })
+
+    cy.get('.govuk-heading-l').contains(pageName)
+    cy.get('.govuk-body').contains(contactSupportText)
+  })
+  it('shows an error if a template cannot be found', () => {
+    waitForApplication()
+
+    cy.visit('/test-page', { failOnStatusCode: false })
+
+    cy.get('.govuk-heading-l').contains(pageName)
+    cy.get('.govuk-body').contains(contactSupportText)
+    cy.get('code').contains(templateNotFoundText)
+  })
+})
diff --git a/cypress/fixtures/routes.js b/cypress/fixtures/routes.js
@@ -13,3 +13,11 @@ router.get('/cypress-test', (req, res) => {
     </html>
 `)
 })
+
+router.get('/error', (req, res, next) => {
+  next(new Error('test error'))
+})
+
+router.get('/test-page', (req, res, next) => {
+  res.render('test-page.html')
+})
diff --git a/lib/nunjucks/views/error-handling/page-not-found.njk b/lib/nunjucks/views/error-handling/page-not-found.njk
@@ -0,0 +1,29 @@
+{% extends "govuk-prototype-kit/layouts/govuk-branded.njk" %}
+
+{% block pageTitle %}
+  Page not found – {{ serviceName }} – GOV.UK Prototype Kit
+{% endblock %}
+{% set mainClasses = "govuk-main-wrapper--l" %}
+
+{% block content %}
+
+  <div class="govuk-grid-row">
+    <div class="govuk-grid-column-two-thirds">
+
+      <h1 class="govuk-heading-l">Page not found</h1>
+      <p class="govuk-body">
+        There is no page at <strong>{{ path }}</strong>
+      </p>
+      <p class="govuk-body">This may be because:</p>
+      <ul class="govuk-list govuk-list--bullet">
+        <li>you typed or pasted the web address incorrectly</li>
+        <li>a link in your code is wrong</li>
+        <li>a form in your code is wrong</li>
+        <li>you have not created the page yet</li>
+      </ul>
+      <p class="govuk-body">
+            You can try and fix this yourself or <a class="govuk-link" href="https://prototype-kit.service.gov.uk/docs/support">contact the GOV.UK Prototype Kit team</a> if you need help.
+        </p>
+    </div>
+  </div>
+{% endblock %}
diff --git a/lib/nunjucks/views/error-handling/server-error.njk b/lib/nunjucks/views/error-handling/server-error.njk
@@ -0,0 +1,20 @@
+{% extends "govuk-prototype-kit/layouts/govuk-branded.njk" %}
+
+{% block pageTitle %}
+  Error – {{ serviceName }} – GOV.UK Prototype Kit
+{% endblock %}
+
+{% block content %}
+
+<div class="govuk-grid-row">
+    <div class="govuk-grid-column-two-thirds">
+      <h1 class="govuk-heading-l">There is an error in your code</h1>
+      <p class="govuk-body">
+          You can try and fix this yourself or <a class="govuk-link" href="https://prototype-kit.service.gov.uk/docs/support">contact the GOV.UK Prototype Kit team</a> if you need help.
+      </p>
+      <pre><code>{{ errorStack }}</code></pre>  
+    </div>
+  </div>
+
+
+{% endblock %}
diff --git a/server.js b/server.js
@@ -178,13 +178,36 @@ app.use((req, res, next) => {
 // We override the default handler because we want to customise
 // how the error appears to users, we want to show a simplified
 // message without the stack trace.
+
 app.use((err, req, res, next) => {
   if (res.headersSent) {
     return next(err)
   }
-  console.error(err.message)
-  res.status(err.status || 500)
-  res.send(err.message)
+  if (req.headers['content-type'] && req.headers['content-type'].indexOf('json') !== -1) {
+    console.error(err.message)
+    res.status(err.status || 500)
+    res.send(err.message)
+    return
+  }
+  switch (err.status) {
+    case 404: {
+      const path = req.path
+      res.status(err.status)
+      res.render('views/error-handling/page-not-found', {
+        path
+      })
+      break
+    }
+    default: {
+      const errorStack = err.stack
+      res.status(500)
+      console.error(err.message)
+      res.render('views/error-handling/server-error', {
+        errorStack
+      })
+      break
+    }
+  }
 })
 
 app.close = stopWatchingNunjucks