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.

Sign up for GitHub

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

Jump to bottom

Exact Redirect between version doesn't work without slash #9507

Closed
jcfr opened this issue Aug 16, 2022 · 7 comments
Closed

Exact Redirect between version doesn't work without slash #9507

jcfr opened this issue Aug 16, 2022 · 7 comments
Labels
Needed: more information A reply from issue author is required Needed: tests Tests are required Support Support question

Comments

@jcfr
Copy link

jcfr commented Aug 16, 2022

Details

Expected Result

We are currently building documentation associated with branch Slicer@5.0 and setup an Exact Redirect from /en/v5.0/$rest -> /en/5.0/

image

And we were expecting the following to work:

from URL to URL
https://slicer.readthedocs.io/en/v5.0 https://slicer.readthedocs.io/en/5.0
https://slicer.readthedocs.io/en/v5.0/ https://slicer.readthedocs.io/en/5.0/
https://slicer.readthedocs.io/en/v5.0/user_guide/modules/transforms.html https://slicer.readthedocs.io/en/5.0/user_guide/modules/transforms.html

Actual Result

Here is what we observe:

from URL to URL Status
https://slicer.readthedocs.io/en/v5.0 https://slicer.readthedocs.io/en/5.0 ❌
https://slicer.readthedocs.io/en/v5.0/ https://slicer.readthedocs.io/en/5.0/ ✔️
https://slicer.readthedocs.io/en/v5.0/user_guide/modules/transforms.html https://slicer.readthedocs.io/en/5.0/user_guide/modules/transforms.html ✔️

Setting up the additional redirect Exact Redirect from /en/v5.0 -> /en/5.0 didn't help either.

image

And there are still observing the following:

from URL to URL Status
https://slicer.readthedocs.io/en/v5.0 https://slicer.readthedocs.io/en/5.0 ❌
https://slicer.readthedocs.io/en/v5.0/ https://slicer.readthedocs.io/en/5.0/ ✔️
https://slicer.readthedocs.io/en/v5.0/user_guide/modules/transforms.html https://slicer.readthedocs.io/en/5.0/user_guide/modules/transforms.html ✔️
@jcfr jcfr changed the title Exact Redirect associated between version doesn't handle slash Exact Redirect between version doesn't handle slash Aug 16, 2022
@jcfr jcfr changed the title Exact Redirect between version doesn't handle slash Exact Redirect between version doesn't work without slash Aug 16, 2022
@jcfr jcfr mentioned this issue Aug 16, 2022
Closed
jcfr added a commit to jcfr/Slicer that referenced this issue Aug 16, 2022
@jcfr
BUG: Update sphinx custom 404 page to handle redirect of vX.Y URL wit…
e2def00 
…hout slash

This commit adds a javascript redirect to handle the redirection of page
like https://slicer.readthedocs.io/en/v5.0 to https://slicer.readthedocs.io/en/v5.0/
and workaround limitation documented in readthedocs/readthedocs.org#9507

Indeed, despite of adding the following "Exact Redirect" to the readthedocs
configuration:

```
/en/v5.0 -> /en/5.0
/en/v5.0/$rest -> /en/5.0/
```
, specifying https://slicer.readthedocs.io/en/v5.0 doesn't redirect to
https://slicer.readthedocs.io/en/5.0
@jcfr jcfr mentioned this issue Aug 16, 2022
Merged
jcfr added a commit to jcfr/Slicer that referenced this issue Aug 16, 2022
@jcfr
BUG: Update sphinx custom 404 page to handle redirect of vX.Y URL wit…
1ba81e9 
…hout slash

This commit adds a javascript redirect to handle the redirection of page
like https://slicer.readthedocs.io/en/v5.0 to https://slicer.readthedocs.io/en/v5.0/
and workaround limitation documented in readthedocs/readthedocs.org#9507

Indeed, despite of adding the following "Exact Redirect" to the readthedocs
configuration:

```
/en/v5.0 -> /en/5.0
/en/v5.0/$rest -> /en/5.0/
```
, specifying https://slicer.readthedocs.io/en/v5.0 doesn't redirect to
https://slicer.readthedocs.io/en/5.0
jcfr added a commit to Slicer/Slicer that referenced this issue Aug 16, 2022
@jcfr
BUG: Update sphinx custom 404 page to handle redirect of vX.Y URL wit…
a3b7544 
…hout slash

This commit adds a javascript redirect to handle the redirection of page
like https://slicer.readthedocs.io/en/v5.0 to https://slicer.readthedocs.io/en/v5.0/
and workaround limitation documented in readthedocs/readthedocs.org#9507

Indeed, despite of adding the following "Exact Redirect" to the readthedocs
configuration:

```
/en/v5.0 -> /en/5.0
/en/v5.0/$rest -> /en/5.0/
```
, specifying https://slicer.readthedocs.io/en/v5.0 doesn't redirect to
https://slicer.readthedocs.io/en/5.0
@humitos
Copy link
Member

humitos commented Aug 17, 2022

Hi! Have you tried from /en/v5.0$rest to /en/5.0? Note the is no trailing slash. I think that should work.

@humitos humitos added Support Support question Needed: more information A reply from issue author is required labels Aug 17, 2022
@stsewd
Copy link
Member

stsewd commented Aug 29, 2022

Setting up the additional redirect Exact Redirect from /en/v5.0 -> /en/5.0 didn't help either.

This should be fixed with #9557. And maybe the original redirect that has $rest will also be fixed.

@humitos
Copy link
Member

humitos commented Aug 30, 2022

@stsewd @ericholscher we should create a test case for this particular issue 👍🏼

@humitos humitos added the Needed: tests Tests are required label Aug 30, 2022
@humitos
Copy link
Member

humitos commented Aug 30, 2022

@jcfr can you confirm if you are still having this issue? We just deployed a potential fix and I quickly tested this by creating a redirect as I suggested in my previous comment (#9507 (comment)) and it worked! 👍🏼

@jcfr
Copy link
Author

jcfr commented Aug 31, 2022

Thanks for the update 🙏 I will follow up on Thursday when back on the computer.

@jcfr
Copy link
Author

jcfr commented Nov 22, 2022 •
edited
Loading

I confirm the fix works as expected:

Before

  • Relying on javascript redirect implemented in Slicer/Slicer@a3b7544)
  • Redirect: /en/v5.0/$rest to /en/5.0/
image

After

  • with updated redirect config suggested in above
    Redirect: /en/v5.0$rest to /en/5.0
image

@jcfr jcfr closed this as completed Nov 22, 2022
@jcfr jcfr mentioned this issue Nov 22, 2022
Merged
@ericholscher
Copy link
Member

@jcfr Thanks for the follow up and closing the issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Needed: more information A reply from issue author is required Needed: tests Tests are required Support Support question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ericholscher @jcfr @humitos @stsewd