flake8
passed.
JSHint
passed.
Notify for ship-it revocations
Review Request #12045 — Created Feb. 4, 2022 and updated
| Information | |
|---|---|
| gdehal | |
| Review Board | |
| master | |
| Reviewers | |
| reviewboard | |
- Create boolean field in admin page for email
- Connect handler to revoke signal
- Prepare ship-it revoked email message
- Create new email template for text and HTML
- Modified email settings tests to include the new field.
- Added email test to verify that the email has actually been sent and contains the right text.
| Summary |
|---|
| Description | From | Last Updated |
|---|---|---|
|
F401 'importlib' imported but unused |
 reviewbot
|
|
|
F401 'django.utils.six' imported but unused |
reviewbot
|
|
|
E501 line too long (87 > 79 characters) |
reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
F401 'reviewboard.reviews.views.build_diff_comment_fragments' imported but unused |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
This should probably use the "Re:" logic from prepare_review_request_mail |
 david
|
|
|
F401 'reviewboard.reviews.models.review_request' imported but unused |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
F811 redefinition of unused 'review_request' from line 17 |
reviewbot
|
|
|
Would it be clearer to write the label as "Send e-mails to users when the reviews' ship-it status are revoked"? |
 sheenaNg
|
|
|
It looks good! One suggestion, could you include docstring for this method similar to other methods in this module? |
sheenaNg
|
|
|
Could you also include docstring for this method? |
sheenaNg
|
|
|
Is this supposed to be review.REVOKED_SHIP_IT_TEXT? |
sheenaNg
|
|
|
I noticed that you included On {{review_request.time_emailed}}, {{review_request.submitter|user_displayname}} wrote: {% quoted_email "notifications/review_request_email.txt" %}} in the email.txt template. I wonder if … |
sheenaNg
|
|
|
Please add a class docstring. |
david
|
|
|
The description needs to go on the next line. |
david
|
|
|
The description needs to go on the next line. |
david
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Description for the return type doesn't need to be indented (align with EmailMessage) |
david
|
|
|
This needs a trailing comma. Please also use single quotes. |
david
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
Commits: |
|
||||||||
|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+2432 -1050) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
reviewboard/__init__.py (Diff revision 2) Show all issues -
Description: |
|
||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||
Diff: |
Revision 3 (+140 -6) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
-
-
reviewboard/notifications/email/signal_handlers.py (Diff revision 3) E302 expected 2 blank lines, found 1
Description: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||
Diff: |
Revision 4 (+265 -13) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
-
reviewboard/notifications/email/message.py (Diff revision 4) F401 'reviewboard.reviews.views.build_diff_comment_fragments' imported but unused
-
reviewboard/notifications/email/message.py (Diff revision 4) E128 continuation line under-indented for visual indent
-
reviewboard/notifications/email/signal_handlers.py (Diff revision 4) F401 'reviewboard.reviews.models.review_request' imported but unused
-
reviewboard/notifications/email/signal_handlers.py (Diff revision 4) E231 missing whitespace after ','
-
reviewboard/notifications/email/signal_handlers.py (Diff revision 4) F811 redefinition of unused 'review_request' from line 17
-
Looks like everything is on the right track!
-
reviewboard/notifications/email/message.py (Diff revision 4) This should probably use the "Re:" logic from
prepare_review_request_mail
Description: |
|
||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||
Diff: |
Revision 5 (+567 -17) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/admin/forms/email_settings.py (Diff revision 5) Would it be clearer to write the label as "Send e-mails to users when the reviews' ship-it status are revoked"?
-
reviewboard/notifications/email/message.py (Diff revision 5) It looks good! One suggestion, could you include docstring for this method similar to other methods in this module?
-
reviewboard/notifications/email/signal_handlers.py (Diff revision 5) Could you also include docstring for this method?
-
reviewboard/notifications/email/signal_handlers.py (Diff revision 5) I noticed that the
send_review_published_mailsignal handler includes a parameter calledto_owner_onlythat indicates whether or not the mail should only be sent to the review request
submitter.
I am just curious of why this field is not added to this method to enable the option to send it to the submitter only? -
reviewboard/templates/notifications/review_ship_it_revoked_email.html (Diff revision 5) Is this supposed to be
review.REVOKED_SHIP_IT_TEXT? -
reviewboard/templates/notifications/review_ship_it_revoked_email.html (Diff revision 5) I noticed that you included
On {{review_request.time_emailed}}, {{review_request.submitter|user_displayname}} wrote: {% quoted_email "notifications/review_request_email.txt" %}}in the email.txt template.I wonder if you should also include that in the .html similar to what is shown in the
review_email.htmlblock footer?
Testing Done: |
|
||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||
Diff: |
Revision 6 (+687 -17) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
reviewboard/notifications/email/message.py (Diff revision 6) E501 line too long (81 > 79 characters)
-
reviewboard/notifications/email/signal_handlers.py (Diff revision 6) E501 line too long (81 > 79 characters)
-
reviewboard/notifications/tests/test_email_sending.py (Diff revision 6) E501 line too long (80 > 79 characters)
-
-
-
-
-
reviewboard/notifications/email/message.py (Diff revision 6) Description for the return type doesn't need to be indented (align with
EmailMessage) -
reviewboard/notifications/email/message.py (Diff revision 6) This needs a trailing comma. Please also use single quotes.
Commits: |
|
||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+736 -26) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Commits: |
|
||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+737 -27) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.