-
-
-
reviewboard/reviews/models/review_request.py (Diff revision 1) Col: 34 E127 continuation line over-indented for visual indent
-
reviewboard/reviews/models/review_request.py (Diff revision 1) Col: 17 E128 continuation line under-indented for visual indent
[WIP] Publishing possible without required fields
Review Request #7886 — Created Jan. 16, 2016 and discarded
Information | |
---|---|
nfredette | |
Review Board | |
master | |
4057, 4064 | |
Reviewers | |
reviewboard, students | |
-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 … |
|
|
Blank line between code blocks (if, for, etc.) and other code. Same with the if statements below. |
|
|
Col: 51 W291 trailing whitespace |
![]() |
|
This function has always returned a boolean, and must continue to do so. Otherwise, we break other parts of the … |
|
|
Col: 34 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 17 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 21 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 25 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 29 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 34 E225 missing whitespace around operator |
![]() |
|
Col: 36 W291 trailing whitespace |
![]() |
|
undefined name 'validate_can_publish' |
![]() |
|
Col: 1 W293 blank line contains whitespace |
![]() |
|
Col: 1 W293 blank line contains whitespace |
![]() |
|
local variable 'errors' is assigned to but never used |
![]() |
|
undefined name 'validate_can_publish' |
![]() |
|
Col: 34 E225 missing whitespace around operator |
![]() |
|
Col: 1 W293 blank line contains whitespace |
![]() |
|
local variable 'errors' is assigned to but never used |
![]() |
|
Col: 38 W291 trailing whitespace |
![]() |
|
Col: 13 E113 unexpected indentation |
![]() |
|
This should be self.validate_can_publish(). |
|
|
undefined name 'validate_can_publish' |
![]() |
|
This needs translation. Also, favour .append over += with lists. |
|
|
This should really use .count instead of len((...).all()) e.g. if draft.target_people.count() or draft.target_groups.count() |
|
|
Same here. |
|
|
It is more pythonic to do if draft.summary. However, if the summary is all whitespace, we probably don't want to … |
|
|
Likewise, this should be if draft.description.trim(). |
|
|
You can just do if errors. |
|
|
We already have a mechanism for reporting errors to the user. This checking should go in the API. Have a … |
|


-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py
-
reviewboard/reviews/models/review_request.py (Diff revision 2) Col: 21 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/models/review_request.py (Diff revision 2) Col: 25 E128 continuation line under-indented for visual indent
-
reviewboard/reviews/models/review_request.py (Diff revision 2) Col: 29 E128 continuation line under-indented for visual indent
-
-
reviewboard/reviews/models/review_request.py (Diff revision 1) 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.
-
reviewboard/reviews/models/review_request.py (Diff revision 1) Blank line between code blocks (if, for, etc.) and other code.
Same with the if statements below.
-
reviewboard/reviews/models/review_request.py (Diff revision 1) 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 aValidationError
for 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.ValidationError
when 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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+36 -3) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py
-
reviewboard/reviews/models/review_request.py (Diff revision 4) Col: 34 E225 missing whitespace around operator
-
-
reviewboard/reviews/models/review_request.py (Diff revision 4) undefined name 'validate_can_publish'
-
reviewboard/reviews/models/review_request.py (Diff revision 4) Col: 1 W293 blank line contains whitespace
-
reviewboard/reviews/models/review_request.py (Diff revision 4) Col: 1 W293 blank line contains whitespace
-
reviewboard/reviews/models/review_request.py (Diff revision 4) local variable 'errors' is assigned to but never used

-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py
-
reviewboard/reviews/models/review_request.py (Diff revision 5) undefined name 'validate_can_publish'
-
reviewboard/reviews/models/review_request.py (Diff revision 5) Col: 34 E225 missing whitespace around operator
-
reviewboard/reviews/models/review_request.py (Diff revision 5) Col: 1 W293 blank line contains whitespace
-
reviewboard/reviews/models/review_request.py (Diff revision 5) local variable 'errors' is assigned to but never used

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py
-
-
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+39 -2) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/models/review_request.py
-
reviewboard/reviews/models/review_request.py (Diff revision 7) undefined name 'validate_can_publish'
Description: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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
-
reviewboard/reviews/models/review_request.py (Diff revision 7) This should be
self.validate_can_publish()
. -
reviewboard/reviews/models/review_request.py (Diff revision 7) This needs translation.
Also, favour
.append
over+=
with lists. -
reviewboard/reviews/models/review_request.py (Diff revision 7) This should really use
.count
instead oflen((...).all())
e.g.
if draft.target_people.count() or draft.target_groups.count()
-
-
reviewboard/reviews/models/review_request.py (Diff revision 7) 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()
. -
reviewboard/reviews/models/review_request.py (Diff revision 7) Likewise, this should be
if draft.description.trim()
. -
-
reviewboard/reviews/models/review_request.py (Diff revision 7) 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.py
for 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: |
|
---|