Summary

Adding the ability for bots to make general comments

Review Request #10095 — Created July 19, 2018 and discarded Jan. 30, 2019, 2:50 p.m.

Information

Owner

jcannon

Repository

ReviewBot

Branch

master

Bugs

Depends On

Blocks

10195, 10302

Reviewers

Groups

reviewbot

People

Description

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)

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

Issues

Description	From	Last Updated
E302 expected 2 blank lines, found 1	reviewbot	July 19, 2018, 1:22 p.m.
E501 line too long (81 > 79 characters)	reviewbot	July 19, 2018, 1:22 p.m.
E722 do not use bare except'	reviewbot	July 19, 2018, 1:22 p.m.
This also needs to clear out self.comments	david	July 27, 2018, 11:04 a.m.
Can we call this InvalidFormDataError?	david	July 27, 2018, 11:04 a.m.
Docstrings should be a single-line summary followed by zero or more paragraphs.	david	July 27, 2018, 11:04 a.m.
Blank line between the docstring and the methods.	david	July 27, 2018, 11:04 a.m.
This needs a docstring.	david	July 27, 2018, 11:04 a.m.
Shouldn't this be return INVALID_FORM_DATA, e.data?	david	July 27, 2018, 11:04 a.m.
Formatting here can be improved (or at least made more consistent with other code): raise InvalidFormData({ 'fields': { comment_type: 'Malformed …	david	July 27, 2018, 11:04 a.m.
Same here: raise InvalidFormData({ 'fields': { comment_type: ('Element missing keys "%s"' % missing_keys), }, })	david	July 27, 2018, 11:04 a.m.
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	Sept. 10, 2018, 1:58 p.m.
It would be slightly cleaner to have these in the try/catch above where we assign comment['filediff']	david	Sept. 10, 2018, 1:58 p.m.
This needs a docstring	david	Sept. 10, 2018, 1:58 p.m.
Add a trailing comma	david	Sept. 10, 2018, 1:58 p.m.
Can we put parens around the value here? comment_type: ('Element missing keys "%s"' % missing_keys),	david	Sept. 10, 2018, 1:58 p.m.
Add a trailing comma	david	Sept. 10, 2018, 1:58 p.m.

flake8 failed.

JSHint passed.

flake8

extension/reviewbotext/resources.py (Diff revision 1)
Show all issues
```
E302 expected 2 blank lines, found 1
```
extension/reviewbotext/resources.py (Diff revision 1)
Show all issues
```
E501 line too long (81 > 79 characters)
```
extension/reviewbotext/resources.py (Diff revision 1)
Show all issues
```
E722 do not use bare except'
```
1. jcannon July 19, 2018, 1:21 p.m.
  Lol I copied this from line 105! (rhs of diff)

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

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

Change Summary:

Appeasing the 8 gods of Flake.

Commit:

-	73fc50f3d9a2b0839bc948d6d2cdb23cbc496d2c
+	788ebd0731f230fbba2e4e1c1c4e1f53eebf7a52

Diff:

Revision 2 (+107 -37)

Show changes

	bot/reviewbot/tasks.py
	bot/reviewbot/processing/review.py
	extension/reviewbotext/resources.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
bump
```

bot/reviewbot/processing/review.py (Diff revision 2)
Show all issues
```
This also needs to clear out self.comments
```
extension/reviewbotext/resources.py (Diff revision 2)
Show all issues
```
Can we call this InvalidFormDataError?
```

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

Show all issues

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

extension/reviewbotext/resources.py (Diff revision 2)
Show all issues
```
Blank line between the docstring and the methods.
```
extension/reviewbotext/resources.py (Diff revision 2)
Show all issues
```
This needs a docstring.
```
extension/reviewbotext/resources.py (Diff revision 2)
Show all issues
```
Shouldn't this be return INVALID_FORM_DATA, e.data?
```

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.',
    },
})

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

Show all issues

Same here:

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

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

	bot/reviewbot/tasks.py
	bot/reviewbot/processing/review.py
	extension/reviewbotext/resources.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
bump
```

Fix it!

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.

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']

extension/reviewbotext/resources.py (Diff revision 3)
Show all issues
```
This needs a docstring
```
extension/reviewbotext/resources.py (Diff revision 3)
Show all issues
```
Add a trailing comma
```

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),

extension/reviewbotext/resources.py (Diff revision 3)
Show all issues
```
Add a trailing comma
```

Status: Discarded

Change Summary:

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