Empty reviews cannot be published

Review Request #10829 — Created Jan. 17, 2020 and discarded

Information

Review Board
release-4.0.x

Reviewers

Bug: empty reviews can be published, and draft review banner is shown when review is empty.
My change: a review cannot published if: "Ship it" is not checked & header is empty & no comment & footer is empty.

  1. Open an empty review dialog, and close it.
  2. Type something in header, click "cancel", then close the review dialog.
  3. Add a new comment, type something, click "cancel", then close the review dialog.
  4. Type something in footer, click "cancel", then close the review dialog.
  5. Type and save text in header & footer, add a non-empty comment, delete them all, then close the review dialog.
Summary ID
Fix the bug that empty reviews can be published
65df2674c186af9adc3dfe73c4da263f39855888
Addressing review feedbacks
1c07be301bafefbaa8ff0134de5d837e8b8dbd34
Remove extra spaces between functions
4a2a77798d4235e3100f1584e1a2c3e1f3d8f0ec
Description From Last Updated

Instead of listing the ticket number in the description, put it into the "Bugs" field of the review request.

daviddavid

Your summary and description can be improved. The summary should be direct and to the point-- "Fix the bug that …

daviddavid

Normal code comments don't need to start with /**--that's a special indication that the comment is for documentation, and we …

daviddavid

Sentences in comments should end in periods.

daviddavid

It might be nice to move this to a new function, which both this and the discard handler can call.

daviddavid
LittleGrey
david
  1. 
      
  2. Show all issues

    Instead of listing the ticket number in the description, put it into the "Bugs" field of the review request.

  3. Show all issues

    Your summary and description can be improved.

    The summary should be direct and to the point-- "Fix the bug that ..." has a lot of words that don't add much. How about "Don't allow empty reviews to be published"

    The description should be a fair bit longer than one sentence. When doing a bug fix, the best format will be one that first explains what the problem was, and then how your change fixes it. The goal here is to make it so people don't have to open the linked bug to understand what the change is about.

  4. Show all issues

    Normal code comments don't need to start with /**--that's a special indication that the comment is for documentation, and we have a doc scanner that extracts those. Just use a normal /* here. You've also got an extra space character at the end of the line.

  5. Show all issues

    Sentences in comments should end in periods.

  6. Show all issues

    It might be nice to move this to a new function, which both this and the discard handler can call.

  7. 
      
LittleGrey
LittleGrey
chipx86
  1. Thanks for working on this!

    I went over the code, and I think there was a misunderstanding somewhere about the bug. The code here attempts to clean up a review that may have been created but is now empty, so it doesn't sit around forever in the database. A useful thing, though the implementation needs more thought (if you opened the Review Dialog in two tabs, made comments in one, closed the other, you'd lose all your work).

    However, that's not what the bug itself is about. That's talking about a case where a person has clicked Publish Review without putting in any comments or text, and so the review request ends up with this useless blank review. This change doesn't currently address this.

    Now, a change could be made here in the Review Dialog to prevent publishing a blank review, but that wouldn't catch problems where, say, an IDE plugin, or a script tried to create a review. So instead, we want to take care of this at a deeper level -- in the server code.

    Publishing reviews

    The code that ultimately handles publishing a review lives in reviewboard/reviews/models/review.py, in Review.publish(). This takes care of:

    • Sending out signals to let other parts of the product know a publish is happening (used to let third-party extensions block a publish, if they want to)
    • Updating timestamps to now
    • Marking the review as published
    • Bumping up some counters for the Dashboard's sidebar
    • Sending out signals to let other parts of the product know the publish happened successfully (used for sending out e-mails and stuff)

    This is where we should fix this bug, because it prevents the Review Dialog, the API, and third-party extensions from publishing.

    How to fix this

    What you'd want to do, before any of the other stuff gets set (before self.public = True in that code) is check that:

    1. The review contains body_top or body_bottom text, or
    2. It contains at least one comment (of one of the 4 supported types -- see where we call .update(...) on a bunch of comment relations)

    You can create a Review.is_empty() method to check for all that -- I think that'd be a great addition, and easy to unit test!

    Now if we're missing any of those, we'd want to raise a PublishError("This review is empty. It cannot be published until it's filled out."). This will get filtered up to the API, to the Review Dialog, to the user, taking care of all the issues.

    Unit tests

    Along with this, you'll need to write unit tests that test both Review.is_empty() and Review.publish(), checking to make sure things work when any of the pieces of data are available, and that they're blocked when all are missing.

    Take a look at Writing Python Unit Tests for some guidelines and info on this.

    This change?

    I think we should discard this for now. You can create a new review request for that work. Again, while I think there's value in getting rid of a review when it's empty, I think it's unsafe to do it in this dialog. It should also be managed at the API level (we actually do this for individual comments today), but it's also probably not a common enough thing to worry about right now. So let's focus on fixing the publishing of empty reviews.

    1. Thank you so much for your feedback! I will give a try on fixing this bug in the server code!

  2. 
      
david
  1. Since we decided this was the wrong approach, can you close this review request as discarded?

    Thanks!

    1. Sure! Thank you for pointing this out!

  2. 
      
LittleGrey
Review request changed

Status: Discarded

Loading...