Skip to content

Commit

Permalink
Standardize error template paths (#11494)
Browse files Browse the repository at this point in the history
* Default error view drop top level errors and split proxito error views

View changes to support just two paths for errors, `errors/dashboard`
and `errors/proxito`.

* Replace one off teapot view

* Move spam template to standard location

* Point proxito views at new errors/proxito path

* Move top level errors to dashboard error templates path

* Add proxito error templates path with links to dashboard templates for now

* Move proxito 404 errors to proxito error path

* Make error view handler more explicit

Instead of using the status code to find the error template in the error
view handler, just specify a template name. This doesn't return the
corresponding HTTP status code with the error from the debug error view.

* Reword DNS exception reason

"Matching DNS record not found" is not as clear as "Domain not found".

* Add missing template

* Update pattern for error view for proxito

Don't try to default the status code in the view, but instead leave this
alone and let the template decide. This allows for 4xx/5xx fallback templates to
state the actual status code.

* Fix URL load order bug
  • Loading branch information
agjohnson authored Sep 6, 2024
1 parent 1af71fe commit 933909a
Show file tree
Hide file tree
Showing 40 changed files with 214 additions and 112 deletions.
51 changes: 21 additions & 30 deletions readthedocs/core/views/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ class ErrorView(TemplateView):
multiple subpaths for errors, as we need to show application themed errors
for dashboard users and minimal error pages for documentation readers.
Template resolution also uses fallback to generic 4xx/5xx error templates.
View arguments:
status_code
Expand All @@ -105,49 +107,38 @@ class ErrorView(TemplateView):
separate path from Proxito error templates.
"""

base_path = "errors/dashboard/"
status_code = 500
base_path = "errors/dashboard"
status_code = None
template_name = None

def get_status_code(self):
status_code = self.status_code
try:
status_code = int(self.kwargs["status_code"])
except (ValueError, KeyError):
pass
return status_code
return self.kwargs.get("status_code", self.status_code)

def get_template_name(self):
return self.kwargs.get("template_name", self.template_name)

def get_template_names(self):
status_code = self.get_status_code()
if settings.RTD_EXT_THEME_ENABLED:
# First try to load the template for the specific HTTP status code
# and fall back to a generic 400/500 level error template
status_code_class = int(status_code / 100)
generic_code = f"{status_code_class}xx"
return [
f"{self.base_path}/{code}.html" for code in [status_code, generic_code]
]
# TODO the legacy dashboard has top level path errors, as is the
# default. This can be removed later.
return f"{status_code}.html"
template_names = []
if (template_name := self.get_template_name()) is not None:
template_names.append(template_name.rstrip("/"))
if (status_code := self.get_status_code()) is not None:
template_names.append(str(status_code))
return [f"{self.base_path}/{file}.html" for file in template_names]

def get_context_data(self, **kwargs):
context_data = super().get_context_data(**kwargs)
context_data["status_code"] = self.get_status_code()
return context_data

def dispatch(self, request, *args, **kwargs):
context = self.get_context_data(**kwargs)
context = self.get_context_data()
status_code = self.get_status_code()
return self.render_to_response(
context,
status=status_code,
)


# TODO replace this with ErrorView and a template in `errors/` instead
class TeapotView(TemplateView):
template_name = "core/teapot.html"

def get(self, request, *args, **kwargs):
context = self.get_context_data(**kwargs)
return self.render_to_response(context, status=418)


class PageNotFoundView(View):

"""Just a 404 view that ignores all URL parameters."""
Expand Down
4 changes: 1 addition & 3 deletions readthedocs/projects/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,7 @@ def get(self, request, *args, **kwargs):
)

if is_show_dashboard_denied(self.get_project()):
template_name = "spam.html"
if settings.RTD_EXT_THEME_ENABLED:
template_name = "errors/dashboard/410.html"
template_name = "errors/dashboard/spam.html"
return render(request, template_name=template_name, status=410)

return super().get(request, *args, **kwargs)
18 changes: 8 additions & 10 deletions readthedocs/proxito/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class ContextualizedHttp404(Http404):
The contextualized exception is handled by proxito's 404 handler
"""

template_name = "errors/404/base.html"
template_name = "errors/proxito/404/base.html"
not_found_subject = pgettext_lazy(_not_found_subject_translation_context, "page")

def __init__(self, http_status=404, path_not_found=None, **kwargs):
Expand Down Expand Up @@ -48,10 +48,8 @@ class DomainDNSHttp404(ContextualizedHttp404):

"""Raised if a DNS record points to us and we don't know the domain."""

template_name = "errors/404/dns.html"
not_found_subject = pgettext_lazy(
_not_found_subject_translation_context, "matching DNS record"
)
template_name = "errors/proxito/404/dns.html"
not_found_subject = pgettext_lazy(_not_found_subject_translation_context, "domain")

def __init__(self, domain, **kwargs):
"""
Expand All @@ -73,7 +71,7 @@ class ProjectHttp404(ContextualizedHttp404):
It indicates a number of reasons for the user.
"""

template_name = "errors/404/no_project.html"
template_name = "errors/proxito/404/no_project.html"
not_found_subject = pgettext_lazy(_not_found_subject_translation_context, "project")

def __init__(self, domain, **kwargs):
Expand All @@ -91,7 +89,7 @@ class SubprojectHttp404(ContextualizedHttp404):

"""Raised if a subproject was not found."""

template_name = "errors/404/no_subproject.html"
template_name = "errors/proxito/404/no_subproject.html"
not_found_subject = pgettext_lazy(
"Names an object not found in a 404 error", "subproject"
)
Expand All @@ -111,7 +109,7 @@ class ProjectFilenameHttp404(ContextualizedHttp404):

"""Raised if a page inside an existing project was not found."""

template_name = "errors/404/no_project_page.html"
template_name = "errors/proxito/404/no_project_page.html"
not_found_subject = pgettext_lazy(
_not_found_subject_translation_context, "documentation page"
)
Expand All @@ -136,7 +134,7 @@ class ProjectTranslationHttp404(ContextualizedHttp404):
If a page isn't found, raise a ProjectPageHttp404.
"""

template_name = "errors/404/no_language.html"
template_name = "errors/proxito/404/no_language.html"
not_found_subject = pgettext_lazy(
"Names an object not found in a 404 error", "translation"
)
Expand All @@ -160,7 +158,7 @@ class ProjectVersionHttp404(ContextualizedHttp404):
Note: The containing project can be a subproject.
"""

template_name = "errors/404/no_version.html"
template_name = "errors/proxito/404/no_version.html"
not_found_subject = pgettext_lazy(
_not_found_subject_translation_context, "documentation version"
)
Expand Down
48 changes: 35 additions & 13 deletions readthedocs/proxito/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@
pip.rtd.io/_/api/*
"""

from functools import reduce
from operator import add

from django.conf import settings
from django.urls import include, path, re_path
from django.views import defaults

from readthedocs.constants import pattern_opts
from readthedocs.core.views import HealthCheckView, TeapotView
from readthedocs.core.views import HealthCheckView
from readthedocs.projects.views.public import ProjectDownloadMedia
from readthedocs.proxito.views.hosting import ReadTheDocsConfigJson
from readthedocs.proxito.views.serve import (
Expand All @@ -49,7 +51,7 @@
ServeSitemapXML,
ServeStaticFiles,
)
from readthedocs.proxito.views.utils import proxito_404_page_handler
from readthedocs.proxito.views.utils import ProxitoErrorView, proxito_404_page_handler

DOC_PATH_PREFIX = getattr(settings, "DOC_PATH_PREFIX", "")

Expand Down Expand Up @@ -154,19 +156,19 @@
# /projects/<project_slug>/
re_path(
r"^projects/(?P<project_slug>{project_slug})/$".format(**pattern_opts),
TeapotView.as_view(),
ProxitoErrorView.as_view(status_code=418),
name="projects_detail",
),
# /projects/<project_slug>/builds/
re_path(
(r"^projects/(?P<project_slug>{project_slug})/builds/$".format(**pattern_opts)),
TeapotView.as_view(),
ProxitoErrorView.as_view(status_code=418),
name="builds_project_list",
),
# /projects/<project_slug>/versions/
re_path(
r"^projects/(?P<project_slug>{project_slug})/versions/$".format(**pattern_opts),
TeapotView.as_view(),
ProxitoErrorView.as_view(status_code=418),
name="project_version_list",
),
# /projects/<project_slug>/downloads/
Expand All @@ -176,7 +178,7 @@
**pattern_opts
)
),
TeapotView.as_view(),
ProxitoErrorView.as_view(status_code=418),
name="project_downloads",
),
# /projects/<project_slug>/builds/<build_id>/
Expand All @@ -186,21 +188,41 @@
**pattern_opts
)
),
TeapotView.as_view(),
ProxitoErrorView.as_view(status_code=418),
name="builds_detail",
),
# /projects/<project_slug>/version/<version_slug>/
re_path(
r"^projects/(?P<project_slug>[-\w]+)/version/(?P<version_slug>[^/]+)/edit/$",
TeapotView.as_view(),
ProxitoErrorView.as_view(status_code=418),
name="project_version_detail",
),
]

urlpatterns = (
health_check_urls + proxied_urls + core_urls + docs_urls + dummy_dashboard_urls
)
debug_urls = [
# For testing error responses and templates
re_path(
r"^{DOC_PATH_PREFIX}error/(?P<template_name>.*)$".format(
DOC_PATH_PREFIX=DOC_PATH_PREFIX,
),
ProxitoErrorView.as_view(),
),
]

groups = [
health_check_urls,
proxied_urls,
core_urls,
docs_urls,
# Fallback paths only required for resolving URLs, evaluate these last
dummy_dashboard_urls,
]

if settings.SHOW_DEBUG_TOOLBAR:
groups.insert(0, debug_urls)

urlpatterns = reduce(add, groups)

# Use Django default error handlers to make things simpler
handler404 = proxito_404_page_handler
handler500 = defaults.server_error
handler500 = ProxitoErrorView.as_view(status_code=500)
4 changes: 3 additions & 1 deletion readthedocs/proxito/views/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ def _spam_response(self, request, project):
from readthedocsext.spamfighting.utils import is_serve_docs_denied # noqa

if is_serve_docs_denied(project):
return render(request, template_name="spam.html", status=410)
return render(
request, template_name="errors/proxito/spam.html", status=410
)


class ServeRedirectMixin:
Expand Down
8 changes: 7 additions & 1 deletion readthedocs/proxito/views/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,17 @@
from django.http import HttpResponse
from django.shortcuts import render

from readthedocs.core.views import ErrorView

from ..exceptions import ContextualizedHttp404

log = structlog.get_logger(__name__) # noqa


class ProxitoErrorView(ErrorView):
base_path = "errors/proxito"


def fast_404(request, *args, **kwargs):
"""
A fast error page handler.
Expand All @@ -18,7 +24,7 @@ def fast_404(request, *args, **kwargs):


def proxito_404_page_handler(
request, template_name="errors/404/base.html", exception=None
request, template_name="errors/proxito/404/base.html", exception=None
):
"""
Serves a 404 error message, handling 404 exception types raised throughout the app.
Expand Down
2 changes: 0 additions & 2 deletions readthedocs/templates/404.html

This file was deleted.

10 changes: 0 additions & 10 deletions readthedocs/templates/errors/404/no_language.html

This file was deleted.

10 changes: 0 additions & 10 deletions readthedocs/templates/errors/404/no_project_page.html

This file was deleted.

10 changes: 0 additions & 10 deletions readthedocs/templates/errors/404/no_subproject.html

This file was deleted.

10 changes: 0 additions & 10 deletions readthedocs/templates/errors/404/no_version.html

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{% extends "errors/base.html" %}
{% extends "errors/dashboard/base.html" %}
{% load core_tags %}
{% load i18n %}

Expand Down
File renamed without changes.
46 changes: 46 additions & 0 deletions readthedocs/templates/errors/dashboard/404.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{% extends "errors/dashboard/base.html" %}
{% load core_tags %}
{% load i18n %}

{% block title %}
{% trans "404 Not Found" %}
{% endblock %}

{% block content %}

<h1>{% trans "404 Not Found" %}</h1>

{% block 404_error_message %}
<section>
<p>{% trans "You have encountered a 404 on Read the Docs. You are either looking for a page that does not exist or a project that has been removed." %}</p>
</section>

<pre style="line-height: 1.25; white-space: pre;">

\ What a maze! /
\ /
\ This page does /
] not exist. [ ,'|
] [ / |
]___ ___[ ,' |
] ]\ /[ [ |: |
] ] \ / [ [ |: |
] ] ] [ [ [ |: |
] ] ]__ __[ [ [ |: |
] ] ] ]\ _ /[ [ [ [ |: |
] ] ] ] (#) [ [ [ [ :===='
] ] ]_].nHn.[_[ [ [
] ] ] HHHHH. [ [ [
] ] / `HH("N \ [ [
]__]/ HHH " \[__[
] NNN [
] N/" [
] N H [
/ N \
/ q, \
/ \
</pre>

{% endblock %}

{% endblock %}
Loading

0 comments on commit 933909a

Please sign in to comment.