Don't allow publishing empty review

Review Request #10834 — Created Jan. 18, 2020 and updated

LittleGrey
Review Board
release-3.0.x
4756
reviewboard

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

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

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

LittleGreyLittleGrey

We like to basically tell a story about the change in the Description field, explaining the problem we had and ...

chipx86chipx86

W293 blank line contains whitespace

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

When dealing with a multi-line string, it's often nicer to start the string on the next indentation level, so that ...

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This can just be passed inline when calling options.error.

chipx86chipx86

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

chipx86chipx86
LittleGrey
  1. 
      
  2. Just realized I forgot to add tests! Will do that!

  3. 
      
LittleGrey
Review request changed

Change Summary:

Added unit tests

Commits:

Summary
-
Delete extra spaces.
-
Don't allow publishing empty review
+
unit tests added
+
Don't allow publishing empty review
+
Delete extra spaces.

Diff:

Revision 2 (+160 -12)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

LittleGrey
Review request changed

Change Summary:

remove extra spaces (again)

Commits:

Summary
-
unit tests added
-
Don't allow publishing empty review
-
Delete extra spaces.
+
unit tests added
+
Don't allow publishing empty review
+
remove extra spaces
+
Delete extra spaces.

Diff:

Revision 3 (+161 -13)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

LittleGrey
chipx86
  1. 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.

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

  3. 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."))
    
  4. 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.

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

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

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

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

    1. With an empty review
    2. With a Ship It
    3. With body_top set
    4. With body_bottom set
    5. With a general comment
    6. With a diff comment
    7. With a file attachment comment
    8. 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).

  8. This can just be passed inline when calling options.error.

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

    1. 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?

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

    3. 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!

  10. 
      
LittleGrey
LittleGrey
LittleGrey
LittleGrey
LittleGrey
hxqlin
  1. 
      
  2. 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.

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

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

    1. try to publish review by clicking the publish button
    2. publish button gets disabled
    3. publishing the review fails
    4. publish button gets renabled
    1. 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

  4. 
      
LittleGrey
Review request changed

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

Loading...