Prevent users from publishing empty (no modifications made) review request drafts.
Review Request #6322 — Created Sept. 14, 2014 and submitted
When a user submits an empty draft for a review request a warning is issued to let them know they did not make any modifications.
The general flow is:
1. In the review_request_draft model determine if there were any changes to any of the fields, if not then raise a 'Not Modified' exception
2. The review_request_draft controller/resource will catch the 'Not Modified' exception and return an appropriate error message to the Client JS to display a meaningful message to the user.
3. Client JS displays error message and the 'Publish Changes' and 'Discard Draft' buttons remain disabled.
4. Once the user updates (edits and confirms by clicking 'OK') a field (Description, Testing Done) then the 'Publish Changes' and 'Discard Draft' buttons are re-enabled.
Tried "not-modifying" the Description, and Testing Done fields to determine the output.
Wrote test case to publish an empty draft (test case passes).
Postive Test Cases:
Added a file (successful publish).
Remove a file (successful publish).
Changing the description field (successful publish).
Add comments (successful publish).
Add branch/target people/bug number.
| Description | From | Last Updated |
|---|---|---|
|
Col: 1 W293 blank line contains whitespace |
 reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 1 W391 blank line at end of file |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
New functions and classes should include docstrings. They're always in the format of: """Single-line summary.""" or: """Single-line summary. Multi-line comment. … |
 chipx86
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 1 W391 blank line at end of file |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
The blank line should remain. |
chipx86
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
local variable 'draft' is assigned to but never used |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
local variable 'draft' is assigned to but never used |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 69 W291 trailing whitespace |
reviewbot
|
|
|
This can just be return bool(self.fields_changed) |
 david
|
|
|
Please revert the change to this line (should have two blank lines between the imports and the class). |
david
|
|
|
Can you move this up above the other if changedesc: block to avoid saving the object? |
david
|
|
|
It looks like you added a tab character here. Please remove it. |
david
|
|
|
Can you swap these? (N before P) |
david
|
|
|
Can you put NOTHING_TO_PUBLISH before PUBLISH_ERROR? |
david
|
|
|
You're not using the exception instance, so this should just be except NotModifiedError: |
david
|
|
|
There's still an extra tab character at the beginning of this line. |
david
|
|
|
This documentation should go below the function header - see the record_field_change function, for example. |
 mike_conley
|
|
|
This msut be inside the function, not on the outside. |
chipx86
|
|
|
Needs a docstring. |
chipx86
|
|
|
Models aren't allowed to access elements in the page. Only ReviewRequestEditorView is allowed to deal with the state of these … |
chipx86
|
|
|
How about "... review request without any modifications." ? |
chipx86
|
-
Some high-level comments:
- Code should never inject functions into other objects (the one being put into
reviewRequest). - You should use
get()andset()instead of accessingattributesdirectly. - Make sure to keep to the < 80 character line length limit.
As discussed in IRC, there's a different way this particular change should probably be tackled. This is really an issue that needs to be handled on the backend, rather than the frontend, in the publishing code.
- Code should never inject functions into other objects (the one being put into
ML
Change Summary:
Changed the logic from the front-end to the back-end
Summary: |
|
|||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||
Testing Done: |
|
|||||||||||||||
Bugs: |
|
ML
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+26 -7) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py reviewboard/reviews/models/review_request_draft.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py reviewboard/reviews/models/review_request_draft.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js -
-
-
-
reviewboard/reviews/models/review_request_draft.py (Diff revision 2) Col: 1 W293 blank line contains whitespace
RM
-
Not sure about the front end, but it seems like the behaviour on the back end could be added to unit tests.
ML
Change Summary:
Fixed issue when changed description field was empty and wrote test case
Testing Done: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||
Diff: |
Revision 3 (+44 -7) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js -
-
-
-
reviewboard/reviews/models/review_request_draft.py (Diff revision 3) Col: 1 W293 blank line contains whitespace
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 3) Col: 1 W293 blank line contains whitespace
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 3) local variable 'draft' is assigned to but never used
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 3) Col: 1 W293 blank line contains whitespace
ML
Change Summary:
Fix whitespace and newline issues
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+43 -6) |
-
Some general comments:
1) The summary is vague. It doesn't tell me what field we're talking about or why it matters. Should have something a little more precise yet still concise.
2) Make sure you check your diff for things like trailing whitespace or unnecessary changes (like extra blank lines in sectinos of code that wasn't otherwise modified). -
reviewboard/changedescs/models.py (Diff revision 3) New functions and classes should include docstrings. They're always in the format of:
"""Single-line summary."""or:
"""Single-line summary. Multi-line comment. """Same with anything else you've introduced.
-
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 3) The blank line should remain.
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js -
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 4) Col: 1 W293 blank line contains whitespace
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 4) local variable 'draft' is assigned to but never used
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 4) Col: 1 W293 blank line contains whitespace
ML
Change Summary:
Fixed whitespace issues and unused variable in test case.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+43 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js
ML
Change Summary:
Fixed whitespace issues, added a method description comment, and modified the method name in ChangeDescription object.
Description: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 6 (+49 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js -
ML
Change Summary:
fixed trailing whitespace issue
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+49 -6) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js
-
-
reviewboard/changedescs/models.py (Diff revision 7) This can just be
return bool(self.fields_changed) -
reviewboard/reviews/models/review_request_draft.py (Diff revision 7) Please revert the change to this line (should have two blank lines between the imports and the class).
-
reviewboard/reviews/models/review_request_draft.py (Diff revision 7) Can you move this up above the other
if changedesc:block to avoid saving the object? -
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 7) It looks like you added a tab character here. Please remove it.
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 7) Can you swap these? (N before P)
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 7) Can you put NOTHING_TO_PUBLISH before PUBLISH_ERROR?
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 7) You're not using the exception instance, so this should just be
except NotModifiedError:
ML
Change Summary:
Fix alphabetica and small issues.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+46 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js
-
-
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 8) There's still an extra tab character at the beginning of this line.
ML
Change Summary:
Replace tab character with spaces
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+45 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js
-
The code looks pretty good. I have just a couple comments/questions:
- Can you change the summary to more accurately reflect what's going on? Something like "Prevent users from publishing empty review request drafts".
- Can you test the positive cases? I'd like to be sure that adding/removing a file attachment or uploading a new diff without changing any of the "fields" still works.
ML
Change Summary:
Editted Summary and added test cases
Summary: |
|
||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
You've got some spelling errors in your description and testing done:
- review_request_drat -> review_request_draft
- re-enabled -> re-enables (to match tense)
- Postivie -> Positive
- successfull -> successful
ML
Change Summary:
fix spelling/grammar errors
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
AS
-
-
reviewboard/reviews/errors.py (Diff revision 9) What is the difference between this and:
class NotModifiedError(Exception): pass
-
I've tested this, and it works as advertised. Great stuff!
I do think, however, that the alert dialog to display error messages is pretty awful. That's not caused by this patch - it pre-existed - but I do think that's something we should get rid of at some point. I filed issue 3578 to switch from alert to something else.
Anyhow, this looks fine to me.
-
reviewboard/changedescs/models.py (Diff revision 9) This documentation should go below the function header - see the record_field_change function, for example.
-
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 9) Oh good, yes - this has burned us before when things have gone wrong on the server side, and the user was just flat-out stuck in this half-published state.
ML
Change Summary:
Changed redundant init method with 'pass'
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+44 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js
-
-
reviewboard/changedescs/models.py (Diff revision 10) This msut be inside the function, not on the outside.
-
-
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 10) Models aren't allowed to access elements in the page. Only ReviewRequestEditorView is allowed to deal with the state of these buttons.
-
reviewboard/webapi/errors.py (Diff revision 10) How about "... review request without any modifications." ?
ML
Change Summary:
Fixed Docstring issues
1) move from above to under method definition
2) added docstring for NotModifiedErrorRe-worded error message displayed to user when publishing empty draft.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 11 (+44 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js
ML
Change Summary:
Removed the lines enabling the 'Publish Changes' and 'Discard Draft' buttons. They are now re-enabled when the user updates a field.
Description: |
|
|---|
ML
Change Summary:
Removed the lines enabling the 'Publish Changes' and 'Discard Draft' buttons. They are now re-enabled when the user updates a field.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 12 (+42 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/errors.py reviewboard/reviews/models/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/reviews/errors.py reviewboard/changedescs/models.py
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/models/reviewRequestEditorModel.js