readthedocs.org icon indicating copy to clipboard operation
readthedocs.org copied to clipboard

Standardize error template paths

Open agjohnson opened this issue 1 year ago • 1 comments

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.

  • Refs https://github.com/readthedocs/ext-theme/issues/304
  • Requires https://github.com/readthedocs/common/pull/220

agjohnson avatar Jul 23 '24 03:07 agjohnson

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

agjohnson avatar Jul 23 '24 23:07 agjohnson

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.

agjohnson avatar Sep 06 '24 03:09 agjohnson

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 avatar Sep 06 '24 08:09 humitos

Maybe the order of the definition changed somehow? /projects/subproject/ should be caught by https://github.com/readthedocs/readthedocs.org/blob/f941fc73b912ab885075f51d3ea6e6e4da7e03b9/readthedocs/proxito/urls.py#L144

humitos avatar Sep 06 '24 09:09 humitos

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:

humitos avatar Sep 06 '24 09:09 humitos

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.

agjohnson avatar Sep 06 '24 18:09 agjohnson