flake8
passed.
JSHint
passed.
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 thepublicstate rolls
back to previous state so it doesn't mess up the state when an error
occurs (publicstate 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! |
 LittleGrey
|
|
|
We like to basically tell a story about the change in the Description field, explaining the problem we had and … |
 chipx86
|
|
|
W293 blank line contains whitespace |
 reviewbot
|
|
|
E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
When dealing with a multi-line string, it's often nicer to start the string on the next indentation level, so that … |
chipx86
|
|
|
I think it's best we keep the terminology as "empty," instead of "blank." Same with other references in this docstring. |
chipx86
|
|
|
Just to be slightly more in line with our terminology, this should use "Ship It". |
chipx86
|
|
|
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
|
|
|
To be sure that we don't have any bad behavior, there should be several versions of this function: With an … |
chipx86
|
|
|
This can just be passed inline when calling options.error. |
chipx86
|
|
|
You can use toBeFalse() instead, here and below. |
chipx86
|
- Change Summary:
-
Added unit tests
- Commits:
-
Summary ID 6d65c4578d6e219dc4da5e9109133f4450376dec 6a22f3dd992478d5b6d9647b1837a1698a3c47ce 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f - Diff:
-
Revision 2 (+160 -12)
- Change Summary:
-
remove extra spaces (again)
- Commits:
-
Summary ID 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 9e1cb9b46acfebefa82e1089adddbd087bfc5b70 - Diff:
-
Revision 3 (+161 -13)
- Commits:
-
Summary ID 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 9e1cb9b46acfebefa82e1089adddbd087bfc5b70 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 9e1cb9b46acfebefa82e1089adddbd087bfc5b70 ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2 - Diff:
-
Revision 4 (+163 -15)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
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.
-
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.")) -
I think it's best we keep the terminology as "empty," instead of "blank." Same with other references in this docstring.
-
-
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 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).
-
-
- 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 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 9e1cb9b46acfebefa82e1089adddbd087bfc5b70 ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 9e1cb9b46acfebefa82e1089adddbd087bfc5b70 ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2 db79445479b1041517604a7103ad206b461c8518 - Diff:
-
Revision 5 (+169 -27)
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 + publicstate 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 ~ publicstate 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 publicstate rolls+ back to previous state.
- Commits:
-
Summary ID 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 9e1cb9b46acfebefa82e1089adddbd087bfc5b70 ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2 db79445479b1041517604a7103ad206b461c8518 79a9e95c26d4437434d1cff3a24afac4ac1a62ca e302fe5b4d9a93739582169d7ea09379adeec795 77361f2ccd649efa6c1547c1d4107ab9cc7eb70f 9e1cb9b46acfebefa82e1089adddbd087bfc5b70 ecc729eed7ed820f9866f718c3e8f54ed0aa9ab2 db79445479b1041517604a7103ad206b461c8518 d80076115ce6a2811ec07cb3d3663d961b29a086 - Diff:
-
Revision 6 (+370 -30)
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 publicstate rollsback to previous state.
-
-
I think it would be helpful to say a little more about what the
publicstate 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 thatpublicwas referring to the review request and meant that it was published, or viewable by others. -
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:
-
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 publicstate rolls~ back to previous state. ~ back to previous state so it doesn't mess up the state when an error + occurs ( publicstate is abool- when it'strue, the review+ is considered as published).