Summary

Refactor e-mail generation and previewing

Review Request #8885 — Created April 6, 2017 and submitted May 22, 2017, 10:39 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

~~8897~~

Reviewers

Groups

reviewboard

People

Description

Previously e-mail generation and previewing used duplicated logic. The
preview_{foo}_email views would duplicate the logic used to generate
the template context that the mail_{foo} methods (that actually built
& sent the e-mails out).

Now, we have utilities for building e-mail messages
(prepare_{foo}_mail) that generate an EMailMessage but do not send
it and a generic e-mail sending & error-handling utility in
send_email. The logic for catching failed e-mails was present in each
method that sent an e-mail and it is now unified in a single place.

Additionally, we have a decorator that generates e-mail preview views
from functions that can generate the necessary arguments to provide to a
prepare_{foo}_email. This way the view only needs to compute the state
necessary, which has resulted in a large amount of deduplication.

Unit tests have been updated to reference the new methods.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
email/decorators.py should be in this diff.	brennie	April 13, 2017, 4:11 p.m.
I don't think you meant to add reviewboard/notifications/original_cbs.py ?	david	April 15, 2017, 1:35 p.m.
'reviewboard.notifications.email.decorators.preview_email' imported but unused	reviewbot	April 12, 2017, 3:08 p.m.
'reviewboard.notifications.email.message.prepare_password_changed_mail' imported but unused	reviewbot	April 12, 2017, 3:08 p.m.
Might be slightly more intuitive as: if not (siteconfig.get('mail_send_review_close_mail') and review_request.public):	david	April 15, 2017, 1:35 p.m.
While here, can you remove the blank line and fix the ordering?	chipx86	April 24, 2017, 7:38 p.m.
"djblets"	chipx86	April 24, 2017, 7:06 p.m.
Let's reverse this check. It's quicker to check a boolean.	chipx86	April 24, 2017, 7:07 p.m.
No ending paren.	chipx86	April 24, 2017, 7:07 p.m.
No ending paren.	chipx86	April 24, 2017, 7:07 p.m.
Let's check trivial first.	chipx86	April 24, 2017, 7:08 p.m.
I think we have a :http: you can use for HTTP codes.	chipx86	April 24, 2017, 7:14 p.m.
This is :django:setting:	chipx86	April 24, 2017, 7:14 p.m.
raise	chipx86	April 24, 2017, 7:14 p.m.
Let's use .update here.	chipx86	April 24, 2017, 7:16 p.m.
We can use build_server_url().	chipx86	April 24, 2017, 7:17 p.m.
If .target_groups is already fetched, this is fine. If not, we should use .values_list(). (If the former, we should document …	chipx86	April 24, 2017, 7:23 p.m.
This won't work. It's missing a second argument. Good indication that we're missing a test condition or tests aren't passing.	chipx86	April 24, 2017, 7:39 p.m.
Can this not be imported higher up?	chipx86	April 24, 2017, 7:25 p.m.
This could be combined.	chipx86	April 24, 2017, 7:31 p.m.
Reads kinda weird. Maybe just "Whether the review should only be sent to the submitter of the review request."	chipx86	April 24, 2017, 7:32 p.m.
Can be inline above in extra_context.	chipx86	April 24, 2017, 7:32 p.m.
Can now use Python 2.7 set syntax.	chipx86	April 24, 2017, 7:32 p.m.
Wrapped weird.	chipx86	April 24, 2017, 7:33 p.m.
Wrapping and paragraph break issues.	chipx86	April 24, 2017, 7:33 p.m.
Let's use .update({...})	chipx86	April 24, 2017, 7:34 p.m.
No need for the parens after for.	chipx86	April 24, 2017, 7:34 p.m.
This can probably be one statement.	chipx86	April 24, 2017, 7:36 p.m.
"updated"	chipx86	April 24, 2017, 7:36 p.m.
Capitalize the beginning of each.	chipx86	April 24, 2017, 7:37 p.m.
Let's put one per line. Helps a little with readability here, given the statement below.	chipx86	April 24, 2017, 7:38 p.m.
I doubt you want these. We should have a unit test covering this case.	chipx86	April 24, 2017, 7:38 p.m.
2 blank lines.	chipx86	April 24, 2017, 7:38 p.m.
Blank line should remain.	chipx86	April 24, 2017, 7:38 p.m.
Trailing comma.	chipx86	April 24, 2017, 7:38 p.m.
local variable 'e' is assigned to but never used	reviewbot	April 27, 2017, 4:18 p.m.
Missing period.	chipx86	May 22, 2017, 10:30 p.m.
E501 line too long (81 > 79 characters)	reviewbot	May 22, 2017, 10:32 p.m.
E501 line too long (81 > 79 characters)	reviewbot	May 22, 2017, 10:32 p.m.
E501 line too long (81 > 79 characters)	reviewbot	May 22, 2017, 10:32 p.m.
E501 line too long (81 > 79 characters)	reviewbot	May 22, 2017, 10:32 p.m.
E501 line too long (81 > 79 characters)	reviewbot	May 22, 2017, 10:32 p.m.
E501 line too long (80 > 79 characters)	reviewbot	May 22, 2017, 10:32 p.m.

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Description:

		Previously e-mail generation and previewing used duplicated logic. The
		`preview_{foo}_email` views would duplicate the logic used to generate
		the template context that the `mail_{foo}` methods (that actually built
		& sent the e-mails out).

		Now, we have utilities for building e-mail messages
		(`prepare_{foo}_mail`) that generate an `EMailMessage` but do not send
		it and a generic e-mail sending & error-handling utility in
		`send_email`. The logic for catching failed e-mails was present in each
		method that sent an e-mail and it is now unified in a single place.

		Additionally, we have a decorator that generates e-mail preview views
		from functions that can generate the necessary arguments to provide to a
		`prepare_{foo}_email`. This way the view only needs to compute the state
		necessary, which has resulted in a large amount of deduplication.

	+	While I was here I also updated the `ReviewPublishedEmailHook` to take
	+	the `to_submitter_only` keyword argument, which it was missing before.
	+
		Unit tests have been updated to reference the new methods.

Description:

		Previously e-mail generation and previewing used duplicated logic. The
		`preview_{foo}_email` views would duplicate the logic used to generate
		the template context that the `mail_{foo}` methods (that actually built
		& sent the e-mails out).

		Now, we have utilities for building e-mail messages
		(`prepare_{foo}_mail`) that generate an `EMailMessage` but do not send
		it and a generic e-mail sending & error-handling utility in
		`send_email`. The logic for catching failed e-mails was present in each
		method that sent an e-mail and it is now unified in a single place.

		Additionally, we have a decorator that generates e-mail preview views
		from functions that can generate the necessary arguments to provide to a
		`prepare_{foo}_email`. This way the view only needs to compute the state
		necessary, which has resulted in a large amount of deduplication.

		While I was here I also updated the `ReviewPublishedEmailHook` to take
~		the `to_submitter_only` keyword argument, which it was missing before.
	~	the `to_submitter_only` keyword argument, which it was missing before. I
	+	also replaced all the usage of
	+	`{{site_domain_method}}://{{site_domain}}` in e-mail templates with the
	+	result of `get_server_url`.

		Unit tests have been updated to reference the new methods.

Depends On:

+	~~8889 - Prefer get_server_url() in notifications templates~~

Diff:

Revision 2 (+954 -826)

Show changes

	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/extensions/hooks.py
	reviewboard/extensions/tests.py
	reviewboard/notifications/original_cbs.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/callbacks.py
	8 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Change Summary:

Update description wrt to refactoring /r/8889 out.

Description:

		Previously e-mail generation and previewing used duplicated logic. The
		`preview_{foo}_email` views would duplicate the logic used to generate
		the template context that the `mail_{foo}` methods (that actually built
		& sent the e-mails out).

		Now, we have utilities for building e-mail messages
		(`prepare_{foo}_mail`) that generate an `EMailMessage` but do not send
		it and a generic e-mail sending & error-handling utility in
		`send_email`. The logic for catching failed e-mails was present in each
		method that sent an e-mail and it is now unified in a single place.

		Additionally, we have a decorator that generates e-mail preview views
		from functions that can generate the necessary arguments to provide to a
		`prepare_{foo}_email`. This way the view only needs to compute the state
		necessary, which has resulted in a large amount of deduplication.

-		While I was here I also updated the `ReviewPublishedEmailHook` to take
-		the `to_submitter_only` keyword argument, which it was missing before. I
-		also replaced all the usage of
-		`{{site_domain_method}}://{{site_domain}}` in e-mail templates with the
-		result of `get_server_url`.
-
		Unit tests have been updated to reference the new methods.

Depends On:

-	~~8829 - Refactor notifications.email into a module~~

Change Summary:

Refactored many changes out of this one.

Depends On:

-	~~8889 - Prefer get_server_url() in notifications templates~~
+	~~8897 - Refactor e-mail information saving into a single location~~

Diff:

Revision 3 (+816 -802)

Show changes

	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/original_cbs.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/callbacks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	1 more

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes failed.

Pyflakes

reviewboard/accounts/views.py (Diff revision 3)

The issue has been resolved. Show all issues

 'reviewboard.notifications.email.decorators.preview_email' imported but unused

reviewboard/accounts/views.py (Diff revision 3)

The issue has been resolved. Show all issues

 'reviewboard.notifications.email.message.prepare_password_changed_mail' imported but unused

Diff:

Revision 4 (+816 -802)

Show changes

	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/original_cbs.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/callbacks.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	1 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

The issue has been resolved. Show all issues

email/decorators.py should be in this diff.

Diff:

Revision 5 (+890 -802)

Show changes

	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/original_cbs.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/callbacks.py
	reviewboard/notifications/email/decorators.py
	reviewboard/notifications/email/message.py
	2 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Diff:

Revision 6 (+890 -803)

Show changes

	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/original_cbs.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/callbacks.py
	reviewboard/notifications/email/decorators.py
	reviewboard/notifications/email/message.py
	2 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

The issue has been resolved. Show all issues

I don't think you meant to add reviewboard/notifications/original_cbs.py ?

reviewboard/notifications/email/callbacks.py (Diff revision 6)

The issue has been resolved. Show all issues

Might be slightly more intuitive as:

if not (siteconfig.get('mail_send_review_close_mail') and
        review_request.public):

Change Summary:

Addressed David's issues.

Diff:

Revision 7 (+890 -803)

Show changes

	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/callbacks.py
	reviewboard/notifications/email/decorators.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	1 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Ship it!

```
Ship It!
```

Change Summary:

Force UTF8 charset in content type.

Diff:

Revision 8 (+891 -803)

Show changes

	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/callbacks.py
	reviewboard/notifications/email/decorators.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/utils.py
	1 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Loving the e-mail changes :) Fantastic cleanup.

reviewboard/notifications/email/__init__.py (Diff revision 8)

The issue has been dropped. Show all issues

While here, can you remove the blank line and fix the ordering?

brennie April 24, 2017, 7:38 p.m.

I think this was already fixed but didnt show up in the diff? :/ Its not in my local copy.

reviewboard/notifications/email/callbacks.py (Diff revision 8)
The issue has been resolved. Show all issues
```
"djblets"
```
reviewboard/notifications/email/callbacks.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Let's reverse this check. It's quicker to check a boolean.
```
reviewboard/notifications/email/callbacks.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No ending paren.
```
reviewboard/notifications/email/callbacks.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No ending paren.
```
reviewboard/notifications/email/callbacks.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Let's check trivial first.
```
reviewboard/notifications/email/decorators.py (Diff revision 8)
The issue has been resolved. Show all issues
```
I think we have a :http: you can use for HTTP codes.
```
reviewboard/notifications/email/decorators.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This is :django:setting:
```
reviewboard/notifications/email/decorators.py (Diff revision 8)
The issue has been resolved. Show all issues
```
raise
```
reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Let's use .update here.
```
reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
We can use build_server_url().
```

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

The issue has been resolved. Show all issues

If .target_groups is already fetched, this is fine. If not, we should use .values_list().
(If the former, we should document that.)

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

The issue has been resolved. Show all issues

This won't work. It's missing a second argument.

Good indication that we're missing a test condition or tests aren't passing.

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

The issue has been dropped. Show all issues

Can this not be imported higher up?

brennie April 24, 2017, 7:38 p.m.

Nope, ciruclar import. reviewboard.reviews.views imports email.messages.prepare_{} funcs.

reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been dropped. Show all issues
```
This could be combined.
```
1. brennie April 24, 2017, 7:38 p.m.
  No they cannot. This requires extra_context.

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

The issue has been resolved. Show all issues

Reads kinda weird. Maybe just "Whether the review should only be sent to the submitter of the review request."

reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been dropped. Show all issues
```
Can be inline above in extra_context.
```
1. brennie April 24, 2017, 7:38 p.m.
  Nope. Requires extra_context to build it.
reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Can now use Python 2.7 set syntax.
```
reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Wrapped weird.
```

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

The issue has been resolved. Show all issues

Wrapping and paragraph break issues.

brennie April 24, 2017, 7:38 p.m.

Reads fine to me, I don't know what you're talking about :P

reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Let's use .update({...})
```
reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No need for the parens after for.
```
reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This can probably be one statement.
```
reviewboard/notifications/email/message.py (Diff revision 8)
The issue has been resolved. Show all issues
```
"updated"
```
reviewboard/notifications/email/utils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Capitalize the beginning of each.
```
reviewboard/notifications/email/utils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Let's put one per line. Helps a little with readability here, given the statement below.
```
reviewboard/notifications/email/utils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
I doubt you want these. We should have a unit test covering this case.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
2 blank lines.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Blank line should remain.
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Trailing comma.
```

Change Summary:

Addressed Christian's issues.

Diff:

Revision 9 (+891 -799)

Show changes

	docs/manual/extending/coderef/index.rst
	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/decorators.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py
	2 more

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes failed.

Pyflakes

reviewboard/notifications/email/utils.py (Diff revision 9)
The issue has been resolved. Show all issues
```
 local variable 'e' is assigned to but never used
```

Change Summary:

flake8

Diff:

Revision 10 (+891 -799)

Show changes

	docs/manual/extending/coderef/index.rst
	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/decorators.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py
	2 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Ship it!

```
Ship It!
```

Ship it!

```
Great cleanup :)
```
reviewboard/notifications/email/decorators.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Missing period.
```

Diff:

Revision 11 (+891 -799)

Show changes

	docs/manual/extending/coderef/index.rst
	reviewboard/accounts/views.py
	reviewboard/accounts/forms/pages.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/email/__init__.py
	reviewboard/notifications/email/decorators.py
	reviewboard/notifications/email/message.py
	reviewboard/notifications/email/signal_handlers.py
	2 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/email/message.py (Diff revision 11)
The issue has been dropped. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/notifications/email/message.py (Diff revision 11)
The issue has been dropped. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/notifications/email/message.py (Diff revision 11)
The issue has been dropped. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 11)
The issue has been dropped. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 11)
The issue has been dropped. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/notifications/email/signal_handlers.py (Diff revision 11)
The issue has been dropped. Show all issues
```
E501 line too long (80 > 79 characters)
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (4b6a38f)