Add general comments ability to Review Bot

Review Request #10335 — Created Nov. 27, 2018 and updated

ammar
ReviewBot
master
d27a1b1...
reviewbot, students

Continuation of work from here:
https://reviews.reviewboard.org/r/10095/

Similar to how a tool can post diff comments on a file, they
can now also post general comments on the review. This also
paves the way for tools to post file and screenshot comments
(but does not add this functionality).

Was able to post an automated review with/without general
comments, opening issues on some comments and not on others.
Also posted an automated review with 35 general comments and
1 diff comment -> only 30 got posted, all of which were the
general comments.
Likewise posted an automated review with 15 general comments
and 17 diff comments -> only 30 (total) got posted, 15 general
and 15 diff.
Additionally, tried using an updated Review Board server with
a worker without the changes -> No errors, everything works
as expected.

  • 3
  • 0
  • 16
  • 0
  • 19
Description From Last Updated
This is intented too far (should line up with reviewboard on the line above) david david
This isn't quite right. This method returns a 2-tuple containing an HTTP return code and the API payload. david david
"JSON" isn't a python type. This method returns a list of the decoded and normalized comments. david david
brennie
  1. 
      
  2. extension/reviewbotext/resources.py (Diff revision 1)
     
     
     
     
     

    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).

  3. extension/reviewbotext/resources.py (Diff revision 1)
     
     

    The value should be a list of errors.

  4. extension/reviewbotext/resources.py (Diff revision 1)
     
     
     
     
     
     

    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)
    
    1. Can not used bulk create here because of the way it handles PKs.

  5. extension/reviewbotext/resources.py (Diff revision 1)
     
     
     

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

  6. extension/reviewbotext/resources.py (Diff revision 1)
     
     
     
     

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

  7. 
      
ammar
Review request changed

Commit:

-6903ba9dc8cb8554f7f49141dc5001de3506a0e5
+a006cb12855fbaad092a4e7d1865766f9ad7ef20

Diff:

Revision 2 (+155 -45)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
shoven
  1. 
      
  2. Description and testing done should probably be wrapped at 72 characters.

  3. bot/reviewbot/processing/review.py (Diff revision 3)
     
     

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

  4. 
      
ammar
david
  1. 
      
  2. extension/reviewbotext/resources.py (Diff revision 4)
     
     

    Mind updating this docstring to the modern format?

  3. extension/reviewbotext/resources.py (Diff revision 4)
     
     

    Can we call this filediff_pks?

  4. extension/reviewbotext/resources.py (Diff revision 4)
     
     
     

    Add a blank line here.

  5. extension/reviewbotext/resources.py (Diff revision 4)
     
     
     

    Add a blank line here.

  6. extension/reviewbotext/resources.py (Diff revision 4)
     
     

    This needs a "Returns" section

  7. 
      
ammar
Review request changed

Commit:

-e8c673be8c40b39f61e260fcd3f4bd8c1e08d2b7
+c34b79ca3200356ca69c38acb77bbe0adf443f1e

Diff:

Revision 5 (+197 -45)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
Review request changed

Commit:

-c34b79ca3200356ca69c38acb77bbe0adf443f1e
+d27a1b1d3cfd2c1643eccdcec915182c62739169

Diff:

Revision 6 (+197 -45)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
david
  1. 
      
  2. extension/reviewbotext/resources.py (Diff revision 6)
     
     

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

  3. extension/reviewbotext/resources.py (Diff revision 6)
     
     

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

  4. extension/reviewbotext/resources.py (Diff revision 6)
     
     

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

  5. 
      
ilaw
  1. 
      
  2. bot/reviewbot/processing/review.py (Diff revision 6)
     
     

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

  3. extension/reviewbotext/resources.py (Diff revision 6)
     
     

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

  4. 
      
Loading...