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 Redirect to work properly when using $rest keyword #4501

Merged
merged 9 commits into from Aug 22, 2018

Conversation

Projects
None yet
4 participants
@humitos
Member

humitos commented Aug 9, 2018

In #3965 we added the ability to use the full_path but it was added only to the first case and it wasn't considered in the case when $rest keyword was used.

With these changes we can "deprecate a whole sub-path" by writing a Exact Redirect like this:

/en/obsolete-version/$rest ->
/en/newest-version/

and all links pointing to the obsolete-version at any sub-path will be redirected to the newest docs.

Closes #4029
Closes #2444
Closes #3794

@humitos humitos requested a review from rtfd/core Aug 9, 2018

@humitos humitos requested a review from stsewd Aug 9, 2018

.. note::
In other words, a *Prefix Redirect* removes a prefix from the original URL.

This comment has been minimized.

@stsewd

stsewd Aug 10, 2018

Member

Not sure if this note is necessary, I found it a little confusing, the above example makes more sense for me

@stsewd

stsewd Aug 10, 2018

Member

Not sure if this note is necessary, I found it a little confusing, the above example makes more sense for me

This comment has been minimized.

@humitos

humitos Aug 13, 2018

Member

I want to have a simple way to explain what Prefix Redirects does because it seems that explaining it by example with a "custom domain" is not enough and confuses people that don't use a custom domain.

So, I'd like to have something very simple explained in one line. Maybe my one-line explanation is not the best and someone can help me here. But I'd like to keep a note like this.

@humitos

humitos Aug 13, 2018

Member

I want to have a simple way to explain what Prefix Redirects does because it seems that explaining it by example with a "custom domain" is not enough and confuses people that don't use a custom domain.

So, I'd like to have something very simple explained in one line. Maybe my one-line explanation is not the best and someone can help me here. But I'd like to keep a note like this.

Show outdated Hide outdated docs/user-defined-redirects.rst Outdated
Note that you should insert the desired language for "en" and version for "latest" to
achieve the desired redirect.
*Exact Redirects* could be also useful to redirect a whole sub-path to a different one by using a special ``$rest`` keyword in the "From URL".
Let's say that you want to redirect your readers of your version ``2.0`` of your documentation under ``/en/2.0/`` because it's deprecated,

This comment has been minimized.

@stsewd

stsewd Aug 10, 2018

Member

This useful when renaming dirs too

@stsewd

stsewd Aug 10, 2018

Member

This useful when renaming dirs too

This comment has been minimized.

@humitos

humitos Aug 13, 2018

Member

When renaming dirs the $rest argument shouldn't be used. Instead, you should just use /old-dir/ to /new-dir/. This way, it will do this under all of the languages and versions.

@humitos

humitos Aug 13, 2018

Member

When renaming dirs the $rest argument shouldn't be used. Instead, you should just use /old-dir/ to /new-dir/. This way, it will do this under all of the languages and versions.

Show outdated Hide outdated docs/user-defined-redirects.rst Outdated
@ericholscher

Looks good. I remember that redirects also have some weird behavior with a leading /, which might be worth investigating here also.

I particularly like the examples. It would probably be useful for us to provide more examples like this, particularly around cases that are common.

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 21, 2018

Member

I remember that redirects also have some weird behavior with a leading /, which might be worth investigating here also.

Good point!

I just added a test for this to check how it behaves. I'm not sure what's the expected behavior here, but what it does sounds useless to me. Example:

From URL: /jp/latest/$rest
To URL: en/latest/

Hitting /jp/latest/guides/install.html redirects to /jp/latest/guides/en/latest/guides/install.html

What would be good use case for this? Should we validate the presence of the leading / and make it mandatory for Exact Redirects? Is this validation only useful when using $rest in From URL?

Member

humitos commented Aug 21, 2018

I remember that redirects also have some weird behavior with a leading /, which might be worth investigating here also.

Good point!

I just added a test for this to check how it behaves. I'm not sure what's the expected behavior here, but what it does sounds useless to me. Example:

From URL: /jp/latest/$rest
To URL: en/latest/

Hitting /jp/latest/guides/install.html redirects to /jp/latest/guides/en/latest/guides/install.html

What would be good use case for this? Should we validate the presence of the leading / and make it mandatory for Exact Redirects? Is this validation only useful when using $rest in From URL?

Trailing/Leading slash validations in RedirectForm
Validate that leading/trailing slash is used in PREFIX redirects when
using $rest keyword.

Add other minor test cases for other types of redirects.
@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 21, 2018

Member

I added some test cases and some validation at Form level to avoid people submitting malformed URL (missing leading/trailing slashes).

I suppose this is ready to merge once the tests pass.

A large project related to these changes is to have real redirects, not only on 404: #4550

Member

humitos commented Aug 21, 2018

I added some test cases and some validation at Form level to avoid people submitting malformed URL (missing leading/trailing slashes).

I suppose this is ready to merge once the tests pass.

A large project related to these changes is to have real redirects, not only on 404: #4550

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Aug 21, 2018

Member

I believe there are other redirects with similar problems. For example:

    @override_settings(USE_SUBDOMAIN=True)
    def test_redirect_page(self):
        Redirect.objects.create(
            project=self.pip, redirect_type='page',
            from_url='install.html', to_url='/tutorial/install.html'
        )
        r = self.client.get('install.html', HTTP_HOST='pip.readthedocs.org')
        self.assertEqual(r.status_code, 302)

This will return False because a path will never not start with a / from Django. So we shouldn't let users save a from_url without a / (or we should add it for them).

Member

ericholscher commented Aug 21, 2018

I believe there are other redirects with similar problems. For example:

    @override_settings(USE_SUBDOMAIN=True)
    def test_redirect_page(self):
        Redirect.objects.create(
            project=self.pip, redirect_type='page',
            from_url='install.html', to_url='/tutorial/install.html'
        )
        r = self.client.get('install.html', HTTP_HOST='pip.readthedocs.org')
        self.assertEqual(r.status_code, 302)

This will return False because a path will never not start with a / from Django. So we shouldn't let users save a from_url without a / (or we should add it for them).

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 22, 2018

Member

Just realized that we do support redirect to external URLs, so we need to support URLs starting with http also. This needs more thinking... As usual :)

I think this is deviating from the original purpose of the PR: fix the exact redirect with the usage of $rest. I want to have this fixed, merged and deployed soon, so I will create an issue to fix other issues around redirects and revert the commit of the validation.

Member

humitos commented Aug 22, 2018

Just realized that we do support redirect to external URLs, so we need to support URLs starting with http also. This needs more thinking... As usual :)

I think this is deviating from the original purpose of the PR: fix the exact redirect with the usage of $rest. I want to have this fixed, merged and deployed soon, so I will create an issue to fix other issues around redirects and revert the commit of the validation.

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Aug 22, 2018

Member

Sounds good. 👍

Member

ericholscher commented Aug 22, 2018

Sounds good. 👍

@humitos humitos merged commit b9f74e3 into master Aug 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@humitos humitos deleted the humitos/redirect/exact branch Aug 22, 2018

@wohali

This comment has been minimized.

Show comment
Hide comment
@wohali

wohali Aug 24, 2018

@humitos When will this fix go live? I'm unable to get it working just yet....Thank you so much for your work on this! 🎊

wohali commented Aug 24, 2018

@humitos When will this fix go live? I'm unable to get it working just yet....Thank you so much for your work on this! 🎊

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 28, 2018

Member

@wohali Hi! We are planning to deploy this tomorrow.

Member

humitos commented Aug 28, 2018

@wohali Hi! We are planning to deploy this tomorrow.

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 29, 2018

Member

This is already deployed and should be working.

Member

humitos commented Aug 29, 2018

This is already deployed and should be working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment