Adding the ability for bots to make general comments

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

Information

ReviewBot
master

Reviewers

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.

Description From Last Updated

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E722 do not use bare except'

reviewbotreviewbot

This also needs to clear out self.comments

daviddavid

Can we call this InvalidFormDataError?

daviddavid

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

daviddavid

Blank line between the docstring and the methods.

daviddavid

This needs a docstring.

daviddavid

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

daviddavid

Formatting here can be improved (or at least made more consistent with other code): raise InvalidFormData({ 'fields': { comment_type: 'Malformed …

daviddavid

Same here: raise InvalidFormData({ 'fields': { comment_type: ('Element missing keys "%s"' % missing_keys), }, })

daviddavid

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

daviddavid

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

daviddavid

This needs a docstring

daviddavid

Add a trailing comma

daviddavid

Can we put parens around the value here? comment_type: ('Element missing keys "%s"' % missing_keys),

daviddavid

Add a trailing comma

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

    This also needs to clear out self.comments

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

    Can we call this InvalidFormDataError?

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

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

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

    Blank line between the docstring and the methods.

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

    This needs a docstring.

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

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

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

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

    Same here:

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

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

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

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

    This needs a docstring

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

    Add a trailing comma

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

    Can we put parens around the value here?

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

    Add a trailing comma

  8. 
      
david
Review request changed
Status:
Discarded
Change Summary:

Discarded in favor of https://reviews.reviewboard.org/r/10335/