Don't allow publishing empty review
Review Request #10834 — Created Jan. 18, 2020 and updated
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 thepublic
state rolls
back to previous state so it doesn't mess up the state when an error
occurs (public
state is abool
- when it'strue
, the review
is considered as published).
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.
Summary | ID |
---|---|
79a9e95c26d4437434d1cff3a24afac4ac1a62ca | |
e302fe5b4d9a93739582169d7ea09379adeec795 | |
77361f2ccd649efa6c1547c1d4107ab9cc7eb70f | |
9e1cb9b46acfebefa82e1089adddbd087bfc5b70 | |
ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2 | |
db79445479b1041517604a7103ad206b461c8518 | |
d80076115ce6a2811ec07cb3d3663d961b29a086 |
Description | From | Last Updated |
---|---|---|
Just realized I forgot to add tests! Will do that! |
![]() |
|
We like to basically tell a story about the change in the Description field, explaining the problem we had and … |
|
|
W293 blank line contains whitespace |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
When dealing with a multi-line string, it's often nicer to start the string on the next indentation level, so that … |
|
|
I think it's best we keep the terminology as "empty," instead of "blank." Same with other references in this docstring. |
|
|
Just to be slightly more in line with our terminology, this should use "Ship It". |
|
|
Slight tweak to the naming, just to be consistent with how we usually name thewse. Can you make this test_publish_with_empty_review? |
|
|
To be sure that we don't have any bad behavior, there should be several versions of this function: With an … |
|
|
This can just be passed inline when calling options.error. |
|
|
You can use toBeFalse() instead, here and below. |
|

Change Summary:
Added unit tests
Commits: |
|
|||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+160 -12) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
reviewboard/reviews/tests/test_review.py (Diff revision 2) E127 continuation line over-indented for visual indent

Change Summary:
remove extra spaces (again)
Commits: |
|
|||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+161 -13) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/reviews/tests/test_review.py (Diff revision 3) E127 continuation line over-indented for visual indent

Commits: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+163 -15) |
Checks run (2 succeeded)
-
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.
-
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) 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) 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) Just to be slightly more in line with our terminology, this should use "Ship It".
-
reviewboard/reviews/tests/test_review.py (Diff revision 4) 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) 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) This can just be passed inline when calling
options.error
. -
reviewboard/static/rb/js/resources/models/tests/draftReviewModelTests.js (Diff revision 4) You can use
toBeFalse()
instead, here and below.

Description: |
|
|||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||||||||||||
Diff: |
Revision 5 (+169 -27) |
Checks run (2 succeeded)

Description: |
|
---|

Description: |
|
---|

Commits: |
|
|||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+370 -30) |
Checks run (2 succeeded)

Description: |
|
---|
-
-
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 thatpublic
was referring to the review request and meant that it was published, or viewable by others. -
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

Description: |
|
---|