Add general comments ability to Review Bot
Review Request #10335 — Created Nov. 27, 2018 and submitted
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. |
|
|
This does len(diff_comments) queries when we can do this in a single query: filediffs_pks = { comment['filediff_id'] for comment in … |
|
|
The value should be a list of errors. |
|
|
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 … |
|
|
The value should be a list of errors (though a single error with the missing keys is fine). |
|
|
F401 'reviewboard.reviews.models.Comment' imported but unused |
![]() |
|
F401 'reviewboard.reviews.models.GeneralComment' imported but unused |
![]() |
|
F821 undefined name 'rdb' |
![]() |
|
Since the data variable you are declaring is only being used once you could do this all inline. |
|
|
Mind updating this docstring to the modern format? |
|
|
Can we call this filediff_pks? |
|
|
Add a blank line here. |
|
|
Add a blank line here. |
|
|
This needs a "Returns" section |
|
|
E501 line too long (80 > 79 characters) |
![]() |
|
F841 local variable 'filediffs' is assigned to but never used |
![]() |
|
This is intented too far (should line up with reviewboard on the line above) |
|
|
This isn't quite right. This method returns a 2-tuple containing an HTTP return code and the API payload. |
|
|
"JSON" isn't a python type. This method returns a list of the decoded and normalized comments. |
|
-
-
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 theKeyError
branch). -
-
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)
-
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).
-
extension/reviewbotext/resources.py (Diff revision 1) Do we want to log this if we are silently ignoring fields?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+155 -45) |
Checks run (1 failed, 1 succeeded)
flake8
-
extension/reviewbotext/resources.py (Diff revision 2) F401 'reviewboard.reviews.models.Comment' imported but unused
-
extension/reviewbotext/resources.py (Diff revision 2) F401 'reviewboard.reviews.models.GeneralComment' imported but unused
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+149 -44) |
Checks run (2 succeeded)
-
-
-
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.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+148 -44) |
Checks run (2 succeeded)
-
-
extension/reviewbotext/resources.py (Diff revision 4) Mind updating this docstring to the modern format?
-
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+197 -45) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
extension/reviewbotext/resources.py (Diff revision 5) F841 local variable 'filediffs' is assigned to but never used
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+197 -45) |
Checks run (2 succeeded)
-
-
extension/reviewbotext/resources.py (Diff revision 6) This is intented too far (should line up with
reviewboard
on the line above) -
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.
-
extension/reviewbotext/resources.py (Diff revision 6) "JSON" isn't a python type. This method returns a
list
of the decoded and normalized comments.
-
-
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
-
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
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+197 -45) |