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
|
ML
- Groups:
-
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:
-
WIP - Checks to make sure the field is modified from the one currently saved in the dbChecks to make sure the field is modified from the one currently saved in the db
- Description:
-
~ WIP - Checks to make sure the field is modified from the one currently saved in the db
~ 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_drat controller/resouce 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 re-enabled the 'Publish Changes' and 'Discard Draft' buttons. - Testing Done:
-
+ Tried "not-modifying" the Description, and Testing Done fields to determine the output.
- Bugs:
-
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 -
-
-
-
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:
-
Tried "not-modifying" the Description, and Testing Done fields to determine the output.
+ + Wrote test case to publish an empty draft (test case passes).
- Commit:
-
5784f52bb2490a71aa0eee8b1f72553a545c80c5b75bdd6a9bdbccc535ae07a99661259677fe3205
- 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 -
-
-
-
-
-
-
ML
- Change Summary:
-
Fix whitespace and newline issues
- Commit:
-
b75bdd6a9bdbccc535ae07a99661259677fe3205e0fdc05a80e42dd5b34b6acb388dbec45a0e6a38
- 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). -
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.
-
-
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 and unused variable in test case.
- Commit:
-
e0fdc05a80e42dd5b34b6acb388dbec45a0e6a3832c8f0af64fc565cb08c36f90f2ba70ac877d133
- 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:
-
+ 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_drat controller/resouce 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 re-enabled the 'Publish Changes' and 'Discard Draft' buttons. - Commit:
-
32c8f0af64fc565cb08c36f90f2ba70ac877d1334dc7b4682549bffea3a1c9de84ff31deb50e336e
- 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:
-
4dc7b4682549bffea3a1c9de84ff31deb50e336e01b3a6b81892dc509b60a849d9e8996df409388d
- 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
ML
- Change Summary:
-
Fix alphabetica and small issues.
- Commit:
-
01b3a6b81892dc509b60a849d9e8996df409388d87c652e8f3ff1b8c8fea9a5127efff3a7674ea0d
- 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
ML
- Change Summary:
-
Replace tab character with spaces
- Commit:
-
87c652e8f3ff1b8c8fea9a5127efff3a7674ea0d08a17c84b096d725532779c90df3d1c840207efb
- 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:
-
Checks to make sure the field is modified from the one currently saved in the dbPrevent users from publishing empty (no modifications made) review request drafts.
- Testing Done:
-
Tried "not-modifying" the Description, and Testing Done fields to determine the output.
Wrote test case to publish an empty draft (test case passes).
+ + Postivie Test Cases:
+ Added a file (successfull publish). + Remove a file (successfull publish). + Changing the description field (successfull publish). + Add comments (successfull publish). + Add branch/target people/bug number.
-
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:
-
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_drat controller/resouce 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 re-enabled the 'Publish Changes' and 'Discard Draft' buttons. ~ 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 re-enables the 'Publish Changes' and 'Discard Draft' buttons. - Testing Done:
-
Tried "not-modifying" the Description, and Testing Done fields to determine the output.
Wrote test case to publish an empty draft (test case passes).
~ Postivie Test Cases:
~ Added a file (successfull publish). ~ Remove a file (successfull publish). ~ Changing the description field (successfull publish). ~ Add comments (successfull publish). ~ 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.
-
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.
-
This documentation should go below the function header - see the record_field_change function, for example.
-
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:
-
08a17c84b096d725532779c90df3d1c840207efb72f8562516e1972e76765cacc2ce99a1b65d9f25
- 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
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:
-
72f8562516e1972e76765cacc2ce99a1b65d9f257cec137cb5df23432d824b09be933987b311b91f
- 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:
-
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 re-enables the 'Publish Changes' and 'Discard Draft' buttons. ~ 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.
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:
-
7cec137cb5df23432d824b09be933987b311b91f70eab4a41c9a87dc3e43ea6b4144f3a57667afa4
-
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