Should we specify what "more advanced queries" we made possible?

coming -> incoming

key argument pairs -> keyword arguments

This needs a type, and the description should be reworded. We don't need to say "Django's queryset"

Same comments here.

And here.

This can probably just be q = q.filter(review__review_request=review_request). No need for a Q object here.

Thanks for taking this on! It'll be great to have this in.
Many of the comments here apply to all the comment type variations.

Not sure how best to organize this, but maybe "All Comments".
An argument could also be made to just put this in "Reviews".

"ID"

This should have , optional after the type.

This should be *args.

This should be **kwargs.

For multi-line conditionals, you can use parens:

if ('review_request_id' in kwargs and
    kwargs['review_request_id'] is not None):


Though in this case, we should have review_request_id in the function's argument list.

We can omit this and leave it up to the parent method.

Same comments as above regarding the * in the names.

This blank line can be removed.

This should probably be in the parameter list.

These can use q &=.

When working with multiple keyword arguments, we often prefer to do one parameter per line to keep it readable and maintainable.

Can you order these in alphabetical order? That'll help make it obvious where any new query arguments should go.

Since we need a ' in the string, and don't use " anywhere, we can use " for the strings itself and avoid the escape.
Same below.

No blank line here.

We probably don't need this anymore, if we're reusing the other mimetype constants.

Private methods should always go after all the public methods in the class.
We also try to add docstrings for helper methods in unit tests. We didn't always, so some older test modules lack this.

This can be one line.
Some older tests didn't do this, but we're moving away from that.

Assigning unused variables to _ is a convention many use, but it's harmful in our codebase because _ is (also by convention) often mapped to gettext.
Fun fact: A lot of online Python guides claim _ is a feature of the language for unused variables, but it's no different than any other variable name. Python doesn't treat it specially.

Just some notes on docstrings. Code looks good!

Instead of "API Class", let's say "API resource"

I don't think "made on a review" makes sense here because we're not dealing with reviews.

Instead of "API Class", let's say "API resource"

Nix "made on a review"

Instead of "API Class", let's say "API resource"

Nix "made on a review"

"different comments" -> "diff comments"

Should be RootDiffCommentResource

Should be RootFileAttachmentCommentResource

RootGeneralCommentResource

A few things that'll be important to fix regarding querying here, to avoid data leaks in results.

Along the lines of what I said in the Reviews API and CommentManager changes, we should ensure that all the unit tests have data that should not match based on access or filtering.

Let's have this explicitly check against None (differentiating from 0, ensuring users can't bypass this with some provided parameter).

Same as above, let's compare explicitly against None.

Same as above, let's explicitly check against None.

If we do __id=, we may force a JOIN to the reviews_review table. We can leave that off and just do review=, which will do the right thing and compare against review_id.

This will also force a JOIN, and we want to avoid that.
Except we can't just hard-code a comparison with the ID. If using a Local Site, then the PK of the object is actually not the ID. The local_id field is. But only if on a Local Site. This gives us LocalSite-bound sequential IDs.
So what we need to do is:

if local_site is None:
    q &= Q(review__review_request=review_request_id)
else:
    q &= (Q(review__review_request__local_id=review_request_id) &
          Q(review__review_request__local_site=local_site))


We'll need to ensure that unit tests cover ID vs. Local ID testing, and that no data leaks between the two.

Let's pre-fetch the user (just the user's ID, actually, using .values_list('pk', flat=True)). If we get 0 results, we can explicitly return self.model.objects.none(), since we know nothing will end up matching.

This can just be file_attachment=.

Same comments as above regarding the JOINs, Local Site handling, and pre-fetching/checking the user ID.

We can probably just say:

These are listed in :py:meth:`get_list`.


Same elsewhere.

Same comments as above regarding the JOINs, Local Site handling, and pre-fetching/checking the user ID.

While here, can you update this to include a blank line between class-level docstrings and members? Good opportunity to fix this, since the lines are being modified here.
There are some other spots where this can be done as well. We only have the trailing blank line for module-level and class-level docstrings (weird inconsistency, but it's standard in Python).

Small nit: Rather than frm and to, let's go ahead and just make these something like timestamp_from and timestamp_to.
Same if we do this anywhere else.

Ship It!