Empty reviews cannot be published
Review Request #10829 — Created Jan. 17, 2020 and discarded
Information | |
---|---|
LittleGrey | |
Review Board | |
release-4.0.x | |
4756 | |
Reviewers | |
reviewboard | |
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.
- Open an empty review dialog, and close it.
- Type something in header, click "cancel", then close the review dialog.
- Add a new comment, type something, click "cancel", then close the review dialog.
- Type something in footer, click "cancel", then close the review dialog.
- Type and save text in header & footer, add a non-empty comment, delete them all, then close the review dialog.
Summary |
---|
Description | From | Last Updated |
---|---|---|
Instead of listing the ticket number in the description, put it into the "Bugs" field of the review request. |
|
|
Your summary and description can be improved. The summary should be direct and to the point-- "Fix the bug that … |
|
|
Normal code comments don't need to start with /**--that's a special indication that the comment is for documentation, and we … |
|
|
Sentences in comments should end in periods. |
|
|
It might be nice to move this to a new function, which both this and the discard handler can call. |
|
-
-
Instead of listing the ticket number in the description, put it into the "Bugs" field of the review request.
-
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.
-
reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1) 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. -
reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1) Sentences in comments should end in periods.
-
reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1) It might be nice to move this to a new function, which both this and the discard handler can call.

Summary: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||
Commits: |
|
|||||||||
Bugs: |
|
|||||||||
Diff: |
Revision 2 (+55 -15) |
Checks run (2 succeeded)

Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+56 -16) |
Checks run (2 succeeded)
-
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
, inReview.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:- The review contains
body_top
orbody_bottom
text, or - 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()
andReview.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.