Summary

Notify for ship-it revocations

Review Request #12045 — Created Feb. 4, 2022 and updated April 7, 2022, 6:40 p.m.

Information

Owner

gdehal

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description


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

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
Add tests and docstrings	6423a47761c811a6754ea8a244dd389f78e8ea5d
Addressed review comments	cb19e9326b5b409dc0e8806df59820f8c970554e
Changed label text	3fc6e424712db0a621d8306f0fbe0c41cd6d506c

Issues

Description	From	Last Updated
F401 'importlib' imported but unused	reviewbot	Feb. 12, 2022, 1:54 p.m.
F401 'django.utils.six' imported but unused	reviewbot	Feb. 12, 2022, 1:54 p.m.
E501 line too long (87 > 79 characters)	reviewbot	Feb. 26, 2022, 12:28 a.m.
E302 expected 2 blank lines, found 1	reviewbot	Feb. 26, 2022, 12:28 a.m.
E302 expected 2 blank lines, found 1	reviewbot	Feb. 26, 2022, 12:28 a.m.
W291 trailing whitespace	reviewbot	March 5, 2022, 12:11 a.m.
F401 'reviewboard.reviews.views.build_diff_comment_fragments' imported but unused	reviewbot	March 5, 2022, 12:11 a.m.
E128 continuation line under-indented for visual indent	reviewbot	March 5, 2022, 12:11 a.m.
This should probably use the "Re:" logic from prepare_review_request_mail	david	March 19, 2022, 12:12 a.m.
F401 'reviewboard.reviews.models.review_request' imported but unused	reviewbot	March 5, 2022, 12:11 a.m.
E231 missing whitespace after ','	reviewbot	March 5, 2022, 12:11 a.m.
F811 redefinition of unused 'review_request' from line 17	reviewbot	March 5, 2022, 12:11 a.m.
Would it be clearer to write the label as "Send e-mails to users when the reviews' ship-it status are revoked"?	sheenaNg	April 7, 2022, 5:22 p.m.
It looks good! One suggestion, could you include docstring for this method similar to other methods in this module?	sheenaNg	April 7, 2022, 5:21 p.m.
Could you also include docstring for this method?	sheenaNg	April 7, 2022, 5:22 p.m.
Is this supposed to be review.REVOKED_SHIP_IT_TEXT?	sheenaNg	April 7, 2022, 6:36 p.m.
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	April 7, 2022, 6:35 p.m.
Please add a class docstring.	david	April 7, 2022, 6:27 p.m.
The description needs to go on the next line.	david	April 7, 2022, 6:27 p.m.
The description needs to go on the next line.	david	April 7, 2022, 6:27 p.m.
E501 line too long (81 > 79 characters)	reviewbot	April 7, 2022, 6:36 p.m.
Description for the return type doesn't need to be indented (align with EmailMessage)	david	April 7, 2022, 6:27 p.m.
This needs a trailing comma. Please also use single quotes.	david	April 7, 2022, 6:27 p.m.
E501 line too long (81 > 79 characters)	reviewbot	April 7, 2022, 6:36 p.m.
E501 line too long (80 > 79 characters)	reviewbot	April 7, 2022, 6:36 p.m.
E501 line too long (81 > 79 characters)	reviewbot	April 7, 2022, 6:40 p.m.

flake8 passed.

JSHint passed.

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

Diff:

Revision 2 (+2432 -1050)

Show changes

	AUTHORS
	conftest.py
	dev-requirements.txt
	setup.py
	setup.cfg
	contrib/internal/prepare-dev.py
	contrib/internal/webapi-lint.py
	docs/manual/admin/sites/search-indexing.rst
	761 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/__init__.py (Diff revision 2)
Show all issues
```
F401 'importlib' imported but unused
```
reviewboard/accounts/backends/ldap.py (Diff revision 2)
Show all issues
```
F401 'django.utils.six' imported but unused
```

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

Diff:

Revision 3 (+140 -6)

Show changes

	reviewboard/admin/forms/email_settings.py
	reviewboard/admin/tests/test_email_settings_form.py
	reviewboard/extensions/hooks.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/signal_handlers.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/extensions/hooks.py (Diff revision 3)
Show all issues
```
E501 line too long (87 > 79 characters)
```
reviewboard/extensions/hooks.py (Diff revision 3)
Show all issues
```
E302 expected 2 blank lines, found 1
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 3)
Show all issues
```
E302 expected 2 blank lines, found 1
```

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

Diff:

Revision 4 (+265 -13)

Show changes

	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/email_settings.py
	reviewboard/admin/tests/test_email_settings_form.py
	reviewboard/extensions/hooks.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/extensions/hooks.py (Diff revision 4)
Show all issues
```
W291 trailing whitespace
```

reviewboard/notifications/email/message.py (Diff revision 4)

Show all issues

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

reviewboard/notifications/email/message.py (Diff revision 4)
Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 4)
Show all issues
```
F401 'reviewboard.reviews.models.review_request' imported but unused
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 4)
Show all issues
```
E231 missing whitespace after ','
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 4)
Show all issues
```
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)

Show all issues

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

Description:

		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

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
	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

Diff:

Revision 5 (+567 -17)

Show changes

	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/email_settings.py
	reviewboard/admin/tests/test_email_settings_form.py
	reviewboard/extensions/hooks.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py
	reviewboard/templates/notifications/review_ship_it_revoked_email.txt
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	[WIP] Notify for ship-it revocations
+	Notify for ship-it revocations

reviewboard/admin/forms/email_settings.py (Diff revision 5)

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"?

reviewboard/notifications/email/message.py (Diff revision 5)

Show all issues

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)
Show all issues
```
Could you also include docstring for this method?
```

reviewboard/notifications/email/signal_handlers.py (Diff revision 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?

gdehal April 7, 2022, 6:35 p.m.

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.

reviewboard/templates/notifications/review_ship_it_revoked_email.html (Diff revision 5)

Show all issues

Is this supposed to be review.REVOKED_SHIP_IT_TEXT?

gdehal April 7, 2022, 6:35 p.m.

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

reviewboard/templates/notifications/review_ship_it_revoked_email.html (Diff revision 5)

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?

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

Diff:

Revision 6 (+687 -17)

Show changes

	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/email_settings.py
	reviewboard/admin/tests/test_email_settings_form.py
	reviewboard/extensions/hooks.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py
	reviewboard/notifications/tests/test_email_sending.py
	2 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/email/message.py (Diff revision 6)
Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 6)
Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/notifications/tests/test_email_sending.py (Diff revision 6)
Show all issues
```
E501 line too long (80 > 79 characters)
```

reviewboard/extensions/hooks.py (Diff revision 6)
Show all issues
```
Please add a class docstring.
```
reviewboard/extensions/hooks.py (Diff revision 6)
Show all issues
```
The description needs to go on the next line.
```
reviewboard/extensions/hooks.py (Diff revision 6)
Show all issues
```
The description needs to go on the next line.
```

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)

reviewboard/notifications/email/message.py (Diff revision 6)
Show all issues
```
This needs a trailing comma. Please also use single quotes.
```

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

Diff:

Revision 7 (+736 -26)

Show changes

	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/email_settings.py
	reviewboard/admin/tests/test_email_settings_form.py
	reviewboard/extensions/hooks.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py
	reviewboard/notifications/tests/test_email_sending.py
	2 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/admin/forms/email_settings.py (Diff revision 7)
Show all issues
```
E501 line too long (81 > 79 characters)
```

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

Diff:

Revision 8 (+737 -27)

Show changes

	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/email_settings.py
	reviewboard/admin/tests/test_email_settings_form.py
	reviewboard/extensions/hooks.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py
	reviewboard/notifications/tests/test_email_sending.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.