-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 exact redirects. #3965
Fix exact redirects. #3965
Conversation
Exact redirects depend on the full path being set against, so that you can do things like redirect versions. This was broken in a previous refactor.
Deployed this as a hotfix for a customer who was broken. Linting errors look unrelated, but probably worth fixing in another PR> |
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.
Changes look good!
I just commented about a minor refactor.
readthedocs/redirects/models.py
Outdated
def redirect_exact(self, path, language=None, version_slug=None): | ||
if language and version_slug: | ||
# reconstruct the full path for an exact redirect | ||
full_path = '/{language}/{version_slug}/{path}'.format( |
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.
Isn't it better here to use self.get_full_path
which uses our resolve_path
and handle more cases?
Hi, I'm hitting this too in swiftenv. We have an exact redirect from @ericholscher I'm not sure what the time-line looks like to get this PR into production. You mentioned you've deployed a hot-fix for a specific customer. Is it possible to get this fix applied to my project swiftenv if the PR won't land soon? I'd really appreciate it. I looked into the failing build regarding lint but that seems to be reporting problems that are elsewhere than changed in this PR. Are these failing files expected?
|
@kylef When we apply the hotfix label, it means the fix is already in production, we just need to circle back to an actual release with the hotfix. It seems you could be hitting additional problems introduced by this hotfix. I can reproduce the 5xx error by going to: Attaching https://sentry.io/read-the-docs/readthedocs-org/issues/530998543/ |
@kylef We're going to test your case on this next deploy. Best to follow up on a separate issue next though, as this is getting merged for our release. |
@agjohnson My apologies, I would have normally filed a separate issue but while trying to find the commit that introduced the possible breakage I discovered this PR (which at the time I though was not deployed). I looked at the test case and though it was the same problem. Will file another shortly. |
@kylef I think your issue is solved. |
I think it is, thanks @humitos and whomever else that involved in fixing it. ❤️ |
Exact redirects depend on the full path being set against, so that you can do things like redirect versions. This was broken in a previous refactor.