From 0326b45d6ad8321e539b7af712ec95b29d16d1eb Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 13 Jun 2023 11:25:11 -0400 Subject: [PATCH 1/2] fix: Update how we render the error pages. We were adding paths for the error pages in two places so one of them simply wasn't being used. The lms urls.py also covered the 429 wich the static_templates_view urls.py did not cover. We don't need both and we need the definition of the handlerNNN variables in urls.py to override the default django error views so I'll leave just those. I also made the `exception` parameter for the `render_404` function optional by adding a default value. We don't use the exception when rendering the 404 page but the exception argument is a part of the default method signature for the function that `render_404` replaces so I didn't want to remove it and cause issues when django tries to call this function. --- lms/djangoapps/static_template_view/urls.py | 6 ------ lms/djangoapps/static_template_view/views.py | 2 +- lms/urls.py | 2 ++ 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/static_template_view/urls.py b/lms/djangoapps/static_template_view/urls.py index c7ecd6992c..231913fbfc 100644 --- a/lms/djangoapps/static_template_view/urls.py +++ b/lms/djangoapps/static_template_view/urls.py @@ -9,12 +9,6 @@ from django.urls import path, re_path from lms.djangoapps.static_template_view import views urlpatterns = [ - # Semi-static views (these need to be rendered and have the login bar, but don't change) - path('404', views.render, {'template': '404.html'}, name="404"), - # display error page templates, for testing purposes - path('404', views.render_404, name='static_template_view.views.render_404'), - path('500', views.render_500, name='static_template_view.views.render_500'), - path('blog', views.render, {'template': 'blog.html'}, name="blog"), path('contact', views.render, {'template': 'contact.html'}, name="contact"), path('donate', views.render, {'template': 'donate.html'}, name="donate"), diff --git a/lms/djangoapps/static_template_view/views.py b/lms/djangoapps/static_template_view/views.py index dc3a44bb79..01b99d51c8 100644 --- a/lms/djangoapps/static_template_view/views.py +++ b/lms/djangoapps/static_template_view/views.py @@ -106,7 +106,7 @@ def render_403(request, exception=None): @fix_crum_request -def render_404(request, exception): # lint-amnesty, pylint: disable=unused-argument +def render_404(request, exception=None): # lint-amnesty, pylint: disable=unused-argument request.view_name = '404' return HttpResponseNotFound(render_to_string('static_templates/404.html', {}, request=request)) diff --git a/lms/urls.py b/lms/urls.py index ddb1d79435..58a5cd0a3f 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -214,6 +214,8 @@ urlpatterns = [ ), path('api/discounts/', include(('openedx.features.discounts.urls', 'openedx.features.discounts'), namespace='api_discounts')), + + # Provide URLs where we can see the rendered error pages without having to force an error. path('403', handler403), path('404', handler404), path('429', handler429), From 3b7facd565f1b4c80ef646afa4b8a8718f5de21c Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 13 Jun 2023 12:20:11 -0400 Subject: [PATCH 2/2] fix: Add names to the error page urls. Some of the static_template_view tests use names to get the URLs for the error pages for testing, so I added names and updated the test to match the new names. I also updated the `test_404` function because we're no longer rendering the 404 page in a different way from the 500 page so the response includes the correct response code and content type. This reduces the number of differences between the 404 handler and the 500 handler. --- lms/djangoapps/static_template_view/tests/test_views.py | 8 ++++---- lms/urls.py | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/static_template_view/tests/test_views.py b/lms/djangoapps/static_template_view/tests/test_views.py index f876c8897b..c8f04fc8c0 100644 --- a/lms/djangoapps/static_template_view/tests/test_views.py +++ b/lms/djangoapps/static_template_view/tests/test_views.py @@ -67,16 +67,16 @@ class MarketingSiteViewTests(TestCase): """ Test the 404 view. """ - url = reverse('static_template_view.views.render_404') + url = reverse('render_404') resp = self.client.get(url) - assert resp.status_code == 200 - assert resp['Content-Type'] == 'text/html' + assert resp.status_code == 404 + assert resp['Content-Type'] == 'text/html; charset=utf-8' def test_500(self): """ Test the 500 view. """ - url = reverse('static_template_view.views.render_500') + url = reverse('render_500') resp = self.client.get(url) assert resp.status_code == 500 assert resp['Content-Type'] == 'text/html; charset=utf-8' diff --git a/lms/urls.py b/lms/urls.py index 58a5cd0a3f..390e6f9c8b 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -216,10 +216,10 @@ urlpatterns = [ namespace='api_discounts')), # Provide URLs where we can see the rendered error pages without having to force an error. - path('403', handler403), - path('404', handler404), - path('429', handler429), - path('500', handler500), + path('403', handler403, name='render_403'), + path('404', handler404, name='render_404'), + path('429', handler429, name='render_429'), + path('500', handler500, name='render_500'), ] if settings.FEATURES.get('ENABLE_MOBILE_REST_API'):