Summary

Don't allow publishing empty review

Review Request #10834 — Created Jan. 18, 2020 and updated Jan. 20, 2020, 11:19 p.m.

Information

Owner

LittleGrey

Repository

Review Board

Branch

release-3.0.x

Bugs

4756

Depends On

Reviewers

Groups

reviewboard

People

Description

When making an empty review (i.e., no comment, body top, body bottom, 

or Ship It) the review is recognized as a valid review and can be 

published. The problem is that when a review is detected as empty, 

the code doesn't raise a publish error so the empty review being 

published is blocked. 
I added a function to detect empty reviews. Once an empty review is 

trying to be published, an error is raised and the public state rolls 

back to previous state so it doesn't mess up the state when an error 

occurs (public state is a bool - when it's true, the review 

is considered as published).

Testing Done

Create a new review -> publish -> publish failed.

Create a new review -> give a comment -> delete the comment -> publish -> publish failed.

Create a new review -> give a header -> delete the header -> publish -> publish failed.

Create a new review -> give a footer -> delete the footer -> publish -> publish failed.

Create a new review -> check "Ship-It" -> publish -> publish success.

Commits

Summary	ID
Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70
modify multi-line str to pass flake8	ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2
Make stylistic changes as suggested by review	db79445479b1041517604a7103ad206b461c8518
Add more unit tests for is_empty_review()	d80076115ce6a2811ec07cb3d3663d961b29a086

Issues

Description	From	Last Updated
Just realized I forgot to add tests! Will do that!	LittleGrey	Jan. 19, 2020, 9:39 a.m.
We like to basically tell a story about the change in the Description field, explaining the problem we had and …	chipx86	Jan. 20, 2020, 7:53 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 19, 2020, 9:35 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Jan. 19, 2020, 9:38 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Jan. 19, 2020, 9:38 a.m.
When dealing with a multi-line string, it's often nicer to start the string on the next indentation level, so that …	chipx86	Jan. 20, 2020, 6:23 p.m.
I think it's best we keep the terminology as "empty," instead of "blank." Same with other references in this docstring.	chipx86	Jan. 20, 2020, 6:41 p.m.
Just to be slightly more in line with our terminology, this should use "Ship It".	chipx86	Jan. 20, 2020, 6:47 p.m.
Slight tweak to the naming, just to be consistent with how we usually name thewse. Can you make this test_publish_with_empty_review?	chipx86	Jan. 20, 2020, 6:49 p.m.
To be sure that we don't have any bad behavior, there should be several versions of this function: With an …	chipx86	Jan. 20, 2020, 9:36 p.m.
This can just be passed inline when calling options.error.	chipx86	Jan. 20, 2020, 7:17 p.m.
You can use toBeFalse() instead, here and below.	chipx86	Jan. 19, 2020, 9:58 a.m.

flake8 passed.

JSHint passed.

Show all issues

Just realized I forgot to add tests! Will do that!

Change Summary:

Added unit tests

Commits:

	Summary	ID
	Don't allow publishing empty review	6d65c4578d6e219dc4da5e9109133f4450376dec
	Delete extra spaces.	6a22f3dd992478d5b6d9647b1837a1698a3c47ce
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f

Diff:

Revision 2 (+160 -12)

Show changes

	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.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review.py (Diff revision 2)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/reviews/tests/test_review.py (Diff revision 2)
Show all issues
```
E127 continuation line over-indented for visual indent
```

Change Summary:

remove extra spaces (again)

Commits:

	Summary	ID
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70

Diff:

Revision 3 (+161 -13)

Show changes

	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.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review.py (Diff revision 3)
Show all issues
```
E127 continuation line over-indented for visual indent
```

Commits:

	Summary	ID
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70
	modify multi-line str to pass flake8	ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2

Diff:

Revision 4 (+163 -15)

Show changes

	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.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

This change looks great! There's a few stylistic things I suggest, and some unit tests I'd like to see, and the Description needs work, but all the core logic? Perfect.

Show all issues

We like to basically tell a story about the change in the Description field, explaining the problem we had and what the new behavior is with this change.

You can find examples anywhere in the tree for this, but I encourage reading through the guide on Writing Good Change Descriptions.

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

Show all issues

When dealing with a multi-line string, it's often nicer to start the string on the next indentation level, so that you have more room to work with (this makes it easier to update down the road, too):

raise PublishError(ugettext(
    "This review is empty. It cannot be published until it's "
    "filled out."))

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

Show all issues

I think it's best we keep the terminology as "empty," instead of "blank." Same with other references in this docstring.

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

Show all issues

Just to be slightly more in line with our terminology, this should use "Ship It".

reviewboard/reviews/tests/test_review.py (Diff revision 4)

Show all issues

Slight tweak to the naming, just to be consistent with how we usually name thewse. Can you make this test_publish_with_empty_review?

reviewboard/reviews/tests/test_review.py (Diff revision 4)

Show all issues

To be sure that we don't have any bad behavior, there should be several versions of this function:

With an empty review
With a Ship It
With body_top set
With body_bottom set
With a general comment
With a diff comment
With a file attachment comment
With a screenshot comment

This will ensure that we don't end up regressing as logic changes (say, we add support for new things down the road that are make a review non-empty -- some custom state set by an extension, or a new comment type, for example).

reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.js (Diff revision 4)
Show all issues
```
This can just be passed inline when calling options.error.
```

reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.js (Diff revision 4)

Show all issues

You can use toBeFalse() instead, here and below.

LittleGrey Jan. 20, 2020, 8 p.m.

Thank you for your review. I do think toBeFalsy() or toBeTruthy() is clearer, but I noticed that toBe(false) is widely used in the legacy test-js files. For example, line 60 in reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.js. Should I keep it consistent with the legacy style?

hxqlin Jan. 20, 2020, 9:57 p.m.

Might just a typo but wanted to point out that toBeFalsy() and toBeTruthy() are not the same as toBeFalse() and toBeTrue()! toBeFalsy() would return true with falsy values like 0 and null, whereas toBeFalse() would not. Another thing about using toBeFalse() and toBeTrue() is that they're not available with the current version of Jasmine anyway, which is probably related to why toBe(false) and toBe(true) are being used. Christian made the same comment on my review request: https://reviews.reviewboard.org/r/10820/#comment49842

LittleGrey Jan. 20, 2020, 11:19 p.m.

Yes I also found toBeFalse() is not available in Jasmine. I didn't know toBeFalsy() is different. Thank you for letting me know of this!

Description:

~		Bug: empty reviews can be published.
	~	When making an empty review, the review is recognized as a valid review and can be published. The problem is that when a review is detected as empty, the code doesn't raise a publish error so the empty review being published is blocked. I added a function to detect empty reviews. Once an empty review is trying to be published, an error is raised and the "public" state rolls back to previous state.
-		My change: block empty reviews being published.

Commits:

	Summary	ID
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70
	modify multi-line str to pass flake8	ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70
	modify multi-line str to pass flake8	ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2
	Make stylistic changes as suggested by review	db79445479b1041517604a7103ad206b461c8518

Diff:

Revision 5 (+169 -27)

Show changes

	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.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		When making an empty review, the review is recognized as a valid review and can be published. The problem is that when a review is detected as empty, the code doesn't raise a publish error so the empty review being published is blocked. I added a function to detect empty reviews. Once an empty review is trying to be published, an error is raised and the "public" state rolls back to previous state.
	~	When making an empty review, the review is recognized as a valid review
	+	and can be published. The problem is that when a review is detected as
	+	empty, the code doesn't raise a publish error so the empty review being
	+	published is blocked. I added a function to detect empty reviews. Once
	+	an empty review is trying to be published, an error is raised and the
	+	`public` state rolls back to previous state.

Description:

		When making an empty review, the review is recognized as a valid review
		and can be published. The problem is that when a review is detected as
		empty, the code doesn't raise a publish error so the empty review being
~		published is blocked. I added a function to detect empty reviews. Once
~		an empty review is trying to be published, an error is raised and the
~		`public` state rolls back to previous state.
	~	published is blocked.
	~
	~	I added a function to detect empty reviews. Once an empty review is
	+	trying to be published, an error is raised and the `public` state rolls
	+	back to previous state.

Commits:

	Summary	ID
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70
	modify multi-line str to pass flake8	ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2
	Make stylistic changes as suggested by review	db79445479b1041517604a7103ad206b461c8518
	Don't allow publishing empty review	79a9e95c26d4437434d1cff3a24afac4ac1a62ca
	Delete extra spaces.	e302fe5b4d9a93739582169d7ea09379adeec795
	unit tests added	77361f2ccd649efa6c1547c1d4107ab9cc7eb70f
	remove extra spaces	9e1cb9b46acfebefa82e1089adddbd087bfc5b70
	modify multi-line str to pass flake8	ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2
	Make stylistic changes as suggested by review	db79445479b1041517604a7103ad206b461c8518
	Add more unit tests for is_empty_review()	d80076115ce6a2811ec07cb3d3663d961b29a086

Diff:

Revision 6 (+370 -30)

Show changes

	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.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		When making an empty review, the review is recognized as a valid review
~		and can be published. The problem is that when a review is detected as
~		empty, the code doesn't raise a publish error so the empty review being
	~	When making an empty review (i.e., no comment, body top, body bottom,
	~	or Ship It) the review is recognized as a valid review and can be
	~	published. The problem is that when a review is detected as empty,
	+	the code doesn't raise a publish error so the empty review being
		published is blocked.

		I added a function to detect empty reviews. Once an empty review is
		trying to be published, an error is raised and the `public` state rolls
		back to previous state.

I think it would be helpful to say a little more about what the public state means in this context. When I first read the description, it wasn't really clear to me what it meant. After I went through the code, I saw that public was referring to the review request and meant that it was published, or viewable by others.

LittleGrey Jan. 20, 2020, 10:53 p.m.

Thank you for your advice! I will make it clearer!

reviewboard/static/rb/js/views/draftReviewBannerView.es6.js (Diff revision 6)

Is this the right interpretation of what the user sees with this code?

try to publish review by clicking the publish button
publish button gets disabled
publishing the review fails
publish button gets renabled

LittleGrey Jan. 20, 2020, 10:53 p.m.

The order is right, but it's not the publish button. There are 3 buttons in the draft review banner: Edit Review, Publish Review, and Discard. The first and third button are disabled after these steps:
1. try to publish an empty review
2. the error msg shows up
3. close the review dialog
4. look at the top banner

Description:

		When making an empty review (i.e., no comment, body top, body bottom,
		or Ship It) the review is recognized as a valid review and can be
		published. The problem is that when a review is detected as empty,
		the code doesn't raise a publish error so the empty review being
		published is blocked.

		I added a function to detect empty reviews. Once an empty review is
		trying to be published, an error is raised and the `public` state rolls
~		back to previous state.
	~	back to previous state so it doesn't mess up the state when an error
	+	occurs (`public` state is a `bool` - when it's `true`, the review
	+	is considered as published).