Added automated flag when closing or editing review requests
Review Request #12051 — Created Feb. 10, 2022 and updated
Information | |
---|---|
sheenaNg | |
Review Board | |
master | |
12038 | |
12223 | |
Reviewers | |
reviewboard | |
- Added
automated
flag to multiple methods related to hostingsvcs close
hook,ReviewRequestResourceAPI Update()
, andreview_request.close()
to store the value inChangeDescription
model to track automated
closing of review requests
- Added
automated
flag to
ReviewRequestResourceDraftAPI Update(), Create()
,
review_request.publish()
,draft.publish()
to store the value in
ChangeDescription
model to track automated upddating/publishing
of review requests
- Added
automated
flag to closed/published signals and email-related methods to render the
review_request_email.txt
differently whenautomated=True
- Added codes to
change.html
template to render RB logo in the
review request change entry whenautomated=True
- Added unit tests to
test_email_sending.py
- Added unit tests to
test_review_request.py
- Added unit tests to
test_review_request_draft.py
- Added unit tests to
test_entries.py
- Added unit tests to
webapi/tests/test_review_request.py
- Added unit tests to
webapi/tests/test_review_request_draft.py
- Added assertion to
test_github.py
,test_rbgateway.py
,
test_bitbucket.py
All tests passed.
- Attached screenshots to show changes in close email .txt and
frontend changeEntry.
Summary | Author |
---|---|
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 | |
sng06 |
Description | From | Last Updated |
---|---|---|
Please wrap your description and testing done fields to 70 columns. |
|
|
While we're here, can we rewrite this docstring to follow our modern standards? We probably also want to add a … |
|
|
Instead of assertEqual(..., True), use assertTrue(...). |
|
|
Instead of assertEqual(..., True), use assertTrue(...). |
|
|
Instead of assertEqual(..., True), use assertTrue(...). |
|
|
Please add ", optional" to the type. |
|
|
These can be combined: if not (user or automate): |
|
|
E128 continuation line under-indented for visual indent |
![]() |
|
Should be marked as optional (and probably the other things in here should be fixed for that as well) |
|
|
As Christian mentioned in our meeting, these should be updated to use kgb's spy assertion methods. |
|
|
E501 line too long (80 > 79 characters) |
![]() |
|
This should be in the imperative mood ("Close" instead of "Closes") |
|
|
Instead of defining a variable for this, let's just pass automated=True to close_review_request. |
|
|
This is getting so long, can we reformat it to list one parameter per line? |
|
|
This can be simplified: extra_context.update({ 'automated': changedesc.extra_data.get('automated', False), }) |
|
|
Test docstrings should always start with "Testing ..." |
|
|
"Testing ..." |
|
|
There's an extra space at the end of this docstring. |
|
|
There's an extra space at the end of this docstring. |
|
|
Please put the """ on the next line. |
|
|
This needs a trailing comma after it. |
|
|
Please put the """ on the next line. |
|
|
Please put the """ on the next line. |
|
|
This needs a trailing comma after it. |
|
|
Please put the """ on the next line. |
|
|
Please put the """ on the next line. |
|
|
This needs a trailing comma after it. |
|
Summary: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||
Diff: |
Revision 2 (+140 -40) |
Checks run (2 succeeded)
Commits: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+534 -70) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/notifications/tests/test_email_sending.py (Diff revision 3) Show all issues
-
Looking good!
-
Regarding the question in your testing done field, editing the existing hook tests is fine to add that one new assertion.
-
-
reviewboard/hostingsvcs/hook_utils.py (Diff revision 3) While we're here, can we rewrite this docstring to follow our modern standards?
We probably also want to add a default value for the
automate
field in case any extensions are using this API. -
reviewboard/hostingsvcs/tests/test_bitbucket.py (Diff revision 3) Instead of
assertEqual(..., True)
, useassertTrue(...)
. -
reviewboard/hostingsvcs/tests/test_github.py (Diff revision 3) Instead of
assertEqual(..., True)
, useassertTrue(...)
. -
reviewboard/hostingsvcs/tests/test_rbgateway.py (Diff revision 3) Instead of
assertEqual(..., True)
, useassertTrue(...)
. -
-
reviewboard/notifications/email/message.py (Diff revision 3) These can be combined:
if not (user or automate):
-
reviewboard/reviews/models/review_request.py (Diff revision 3) Should be marked as optional (and probably the other things in here should be fixed for that as well)
-
reviewboard/reviews/tests/test_review_request.py (Diff revision 3) As Christian mentioned in our meeting, these should be updated to use kgb's spy assertion methods.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+571 -133) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
Checks run (2 succeeded)
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+839 -145) |
Checks run (2 succeeded)
Change Summary:
Added automate field and codes to draft publish operations
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+929 -169) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/reviews/models/review_request.py (Diff revision 6) E501 line too long (80 > 79 characters)
Change Summary:
Added unit tests to test draft operations-related methods
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+1717 -215) |
Checks run (2 succeeded)
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+1887 -373) |
Checks run (2 succeeded)
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+1907 -391) |
Checks run (2 succeeded)
-
-
reviewboard/hostingsvcs/hook_utils.py (Diff revision 9) This should be in the imperative mood ("Close" instead of "Closes")
-
reviewboard/hostingsvcs/hook_utils.py (Diff revision 9) Instead of defining a variable for this, let's just pass
automated=True
toclose_review_request
. -
reviewboard/notifications/email/message.py (Diff revision 9) This is getting so long, can we reformat it to list one parameter per line?
-
reviewboard/notifications/email/message.py (Diff revision 9) This can be simplified:
extra_context.update({ 'automated': changedesc.extra_data.get('automated', False), })
-
reviewboard/notifications/tests/test_email_sending.py (Diff revision 9) Test docstrings should always start with "Testing ..."
-
-
reviewboard/reviews/tests/test_entries.py (Diff revision 9) There's an extra space at the end of this docstring.
-
reviewboard/reviews/tests/test_entries.py (Diff revision 9) There's an extra space at the end of this docstring.
-
reviewboard/webapi/tests/test_review_request.py (Diff revision 9) Please put the """ on the next line.
-
reviewboard/webapi/tests/test_review_request.py (Diff revision 9) This needs a trailing comma after it.
-
reviewboard/webapi/tests/test_review_request.py (Diff revision 9) Please put the """ on the next line.
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 9) Please put the """ on the next line.
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 9) This needs a trailing comma after it.
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 9) Please put the """ on the next line.
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 9) Please put the """ on the next line.
-
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 9) This needs a trailing comma after it.
Change Summary:
Addressed code review feedback
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+1949 -433) |