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

check for matching alias before subproject slug #2787

Merged

Conversation

@funkyHat
Copy link
Contributor

@funkyHat funkyHat commented Apr 10, 2017

fixes #2731

with override_settings(USE_SUBDOMAIN=False):
resp = self.client.get('/docs/pip/projects/sub_alias/')
self.assertEqual(resp.status_code, 302)
self.assertEqual(
resp._headers['location'][1],
'http://readthedocs.org/docs/pip/projects/sub_alias/ja/latest/'
)

def test_resolver_subproject_subdomain_alias(self):
Copy link
Contributor Author

@funkyHat funkyHat Apr 10, 2017

This test causes test_resolver_subproject_alias to fail if it runs first ("fixed" by creatively renaming so it runs after).
I had a bit of a look around to see if there's a cache I can easily clean but couldn't find anything.
Depends how much of an issue you think this is, but if you're worried & can give me a pointer I'll happily update it to dtrt

@funkyHat funkyHat force-pushed the subproject_check_alias_before_slug branch from f856067 to e07be83 Jun 29, 2017
@funkyHat
Copy link
Contributor Author

@funkyHat funkyHat commented Oct 25, 2017

Hi all, it's been 6 months since I've seen an update on this PR, and the issue is still affecting readthedocs.org ... any chance we can get this merged/let me know if changes are required?

Thanks

with override_settings(USE_SUBDOMAIN=False):
resp = self.client.get('/docs/pip/projects/sub_alias/')
self.assertEqual(resp.status_code, 302)
self.assertEqual(
resp._headers['location'][1],
'http://readthedocs.org/docs/pip/projects/sub_alias/ja/latest/'
)

def test_resolver_subproject_subdomain_alias(self):
with override_settings(USE_SUBDOMAIN=True):
Copy link
Member

@safwanrahman safwanrahman Oct 25, 2017

Can you use override_settings as decorator?

Copy link
Contributor Author

@funkyHat funkyHat Oct 28, 2017

done :)

@funkyHat
Copy link
Contributor Author

@funkyHat funkyHat commented Nov 10, 2017

Do you need anything else before this can be merged? Happy to make any required changes

@RichardLitt RichardLitt requested a review from ericholscher Nov 10, 2017
)
subproject = rel.child
except (ProjectRelationship.DoesNotExist, KeyError):
subproject = Project.objects.get(slug=subproject_slug)
Copy link
Member

@berkerpeksag berkerpeksag Jan 7, 2018

We could replace the whole try...except...raise block with get_object_or_404(): https://docs.djangoproject.com/en/1.9/topics/http/shortcuts/#get-object-or-404


class ResolverAltSetUp(object):

def setUp(self):
Copy link
Member

@berkerpeksag berkerpeksag Jan 7, 2018

Copy link
Contributor Author

@funkyHat funkyHat Jan 18, 2018

It does save 8s (out of ~20 for the module 👍), but it also seems to change the results:


self = <readthedocs.rtd_tests.tests.test_resolver.SmartResolverPathTests testMethod=test_resolver_subproject_subdomain>

    def test_resolver_subproject_subdomain(self):
        with override_settings(USE_SUBDOMAIN=False):
            import pdb; pdb.set_trace()
            url = resolve_path(project=self.subproject, filename='index.html')
>           self.assertEqual(url, '/docs/pip/projects/sub/ja/latest/')
E           AssertionError: u'/docs/pip/projects/sub/' != '/docs/pip/projects/sub/ja/latest/'

rtd_tests/tests/test_resolver.py:105: AssertionError
_______________________________ SmartResolverPathTestsAlt.test_resolver_subproject_subdomain _______________________________

self = <readthedocs.rtd_tests.tests.test_resolver.SmartResolverPathTestsAlt testMethod=test_resolver_subproject_subdomain>

    def test_resolver_subproject_subdomain(self):
        with override_settings(USE_SUBDOMAIN=False):
            import pdb; pdb.set_trace()
>           url = resolve_path(project=self.subproject, filename='index.html')
E           AssertionError: u'/docs/pip/projects/sub/' != '/docs/pip/projects/sub/ja/latest/'

I'm not really sure what's going on to cause that, unfortunately. Any clues?

Copy link
Member

@berkerpeksag berkerpeksag Jan 19, 2018

Is SmartResolverPathTestsAlt.test_resolver_subproject_subdomain the only failing test? SmartResolverPathTests is the base class of SmartResolverPathTestsAlt and ResolverBase is the base class of SmartResolverPathTests. There is a setUp method defined in ResolverBase:

https://github.com/rtfd/readthedocs.org/blob/6a669349cf5349087a2369de8a6011b434ac858e/readthedocs/rtd_tests/tests/test_resolver.py#L18-L26

That method can be cause of the test failure.

Adding

setUp = None

to SmartResolverPathTestsAlt may help.

There is also the following warning in setUpTestData documentation:

Be careful not to modify any objects created in setUpTestData() in your test methods. Modifications to in-memory objects from setup work done at the class level will persist between test methods. If you do need to modify them, you could reload them in the setUp() method with refresh_from_db(), for example.

And we indeed do modify objects created in setUpTestData. For example:

https://github.com/rtfd/readthedocs.org/blob/6a669349cf5349087a2369de8a6011b434ac858e/readthedocs/rtd_tests/tests/test_resolver.py#L61

Copy link
Contributor Author

@funkyHat funkyHat Jan 19, 2018

test_resolver_subproject_subdomain from both SmartResolverPathTests and SmartResolverPathTestsAlt (i.e. yes, the one defined test).
These 2 failures are happening with both setUp methods (i.e. in ResolverBase and ResolverAltSetUp) converted to seUpTestData classmethods.

Are you happy to leave this optimisation for now? Reworking all the modifications seems like overkill for 8s savings (maybe should be a separate PR in any case)?

Copy link
Member

@berkerpeksag berkerpeksag Jan 19, 2018

Yes, let's leave it to a separate PR.

self.translation = fixture.get(Project, slug='trans', language='ja',
users=[ self.owner],
main_language_project=None)
self.subproject = fixture.get(Project, slug='sub', language='ja', users=[
Copy link
Member

@berkerpeksag berkerpeksag Jan 7, 2018

I'd revert these style changes if the linting tool is happy with the old style.

@funkyHat funkyHat force-pushed the subproject_check_alias_before_slug branch from 701e221 to 9bb0f28 Jan 18, 2018
subproject = Project.objects.get(slug=subproject_slug)
except Project.DoesNotExist:
raise Http404
get_object_or_404(Project, slug=subproject_slug)
Copy link
Member

@berkerpeksag berkerpeksag Jan 19, 2018

Shouldn't this line be read as

subproject = get_object_or_404(Project, slug=subproject_slug)

?

@tannewt
Copy link

@tannewt tannewt commented Mar 7, 2018

I just ran into this as well. I would love to see this fixed so we don't need to encode language and version into URLs unnecessarily.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 7, 2018

Can you include a link to something that reproduces this issue currently? There are likely other bugs that are causing this code to be hit.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 7, 2018

Ah, I found #2731 -- will give it a peek.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 7, 2018

Looks good. Thanks for the patch -- sorry it took so long to review!

@ericholscher ericholscher merged commit d127ea5 into readthedocs:master Mar 7, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants