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

Remove django guardian from querysets #5853

Merged
merged 2 commits into from Jun 27, 2019

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jun 27, 2019

We set these permissions when we save a project or a version

https://github.com/rtfd/readthedocs.org/blob/1ee57dec16f352d14ca17c1bcd8877a9a88a8f0a/readthedocs/projects/models.py#L424-L425
https://github.com/rtfd/readthedocs.org/blob/1ee57dec16f352d14ca17c1bcd8877a9a88a8f0a/readthedocs/builds/models.py#L249-L250

We set them to all the owners of a project,
we already know all projects of an user with user.projects.

For now I'm just removing guardian from the querysets,
in another PR I'm going to remove the dep of guardian

This also removes the use of distinct (this generates problems when trying to do another or |) (This failed, I'm keeping distinct :/)

  • self.all() - This doesn't look like it's going to return duplicated results
  • self.filter() - this is filtering for selected pk's only, I don't think we can have duplicated results neither
We set these permissions when we save a project or a version

https://github.com/rtfd/readthedocs.org/blob/1ee57dec16f352d14ca17c1bcd8877a9a88a8f0a/readthedocs/projects/models.py#L424-L425
https://github.com/rtfd/readthedocs.org/blob/1ee57dec16f352d14ca17c1bcd8877a9a88a8f0a/readthedocs/builds/models.py#L249-L250

We set them to all the owners of a project,
we already know all projects of an user with `user.projects`.

For now I'm just removing guardian from the querysets,
in another PR I'm going to remove the dep of guardian
@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 27, 2019

I triggered a couple of builds locally, nothing broken so far

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 27, 2019

Got a duplicated :/

@stsewd stsewd requested a review from Jun 27, 2019
@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 27, 2019

Wasn't able to remove the distinct call, but I have other ideas, but in another PR after this.

@@ -91,9 +91,9 @@ def _add_user_repos(self, queryset, user=None):
if user.has_perm('builds.view_version'):
Copy link
Member Author

@stsewd stsewd Jun 27, 2019

btw, we don't set this in any part of the code, so I guess this can only be done in the db, but again, I don't think we do that or want to do that.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 27, 2019

I didn't see so much changes in the number of queries, actually the number of queries got reduced in the project dashboard page

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 27, 2019

There's also this, but I think it is the same as the project save:

https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/projects/forms.py#L487-L489

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 27, 2019

Yeah, it's the same, it adds another user to the project-user relationship

stsewd added a commit to stsewd/readthedocs.org that referenced this issue Jun 27, 2019
Copy link
Member

@ericholscher ericholscher left a comment

This is an obvious and simple change. Hopefully it doesn't break everything :D

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jun 27, 2019

Let's not break the internet 🤞

@stsewd stsewd merged commit e4a7766 into readthedocs:master Jun 27, 2019
1 check passed
@stsewd stsewd deleted the remove-part-of-guardian branch Jun 27, 2019
stsewd added a commit to stsewd/readthedocs.org that referenced this issue Jun 27, 2019
@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Jul 4, 2019

Tested this with external and internal versions and its working as expected now. Thanks :)

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.

None yet

3 participants