Adding the ability for bots to make general comments

Review Request #10095 — Created July 19, 2018 and updated

jcannon
ReviewBot
master
10302, 10195
a48b619...
reviewbot

Adding the ability for bots to make general comments.

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.

  • 6
  • 0
  • 11
  • 0
  • 17
Description From Last Updated
Can we move this code into the try/catch above? That way we only have one place that's returning INVALID_FORM_DATA. We ... david david
It would be slightly cleaner to have these in the try/catch above where we assign comment['filediff'] david david
This needs a docstring david david
Add a trailing comma david david
Can we put parens around the value here? comment_type: ('Element missing keys "%s"' % missing_keys), david david
Add a trailing comma david david
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

jcannon
  1. 
      
  2. extension/reviewbotext/resources.py (Diff revision 1)
     
     
     
     
     
     

    This wasn't being done before, and invalid JSON would result in APIError 500.

  3. 
      
jcannon
jcannon
  1. bump

  2. 
      
david
  1. 
      
  2. bot/reviewbot/processing/review.py (Diff revision 2)
     
     

    This also needs to clear out self.comments

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

    Can we call this InvalidFormDataError?

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

    Docstrings should be a single-line summary followed by zero or more paragraphs.

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

    Blank line between the docstring and the methods.

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

    This needs a docstring.

  7. extension/reviewbotext/resources.py (Diff revision 2)
     
     

    Shouldn't this be return INVALID_FORM_DATA, e.data?

  8. extension/reviewbotext/resources.py (Diff revision 2)
     
     
     

    Formatting here can be improved (or at least made more consistent with other code):

    raise InvalidFormData({
        'fields': {
            comment_type: 'Malformed JSON.',
        },
    })
    
  9. extension/reviewbotext/resources.py (Diff revision 2)
     
     
     
     
     

    Same here:

    raise InvalidFormData({
        'fields': {
            comment_type: ('Element missing keys "%s"'
                           % missing_keys),
        },
    })
    
  10. 
      
jcannon
Review request changed

Change Summary:

Integrated David's feedback + more testing

Testing Done:

   

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 -> only 30 got posted.
~   Likewise posted an automated review with 15 general comments and 17 diff comments -> only 30 (total) got posted, 15 general and 15 diff.

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

Commit:

-788ebd0731f230fbba2e4e1c1c4e1f53eebf7a52
+a48b61944225df16cf604036c9592150ba7f420c

Diff:

Revision 3 (+118 -37)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
jcannon
  1. bump

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

    Can we move this code into the try/catch above? That way we only have one place that's returning INVALID_FORM_DATA.

    We could also do all of the normalization before creating the Review object, which would eliminate the need for the TODO comment since we wouldn't have created anything in the db.

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

    It would be slightly cleaner to have these in the try/catch above where we assign comment['filediff']

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

    This needs a docstring

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

    Add a trailing comma

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

    Can we put parens around the value here?

    comment_type: ('Element missing keys "%s"'
                   % missing_keys),
    
  7. extension/reviewbotext/resources.py (Diff revision 3)
     
     

    Add a trailing comma

  8. 
      
Loading...