Avoid to submit a non-public draft
Review Request #6856 — Created Jan. 31, 2015 and submitted
Fix not dealing with submit action with review_request.py
Add validation to the ReviewRequest model to prevent state transitions to SUBMITTED if the review request is not public.
Then get rid of the "Submit" button in a review request if it's not currently public on the UI level.
Changes have been made on following tests because they just creat a non-public empty draft to submit. This should not be allowed.
./reviewboard/manage.py test -- reviewboard.reviews.tests:ReviewRequestCounterTests.test_closing_closed_requests
I publish the review request first before discard it.
./reviewboard/manage.py test -- reviewboard.reviews.tests:ReviewRequestCounterTests.test_reopen_submitted_draft_requests
I use test_closing_requests instead of test_closing_draft_requests because test_closing_requests can create a public draft instead.
./reviewboard/manage.py test -- reviewboard.reviews.tests:ReviewRequestTests.test_public_with_discard_reopen_submitted
I publish the review request after reopen a discarded request before submit it.
./reviewboard/manage.py test -- reviewboard.notifications.tests:WebHookSignalDispatchTests.test_review_request_reopened
I create a published request instead.
./reviewboard/manage.py test -- reviewboard.notifications.tests:WebHookSignalDispatchTests.test_review_request_closed_submitted
I create a published request instead.
Description | From | Last Updated |
---|---|---|
You should tie this in to the import for PermissioError, like this: from reviewboard.reviews.errors import (PermissionError, PublishError) |
mike_conley | |
I think we still want to make it possible to Delete Permanently and Discard a review request that is not … |
mike_conley | |
I suspect we're going to want to add a new test too to ensure that we cannot transition from PENDING_REVIEW … |
mike_conley | |
TBH, I don't think it's necessary to test the string representation of the exception. See test_publish_changedesc_none, for example. |
mike_conley | |
There should be two newlines between root-script level items. |
mike_conley | |
local variable 'pe' is assigned to but never used |
reviewbot |
-
Just a few issues, please a reminder to please read this document on writing good change descriptions / testing done documentation.
-
reviewboard/reviews/models/review_request.py (Diff revision 1) You should tie this in to the import for PermissioError, like this:
from reviewboard.reviews.errors import (PermissionError, PublishError)
-
reviewboard/templates/reviews/review_request_actions_secondary.html (Diff revision 1) I think we still want to make it possible to Delete Permanently and Discard a review request that is not public, so we should just wrap the Submitted option here.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+18 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html
-
This looks really good, Chenxi. I think you're very close - my only suggestion is to add a new test to exercise the restriction you've put in.
-
reviewboard/reviews/tests.py (Diff revision 2) I suspect we're going to want to add a new test too to ensure that we cannot transition from PENDING_REVIEW to SUBMITTED if the review request is not public.
Change Summary:
add a new test to ensure that we cannot transition from PENDING_REVIEW to SUBMITTED if the review request is not public
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+41 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html
-
-
reviewboard/reviews/tests.py (Diff revision 3) TBH, I don't think it's necessary to test the string representation of the exception. See test_publish_changedesc_none, for example.
-
reviewboard/reviews/tests.py (Diff revision 3) There should be two newlines between root-script level items.
Change Summary:
remove unnecessary part
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+37 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+37 -6) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/tests.py reviewboard/notifications/tests.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_request_actions_secondary.html