what do you mean by "should be consolidated?" perhaps I can address this TODO in my changes

i feel iffy about changing the logic of the _query() function to this extent. now if we're querying for unpublished reviews and a user is given, it will only return unpublished reviews that are owned by that user. i made this change because that's what i need for the accessible() function to work properly, otherwise it would return drafts that are owned by other users. i tried to think of other ways for accessible() to only return drafts owned by the given user, but none worked out. if you see any ways to achieve this without having to modify _query(), or if it's fine to modify _query() like this let me know.

i'm wondering if there's a way to add users to a local site

Let's add a period at the end of this.

We can get rid of this now, in favor of LocalSite.ALL (which we'll want to document as a valid type for local_site -- I think I have this in one of my changes).

Since this method is new, we can default this to off. It's a bit inconsistent with other changes, but is probably the correct way to go.

I think we should just require a User. Something I want to phase out is the whole user-or-username aspect of the other methods, because it eventually leads to unwanted extra queries.
The reason is that, for the kinds of checks we perform, we need a User anyway. So the query function has to get one. However, if the code calling this eventually needs the same User, they have to query for it again, which is therefore slower.
Requiring a User ensures the caller has to think about when it's getting a user and how.

When we have functions with multiple keyword arguments, it's best to do one per line. That helps with both readability and maintainability.

These queries are very expensive, full of nested joins and stuff. We don't want to blindly OR those together, as it'll get pretty hairy pretty quickly.
Let's look at how this looks. We'll go with a simple example. Just one _query():

str(Review.objects._query(some_non_admin_user, public=True, status=None, filter_private=True).query)


Which gives us something like the following (after I run it through a formatter):

SELECT
  DISTINCT "reviews_review"."id",
  "reviews_review"."review_request_id",
  "reviews_review"."user_id",
  "reviews_review"."timestamp",
  "reviews_review"."public",
  "reviews_review"."ship_it",
  "reviews_review"."base_reply_to_id",
  "reviews_review"."email_message_id",
  "reviews_review"."time_emailed",
  "reviews_review"."body_top",
  "reviews_review"."body_top_rich_text",
  "reviews_review"."body_bottom",
  "reviews_review"."body_bottom_rich_text",
  "reviews_review"."body_top_reply_to_id",
  "reviews_review"."body_bottom_reply_to_id",
  "reviews_review"."extra_data",
  "reviews_review"."rich_text",
  "reviews_review"."reviewed_diffset_id"
FROM
  "reviews_review"
  INNER JOIN "reviews_reviewrequest" ON (
    "reviews_review"."review_request_id" = "reviews_reviewrequest"."id"
  )
  LEFT OUTER JOIN "reviews_reviewrequest_target_people" ON (
    "reviews_reviewrequest"."id" = "reviews_reviewrequest_target_people"."reviewrequest_id"
  )
  LEFT OUTER JOIN "reviews_reviewrequest_target_groups" ON (
    "reviews_reviewrequest"."id" = "reviews_reviewrequest_target_groups"."reviewrequest_id"
  )
WHERE
  (
    "reviews_review"."public" = TRUE
    AND "reviews_review"."base_reply_to_id" IS NULL
    AND "reviews_reviewrequest"."local_site_id" IS NULL
    AND (
      "reviews_review"."user_id" = 1
      OR (
        (
          "reviews_reviewrequest"."repository_id" IS NULL
          OR "reviews_reviewrequest"."repository_id" IN (1, 3, 4,)
        )
        AND (
          "reviews_reviewrequest_target_people"."user_id" = 1
          OR "reviews_reviewrequest_target_groups"."group_id" IS NULL
          OR "reviews_reviewrequest_target_groups"."group_id" IN (1, 2, 3, 4)
        )
      )
    )
  )
ORDER BY
  "reviews_review"."timestamp" ASC


If we run another (with public=False), we get something similar.
Now let's do the OR:

str((Review.objects._query(non_admin_user, public=True, status=None, filter_private=True) | Review.objects._query(non_admin_user, public=False, status=None, filter_private=True)).query)


We get:

SELECT
  DISTINCT "reviews_review"."id",
  "reviews_review"."review_request_id",
  "reviews_review"."user_id",
  "reviews_review"."timestamp",
  "reviews_review"."public",
  "reviews_review"."ship_it",
  "reviews_review"."base_reply_to_id",
  "reviews_review"."email_message_id",
  "reviews_review"."time_emailed",
  "reviews_review"."body_top",
  "reviews_review"."body_top_rich_text",
  "reviews_review"."body_bottom",
  "reviews_review"."body_bottom_rich_text",
  "reviews_review"."body_top_reply_to_id",
  "reviews_review"."body_bottom_reply_to_id",
  "reviews_review"."extra_data",
  "reviews_review"."rich_text",
  "reviews_review"."reviewed_diffset_id"
FROM
  "reviews_review"
  INNER JOIN "reviews_reviewrequest" ON (
    "reviews_review"."review_request_id" = "reviews_reviewrequest"."id"
  )
  LEFT OUTER JOIN "reviews_reviewrequest_target_people" ON (
    "reviews_reviewrequest"."id" = "reviews_reviewrequest_target_people"."reviewrequest_id"
  )
  LEFT OUTER JOIN "reviews_reviewrequest_target_groups" ON (
    "reviews_reviewrequest"."id" = "reviews_reviewrequest_target_groups"."reviewrequest_id"
  )
WHERE
  (
    (
      "reviews_review"."public" = TRUE
      AND "reviews_review"."base_reply_to_id" IS NULL
      AND "reviews_reviewrequest"."local_site_id" IS NULL
      AND (
        "reviews_review"."user_id" = 1
        OR (
          (
            "reviews_reviewrequest"."repository_id" IS NULL
            OR "reviews_reviewrequest"."repository_id" IN (1, 2, 3, 4)
          )
          AND (
            "reviews_reviewrequest_target_people"."user_id" = 1
            OR "reviews_reviewrequest_target_groups"."group_id" IS NULL
            OR "reviews_reviewrequest_target_groups"."group_id" IN (1, 2, 3, 4)
          )
        )
      )
    )
    OR (
      "reviews_review"."public" = FALSE
      AND "reviews_review"."base_reply_to_id" IS NULL
      AND "reviews_reviewrequest"."local_site_id" IS NULL
      AND (
        "reviews_review"."user_id" = 1
        OR (
          (
            "reviews_reviewrequest"."repository_id" IS NULL
            OR "reviews_reviewrequest"."repository_id" IN (1, 2, 3 3, 4)
          )
          AND (
            "reviews_reviewrequest_target_people"."user_id" = 1
            OR "reviews_reviewrequest_target_groups"."group_id" IS NULL
            OR "reviews_reviewrequest_target_groups"."group_id" IN (1, 2, 3, 4)
          )
        )
      )
    )
  )
ORDER BY
  "reviews_review"."timestamp" ASC


Look how expensive that just go with the OR. Way more complicated, and the database is going to take who knows what strategy to check things -- probably a row scan, meaning it searches nearly every single row in the database. Sort of a worst-case scenario. So let's help with that.
Since public=True | public=False really just means "any value for public, we really just want to eliminate that part of the query. And then have one block of ACL checks.
We can do this by allowing public to be None, and just omitting the Q(public=...) if it is None. That gets us everything we need, keeping the simpler, faster (still expensive but soon less so) version of the query.
Database query work tends to be more complex than it appears on the surface, especially since the Django ORM hides a lot of complexity from us. There isn't a great way to test complexity of queries in Django, so it boils down to running the queries like above and seeing what we get (and possibly using SQL like EXPLAIN to figure out what the database would do with it). Happy to go into those tricks offline.

We can just do an assert for str inside the else, to elimate an isinstance check.

The more fixtures we load, the more expensive per-test setup. We should avoid test_site except for tests that specifically need it, and same with test_scmtools.

Two important things here:

Private functions always go after public functions.
This function is very specific to certain tests we have now. There are more coming, where these won't be appropriate, and that can lead to shoe-horning in additional behaviors (same problem our existing fixtures have).

I suggest just passing in AnonymousUser() where we need it, and looking up the users where we need them.

To keep code consistent, let's always use review_request and review as variable names.

assertIn / assertNotIn can be very misleading. It doesn't always tell us the whole story. For instance, we might expect that because an item is or is not in the results that we've gotten the results we expect, but what if we get a review request we don't expect? Or assumed there'd be one in there where there isn't?
Not necessarily an issue for all of these tests, but since we're testing for accessibility, we want to be very explicit. Let's instead do:

self.assertQuerysetEqual(
    Review.objects.accessible(...),
    [list of review request options])


Same applies to all tests.

Looking great! Thanks for bearing with me on the changes :)
Two small nits, and then Ship It!

No blank line after method-level docstrings.

Let's change this to "Added the ..." That'll be more consistent, and reads a bit better.

Ship It!