From 66e66a28e39db30314287aafa32e633bfbb44827 Mon Sep 17 00:00:00 2001 From: "Rasika.Abeyrathna" Date: Fri, 15 Dec 2023 16:40:48 +0000 Subject: [PATCH 1/6] HOTT-4630: Extend DutyCalculator ErrorsController --- app/controllers/errors_controller.rb | 40 ++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index f741a511..b0ed56df 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -24,4 +24,44 @@ def unprocessable_entity format.json { render json: { error: 'Unprocessable entity' }, status: :unprocessable_entity } end end + + def bad_request + @skip_news_banner = true + + respond_to do |format| + format.html { render status: :bad_request } + format.json { render json: { error: 'Bad request' }, status: :bad_request } + format.all { render status: :bad_request, plain: 'Bad request' } + end + end + + def not_implemented + @skip_news_banner = true + + respond_to do |format| + format.html { render status: :bad_request } + format.json { render json: { error: 'Bad request: Not implemented' }, status: :bad_request } + format.all { render status: :bad_request, plain: 'Bad request: Not Implemented' } + end + end + + def method_not_allowed + @skip_news_banner = true + + respond_to do |format| + format.html { render status: :bad_request } + format.json { render json: { error: 'Bad request: Method not allowed' }, status: :bad_request } + format.all { render status: :bad_request, plain: 'Bad request' } + end + end + + def not_acceptable + @skip_news_banner = true + + respond_to do |format| + format.html { render status: :bad_request } + format.json { render json: { error: 'Bad request: Not acceptable' }, status: :bad_request } + format.all { render status: :bad_request, plain: 'Bad request' } + end + end end From ee8a12083673de62a18cd5b479fd0e07b3dd4748 Mon Sep 17 00:00:00 2001 From: Alexdesi Date: Thu, 21 Dec 2023 12:27:26 +0000 Subject: [PATCH 2/6] HOTT-4630: Add custom exception handling responses for various status code. Add request specs. --- app/controllers/errors_controller.rb | 85 ++++++++++------ app/views/errors/error.html.erb | 12 +++ .../errors/internal_server_error.html.erb | 9 -- app/views/errors/not_found.html.erb | 9 -- .../errors/unprocessable_entity.html.erb | 9 -- config/routes.rb | 6 +- spec/requests/errors_controller_spec.rb | 96 +++++++++++++++++++ 7 files changed, 168 insertions(+), 58 deletions(-) create mode 100644 app/views/errors/error.html.erb delete mode 100644 app/views/errors/internal_server_error.html.erb delete mode 100644 app/views/errors/not_found.html.erb delete mode 100644 app/views/errors/unprocessable_entity.html.erb create mode 100644 spec/requests/errors_controller_spec.rb diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index b0ed56df..3924beee 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -3,65 +3,90 @@ class ErrorsController < ApplicationController helper_method :user_session - def not_found + # /400 + def bad_request + message = "The request you made is not valid.
+ Please contact support for assistance or try a different request.".html_safe + respond_to do |format| - format.html { render status: :not_found } - format.json { render json: { error: 'Resource not found' }, status: :not_found } - format.all { render status: :not_found, body: nil } + 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 internal_server_error + # /404 + def not_found + message = 'You may have mistyped the address or the page may have moved.' + respond_to do |format| - format.html { render status: :internal_server_error } - format.json { render json: { error: 'Internal server error' }, status: :internal_server_error } + format.html { render 'error', status: :not_found, locals: { header: 'The page you were looking for does not exist.', message: } } + format.json { render json: { error: 'Resource not found' }, status: :not_found } + format.all { render status: :not_found, plain: 'Resource not found' } end end - def unprocessable_entity + # /405 + def method_not_allowed + message = "We're sorry, but this request method is not supported.
+ Please contact support for assistance or try a different request.".html_safe + respond_to do |format| - format.html { render status: :unprocessable_entity } - format.json { render json: { error: 'Unprocessable entity' }, status: :unprocessable_entity } + 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: 'Bad request' } end end - def bad_request - @skip_news_banner = true + # /406 + def not_acceptable + message = "Unfortunately, we cannot fulfill your request as it is not in a format we can accept.
+ Please contact support for assistance or try a different request.".html_safe respond_to do |format| - format.html { render status: :bad_request } - format.json { render json: { error: 'Bad request' }, status: :bad_request } - format.all { render status: :bad_request, plain: 'Bad request' } + 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 accepdtable' } end end - def not_implemented - @skip_news_banner = true + # /422 + def unprocessable_entity + message = "Maybe you tried to change something you didn't have access to." respond_to do |format| - format.html { render status: :bad_request } - format.json { render json: { error: 'Bad request: Not implemented' }, status: :bad_request } - format.all { render status: :bad_request, plain: 'Bad request: Not Implemented' } + format.html do + render 'error', status: :unprocessable_entity, + locals: { header: 'The change you wanted was rejected.', message: } + end + format.json { render json: { error: 'Unprocessable entity' }, status: :unprocessable_entity } + format.all { render status: :unprocessable_entity, plain: 'Unprocessable entity' } end end - def method_not_allowed - @skip_news_banner = true + # /500 + def internal_server_error + message = 'We could not proceed with calculating your duty.' respond_to do |format| - format.html { render status: :bad_request } - format.json { render json: { error: 'Bad request: Method not allowed' }, status: :bad_request } - format.all { render status: :bad_request, plain: 'Bad request' } + format.html do + render 'error', status: :internal_server_error, + locals: { header: 'Sorry, there is a problem with the service', message: } + end + 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 not_acceptable - @skip_news_banner = true + # /501 + def not_implemented + message = 'We\'re sorry, but the requested action is not supported by our server at this time.
+ Please contact support for assistance or try a different request.'.html_safe respond_to do |format| - format.html { render status: :bad_request } - format.json { render json: { error: 'Bad request: Not acceptable' }, status: :bad_request } - format.all { render status: :bad_request, plain: 'Bad request' } + 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 end diff --git a/app/views/errors/error.html.erb b/app/views/errors/error.html.erb new file mode 100644 index 00000000..ee16a10b --- /dev/null +++ b/app/views/errors/error.html.erb @@ -0,0 +1,12 @@ +
+
+
+

<%= header %>

+

<%= message %>

+ + <% if user_session.commodity_code.present? %> +

Click here to <%= link_to('start again', import_date_path(referred_service: user_session.referred_service, commodity_code: user_session.commodity_code), class: 'govuk-link') %>.

+ <% end %> +
+
+
diff --git a/app/views/errors/internal_server_error.html.erb b/app/views/errors/internal_server_error.html.erb deleted file mode 100644 index 2ebf8c58..00000000 --- a/app/views/errors/internal_server_error.html.erb +++ /dev/null @@ -1,9 +0,0 @@ -
-
-
-

Sorry, there is a problem with the service

-

We could not proceed with calculating your duty.

-

Click here to <%= link_to('start again', import_date_path(referred_service: user_session.referred_service, commodity_code: user_session.commodity_code, class: 'govuk-link')) %>.

-
-
-
diff --git a/app/views/errors/not_found.html.erb b/app/views/errors/not_found.html.erb deleted file mode 100644 index b8e06227..00000000 --- a/app/views/errors/not_found.html.erb +++ /dev/null @@ -1,9 +0,0 @@ -
-
-
-

The page you were looking for doesn't exist.

-

You may have mistyped the address or the page may have moved.

-

Click here to <%= link_to('start again', import_date_path(referred_service: user_session.referred_service, commodity_code: user_session.commodity_code), class: 'govuk-link') %>.

-
-
-
diff --git a/app/views/errors/unprocessable_entity.html.erb b/app/views/errors/unprocessable_entity.html.erb deleted file mode 100644 index 4ab3b684..00000000 --- a/app/views/errors/unprocessable_entity.html.erb +++ /dev/null @@ -1,9 +0,0 @@ -
-
-
-

The change you wanted was rejected.

-

Maybe you tried to change something you didn't have access to.

-

Click here to <%= link_to('start again', import_date_path(referred_service: user_session.referred_service, commodity_code: user_session.commodity_code), class: 'govuk-link') %>.

-
-
-
diff --git a/config/routes.rb b/config/routes.rb index 0fdb1a56..6a7d89ae 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,5 +1,5 @@ Rails.application.routes.draw do - root to: proc { [404, {'Content-Type' => 'text/html'}, ['Not found.']] } + root to: proc { [404, { 'Content-Type' => 'text/html' }, ['Not found.']] } get 'healthcheckz', to: 'healthcheck#checkz' @@ -64,7 +64,11 @@ get 'prefill', to: 'steps/prefill_user_session#show' end + get '400', to: 'errors#bad_request', via: :all get '404', to: 'errors#not_found', via: :all + get '405', to: 'errors#method_not_allowed', via: :all + get '406', to: 'errors#not_acceptable', via: :all get '422', to: 'errors#unprocessable_entity', via: :all get '500', to: 'errors#internal_server_error', via: :all + get '501', to: 'errors#not_implemented', via: :all end diff --git a/spec/requests/errors_controller_spec.rb b/spec/requests/errors_controller_spec.rb new file mode 100644 index 00000000..4aceb512 --- /dev/null +++ b/spec/requests/errors_controller_spec.rb @@ -0,0 +1,96 @@ +RSpec.describe ErrorsController, type: :request do + include ApplicationHelper + + 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 /400.json' do + let(:make_request) { get '/400.json' } + + it_behaves_like 'a json error response', 400, 'Bad request' + end + + 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 /422.json' do + let(:make_request) { get '/422.json' } + + it_behaves_like 'a json error response', 422, 'Unprocessable entity' + 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 + + # Test html error responses + + describe 'GET /400' do + let(:make_request) { get '/400' } + + it { expect(body).to include 'Bad request' } + end + + describe 'GET /404' do + let(:make_request) { get '/404' } + + it { expect(body).to include "The page you were looking for does not exist." } + end + + describe 'GET /406' do + let(:make_request) { get '/406' } + + it { expect(body).to include 'Not acceptable' } + end + + describe 'GET /422' do + let(:make_request) { get '/422' } + + it { expect(body).to include 'The change you wanted was rejected.' } + end + + describe 'GET /500' do + let(:make_request) { get '/500' } + + it { expect(body).to include 'Sorry, there is a problem with the service' } + end + + describe 'GET #501' do + let(:make_request) { get '/501' } + + it { expect(body).to include 'Not implemented' } + end +end From c99a97cb1b0cbafc6ad223b8506859f796380e51 Mon Sep 17 00:00:00 2001 From: Alexdesi Date: Thu, 21 Dec 2023 12:29:17 +0000 Subject: [PATCH 3/6] Remove redundant controllers/errors_controller_spec.rb, since request specs are testing the same logic. --- spec/controllers/errors_controller_spec.rb | 22 ---------------------- 1 file changed, 22 deletions(-) delete mode 100644 spec/controllers/errors_controller_spec.rb diff --git a/spec/controllers/errors_controller_spec.rb b/spec/controllers/errors_controller_spec.rb deleted file mode 100644 index 8f441768..00000000 --- a/spec/controllers/errors_controller_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -RSpec.describe ErrorsController, type: :controller do - describe 'GET #not_found' do - it 'returns not found' do - get :not_found - expect(response).to have_http_status(:not_found) - end - end - - describe 'GET #internal_server_error' do - it 'returns internal_server_error' do - get :internal_server_error - expect(response).to have_http_status(:internal_server_error) - end - end - - describe 'GET #unprocessable_entity' do - it 'returns unprocessable_entity' do - get :unprocessable_entity - expect(response).to have_http_status(:unprocessable_entity) - end - end -end From 23ed226e3c1bca562d111eee84530ae92edfeb8e Mon Sep 17 00:00:00 2001 From: Alexdesi Date: Thu, 21 Dec 2023 12:36:21 +0000 Subject: [PATCH 4/6] Small Rubocop fixes. --- spec/requests/errors_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/requests/errors_controller_spec.rb b/spec/requests/errors_controller_spec.rb index 4aceb512..9ae481a1 100644 --- a/spec/requests/errors_controller_spec.rb +++ b/spec/requests/errors_controller_spec.rb @@ -67,7 +67,7 @@ describe 'GET /404' do let(:make_request) { get '/404' } - it { expect(body).to include "The page you were looking for does not exist." } + it { expect(body).to include 'The page you were looking for does not exist.' } end describe 'GET /406' do @@ -90,7 +90,7 @@ describe 'GET #501' do let(:make_request) { get '/501' } - + it { expect(body).to include 'Not implemented' } end end From 0faabd8872ae53438f2bb1de8ab278cfa6925b9a Mon Sep 17 00:00:00 2001 From: Alexdesi Date: Thu, 21 Dec 2023 14:16:24 +0000 Subject: [PATCH 5/6] HOTT-4630: Removed other redundant error spec. Refactor view error specs. --- .../steps/errors_controller_spec.rb | 28 ------------------- spec/views/errors/error_spec.rb | 13 +++++++++ .../errors/internal_server_error_spec.rb | 11 -------- spec/views/errors/not_found_spec.rb | 11 -------- .../views/errors/unprocessable_entity_spec.rb | 11 -------- 5 files changed, 13 insertions(+), 61 deletions(-) delete mode 100644 spec/controllers/steps/errors_controller_spec.rb create mode 100644 spec/views/errors/error_spec.rb delete mode 100644 spec/views/errors/internal_server_error_spec.rb delete mode 100644 spec/views/errors/not_found_spec.rb delete mode 100644 spec/views/errors/unprocessable_entity_spec.rb diff --git a/spec/controllers/steps/errors_controller_spec.rb b/spec/controllers/steps/errors_controller_spec.rb deleted file mode 100644 index 0b0fba24..00000000 --- a/spec/controllers/steps/errors_controller_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -RSpec.describe ErrorsController do - let(:commodity_code) { '0702000007' } - let(:referred_service) { 'uk' } - - describe 'GET /404' do - subject(:response) { get :not_found } - - it { is_expected.to have_http_status(:not_found) } - it { is_expected.to render_template(:not_found) } - it { is_expected.to render_template(:application) } - end - - describe 'GET /422' do - subject(:response) { get :unprocessable_entity } - - it { is_expected.to have_http_status(:unprocessable_entity) } - it { is_expected.to render_template(:unprocessable_entity) } - it { is_expected.to render_template(:application) } - end - - describe 'GET /500' do - subject(:response) { get :internal_server_error } - - it { is_expected.to have_http_status(:internal_server_error) } - it { is_expected.to render_template(:internal_server_error) } - it { is_expected.to render_template(:application) } - end -end diff --git a/spec/views/errors/error_spec.rb b/spec/views/errors/error_spec.rb new file mode 100644 index 00000000..a1ea14d6 --- /dev/null +++ b/spec/views/errors/error_spec.rb @@ -0,0 +1,13 @@ +RSpec.describe 'errors/error', type: :view do + let(:user_session) { build(:user_session, :with_commodity_information) } + + it 'renders a link to the import_date_path' do + render template: 'errors/error', + locals: { user_session:, header: 'Error header', message: 'Error message' } + + expected_path = import_date_path(referred_service: user_session.referred_service, + commodity_code: user_session.commodity_code) + + expect(rendered).to include(expected_path) + end +end diff --git a/spec/views/errors/internal_server_error_spec.rb b/spec/views/errors/internal_server_error_spec.rb deleted file mode 100644 index 0f3bdc99..00000000 --- a/spec/views/errors/internal_server_error_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -RSpec.describe 'errors/internal_server_error', type: :view do - let(:user_session) { build(:user_session, :with_commodity_information) } - - it 'renders a link to the import_date_path' do - render template: 'errors/internal_server_error', locals: { user_session: } - - expected_path = import_date_path(referred_service: user_session.referred_service, commodity_code: user_session.commodity_code) - - expect(rendered).to include(expected_path) - end -end diff --git a/spec/views/errors/not_found_spec.rb b/spec/views/errors/not_found_spec.rb deleted file mode 100644 index 9c2f98b6..00000000 --- a/spec/views/errors/not_found_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -RSpec.describe 'errors/not_found', type: :view do - let(:user_session) { build(:user_session, :with_commodity_information) } - - it 'renders a link to the import_date_path' do - render template: 'errors/not_found', locals: { user_session: } - - expected_path = import_date_path(referred_service: user_session.referred_service, commodity_code: user_session.commodity_code) - - expect(rendered).to include(expected_path) - end -end diff --git a/spec/views/errors/unprocessable_entity_spec.rb b/spec/views/errors/unprocessable_entity_spec.rb deleted file mode 100644 index 7476464f..00000000 --- a/spec/views/errors/unprocessable_entity_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -RSpec.describe 'errors/unprocessable_entity', type: :view do - let(:user_session) { build(:user_session, :with_commodity_information) } - - it 'renders a link to the import_date_path' do - render template: 'errors/unprocessable_entity', locals: { user_session: } - - expected_path = import_date_path(referred_service: user_session.referred_service, commodity_code: user_session.commodity_code) - - expect(rendered).to include(expected_path) - end -end From 309d354f7642b03dc05329c68127c55ee6d1ecff Mon Sep 17 00:00:00 2001 From: William Fish Date: Fri, 22 Dec 2023 12:02:13 +0000 Subject: [PATCH 6/6] HOTT-4630: Code review --- app/controllers/errors_controller.rb | 7 ------- config/environments/test.rb | 5 +++-- spec/requests/errors_controller_spec.rb | 4 ---- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index 3924beee..11e33b02 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -3,7 +3,6 @@ class ErrorsController < ApplicationController helper_method :user_session - # /400 def bad_request message = "The request you made is not valid.
Please contact support for assistance or try a different request.".html_safe @@ -15,7 +14,6 @@ def bad_request end end - # /404 def not_found message = 'You may have mistyped the address or the page may have moved.' @@ -26,7 +24,6 @@ def not_found end end - # /405 def method_not_allowed message = "We're sorry, but this request method is not supported.
Please contact support for assistance or try a different request.".html_safe @@ -38,7 +35,6 @@ def method_not_allowed end end - # /406 def not_acceptable message = "Unfortunately, we cannot fulfill your request as it is not in a format we can accept.
Please contact support for assistance or try a different request.".html_safe @@ -50,7 +46,6 @@ def not_acceptable end end - # /422 def unprocessable_entity message = "Maybe you tried to change something you didn't have access to." @@ -64,7 +59,6 @@ def unprocessable_entity end end - # /500 def internal_server_error message = 'We could not proceed with calculating your duty.' @@ -78,7 +72,6 @@ def internal_server_error end end - # /501 def not_implemented message = 'We\'re sorry, but the requested action is not supported by our server at this time.
Please contact support for assistance or try a different request.'.html_safe diff --git a/config/environments/test.rb b/config/environments/test.rb index 00f6a5a8..58063790 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -20,11 +20,12 @@ # Configure public file server for tests with Cache-Control for performance. config.public_file_server.enabled = true config.public_file_server.headers = { - "Cache-Control" => "public, max-age=#{1.hour.to_i}" + "Cache-Control" => "public, max-age=#{1.hour.to_i}", } # Show full error reports and disable caching. - config.consider_all_requests_local = true + config.consider_all_requests_local = false + config.action_dispatch.show_exceptions = true config.action_controller.perform_caching = false config.cache_store = :null_store diff --git a/spec/requests/errors_controller_spec.rb b/spec/requests/errors_controller_spec.rb index 9ae481a1..9b976671 100644 --- a/spec/requests/errors_controller_spec.rb +++ b/spec/requests/errors_controller_spec.rb @@ -12,8 +12,6 @@ it { expect(json_response).to include 'error' => message } end - # Tests Json error responses - describe 'GET /400.json' do let(:make_request) { get '/400.json' } @@ -56,8 +54,6 @@ it_behaves_like 'a json error response', 501, 'Not implemented' end - # Test html error responses - describe 'GET /400' do let(:make_request) { get '/400' }