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

Fix /en/latest redirects #6564

Merged
merged 5 commits into from Jan 27, 2020
Merged

Fix /en/latest redirects #6564

merged 5 commits into from Jan 27, 2020

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 21, 2020

This is caused by the URLConf not matching because of the missing last /.
So to fix it we:

  • Add a custom URLconf that catches just the lang/version in a URL without a slash
  • Pass a variable into the ServeDocs function so it doesn't try and serve it and 404's
  • The 404 handlers will properly parse it using this new URLConf, and then properly find the index.html, appending a /

@ericholscher
Copy link
Member Author

ericholscher commented Jan 22, 2020

David +0'd this in chat, so going to hotfix it to get things working in prod.

@ericholscher ericholscher added the PR: hotfix label Jan 22, 2020
This is caused by the URLConf not matching because of the missing last /.
So to fix it we:

* Add a custom URLconf that catches just the lang/version in a URL without a slash
* Pass a variable into the ServeDocs function so it doesn't try and serve it and 404's
* The 404 handlers will properly parse it using this new URLConf, and then properly find the index.html, appending a /
@ericholscher
Copy link
Member Author

ericholscher commented Jan 22, 2020

This has been applied, and fixed the issue.

Copy link
Member

@humitos humitos left a comment

I think the fix works and it's OK. Although, I would like to discuss a different pattern to handle this. It seems that we are using ServeDocs when it's not needed and just adding complexity to the flow. I've made a suggestion, but I'm open to other ideas as well.

readthedocs/proxito/tests/test_full.py Outdated Show resolved Hide resolved
r'(?P<lang_slug>{lang_slug})/'
r'(?P<version_slug>{version_slug})$'.format(**pattern_opts)
),
ServeDocs.as_view(redirect_root=True),
Copy link
Member

@humitos humitos Jan 22, 2020

Choose a reason for hiding this comment

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

From the tests, I understand that the "root" is not the problem, but "directory indexing" is. I would suggest to adapt the name of the variable and comment related to match the logic.

Copy link
Member

@humitos humitos Jan 22, 2020

Choose a reason for hiding this comment

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

Also, if we are short-cutting the flow immediately in the .get method, there is no need to use ServeDocs here. We can just use handler_404/fast_404 and return 404 directly.

Copy link
Member Author

@ericholscher ericholscher Jan 23, 2020

Choose a reason for hiding this comment

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

True 👍

readthedocs/proxito/urls.py Outdated Show resolved Hide resolved
# Hack /en/latest so it redirects properly
# We don't want to serve the docs here,
# because it's at a different level of serving so relative links break.
Copy link
Member

@humitos humitos Jan 22, 2020

Choose a reason for hiding this comment

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

Why we don't make the previous URLConf with optional trailing slash instead of creating a new one?

Copy link
Member Author

@ericholscher ericholscher Jan 23, 2020

Choose a reason for hiding this comment

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

That is what the comment is explaining :)

Copy link
Member

@humitos humitos left a comment

👍 💯 🐛

@@ -46,18 +46,4 @@ server {
add_header X-Served Proxito-404-Fallback always;
}

# Main RTD site for non-doc serving needs
location @fallback {
Copy link
Member

@humitos humitos Jan 27, 2020

Choose a reason for hiding this comment

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

I suppose we can remove this block from -ops as well.

Copy link
Member Author

@ericholscher ericholscher Jan 27, 2020

Choose a reason for hiding this comment

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

Yea, I believe it should already be removed.

Copy link
Member Author

@ericholscher ericholscher Jan 27, 2020

Choose a reason for hiding this comment

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

@ericholscher ericholscher merged commit d835193 into master Jan 27, 2020
3 checks passed
@ericholscher ericholscher deleted the hack-redirects branch Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants