HOTT-4629: Improve exception handling

app/controllers/application_controller.rb

@@ -57,6 +57,10 @@ def disable_search_form
     @no_shared_search = true
   end
 
+  def skip_news_banner
+    @skip_news_banner = true
+  end
+
   def disable_last_updated_footnote
     @tariff_last_updated = nil
   end

app/controllers/errors_controller.rb

@@ -5,7 +5,9 @@ class ErrorsController < ApplicationController
   skip_before_action :set_search
   skip_before_action :bots_no_index_if_historical
 
-  before_action :disable_search_form, :disable_switch_service_banner
+  before_action :disable_search_form,
+                :disable_switch_service_banner,
+                :skip_news_banner
 
   def not_found
     respond_to do |format|
@@ -15,19 +17,72 @@ def not_found
     end
   end
 
+  def unprocessable_entity
+    message = "We're sorry, but we cannot process your request at this time.<br>
+               Please contact support for assistance or try a different request.".html_safe
+
+    respond_to do |format|
+      format.html { render 'error', status: :unprocessable_entity, locals: { header: 'Unprocessable entity', message: } }
+      format.json { render json: { error: 'Unprocessable entity' }, status: :unprocessable_entity }
+      format.all { render status: :unprocessable_entity, plain: 'Unprocessable entity' }
+    end
+  end
+
   def internal_server_error
-    @skip_news_banner = true
+    message = 'We are experiencing technical difficulties'
 
     respond_to do |format|
-      format.html { render status: :internal_server_error }
+      format.html { render 'error', status: :internal_server_error, locals: { header: 'We are experiencing technical difficulties', message: } }
       format.json { render json: { error: 'Internal server error' }, status: :internal_server_error }
       format.all { render status: :internal_server_error, plain: 'Internal server error' }
     end
   end
 
-  def maintenance
-    @skip_news_banner = true
+  def bad_request
+    message = "The request you made is not valid.<br>
+               Please contact support for assistance or try a different request.".html_safe
+
+    respond_to do |format|
+      format.html { render 'error', status: :bad_request, locals: { header: 'Bad request', message: } }
+      format.json { render json: { error: 'Bad request' }, status: :bad_request }
+      format.all { render status: :bad_request, plain: 'Bad request' }
+    end
+  end
+
+  def method_not_allowed
+    message = "We're sorry, but this request method is not supported.<br>
+               Please contact support for assistance or try a different request.".html_safe
+
+    respond_to do |format|
+      format.html { render 'error', status: :method_not_allowed, locals: { header: 'Method not allowed', message: } }
+      format.json { render json: { error: 'Method not allowed' }, status: :method_not_allowed }
+      format.all { render status: :method_not_allowed, plain: 'Method not allowed' }
+    end
+  end
+
+  def not_acceptable
+    message = "Unfortunately, we cannot fulfill your request as it is not in a format we can accept.<br>
+               Please contact support for assistance or try a different request.".html_safe
+
+    respond_to do |format|
+      format.html { render 'error', status: :not_acceptable, locals: { header: 'Not acceptable', message: } }
+      format.json { render json: { error: 'Not acceptable' }, status: :not_acceptable }
+      format.all { render status: :not_acceptable, plain: 'Not acceptable' }
+    end
+  end
 
+  def not_implemented
+    message = 'We\'re sorry, but the requested action is not supported by our server at this time.<br>
+               Please contact support for assistance or try a different request.'.html_safe
+
+    respond_to do |format|
+      format.html { render 'error', status: :not_implemented, locals: { header: 'Not implemented', message: } }
+      format.json { render json: { error: 'Not implemented' }, status: :not_implemented }
+      format.all { render status: :not_implemented, plain: 'Not implemented' }
+    end
+  end
+
+  def maintenance
     respond_to do |format|
       format.html { render status: :service_unavailable }
       format.json { render json: { error: 'Maintenance mode' }, status: :service_unavailable }

app/views/errors/internal_server_error.html.erb → app/views/errors/error.html.erb

@@ -1,12 +1,16 @@
 <header class="page-header group">
   <div>
-    <h1 class="govuk-heading-l">We are experiencing technical difficulties</h1>
+    <h1 class="govuk-heading-l">
+      <h1><%= header %></h1>
+    </h1>
   </div>
 </header>
 <div class="article-container">
   <article role="article" class="group">
     <div class="inner">
-      <p>Please try again in a few moments.</p>
+    <p>
+      <%= message %>
+    </p>
     </div>
   </article>
 </div>

config/environments/development.rb

@@ -11,6 +11,9 @@
   # Do not eager load code on boot.
   config.eager_load = false
 
+  # Raise exceptions instead of rendering exception templates.
+  config.action_dispatch.show_exceptions = true
+
   # Show full error reports.
   config.consider_all_requests_local = true
 

config/environments/test.rb

@@ -22,13 +22,16 @@
     'Cache-Control' => "public, max-age=#{1.hour.to_i}",
   }
 
+  # Raise exceptions instead of rendering exception templates.
+  config.action_dispatch.show_exceptions = true
+
   # Show full error reports and disable caching.
-  config.consider_all_requests_local       = true
+  config.consider_all_requests_local       = false
   config.action_controller.perform_caching = false
   config.cache_store = :null_store
 
   # Raise exceptions instead of rendering exception templates.
-  config.action_dispatch.show_exceptions = false
+  config.action_dispatch.show_exceptions = true
 
   # Disable request forgery protection in test environment.
   config.action_controller.allow_forgery_protection = false

config/routes.rb

@@ -210,8 +210,13 @@
   root to: redirect(TradeTariffFrontend.production? ? 'https://www.gov.uk/trade-tariff' : '/sections', status: 302)
 
   get '/robots.:format', to: 'pages#robots'
+  match '/400', to: 'errors#bad_request', via: :all
   match '/404', to: 'errors#not_found', via: :all
+  match '/405', to: 'errors#method_not_allowed', via: :all
+  match '/406', to: 'errors#not_acceptable', via: :all
+  match '/422', to: 'errors#unprocessable_entity', via: :all
   match '/500', to: 'errors#internal_server_error', via: :all
+  match '/501', to: 'errors#not_implemented', via: :all
   match '/503', to: 'errors#maintenance', via: :all
   match '*path', to: 'errors#not_found', via: :all
 end

spec/requests/application_controller_spec.rb

@@ -0,0 +1,14 @@
+RSpec.describe ApplicationController, type: :request do
+  describe 'GET #index' do
+    subject(:do_response) { get('/healthcheck') && response }
+
+    context 'when a handled error is raised' do
+      before { allow(Section).to receive(:all).and_raise(exception) }
+
+      let(:exception) { ActionController::InvalidAuthenticityToken }
+
+      it { is_expected.to have_http_status(:unprocessable_entity) }
+      it { expect(do_response.body).to include('Unprocessable entity') }
+    end
+  end
+end

spec/requests/errors_controller_spec.rb

@@ -0,0 +1,84 @@
+require 'spec_helper'
+
+RSpec.describe ErrorsController, type: :request do
+  subject(:rendered) { make_request && response }
+
+  let(:json_response) { JSON.parse(rendered.body) }
+  let(:body) { rendered.body }
+
+  shared_examples 'a json error response' do |status_code, message|
+    it { is_expected.to have_http_status status_code }
+    it { is_expected.to have_attributes media_type: 'application/json' }
+    it { expect(json_response).to include 'error' => message }
+  end
+
+  # Tests Json error responses
+
+  describe 'GET /404.json' do
+    let(:make_request) { get '/404.json' }
+
+    it_behaves_like 'a json error response', 404, 'Resource not found'
+  end
+
+  describe 'GET /405.json' do
+    let(:make_request) { get '/405.json' }
+
+    it_behaves_like 'a json error response', 405, 'Method not allowed'
+  end
+
+  describe 'GET /406.json' do
+    let(:make_request) { get '/406.json' }
+
+    it_behaves_like 'a json error response', 406, 'Not acceptable'
+  end
+
+  describe 'GET /500.json' do
+    let(:make_request) { get '/500.json' }
+
+    it_behaves_like 'a json error response', 500, 'Internal server error'
+  end
+
+  describe 'GET /501.json' do
+    let(:make_request) { get '/501.json' }
+
+    it_behaves_like 'a json error response', 501, 'Not implemented'
+  end
+
+  describe 'GET /503.json' do
+    let(:make_request) { get '/503.json' }
+
+    it_behaves_like 'a json error response', 503, 'Maintenance mode'
+  end
+
+  # Test html error responses
+
+  describe 'GET /404' do
+    let(:make_request) { get '/404' }
+
+    it { expect(body).to include 'Page not found' }
+  end
+
+  describe 'GET /406' do
+    let(:make_request) { get '/406' }
+
+    it { expect(body).to include 'Not acceptable' }
+  end
+
+  describe 'GET /500' do
+    let(:make_request) { get '/500' }
+
+    it { expect(body).to include 'We are experiencing technical difficulties' }
+  end
+
+  describe 'GET #not_implemented' do
+    let(:make_request) { get '/501' }
+
+    it { expect(body).to include 'Not implemented' }
+  end
+
+  describe 'GET /503' do
+    let(:make_request) { get '/503' }
+
+    it { expect(body).to include 'Maintenance' }
+  end
+end

spec/requests/pages/glossary_controller_spec.rb

@@ -18,9 +18,11 @@
     end
 
     context 'with unknown page' do
-      let(:fetch_page) { get glossary_term_path('unknown_page') }
+      before do
+        get glossary_term_path('unknown_page')
+      end
 
-      it { expect { fetch_page }.to raise_exception Pages::Glossary::UnknownPage }
+      it { expect(response).to have_http_status(:internal_server_error) }
     end
   end
 end

spec/requests/pages_controller_spec.rb

@@ -93,19 +93,21 @@
     context 'when howto does not exist' do
       let(:howto) { 'non-existent' }
 
-      it { expect { do_request }.to raise_error(ActionController::RoutingError) }
+      it { expect(do_request).to have_http_status(:not_found) }
     end
 
     context 'when howto format is html' do
       let(:howto) { 'origin.html' }
 
-      it { expect { do_request }.not_to raise_error }
+      it { expect(do_request.body).to include 'What is origin?' }
     end
 
     context 'when howto format is not html' do
       let(:howto) { 'origin.json' }
 
-      it { expect { do_request }.to raise_error(ActionController::RoutingError) }
+      it 'returns resource not found' do
+        expect(JSON.parse(do_request.body)).to eq('error' => 'Resource not found')
+      end
     end
   end
 end