Adding the ability for bots to make general comments

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

jcannon
ReviewBot
master
10195, 10302
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.

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)
     
     

    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
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. 
      
david
Review request changed

Status: Discarded

Change Summary:

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

Loading...