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
Don't allow publishing empty review
Delete extra spaces.
unit tests added
remove extra spaces
modify multi-line str to pass flake8
Make stylistic changes as suggested by review
Add more unit tests for is_empty_review()

Screenshots

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
-	Don't allow publishing empty review
-	Delete extra spaces.
+	Don't allow publishing empty review
+	Delete extra spaces.
+	unit tests added

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
-	Don't allow publishing empty review
-	Delete extra spaces.
-	unit tests added
+	Don't allow publishing empty review
+	Delete extra spaces.
+	unit tests added
+	remove extra spaces

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
-	Don't allow publishing empty review
-	Delete extra spaces.
-	unit tests added
-	remove extra spaces
+	Don't allow publishing empty review
+	Delete extra spaces.
+	unit tests added
+	remove extra spaces
+	modify multi-line str to pass flake8

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
-	Don't allow publishing empty review
-	Delete extra spaces.
-	unit tests added
-	remove extra spaces
-	modify multi-line str to pass flake8
+	Don't allow publishing empty review
+	Delete extra spaces.
+	unit tests added
+	remove extra spaces
+	modify multi-line str to pass flake8
+	Make stylistic changes as suggested by review

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
-	Don't allow publishing empty review
-	Delete extra spaces.
-	unit tests added
-	remove extra spaces
-	modify multi-line str to pass flake8
-	Make stylistic changes as suggested by review
+	Don't allow publishing empty review
+	Delete extra spaces.
+	unit tests added
+	remove extra spaces
+	modify multi-line str to pass flake8
+	Make stylistic changes as suggested by review
+	Add more unit tests for is_empty_review()

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