Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements custom 404 and 500 pages for handling custom 404 and error pages #1958

Merged
merged 2 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/controllers/errors_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# frozen_string_literal: true
class ErrorsController < ApplicationController
skip_before_action :authenticate_user!

def not_found
render(status: :not_found)
end

def internal_server_error
render(status: :internal_server_error)
end
end
11 changes: 9 additions & 2 deletions app/controllers/works_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,10 @@ def assign_curator
render json: {}
end
rescue => ex
# This is necessary for JSON responses
Rails.logger.error("Error changing curator for work: #{work.id}. Exception: #{ex.message}")
render json: { errors: ["Cannot save dataset"] }, status: :bad_request
Honeybadger.notify("Error changing curator for work: #{work.id}. Exception: #{ex.message}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

render(json: { errors: ["Cannot save dataset"] }, status: :bad_request)
end

def add_message
Expand Down Expand Up @@ -314,10 +316,15 @@ def rescue_aasm_error
if action_name == "create"
handle_error_for_create(generic_error)
else
redirect_to root_url, notice: "We apologize, an error was encountered: #{generic_error.message}. Please contact the PDC Describe administrators."
redirect_to error_url, notice: "We apologize, an error was encountered: #{generic_error.message}. Please contact the PDC Describe administrators."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

end
end

rescue_from StandardError do |generic_error|
Honeybadger.notify("We apologize, an error was encountered: #{generic_error.message}.")
redirect_to error_url, notice: "We apologize, an error was encountered: #{generic_error.message}. Please contact the PDC Describe administrators."
end

# @note No testing coverage but not a route, not called
def handle_error_for_create(generic_error)
if @work.persisted?
Expand Down
2 changes: 2 additions & 0 deletions app/views/errors/internal_server_error.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<h1 class="welcome-headers">There was an error encountered handling your request!</h1>
<p>For assistance, please e-mail <a href="mailto:[email protected]">[email protected]</a> for support.</p>
2 changes: 2 additions & 0 deletions app/views/errors/not_found.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<h1 class="welcome-headers">The page you were looking for doesn't exist.</h1>
<p>For assistance, please e-mail <a href="mailto:[email protected]">[email protected]</a> for support.</p>
2 changes: 2 additions & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,7 @@ class Application < Rails::Application
# Explicitly set timezome rather than relying on system,
# which may be different in CI environment.
config.time_zone = "America/New_York"

config.exceptions_app = routes
end
end
2 changes: 1 addition & 1 deletion config/environments/development.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
config.sass.cache = false

# Show full error reports.
config.consider_all_requests_local = true
config.consider_all_requests_local = false

# Enable/disable caching. By default caching is disabled.
# Run rails dev:cache to toggle caching.
Expand Down
2 changes: 1 addition & 1 deletion config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
}

# 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

Expand Down
3 changes: 3 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,7 @@

# Anything still unmatched by the end of the routes file should go to the not_found page
# match '*a', to: redirect('/404'), via: :get

match "/404", to: "errors#not_found", via: :all, as: :not_found
match "/500", to: "errors#internal_server_error", via: :all, as: :error
end
67 changes: 0 additions & 67 deletions public/404.html

This file was deleted.

66 changes: 0 additions & 66 deletions public/500.html

This file was deleted.

11 changes: 5 additions & 6 deletions spec/controllers/works_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -634,9 +634,9 @@

it "does not redirect to the Work show view if not exact (missing slash)" do
stub_s3
expect do
get :resolve_doi, params: { doi: work.doi.gsub("10.34770", "") }
end.to raise_error(ActiveRecord::RecordNotFound)
doi_param = work.doi.gsub("10.34770", "")
get(:resolve_doi, params: { doi: doi_param })
expect(response).to redirect_to(error_url)
end
end
end
Expand Down Expand Up @@ -667,9 +667,8 @@

it "does not redirect to the Work show view if not exact (missing slash)" do
stub_s3
expect do
get :resolve_ark, params: { ark: work.ark.gsub("ark:", "") }
end.to raise_error(ActiveRecord::RecordNotFound)
get :resolve_ark, params: { ark: work.ark.gsub("ark:", "") }
expect(response).to redirect_to(error_url)
end
end
end
Expand Down
3 changes: 2 additions & 1 deletion spec/requests/works_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@
end

it "will raise an error" do
expect { get work_url(work) }.to raise_error(Work::InvalidGroupError, "The Work test-id does not belong to any Group")
get work_url(work)
expect(response).to redirect_to(error_url)
end
end

Expand Down
27 changes: 27 additions & 0 deletions spec/system/root_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,31 @@

expect(page).to have_content "Welcome"
end

context "when requesting a URL which does not exist", type: :system do
it "renders the custom 404 page" do
visit "/invalid"

expect(page.status_code).to eq(404)
expect(page).to have_content "The page you were looking for doesn't exist."
end
end

context "when an error occurs while requesting a URL", type: :system do
let(:user) { FactoryBot.create(:super_admin_user) }
let(:work) { FactoryBot.create(:draft_work) }

before do
sign_in(user)
work
allow(Work).to receive(:find).and_raise(StandardError, "test")
end

it "renders the custom 500 page" do
visit "/works/#{work.id}"

expect(page.status_code).to eq(500)
expect(page).to have_content("We apologize, an error was encountered: test")
end
end
end