I think we can have a more performant query if this becomes something like:

q &= (
    Q(user=user) |
    (Q(public=True) &
     (repo_q &
      Q(review_request__target_people=user) |
      group_q))
)


That allows the query planner to have to consider only one set of rows for a public=True, and short-circuits that if we know the user matches.
It does mean repeating some of that above, but we can just take the (repo_q ...) bits and put that into a reusable acl_check_q or something.

Seems like a good performance improvement. Let's document these conditions for an empty result with a comment.

So going through the other changes for some duplicate object fixes I'm putting together, I think we need to take the opportunity to better organize these tests so it's explicitly clear what data we're working with.
As such, with these tests (and ideally we'd update the others to match, but let's start here), let's have a pattern of putting a comment above each distinct group of related data explaining what access policy that's testing. For example:

# Published review on a publicly-accessible review request.
review1 = self.create_review(review_request, publish=True)

...

# Draft review on a publicly-accessible review request.
...

# Published review on a private repository the user has access to.
...

# Published review on a private repository the user does not have access to.
...


etc.
That way it's very easy for us to see what data we're working with, and it's very easy to know where the code should live.
Bonus points for a comment above each assertQuerysetEqual that says what we're testing for, and usage of assertQueries() as part of that to ensure we're building exactly the queries we want.

For booleans, we allow ('1', 'true', 'True').
Not ideal to repeat this everywhere, but let's call that a problem for another day.

How should we check that the diffset related to the review is accessible by the user? I see the logic for it in ReviewRequest._is_diffset_accessible_by(user, diffset) but how can we efficiently do this in the Managers through a query?

Should I consolidate these common queries into a variable or something? Then use the variable instead of repeating the full queries

This is a lot of work! Thanks for doing this :)
Some small nits and one thing that may be lacking a test.

This should be review__public, right?
If so, and no tests failed, this means we're missing a test.

Imports should be in alphabetical order.

Comments should generally end with a period.

Blank line before new blocks (same below).

For multi-line lists, we generally do:

[
    review1, review2, ...,
    review5, ...,
]


Or, when appropriate, one item per row (generally more appropriate when we may be operating more on the list over time, to minimize changes to the code).

Given this is one Q, we can just put this on the same line.
Same in other places.

Another example of where we want periods at the end of the sentence.
Really applies to all such comments.

Awesome, home stretch. One tiny code styling nit (in a handful of places), but everything else looks ready to go.

Missing a trailing comma (we always want to terminate multi-column list items with a trailing comma to minimize line changes if we add to the list).
This applies to other queries as well.

Ship It!

Ship It!