Call distinct to the end of the querysets

[stsewd]

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]

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

[ericholscher]

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]

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 https://github.com/readthedocs/readthedocs-corporate/pull/559/commits/0e39f8bffeebe2399454db514fe69d8c9cfbb446

[stsewd]

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

[ericholscher]

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.

[ericholscher]

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

[humitos]

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]

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

[humitos]

We have git log and tests

git log and tests are not docs 😉