Summary:

Fix empty review can be published bug.

Review Request #11185 — Created Sept. 19, 2020 and updated March 21, 2021, 11:51 p.m.

Information
Owner:	keanweng
Repository:	Review Board
Branch:	release-4.0.x
Bugs:	4756
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

When publishing a review, an empty review can be published and it will 

show as a blank review.
The publish() function of review has been updated to check for empty 

text in a review's Header, Comment and Footer. When a review with empty 

Header, Comment and Footer is being published, the function throws a 

PublishError. A review with only Ship It and no text is considered 

to be valid and will not raise an error. Updated #982 Testing sending 

e-mails and filtering out users not on a local site to use a non-empty 

Review as we now check for empty reviews.

Testing Done

Ran runtests.py, failed #788 Testing GitHub.authorize. Tested on
browser to ensure the fix works on Web API.

Commits

Summary
	Fix empty review can be published bug.

Screenshots

Issues

Description	From	Last Updated
We'll want to see two Python-side unit tests that checks the "empty review" state, one with and one without validate_fields …	chipx86	Sept. 24, 2020, 1:56 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 19, 2020, 3:30 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 19, 2020, 3:30 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 19, 2020, 3:30 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 19, 2020, 3:30 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 19, 2020, 3:30 p.m.
Looks like this was included in this change, unintentionally. You'll want to make sure your commit only has your desired …	chipx86	Sept. 24, 2020, 1:56 a.m.
Looks like you're setting these because we otherwise have a blank review which can't be published. This code is pretty …	chipx86	Sept. 24, 2020, 2:22 a.m.
Can you put PublishError before RevokeShipItError? We like to keep things in alphabetical order.	chipx86	Sept. 24, 2020, 1:56 a.m.
Since you're introducing new arguments, you'll need to update this to reflect our modern doc standards, like "Publish" instead of …	chipx86	Sept. 24, 2020, 2:13 a.m.
These can be combined into one conditional.	chipx86	Sept. 24, 2020, 1:10 p.m.
Ah interesting! Looks like you caught an issue we didn't catch before. That usually means "time to write a unit …	chipx86	Sept. 26, 2020, 1:17 a.m.
F841 local variable 'review' is assigned to but never used	reviewbot	Sept. 26, 2020, 1:17 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Sept. 26, 2020, 1:17 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Sept. 26, 2020, 1:17 a.m.
W293 blank line contains whitespace	reviewbot	Sept. 26, 2020, 1:17 a.m.
W293 blank line contains whitespace	reviewbot	Sept. 26, 2020, 1:17 a.m.
W293 blank line contains whitespace	reviewbot	Sept. 26, 2020, 1:17 a.m.
W292 no newline at end of file	reviewbot	Sept. 26, 2020, 1:17 a.m.
F841 local variable 'review' is assigned to but never used	reviewbot	Sept. 26, 2020, 1:17 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Sept. 26, 2020, 1:17 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Sept. 26, 2020, 1:17 a.m.
W293 blank line contains whitespace	reviewbot	Sept. 26, 2020, 1:17 a.m.
W293 blank line contains whitespace	reviewbot	Sept. 26, 2020, 1:17 a.m.
W293 blank line contains whitespace	reviewbot	Sept. 26, 2020, 1:17 a.m.
W292 no newline at end of file	reviewbot	Sept. 26, 2020, 1:17 a.m.
This isn't part of your change, and should be removed. We have a better fix for this in the code …	david	Dec. 3, 2020, 4:10 p.m.
There's an extra blank line here.	david	Dec. 3, 2020, 4:10 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/review.py (Diff revision 1)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
Show all issues
```
E128 continuation line under-indented for visual indent
```

Commits:

	Summary
-	Hotfix for GitHub Auth token when adding repo.
-	Add validate_fields to check for empty reviews when publishing.
-	Set review.model.public to false when PublishError.
-	Populate review text as it cannot be empty.
-	Modify validate to check for comments, emails and Ship It.
+	Hotfix for GitHub Auth token when adding repo.
+	Add validate_fields to check for empty reviews when publishing.
+	Set review.model.public to false when PublishError.
+	Populate review text as it cannot be empty.
+	Modify validate to check for comments, emails and Ship It.
+	Update indent for Review Bot indent issue.

Diff:

Revision 2 (+48 -20)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/reviews/models/review.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

We'll want to see two Python-side unit tests that checks the "empty review" state, one with and one without validate_fields set.

reviewboard/hostingsvcs/github.py (Diff revision 2)

Show all issues

Looks like this was included in this change, unintentionally. You'll want to make sure your commit only has your desired changes, and post only that commit for review (rbt post HEAD, usually).

reviewboard/notifications/tests/test_email_sending.py (Diff revision 2)

Show all issues

Looks like you're setting these because we otherwise have a blank review which can't be published.

This code is pretty old, and fortunately we have some modern test object creaation helpers that provide standard default data. Would you be up for switching to those instead? It works like:

self.create_review(review_request=review_request,
                   user=site_user4,
                   publish=True)

Note that publish=True: You can ditch the publish() calls with this.

reviewboard/reviews/models/review.py (Diff revision 2)

Show all issues

Can you put PublishError before RevokeShipItError? We like to keep things in alphabetical order.

reviewboard/reviews/models/review.py (Diff revision 2)

Show all issues

Since you're introducing new arguments, you'll need to update this to reflect our modern doc standards, like "Publish" instead of "Publishes," and an Args and Raises.

See our doc standards.

reviewboard/reviews/models/review.py (Diff revision 2)

Show all issues

These can be combined into one conditional.

reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js (Diff revision 2)

Show all issues

Ah interesting! Looks like you caught an issue we didn't catch before.

That usually means "time to write a unit test!" There's a test file for this in the tree that you'll want to ensure is asserting the state of this flag on error.

Change Summary:

Fix 6 issues from Revision 2.

Commits:

	Summary
-		Hotfix for GitHub Auth token when adding repo.
-		Add validate_fields to check for empty reviews when publishing.
-		Set review.model.public to false when PublishError.
-		Populate review text as it cannot be empty.
-		Modify validate to check for comments, emails and Ship It.
-		Update indent for Review Bot indent issue.
+		Fix empty review can be published bug.

Diff:

Revision 3 (+158 -24)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/tests/test_review.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/tests/test_email_sending.py (Diff revision 3)
Show all issues
```
F841 local variable 'review' is assigned to but never used
```
reviewboard/reviews/models/review.py (Diff revision 3)
Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 3)
Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
Show all issues
```
W292 no newline at end of file
```

Change Summary:

Jasmine trigger error change.

Commits:

	Summary
-		Fix empty review can be published bug.
+		Fix empty review can be published bug.
+		Jasmine trigger error on calling publish.

Diff:

Revision 4 (+202 -24)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/tests/test_review.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.es6.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/tests/test_email_sending.py (Diff revision 4)
Show all issues
```
F841 local variable 'review' is assigned to but never used
```
reviewboard/reviews/models/review.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 4)
Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
Show all issues
```
W292 no newline at end of file
```

Change Summary:

Fix flake8 issues.

Commits:

	Summary
-		Fix empty review can be published bug.
-		Jasmine trigger error on calling publish.
+		Fix empty review can be published bug.

Diff:

Revision 5 (+202 -28)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/tests/test_review.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/github.py (Diff revision 5)

Show all issues

This isn't part of your change, and should be removed. We have a better fix for this in the code now.

reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.es6.js (Diff revision 5)
Show all issues
```
There's an extra blank line here.
```

Change Summary:

Address feedback.

Commits:

	Summary
-		Fix empty review can be published bug.
+		Fix empty review can be published bug.

Diff:

Revision 6 (+198 -26)

Show changes

	reviewboard/notifications/tests/test_email_sending.py
	reviewboard/reviews/models/review.py
	reviewboard/reviews/tests/test_review.py
	reviewboard/static/rb/js/resources/models/draftReviewModel.es6.js
	reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.