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

Call distinct to the end of the querysets #5872

Merged
merged 2 commits into from Jul 11, 2019
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jul 1, 2019

This allow us to extend easily the querysets
and use | (or) to merge to querysets
(this can't be done after calling distinct)

This needs changes in .com

This allow us to extend easily the querysets
and use `|` (or) to merge to querysets
(this can't be done after calling distinct)

This needs changes in .com
@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 1, 2019

I did some builds and navigate the site, everything keeps working

@stsewd stsewd requested a review from Jul 2, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 2, 2019

Seems like this could likely use some additional tests. I'm guessing the distinct() call is because we're adding users querysets which could already exist in the base queryset? I imagine you have a bit more context for why this is needed, and should be written down somewhere, and likely shown in a test.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 2, 2019

So, _add_user_repos is the one query that can generate duplicate results (as far as I can tell). When .distinct is used, we can't no longer do old_queryset | new_queryset, this doesn't allow us to merge to results (likely this help us to fix .public ignoring previous filters or maybe not).

Another reason for this is to be consistent too, we sometimes are returning .distinct which can't use | and other times just a normal queryset. And this would help me to move the auth checking to a central place. See readthedocs/readthedocs-corporate@0e39f8b

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 2, 2019

Coverage is good https://codecov.io/gh/readthedocs/readthedocs.org/pull/5872/diff, but it can be improved

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 2, 2019

I don't care about coverage as much as making sure we actually have a test for the logic we care about. We should make sure we have a test that is creating duplicated objects, and then confirming they are removed.

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

@ericholscher ericholscher left a comment

Looks good, thanks for writing up these tests. I like having this stuff extra tested. 🔒

Copy link
Member

@humitos humitos left a comment

It would be good to have a comment in the code (on the class maybe) explaining a little more why this is done in one place (each method) and not in the other (_add_user_repos).

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 11, 2019

@humitos feel free to add the comment. We have git log and tests to prevent future us from breaking this.

@stsewd stsewd merged commit 2242c32 into master Jul 11, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the stsewd/distinct-querysets branch Jul 11, 2019
@humitos
Copy link
Member

@humitos humitos commented Jul 11, 2019

We have git log and tests

git log and tests are not docs 😉

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