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

Use querysets from the class not from an instance #5783

Merged
merged 5 commits into from Jun 13, 2019

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jun 11, 2019

When filtering using public and using a user,
the queryset hit this

https://github.com/rtfd/readthedocs.org/blob/45df7fd0da44be9eab3c0cb2888f6a9a15421fc5/readthedocs/builds/querysets.py#L22-L24

When the user is authenticated, we call to get_objects_for_user
which gets all the versions from all the user's projects.
Overriding any previous filter (project.versions in this case)

We don't see this in production because we serve from another domain.
And we don't see this in the corporate site because we override the
serve_docs view.

Fix #4350
Closes #4356

When filtering using `public` and using a user,
the queryset hit this

https://github.com/rtfd/readthedocs.org/blob/45df7fd0da44be9eab3c0cb2888f6a9a15421fc5/readthedocs/builds/querysets.py#L22-L24

When the user is authenticated, we call to `get_objects_for_user`
which gets all the versions from all the user's projects.
Overriding any previous filter (`project.versions` in this case)

We don't see this in production because we serve from another domain.
And we don't see this in the corporate site because we override the
serve_docs view.

Fix readthedocs#4350
Closes readthedocs#4356
@stsewd stsewd requested a review from Jun 11, 2019
@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 11, 2019

Writing a test now

@@ -25,7 +25,7 @@ def get_version_compare_data(project, base_version=None):
:param base_version: We assert whether or not the base_version is also the
highest version in the resulting "is_highest" value.
"""
versions_qs = project.versions.public().filter(active=True)
versions_qs = Version.objects.public(project=project)
Copy link
Member Author

@stsewd stsewd Jun 11, 2019

filter by active is the default in .org and .com querysets

'readthedocs.core.views.serve._serve_symlink_docs',
new=mock.MagicMock(return_value=HttpResponse(content_type='text/html')),
)
def test_user_with_multiple_projects_serve_from_same_domain(self):
Copy link
Member Author

@stsewd stsewd Jun 11, 2019

This test case fails on master :)

@xrmx
Copy link
Contributor

@xrmx xrmx commented Jun 11, 2019

@stsewd any chance you can check this pr pass the tests of #4356 please?

@humitos
Copy link
Member

@humitos humitos commented Jun 11, 2019

I don't fully follow the problem and the solution.

What are the steps to reproduce it?

The solution seems to implicitly say that "project.versions." can't be used, is that correct?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 11, 2019

The solution seems to implicitly say that "project.versions." can't be used, is that correct?

Yes.

Steps to reproduce:

  • Create two projects with a common branch name (latest)
  • The projects need to share the same owner
  • Build the version of both projects
  • While logged in (make sure to not have old cookies by logging out first)
  • View the docs of one -- it returns the error (get returned 2 objects)
  • (you need to use the same domain from the web app)

The problem is that when the user is authenticated, get_objects_for_user is called, which returns all the versions of all projects.

We filter this before, but all versions get added later anyway, so that breaks the unique slug on the queryset.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 11, 2019

@stsewd any chance you can check this pr pass the tests of #4356 please?

It should pass, but it could break when using private versions and other privacy related stuff.

Copy link
Contributor

@davidfischer davidfischer left a comment

Looks good. I think this is a bit cleaner as well.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 13, 2019

Should we add the tests from #4356 here, so we can close that one too?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 13, 2019

@ericholscher the test that was added here replace that test, and the test for admin access was added because the other PR drops the public from the queryset.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 13, 2019

Sounds good 👍

@stsewd stsewd merged commit af2ad69 into readthedocs:master Jun 13, 2019
1 check passed
@stsewd stsewd deleted the use-querysets-as-expected branch Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants