Notify for ship-it revocations

Review Request #12045 — Created Feb. 4, 2022 and updated

Information

Review Board
master

Reviewers

  • 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 ID
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4
Add ship-it revoked email template
ffc09ed5e8de08ffbef47a92c0e847d52fdde6ce
Add tests and docstrings
6423a47761c811a6754ea8a244dd389f78e8ea5d
Addressed review comments
cb19e9326b5b409dc0e8806df59820f8c970554e
Changed label text
3fc6e424712db0a621d8306f0fbe0c41cd6d506c
Description From Last Updated

F401 'importlib' imported but unused

reviewbotreviewbot

F401 'django.utils.six' imported but unused

reviewbotreviewbot

E501 line too long (87 > 79 characters)

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

F401 'reviewboard.reviews.views.build_diff_comment_fragments' imported but unused

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

This should probably use the "Re:" logic from prepare_review_request_mail

daviddavid

F401 'reviewboard.reviews.models.review_request' imported but unused

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

F811 redefinition of unused 'review_request' from line 17

reviewbotreviewbot

Would it be clearer to write the label as "Send e-mails to users when the reviews' ship-it status are revoked"?

sheenaNgsheenaNg

It looks good! One suggestion, could you include docstring for this method similar to other methods in this module?

sheenaNgsheenaNg

Could you also include docstring for this method?

sheenaNgsheenaNg

Is this supposed to be review.REVOKED_SHIP_IT_TEXT?

sheenaNgsheenaNg

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 …

sheenaNgsheenaNg

Please add a class docstring.

daviddavid

The description needs to go on the next line.

daviddavid

The description needs to go on the next line.

daviddavid

E501 line too long (81 > 79 characters)

reviewbotreviewbot

Description for the return type doesn't need to be indented (align with EmailMessage)

daviddavid

This needs a trailing comma. Please also use single quotes.

daviddavid

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot
gdehal
Review request changed
Commits:
Summary ID
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gdehal
Review request changed
Description:
   
  • Create boolean field in admin page for email
  +
  • Connect handler to revoke signal
Commits:
Summary ID
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gdehal
Review request changed
Description:
   
  • Create boolean field in admin page for email
   
  • Connect handler to revoke signal
  +
  • Prepare ship-it revoked email message
Commits:
Summary ID
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. Looks like everything is on the right track!

  2. Show all issues

    This should probably use the "Re:" logic from prepare_review_request_mail

  3. 
      
gdehal
gdehal
sheenaNg
  1. 
      
  2. Show all issues

    Would it be clearer to write the label as "Send e-mails to users when the reviews' ship-it status are revoked"?

  3. Show all issues

    It looks good! One suggestion, could you include docstring for this method similar to other methods in this module?

  4. Show all issues

    Could you also include docstring for this method?

  5. I noticed that the send_review_published_mail signal handler includes a parameter called to_owner_only that 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?

    1. Mainly because the signal does not come with this flag since in the UI there is no ability to select that option at this time.

  6. Show all issues

    Is this supposed to be review.REVOKED_SHIP_IT_TEXT?

    1. No, I could have used that but for the HTML template I've opted to use strikethrough text with CSS.

  7. Show all issues

    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.html block footer?

  8. 
      
gdehal
Review request changed
Testing Done:
   
  • 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.
Commits:
Summary ID
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4
Add ship-it revoked email template
ffc09ed5e8de08ffbef47a92c0e847d52fdde6ce
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4
Add ship-it revoked email template
ffc09ed5e8de08ffbef47a92c0e847d52fdde6ce
Add tests and docstrings
6423a47761c811a6754ea8a244dd389f78e8ea5d

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    Please add a class docstring.

  3. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    The description needs to go on the next line.

  4. reviewboard/extensions/hooks.py (Diff revision 6)
     
     
    Show all issues

    The description needs to go on the next line.

  5. reviewboard/notifications/email/message.py (Diff revision 6)
     
     
     
    Show all issues

    Description for the return type doesn't need to be indented (align with EmailMessage)

  6. Show all issues

    This needs a trailing comma. Please also use single quotes.

  7. 
      
gdehal
Review request changed
Commits:
Summary ID
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4
Add ship-it revoked email template
ffc09ed5e8de08ffbef47a92c0e847d52fdde6ce
Add tests and docstrings
6423a47761c811a6754ea8a244dd389f78e8ea5d
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4
Add ship-it revoked email template
ffc09ed5e8de08ffbef47a92c0e847d52fdde6ce
Add tests and docstrings
6423a47761c811a6754ea8a244dd389f78e8ea5d
Addressed review comments
cb19e9326b5b409dc0e8806df59820f8c970554e

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gdehal
Review request changed
Commits:
Summary ID
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4
Add ship-it revoked email template
ffc09ed5e8de08ffbef47a92c0e847d52fdde6ce
Add tests and docstrings
6423a47761c811a6754ea8a244dd389f78e8ea5d
Addressed review comments
cb19e9326b5b409dc0e8806df59820f8c970554e
Create boolean field in admin page for email
dee2620629820bf3ca6f76b811d29484a4d9ab61
Connect handler to revoke signal
57f846201057f8be07d382e8d5ffdd180edf95d0
Prepare ship-it revoked email message
e3d46591923cf2b30e45228ee53ca7a9b91629f4
Add ship-it revoked email template
ffc09ed5e8de08ffbef47a92c0e847d52fdde6ce
Add tests and docstrings
6423a47761c811a6754ea8a244dd389f78e8ea5d
Addressed review comments
cb19e9326b5b409dc0e8806df59820f8c970554e
Changed label text
3fc6e424712db0a621d8306f0fbe0c41cd6d506c

Checks run (2 succeeded)

flake8 passed.
JSHint passed.