[WIP] Publishing possible without required fields
Review Request #7886 — Created Jan. 16, 2016 and discarded
-Added checks and improved error message from can_publish() and publishing failures.- returned can_publish() to its former glory.
A list of errors will be presented to the user when they run 'rbt publish ##'
validate_can_publish(self, errors=False) has been added. passing true will generate an exception whereas false or default will only return a boolean if it is a valid review request having a reviewer, summary, and description.
--Merged Review Request with request 7882
TODO: create automated test
test publish with user + summary fails
test publish with user + description fails
test publish with group + summary fails
test publish with group + description fails
| Description | From | Last Updated |
|---|---|---|
|
The summary should be in the form of "Return <...>", and should have a trailing period. How about: "Return whether … |
 chipx86
|
|
|
Blank line between code blocks (if, for, etc.) and other code. Same with the if statements below. |
chipx86
|
|
|
Col: 51 W291 trailing whitespace |
 reviewbot
|
|
|
This function has always returned a boolean, and must continue to do so. Otherwise, we break other parts of the … |
chipx86
|
|
|
Col: 34 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 21 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 25 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 29 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 34 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 36 W291 trailing whitespace |
reviewbot
|
|
|
undefined name 'validate_can_publish' |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
local variable 'errors' is assigned to but never used |
reviewbot
|
|
|
undefined name 'validate_can_publish' |
reviewbot
|
|
|
Col: 34 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
local variable 'errors' is assigned to but never used |
reviewbot
|
|
|
Col: 38 W291 trailing whitespace |
reviewbot
|
|
|
Col: 13 E113 unexpected indentation |
reviewbot
|
|
|
This should be self.validate_can_publish(). |
 brennie
|
|
|
undefined name 'validate_can_publish' |
reviewbot
|
|
|
This needs translation. Also, favour .append over += with lists. |
brennie
|
|
|
This should really use .count instead of len((...).all()) e.g. if draft.target_people.count() or draft.target_groups.count() |
brennie
|
|
|
Same here. |
brennie
|
|
|
It is more pythonic to do if draft.summary. However, if the summary is all whitespace, we probably don't want to … |
brennie
|
|
|
Likewise, this should be if draft.description.trim(). |
brennie
|
|
|
You can just do if errors. |
brennie
|
|
|
We already have a mechanism for reporting errors to the user. This checking should go in the API. Have a … |
brennie
|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py -
-
-
-
-
The summary should be in the form of "Return <...>", and should have a trailing period. How about:
"Return whether the draft can be published."
We then also add some other indicators for our newly generated codebase docs. This docstring should have the form of:
"""Return [...] [...] Returns: bool: Whether the draft can be published. """Always make sure all sentences are explanatory to those not familiar with the code and are coming into this as a new user, and always use correct grammar and punctuation.
-
-
This function has always returned a boolean, and must continue to do so. Otherwise, we break other parts of the codebase, as well as third-party extensions.
We may also have various ways that a consumer would want to represent any error states here. A single string isn't going to really work.
Forms in Django have a concept of validation, where you call
full_clean()on a form of model and it either returns or it raises aValidationErrorfor what it finds. I'd suggest a similar thing.Let's define a
validate_can_publish()method that does this work instead. Instead of building a list of strings, it can raise adjango.core.exceptions.ValidationErrorwhen encountering things.can_publish()can then call into this, and return a boolean depending on whether it raised an error.That way,
can_publish()can be a simple call, but anything needing more complex validation with error results can callvalidate_can_publish()instead.
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py
- Summary:
-
Publishing possible without required fields[WIP] Publishing possible without required fields
-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py -
-
-
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py -
-
-
-
- Description:
-
~ added checks and improved error message from can_publish() and publishing failures
~ -Added checks and improved error message from can_publish() and publishing failures.- returned can_publish() to its former glory.
~ now a list of errors will be presented to the user
~ A list of errors will be presented to the user when they run 'rbt publish ##'
+ + + + validate_can_publish(self, errors=False) has been added. passing true will generate an exception whereas false or default will only return a boolean if it is a valid review request having a reviewer, summary, and description.
- Description:
-
-Added checks and improved error message from can_publish() and publishing failures.- returned can_publish() to its former glory.
A list of errors will be presented to the user when they run 'rbt publish ##'
validate_can_publish(self, errors=False) has been added. passing true will generate an exception whereas false or default will only return a boolean if it is a valid review request having a reviewer, summary, and description.
+ + --Merged Review Request with request 7882
+ + + + TODO: create automated test
- Bugs:
-
Hey Nathan, this is a solid start. However, you'll be wanting to make the changes to the Web API, specifically the
ReviewRequestDraftResource(inwebapi/resources/review_request_draft.py).In that file, you'll want to look at
update()which is responsible for setting values on a review request. The value taht ends up triggering this ispublic=1.W
-
-
-
This should really use
.countinstead oflen((...).all())e.g.
if draft.target_people.count() or draft.target_groups.count() -
-
It is more pythonic to do
if draft.summary. However, if the summary is all whitespace, we probably don't want to allow it. Hence we should checkdraft.summary.strip(). -
-
-
We already have a mechanism for reporting errors to the user. This checking should go in the API.
Have a look at lines 471--489 of
reviewboard/webapi/resources/review_request.pyfor an example of data validation for review request drafts. Your logic will go there. -
Feel free to ask my any questions about this review.
-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py
- Description:
-
-Added checks and improved error message from can_publish() and publishing failures.- returned can_publish() to its former glory.
A list of errors will be presented to the user when they run 'rbt publish ##'
validate_can_publish(self, errors=False) has been added. passing true will generate an exception whereas false or default will only return a boolean if it is a valid review request having a reviewer, summary, and description.
--Merged Review Request with request 7882
~ TODO: create automated test
~ TODO: create automated test
+ test publish with user + summary fails + test publish with user + description fails + test publish with group + summary fails + test publish with group + description fails
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py