This does len(diff_comments) queries when we can do this in a single query:

filediffs_pks = {
    comment['filediff_id']
    for comment in diff_comments
}

filediffs = {
    filediff.pk: filediff
    for filediff in FileDiff.objects.filter(
        pk__in=filediff_pks,
        diffset__history__review_request=review_request
    )
}

for comment in diff_comments:
    filediff_id = comment.pop('filediff_id')

    try:
        comment['filediff'] = filediffs[filediff_id]
    except KeyError:
        return INVALID_FORM_DATA, {
            'fields': {
                'diff_comments': [
                    'Invalid filediff ID: %s' % filediff_id,
                ],
            },
        }


This also means do you don't need the except ObjectDoesNotExist branch (since that case is handled in the KeyError branch).

The value should be a list of errors.

This really should use bulk_create to do 2 queries instead of len(diff_comments) + len(general_comments) queries, e.g.

for comment_type, comments in ((Comment, diff_comments),
                               (GeneralComment, general_comments)):
    models = [
        comment_type(review=new_review, **comment)
        for comment in comments
    ]

    comment_type.bulk_create(models)

The value should be a list of errors (though a single error with the missing keys is fine).

Do we want to log this if we are silently ignoring fields?

Description and testing done should probably be wrapped at 72 characters.

Since the data variable you are declaring is only being used once you could do this all inline.

Mind updating this docstring to the modern format?

Can we call this filediff_pks?

Add a blank line here.

Add a blank line here.

This needs a "Returns" section

This is intented too far (should line up with reviewboard on the line above)

This isn't quite right. This method returns a 2-tuple containing an HTTP return code and the API payload.

"JSON" isn't a python type. This method returns a list of the decoded and normalized comments.

Can have a variable for number of general comments too since it's used a few times

Description makes me think it should be a bool but is not, same with body_bottom_rich_text

Ship It!