Skip to content

Commit

Permalink
Chore: Switch to route based error handling (#3906)
Browse files Browse the repository at this point in the history
## Because
- Currently we use two different ways of handling errors: a complex
custom handler that really is just matching it to a specific controller,
and static error pages. We can handle errors more cleanly and uniformly
by switching all error pages to instead use Rails route based handling,
which lets us map `/STATUS_CODE` requests to specific controller actions


## This PR
- Switches our error handling method to routes
- adds routes in `routes.rb` for `422` and `500`
- adds a controller and rewrites `ErrorsController` to handle these:
`ErrorsController` handles `500` and `422` and `NotFoundController`
handles `404`
- I chose this approach because it enables us to separate `500` and
`422` from the application logic, while `NotFoundController` still
inherits from `ApplicationController`, and therefore gets to keep the
nice default TOP layout, as its much more likely to be encountered
- adds tests for checking that each of the error pages return the
correct status code (and ergo work), and an additional test to check
that `500` actually returns our custom page and not just the fallback
page.
- drops the now unnecessary `/public/500.html`, `/public/422`, and
`/lib/custom_public_exceptions` files
   


## Issue
<!--
If this PR closes an open issue in this repo, replace the XXXXX below
with the issue number, e.g. Closes #2013.

If this PR closes an open issue in another TOP repo, replace the #XXXXX
with the URL of the issue, e.g. Closes
https://github.com/TheOdinProject/curriculum/issues/XXXXX

If this PR does not close, but is related to another issue or PR, you
can link it as above without the 'Closes' keyword, e.g. 'Related to
#2013'.
-->
Closes #3876 

## Pull Request Requirements
<!-- Replace the whitespace between the square brackets with an 'x',
e.g. [x]. After you create the PR, they will become checkboxes that you
can click on. -->
- [x] I have thoroughly read and understand [The Odin Project
Contributing
Guide](https://github.com/TheOdinProject/theodinproject/blob/main/CONTRIBUTING.md)
- [x] The title of this PR follows the `keyword: brief description of
change` format, using one of the following keywords:
  - `Feature` - adds new or amends existing user-facing behavior
- `Chore` - changes that have no user-facing value, refactors,
dependency bumps, etc
  - `Fix` - bug fixes
-   [x] The `Because` section summarizes the reason for this PR
- [x] The `This PR` section has a bullet point list describing the
changes in this PR
- [x] I have verified all tests and linters pass after making these
changes.
- [x] If this PR addresses an open issue, it is linked in the `Issue`
section
-   [x] If applicable, this PR includes new or updated automated tests
  • Loading branch information
Asartea authored Jul 4, 2023
1 parent baf481e commit 76587c0
Show file tree
Hide file tree
Showing 11 changed files with 95 additions and 183 deletions.
10 changes: 9 additions & 1 deletion app/controllers/errors_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
class ErrorsController < ApplicationController
class ErrorsController < ActionController::Base # rubocop:disable Rails/ApplicationController
def not_found
render status: :not_found
end

def unprocessable_entity
render status: :unprocessable_entity
end

def internal_server_error
render status: :internal_server_error
end
end
9 changes: 9 additions & 0 deletions app/views/errors/internal_server_error.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<%= title('Internal Server Error (500)') %>

<p class="text-base text-gold-600">500</p>
<h1 class="mt-4 text-5xl text-slate-900 font-bold">Server error</h1>
<p class="leading-7 mt-6 text-slate-600">We're sorry, but something went wrong.</p>
<div class="mt-10 flex items-center justify-center gap-x-6">
<%= link_to 'Go back home', root_path, class: 'button button--primary' %>
<a href="https://github.com/TheOdinProject/theodinproject/issues/new/choose" class="text-gold-600 hover:text-gold-500">Report an issue on GitHub <span aria-hidden="true">&rarr;</span></a>
</div>
8 changes: 5 additions & 3 deletions app/views/errors/not_found.html.erb
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
<div class="bg-blue-400 dark:bg-gray-900 h-screen">
<%= title('Page not found (404)') %>

<div class="bg-blue-400 dark:bg-gray-900 h-screen w-screen">
<div class="grid grid-cols-12">
<div class="col-span-12 lg:col-span-10 lg:col-start-2">
<div class="text-center pt-12">
<h1 class="text-white text-3xl lg:text-4xl font-semibold pb-6 dark:text-gray-200">Looks like you're lost, Odinite!</h1>
<p class="text-gray-100 dark:text-gray-400">While you're here, maybe take a second to learn about <%= link_to 'HTTP status codes', 'https://en.wikipedia.org/wiki/List_of_HTTP_status_codes', class: 'text-white font-bold underline' %></p>
<p class="text-gray-100 dark:text-gray-400">Or just head back to <%= link_to 'theodinproject.com', home_path, class: 'text-gold-800 dark:text-gold-600 font-bold underline' %></p>
<p class="text-gray-100 dark:text-gray-400">While you're here, maybe take a second to learn about <%= link_to 'HTTP status codes', 'https://en.wikipedia.org/wiki/List_of_HTTP_status_codes', class: 'text-slate-800 hover:text-slate-700 font-bold underline' %></p>
<p class="text-gray-100 dark:text-gray-400">Or just <%= link_to 'head back home', root_path, class: 'text-slate-800 hover:text-slate-700 font-bold underline' %></p>
</div>

<div class="h-[60vh]">
Expand Down
9 changes: 9 additions & 0 deletions app/views/errors/unprocessable_entity.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<%= title('Unprocessable entity (500)') %>

<p class="text-base text-gold-600">422</p>
<h1 class="mt-4 text-5xl text-slate-900 font-bold">Unprocessable entity</h1>
<p class="leading-7 mt-6 text-slate-600">The change you wanted was rejected. Maybe you tried to change something you didn't have access to</p>
<div class="mt-10 flex items-center justify-center gap-x-6">
<%= link_to 'Go back home', root_path, class: 'button button--primary' %>
<a href="https://github.com/TheOdinProject/theodinproject/issues/new/choose" class="text-gold-600 hover:text-gold-500">Report an issue on GitHub <span aria-hidden="true">&rarr;</span></a>
</div>
19 changes: 19 additions & 0 deletions app/views/layouts/errors.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<!DOCTYPE html>
<html>
<head>
<title>
<%= content_for(:title) || 'Your Career in Web Development Starts Here | The Odin Project' %>
</title>
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<%= stylesheet_link_tag 'application_stylesheet', media: 'all' %>
<%= stylesheet_pack_tag 'application' %>
<%= javascript_pack_tag 'application', 'data-turbo-track': 'reload', defer: true %>
</head>
<body class="text-center h-screen m-0">
<main class="grid place-items-center min-h-screen">
<div>
<%= yield %>
</div>
</main>
</body>
</html>
3 changes: 1 addition & 2 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ class Application < Rails::Application
# Application configuration can go into files in config/initializers
# -- all .rb files in that directory are automatically loaded after loading
# the framework and any gems in your application.
require Rails.root.join('lib/custom_public_exceptions')
config.exceptions_app = CustomPublicExceptions.new(Rails.public_path)
config.exceptions_app = routes
config.middleware.insert_after ActionDispatch::Static, Rack::Deflater
end
end
6 changes: 4 additions & 2 deletions config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# rubocop:disable Lint/MissingCopEnableDirective, Metrics/BlockLength
Rails.application.routes.draw do
match '/404' => 'errors#not_found', via: :all
match '422' => 'errors#unprocessable_entity', via: :all
match '500' => 'errors#internal_server_error', via: :all

draw(:redirects)
ActiveAdmin.routes(self)

Expand Down Expand Up @@ -105,6 +109,4 @@

resources :notifications, only: %i[index update]
resource :themes, only: :update

match '/404' => 'errors#not_found', via: %i[get post patch delete]
end
11 changes: 0 additions & 11 deletions lib/custom_public_exceptions.rb

This file was deleted.

82 changes: 0 additions & 82 deletions public/422.html

This file was deleted.

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

This file was deleted.

39 changes: 39 additions & 0 deletions spec/requests/errors_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
require 'rails_helper'

RSpec.describe 'Error Pages' do
describe 'GET 404' do
it 'returns the correct status code' do
get '/404'
expect(response).to have_http_status(:not_found)
end

it 'returns the correct page' do
get '/404'
expect(response.body).to include('Looks like you\'re lost, Odinite!')
end
end

describe 'GET 422' do
it 'returns the correct status code' do
get '/422'
expect(response).to have_http_status(:unprocessable_entity)
end

it 'returns the correct page' do
get '/422'
expect(response.body).to include('The change you wanted was rejected. Maybe you tried to change something')
end
end

describe 'GET 500' do
it 'returns the correct status code' do
get '/500'
expect(response).to have_http_status(:internal_server_error)
end

it 'returns the correct page' do
get '/500'
expect(response.body).to include('We\'re sorry, but something went wrong.')
end
end
end

0 comments on commit 76587c0

Please sign in to comment.