-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Support single version subprojects URLs to serve from Django #5690
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
Conversation
In fact, this problem is more related to "Given an URL, parse it to get all the RTD objects from it" as we have talked. Currently, we are doing this at Maybe it's time to work on this if we are considering the "big refactor". |
It's not super clear to me where the design decision lays in this PR. Perhaps it would be more helpful to put this change into more context and summarize in more detail where you think the problems are with this implementation. It's also not clear why the middleware refactor is required, so I don't feel I have enough context to offer guidance. |
Basically, if
The problem is that it does not follow our pattern of using the specific urlconf depending on project specific values (like
When accessing a subproject (e.g. Besides, improving the middleware by setting this extra value(s) is useful in another places like for custom 404 page in subprojects. |
If this approach works, I'm +1 on shipping it and fixing a user's approach. |
Hopefully this code will all disappear in the next couple months, so I'm OK with hacks :) |
readthedocs/core/views/serve.py
Outdated
# the index file instead. | ||
if subproject and subproject.single_version: | ||
# TODO: find a way to import ``serve_docs`` from corporate | ||
return serve_docs(request, project, project.slug, subproject, subproject.slug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only needed for corporate, the only missing thing in this hacky PR is to import serve_docs
from corporate before merging (readthedocsinc.core.views.serve_docs
).
Use the serve_docs from Corporate as a HACK and do nothing extra if it fails for any reason
3319558
to
f43ece3
Compare
I finished the hack for this PR. It should not affect .org at all. We should check that this works when deploying .com. |
This was moved to Corporate project, where it belongs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine I guess -- I really dislike this pattern, but at least it's temporary.
readthedocs/core/views/serve.py
Outdated
from readthedocsinc.core.views import serve_docs as corporate_serve_docs # noqa | ||
return corporate_serve_docs(request, project, project.slug, subproject, subproject.slug) | ||
except Exception: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should log here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I added logs in both cases.
Currently, Read the Docs Community does not have this problem because we serve directly from NGINX without passing through Django to get the
X-Accel-Redirect
header in its response.Although, on Corporate we are returning an infinite redirect when accessing
/projects/subproject/
URL when subproject is a Single Version project.redirect_project_slug
view that is called when hitting/projects/subproject/
to redirect to its default lang/version --this does not apply on single version subprojecturls/subdomain.py
file (and not onurls/single_version.py
), which should not be called when accessing a single version project is hitSingleVersionMiddleware
(which overrides therequest.urlconf
) is not detecting that we are accessing to a single version project, because it uses therequest.slug
(which is the superproject) to check it. Related to Proxito: look for custom 404 pages in the parent project of subprojects #5557 (comment)The current changes on this PR makes it work locally when
USE_SUBDOMAIN=True
, although it's not the best fix for this. To define this URL where it belongs to, we need to touch ourSubdomainMiddleware
to properly setsubproject_slug
and use that in different places instead --which could be a bigger refactor/improvement.