Use custom error pages (#1921)

* Use custom error pages * Revert config.consider_all_requests_local * Update tests * Render instead of redirect * Remove unnecessary error routes * Delete 404 and 500 files
autolab · Jun 24, 2023 · b8831e3 · b8831e3
1 parent cace108
commit b8831e3
Show file tree

Hide file tree

Showing 30 changed files with 28 additions and 152 deletions.
diff --git a/app/controllers/admins_controller.rb b/app/controllers/admins_controller.rb
@@ -2,10 +2,6 @@
 # this controller contains methods for system-wise
 # admin functionality
 class AdminsController < ApplicationController
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   skip_before_action :set_course
 
   action_auth_level :email_instructors, :administrator

diff --git a/app/controllers/annotations_controller.rb b/app/controllers/annotations_controller.rb
@@ -8,10 +8,6 @@ class AnnotationsController < ApplicationController
   before_action :set_assessment
   before_action :set_submission
   before_action :set_annotation, except: [:create, :shared_comments]
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   respond_to :json
 
   # POST /:course/annotations.json

diff --git a/app/controllers/announcements_controller.rb b/app/controllers/announcements_controller.rb
@@ -3,10 +3,6 @@
 class AnnouncementsController < ApplicationController
   before_action :set_announcement, except: %i[index new create]
 
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   action_auth_level :index, :instructor
   def index
     @announcements = if @cud.user.administrator?

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -20,10 +20,6 @@ class ApplicationController < ActionController::Base
   before_action :update_persistent_announcements
   before_action :set_breadcrumbs
 
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   # this is where Error Handling is configured. this routes exceptions to
   # the error handler in the HomeController, unless we're in development mode
   #
@@ -146,14 +142,14 @@ def set_course
                   (params[:controller] == "courses" ? params[:name] : nil)
     @course = Course.find_by(name: course_name) if course_name
 
-    render file: Rails.root.join("public/404.html"), status: :not_found and return unless @course
+    render("home/error_404") && return unless @course
 
     # set course logger
     begin
       COURSE_LOGGER.setCourse(@course)
     rescue StandardError => e
       flash[:error] = e.to_s
-      redirect_to(controller: :home, action: :error) && return
+      render("home/error_500") && return
     end
     ASSESSMENT_LOGGER.setCourse(@course)
   end
@@ -381,7 +377,7 @@ def render_error(exception)
           end
         end
 
-        render "home/error"
+        render "home/error_500"
       end
       format.json { head :internal_server_error }
       format.js { head :internal_server_error }

diff --git a/app/controllers/assessment_user_data_controller.rb b/app/controllers/assessment_user_data_controller.rb
@@ -6,9 +6,6 @@ class AssessmentUserDataController < ApplicationController
   # inherited from ApplicationController
   before_action :set_assessment
   before_action :set_aud
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   action_auth_level :edit, :instructor
   def edit

diff --git a/app/controllers/assessments_controller.rb b/app/controllers/assessments_controller.rb
@@ -10,10 +10,6 @@ class AssessmentsController < ApplicationController
   include ActiveSupport::Callbacks
   include AssessmentAutogradeCore
 
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   autolab_require Rails.root.join("app/controllers/assessment/handin.rb")
   include AssessmentHandin
 

diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb
@@ -9,10 +9,6 @@ class AttachmentsController < ApplicationController
   before_action :set_attachment, except: %i[index new create]
   before_action :add_attachments_breadcrumb
 
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   action_auth_level :index, :instructor
   def index
     @attachments = @is_assessment ? @assessment.attachments : @course.attachments

diff --git a/app/controllers/course_user_data_controller.rb b/app/controllers/course_user_data_controller.rb
@@ -1,10 +1,6 @@
 class CourseUserDataController < ApplicationController
   before_action :add_users_breadcrumb
 
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   action_auth_level :index, :student
   def index
     @requestedUser = @cud

diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb
@@ -11,10 +11,6 @@ class CoursesController < ApplicationController
   # if there's no course, there are no persistent announcements for that course
   skip_before_action :update_persistent_announcements, only: %i[courses_redirect index new create]
 
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
-
   def index
     courses_for_user = User.courses_for_user current_user
 

diff --git a/app/controllers/extensions_controller.rb b/app/controllers/extensions_controller.rb
@@ -6,9 +6,6 @@
 class ExtensionsController < ApplicationController
   # inherited from ApplicationController
   before_action :set_assessment
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   # TODO
   action_auth_level :index, :instructor

diff --git a/app/controllers/gradebooks_controller.rb b/app/controllers/gradebooks_controller.rb
@@ -4,9 +4,6 @@
 require "utilities"
 
 class GradebooksController < ApplicationController
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
   action_auth_level :show, :student
   def show
     if @cud.instructor?

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
@@ -4,9 +4,6 @@ class GroupsController < ApplicationController
   before_action :check_assessment_for_groups
   before_action :set_group, only: %i[show edit update destroy add join leave]
   respond_to :html
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   ##
   # can be used by instructors to check groups.  Students get redirected,

diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb
@@ -7,9 +7,6 @@ class HomeController < ApplicationController
   skip_before_action :authorize_user_for_course
   skip_before_action :authenticate_for_action
   skip_before_action :update_persistent_announcements
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   def developer_login
     return unless request.post?
@@ -59,4 +56,6 @@ def contact
   end
 
   def error_404; end
+
+  def error_500; end
 end
diff --git a/app/controllers/jobs_controller.rb b/app/controllers/jobs_controller.rb
@@ -10,9 +10,6 @@ class JobsController < ApplicationController
 
   # index - This is the default action that generates lists of the
   # running, waiting, and completed jobs.
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
   action_auth_level :index, :student
   def index
     # Instance variables that will be used by the view

diff --git a/app/controllers/problems_controller.rb b/app/controllers/problems_controller.rb
@@ -6,9 +6,6 @@ class ProblemsController < ApplicationController
   # inherited from ApplicationController
   before_action :set_assessment
   before_action :set_problem, only: %i[edit update destroy]
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   action_auth_level :new, :instructor
   def new

diff --git a/app/controllers/schedulers_controller.rb b/app/controllers/schedulers_controller.rb
@@ -4,9 +4,6 @@
 # hasn't ran in more than its period's time, it's function is run.  This is awful.
 #
 class SchedulersController < ApplicationController
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
   action_auth_level :index, :instructor
   def index
     @schedulers = Scheduler.where(course_id: @course.id)

diff --git a/app/controllers/scoreboards_controller.rb b/app/controllers/scoreboards_controller.rb
@@ -5,9 +5,6 @@ class ScoreboardsController < ApplicationController
   before_action :set_assessment
   before_action :set_assessment_breadcrumb, only: [:edit]
   before_action :set_scoreboard, except: [:create]
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   action_auth_level :create, :instructor
   def create

diff --git a/app/controllers/scores_controller.rb b/app/controllers/scores_controller.rb
@@ -6,9 +6,6 @@ class ScoresController < ApplicationController
   before_action :set_assessment
   before_action :set_submission
   before_action :set_score, except: [:create]
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   action_auth_level :create, :course_assistant
   def create

diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb
@@ -9,9 +9,6 @@ class SubmissionsController < ApplicationController
   before_action :set_assessment
   before_action :set_submission, only: %i[destroy destroyConfirm download edit update view]
   before_action :get_submission_file, only: %i[download view]
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
 
   # this page loads.  links/functionality may be/are off
   action_auth_level :index, :instructor

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
@@ -3,9 +3,6 @@ class UsersController < ApplicationController
   skip_before_action :authorize_user_for_course
   skip_before_action :authenticate_for_action
   skip_before_action :update_persistent_announcements
-  rescue_from ActionView::MissingTemplate do |_exception|
-    redirect_to("/home/error_404")
-  end
   before_action :set_gh_oauth_client, only: [:github_oauth, :github_oauth_callback]
   before_action :set_user,
                 only: [:github_oauth, :github_revoke, :lti_launch_initialize,

diff --git a/app/views/home/error.html.erb → app/views/home/error_500.html.erb b/app/views/home/error.html.erb → app/views/home/error_500.html.erb
diff --git a/config/environments/development.rb b/config/environments/development.rb
@@ -64,4 +64,7 @@
   # See (TODO replace with doc link)
   config.x.github.client_id = ENV['GITHUB_CLIENT_ID']
   config.x.github.client_secret = ENV['GITHUB_CLIENT_SECRET']
+
+  # Use custom routes for error pages
+  config.exceptions_app = self.routes
 end
diff --git a/config/environments/production.rb.template b/config/environments/production.rb.template
@@ -118,4 +118,7 @@ Autolab3::Application.configure do
   config.x.github.client_secret = ENV['GITHUB_CLIENT_SECRET']
   # LTI configuration file storage location. Needs to be in a private folder
   config.lti_config_location  = Rails.root.join('config').to_s
+
+  # Use custom routes for error pages
+  config.exceptions_app = self.routes
 end
diff --git a/config/environments/test.rb b/config/environments/test.rb
@@ -50,4 +50,7 @@
   # Raises error for missing translations.
   # config.action_view.raise_on_missing_translations = true
   config.lti_config_location  = Rails.root.join('spec/fixtures/lti_config_files').to_s
+
+  # Use custom routes for error pages
+  config.exceptions_app = self.routes
 end
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
@@ -15,7 +15,7 @@
   #   # Put your admin authentication logic here.
   #   # Example implementation:
   #   Admin.find_by_id(session[:admin_id]) || redirect_to(new_admin_session_url)
-    (!current_user && warden.authenticate!(:scope => :user)) || current_user.administrator? || redirect_to("/404.html")
+    (!current_user && warden.authenticate!(:scope => :user)) || current_user.administrator? || redirect_to(controller: :home, action: :error_404)
   end
 
   # Authorization Code expiration time (default 10 minutes).

diff --git a/config/routes.rb b/config/routes.rb
@@ -60,8 +60,6 @@
     if Rails.env == "development" || Rails.env == "test"
       match "developer_login", via: [:get, :post]
     end
-    get "error"
-    get "error_404"
     get "no_user"
   end
 
@@ -244,4 +242,7 @@
     get "get_branches"
     get "get_commits"
   end
+
+  get "/404", to: "home#error_404"
+  get "/500", to: "home#error_500"
 end
diff --git a/public/404.html b/public/404.html
diff --git a/public/422.html b/public/422.html
diff --git a/public/500.html b/public/500.html
diff --git a/spec/controllers/home_controller_spec.rb b/spec/controllers/home_controller_spec.rb
@@ -19,9 +19,17 @@
     end
   end
 
-  describe "#error" do
+  describe "#error_404" do
     it "renders successfully" do
-      get :error
+      get :error_404
+      expect(response).to be_successful
+      expect(response.body).to match(/Not found/m)
+    end
+  end
+
+  describe "#error_500" do
+    it "renders successfully" do
+      get :error_500
       expect(response).to be_successful
       expect(response.body).to match(/Internal error/m)
     end