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	ID
Fix empty review can be published bug. 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.	70a42fa7cb8c3452ee7c300742c4a8ca5f8539aa

Summary

Fix empty review can be published bug.

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.

70a42fa7cb8c3452ee7c300742c4a8ca5f8539aa

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)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```

Commits:

	Summary	ID
	Hotfix for GitHub Auth token when adding repo.	741de0992ca7ec66cfd14b365880227a74ca42a7
	Add validate_fields to check for empty reviews when publishing.	88774decfcf087b44e0f446af3b547fd87c7c151
	Set review.model.public to false when PublishError.	f20e42989c3546f053cb92522aed1584b29eac60
	Populate review text as it cannot be empty.	4cf9ad98d084e49e6459162c7ab20eb977f5011d
	Modify validate to check for comments, emails and Ship It.	170f5932cc151504b22d34e79e8d85788e3039c6
	Hotfix for GitHub Auth token when adding repo.	ee3f1f85814ea025b3a6dff616eacab7b6dc5c3f
	Add validate_fields to check for empty reviews when publishing.	88eb60a7bbb0e8bf98bf3c12da079154a53bb56f
	Set review.model.public to false when PublishError.	90fb63ac2d03a030874c8020a460783975abc827
	Populate review text as it cannot be empty.	ea074c9d6f6d69b44cdcdab535b012b87bb08aa1
	Modify validate to check for comments, emails and Ship It.	4a12eb1e06d440ba1ba8bab71436871f1904a4eb
	Update indent for Review Bot indent issue.	47e132043bf149a56df183d1af490e0a5532ef78

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.

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. Show all issues

These can be combined into one conditional.

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

The issue has been resolved. 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	ID
	Hotfix for GitHub Auth token when adding repo.	ee3f1f85814ea025b3a6dff616eacab7b6dc5c3f
	Add validate_fields to check for empty reviews when publishing.	88eb60a7bbb0e8bf98bf3c12da079154a53bb56f
	Set review.model.public to false when PublishError.	90fb63ac2d03a030874c8020a460783975abc827
	Populate review text as it cannot be empty.	ea074c9d6f6d69b44cdcdab535b012b87bb08aa1
	Modify validate to check for comments, emails and Ship It.	4a12eb1e06d440ba1ba8bab71436871f1904a4eb
	Update indent for Review Bot indent issue.	47e132043bf149a56df183d1af490e0a5532ef78
	Fix empty review can be published bug. 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.	95f5466dc4edf1d491d61ad91b0819b62b42796f

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)
The issue has been resolved. Show all issues
```
F841 local variable 'review' is assigned to but never used
```
reviewboard/reviews/models/review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Change Summary:

Jasmine trigger error change.

Commits:

	Summary	ID
	Fix empty review can be published bug. 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.	95f5466dc4edf1d491d61ad91b0819b62b42796f
	Fix empty review can be published bug. 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.	95f5466dc4edf1d491d61ad91b0819b62b42796f
	Jasmine trigger error on calling publish.	2d3b5ca4a165fe0646f366a182e93d9d100d12e1

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)
The issue has been resolved. Show all issues
```
F841 local variable 'review' is assigned to but never used
```
reviewboard/reviews/models/review.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/models/review.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 4)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Change Summary:

Fix flake8 issues.

Commits:

	Summary	ID
	Fix empty review can be published bug. 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.	95f5466dc4edf1d491d61ad91b0819b62b42796f
	Jasmine trigger error on calling publish.	2d3b5ca4a165fe0646f366a182e93d9d100d12e1
	Fix empty review can be published bug. 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.	158738ba093ee9380d001634020de30675d9ecc6

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)

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
There's an extra blank line here.
```

Change Summary:

Address feedback.

Commits:

	Summary	ID
	Fix empty review can be published bug. 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.	158738ba093ee9380d001634020de30675d9ecc6
	Fix empty review can be published bug. 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.	70a42fa7cb8c3452ee7c300742c4a8ca5f8539aa

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.