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

Use custom error pages #1921

Merged
merged 7 commits into from
Jun 24, 2023
Merged

Use custom error pages #1921

merged 7 commits into from
Jun 24, 2023

Conversation

damianhxy
Copy link
Member

@damianhxy damianhxy commented Jun 8, 2023

Summary

Summary generated by Reviewpad on 24 Jun 23 16:20 UTC

This pull request includes changes to multiple files, which involve removing rescue_from blocks for ActionView::MissingTemplate exceptions, adding custom error pages using new error_404 and error_500 methods, changing authorization levels for some controller actions, modifying before and after action settings, rearranging some skip_before_action statements, and adding new configurations to use custom routes for error pages in the development and production environments. In addition, some unnecessary get routes were removed and new routes using get were added. Finally, some methods were added and removed from certain files such as home_controller_spec.rb, admins_controller.rb, and scoreboards_controller.rb.

Description

  • Remove the use of rescue_from ActionView::MissingTemplate since we have rescue_from Exception, with: :render_error under application_controller.rb
  • Add an error_500 method to home_controller for routing purposes
  • Rename error.html.erb to error_500.html.erb
  • Add routes for /404 and /500
  • Add config.exceptions_app = self.routes to environments, so that routes.rb is used for 404 and 500 errors
  • Use render instead of redirect_to in set_course so that the url is preserved
  • Update tests for error pages

References

Motivation and Context

Currently, 404 errors are implicitly handled by Rails rendering the 404.html page. Not only does this look bad because it doesn't include the layout, but also assets (favicon, image) do not properly load because they use absolute paths.

This PR shifts towards using the existing error_404 error page that was previously used for MissingTemplate errors.

Closes #682

How Has This Been Tested?

Tested using docker compose install (checkout this PR in the autolab subdirectory and rebuild). It'll help to add a throw Exception on some route for testing later, e.g. assessments_controller#show.

Note, this PR changes production.rb.template by adding the line config.exceptions_app = self.routes. However, this doesn't apply to existing instances of Autolab.

Below is a comparison of the various error pages with and without this additional line. Basically, the only difference is that 404 errors (outside of controllers that use set_course) work properly

Currently on Autolab prod / nightly

Invalid course name: e.g. {domain}/courses/dummy/assessments
Screenshot 2023-06-09 at 01 14 47

Invalid page: e.g. {domain}/a/b
Screenshot 2023-06-09 at 01 14 55

Exception
(Not tested, but should be the same as the following)

This PR: Before adding config.exceptions_app = self.routes

Invalid course name: e.g. {domain}/courses/dummy/assessments
Screenshot 2023-06-09 at 00 09 28

Invalid page: e.g. {domain}/a/b
Screenshot 2023-06-09 at 00 10 45

Exception: on the page you added the Exception to
Screenshot 2023-06-09 at 00 11 00

This PR: After adding config.exceptions_app = self.routes

Invalid course name: e.g. {domain}/courses/dummy/assessments
Screenshot 2023-06-09 at 00 20 50

Invalid page: e.g. {domain}/a/b
Screenshot 2023-06-09 at 00 20 24

Exception: on the page you added the Exception to
Screenshot 2023-06-09 at 00 20 40

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@reviewpad reviewpad bot added the medium Pull request is medium label Jun 8, 2023
@damianhxy damianhxy marked this pull request as ready for review June 8, 2023 17:17
@reviewpad reviewpad bot requested a review from najclark June 8, 2023 17:17
Copy link
Contributor

@najclark najclark left a comment

Choose a reason for hiding this comment

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

lgtm, confirmed behavior works as expected

public/404.html Outdated Show resolved Hide resolved
@damianhxy damianhxy added this pull request to the merge queue Jun 24, 2023
Merged via the queue into master with commit b8831e3 Jun 24, 2023
1 check passed
@damianhxy damianhxy deleted the error_pages branch June 24, 2023 16:56
@damianhxy damianhxy mentioned this pull request Dec 31, 2023
6 tasks
NicholasMy pushed a commit to UB-CSE-IT/Autolab that referenced this pull request Jan 3, 2024
* 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

(cherry picked from commit b8831e3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium Pull request is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 and 500 pages don't load assets
2 participants