Empty reviews cannot be published
Review Request #10829 — Created Jan. 17, 2020 and discarded
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 | ID |
---|---|
65df2674c186af9adc3dfe73c4da263f39855888 | |
1c07be301bafefbaa8ff0134de5d837e8b8dbd34 | |
4a2a77798d4235e3100f1584e1a2c3e1f3d8f0ec |
Description | From | Last Updated |
---|---|---|
Instead of listing the ticket number in the description, put it into the "Bugs" field of the review request. |
david | |
Your summary and description can be improved. The summary should be direct and to the point-- "Fix the bug that … |
david | |
Normal code comments don't need to start with /**--that's a special indication that the comment is for documentation, and we … |
david | |
Sentences in comments should end in periods. |
david | |
It might be nice to move this to a new function, which both this and the discard handler can call. |
david |
-
-
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.
-
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. -
-
- Summary:
-
Fix the bug that empty reviews can be publishedEmpty reviews cannot be published
- Description:
-
~ Fix the bug that empty reviews can be published (ticket 4756: https://hellosplat.com/s/beanbag/tickets/4756/)
~ 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. - Commits:
-
Summary ID 65df2674c186af9adc3dfe73c4da263f39855888 65df2674c186af9adc3dfe73c4da263f39855888 1c07be301bafefbaa8ff0134de5d837e8b8dbd34 - Bugs:
-
Checks run (2 succeeded)
flake8 passed.JSHint passed.
- Commits:
-
Summary ID 65df2674c186af9adc3dfe73c4da263f39855888 1c07be301bafefbaa8ff0134de5d837e8b8dbd34 65df2674c186af9adc3dfe73c4da263f39855888 1c07be301bafefbaa8ff0134de5d837e8b8dbd34 4a2a77798d4235e3100f1584e1a2c3e1f3d8f0ec
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.