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

Standardize error template paths #11494

Merged
merged 12 commits into from
Sep 6, 2024
Merged

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jul 23, 2024

In trying to rework the templates for proxito, I found it quite confusing
keeping the dashboard and proxito templates separate. This separates proxito
specific errors into a separate path just for these templates, errors/proxito,
instead of using various paths and naming conventions for these files.

This makes it possible for the new dashboard to isolate proxito error templates
and to extend from a common proxito error base.

@agjohnson
Copy link
Contributor Author

I was expecting this PR to fail miserably, but seems it is either somehow okay or we don't have enough test coverage 🤷

@agjohnson agjohnson marked this pull request as ready for review July 23, 2024 23:58
@agjohnson agjohnson requested review from a team as code owners July 23, 2024 23:58
@agjohnson agjohnson requested review from stsewd and removed request for ericholscher July 24, 2024 00:00
@agjohnson agjohnson force-pushed the agj/standardize-error-templates branch 2 times, most recently from 1a013b9 to b3078cf Compare August 26, 2024 23:12
View changes to support just two paths for errors, `errors/dashboard`
and `errors/proxito`.
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.
"Matching DNS record not found" is not as clear as "Domain not found".
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.
@agjohnson agjohnson force-pushed the agj/standardize-error-templates branch from b3078cf to 7b7e84c Compare September 6, 2024 03:28
@agjohnson
Copy link
Contributor Author

Hrm, seems the last remaining piece is whatever these failures are:

------------------------------ Captured log call -------------------------------
WARNING  django.request:log.py:241 I'm a Teapot: /projects/subproject/
____________________ RedirectTests.test_subproject_root_url ____________________

self = <readthedocs.proxito.tests.test_redirects.RedirectTests testMethod=test_subproject_root_url>

    def test_subproject_root_url(self):
        r = self.client.get(
            "/projects/subproject/",
            secure=True,
            headers={"host": "project.dev.readthedocs.io"},
        )
>       self.assertEqual(r.status_code, 302)
E       AssertionError: 418 != 302

readthedocs/proxito/tests/test_redirects.py:89: AssertionError

I'm a little confused by them right now. I can see why these tests could fail with the 418 status response, but I can't yet explain why.

@humitos
Copy link
Member

humitos commented Sep 6, 2024

I don't have the answer here, but it seems those tests are hitting the dummy_dashboard_urls instead of the documentation Proxito urls.

@humitos
Copy link
Member

humitos commented Sep 6, 2024

Maybe the order of the definition changed somehow? /projects/subproject/ should be caught by

re_path(r"^(?P<path>.*)$", ServeDocs.as_view(), name="docs_detail"),

@humitos
Copy link
Member

humitos commented Sep 6, 2024

Yeap, that's the issue. This diff makes that test to pass again:

diff --git a/readthedocs/proxito/urls.py b/readthedocs/proxito/urls.py
index ba465b3bc..2f3d33b8c 100644
--- a/readthedocs/proxito/urls.py
+++ b/readthedocs/proxito/urls.py
@@ -212,9 +212,10 @@ debug_urls = [
 groups = [
     health_check_urls,
     proxied_urls,
-    dummy_dashboard_urls,
     core_urls,
     docs_urls,
+    # Dummy dashboard URLs are required to be defined at the end
+    dummy_dashboard_urls,
 ]
 
 if settings.SHOW_DEBUG_TOOLBAR:

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Looks great! 💯

I left a comment with the solution for the test that we should include here as well.

@agjohnson
Copy link
Contributor Author

Thanks for that! I figured it had to be something around the URL changes too, I ended my day before getting into debugging at all though.