Add general comments ability to Review Bot

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

Information

ReviewBot
master
48ba77c...

Reviewers

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.

Description From Last Updated

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

shovenshoven

This does len(diff_comments) queries when we can do this in a single query: filediffs_pks = { comment['filediff_id'] for comment in …

brenniebrennie

The value should be a list of errors.

brenniebrennie

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 …

brenniebrennie

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

brenniebrennie

F401 'reviewboard.reviews.models.Comment' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.models.GeneralComment' imported but unused

reviewbotreviewbot

F821 undefined name 'rdb'

reviewbotreviewbot

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

shovenshoven

Mind updating this docstring to the modern format?

daviddavid

Can we call this filediff_pks?

daviddavid

Add a blank line here.

daviddavid

Add a blank line here.

daviddavid

This needs a "Returns" section

daviddavid

E501 line too long (80 > 79 characters)

reviewbotreviewbot

F841 local variable 'filediffs' is assigned to but never used

reviewbotreviewbot

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

daviddavid

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

daviddavid

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

daviddavid
brennie
  1. 
      
  2. extension/reviewbotext/resources.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    The value should be a list of errors.

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

    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)
     
     
     
    Show all issues

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
shoven
  1. 
      
  2. Show all issues

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

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

    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)
     
     
    Show all issues

    Mind updating this docstring to the modern format?

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

    Can we call this filediff_pks?

  4. extension/reviewbotext/resources.py (Diff revision 4)
     
     
     
    Show all issues

    Add a blank line here.

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

    Add a blank line here.

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

    This needs a "Returns" section

  7. 
      
ammar
Review request changed
Commit:
e8c673be8c40b39f61e260fcd3f4bd8c1e08d2b7
c34b79ca3200356ca69c38acb77bbe0adf443f1e

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
david
  1. 
      
  2. extension/reviewbotext/resources.py (Diff revision 6)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    "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. 
      
ammar
david
  1. Ship It!
  2. 
      
ammar
Review request changed
Status:
Completed
Change Summary:
Pushed to master (b18baba)