Summary

Cleanup and refactor the e-mail module

Review Request #7560 — Created July 30, 2015 and submitted Aug. 4, 2015, 2:35 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

Blocks

7562

Reviewers

Groups

reviewboard

People

Description

The recipient building portion of send_review_mail has been
refactored out into two methods: build_recipients, which returns sets
of Users and Groups that should receive the e-mail, as well as
recipients_to_addresses, which builds sets of addresses with fewer
queries than before.

The logic of determing the recipients of an e-mail has been moved out
of send_review_mail and the signature of the function has changed to
accept the TO and CC fields as sets or lists of Users and
Groups. The logic has also been corrected to no longer send e-mails
to the following:

users who have starred a review request but are no longer in the
review request's local site;
users in the limit_recipients_to list who are inactive; and
users in the limit_recipients_to list who are no longer part of
the review request's local site.

All methods that call into send_review_mail to send e-mail now must
calculate the addresses they will be sending to. This is to allow for
a future change to allow extensions to filter the TO and CC
fields of e-mails.

A large portion of this change is a style and docstring cleanup to
bring it in line with our style guidelines as well as adding numerous
new unit tests to cover all the behaviour of the two new functions
and ensure it does not regress.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
"through the", here and below.	chipx86	July 30, 2015, 2:24 a.m.
I'd say "site configuration setting".	chipx86	July 30, 2015, 2:24 a.m.
Missing "full name".	chipx86	July 30, 2015, 2:24 a.m.
Missing period.	chipx86	July 30, 2015, 2:25 a.m.
I think to link this right, it'll need to be reviewboard.reviews.models.groups.Group. You'd have to check with a docs build.	chipx86	July 30, 2015, 6:31 p.m.
This is pretty old code. We can just do: if ',' not in group.mailing_list:	chipx86	July 30, 2015, 6:56 p.m.
Blank line between these.	chipx86	July 30, 2015, 6:32 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 30, 2015, 2:24 a.m.
Missing a trailing period.	chipx86	July 30, 2015, 6:32 p.m.
Before, extra_recipients was a list of usernames. Now it's treated as a list of users. Is that correct? If so, …	chipx86	July 30, 2015, 6:49 p.m.
This is checking if the function is present. It needs to be called.	chipx86	July 30, 2015, 6:49 p.m.
I know I provided the code snippet earlier, but I think if we do is_active=True first, it'll let the DB …	chipx86	July 30, 2015, 6:51 p.m.
Same here.	chipx86	July 30, 2015, 6:52 p.m.
This will fail if the , is first, but anyway, it should be: if ',' in recipient.mailing_list:	chipx86	July 30, 2015, 6:52 p.m.
We should specifically check if it's a string, and have an else that logs an error saying it's an unknown …	chipx86	July 30, 2015, 6:54 p.m.
Should be one line.	chipx86	July 30, 2015, 6:54 p.m.
Missing docs.	chipx86	July 30, 2015, 6:55 p.m.
Alignment problem. Also, in this case, you can pass a list or tuple in to issubset(). No need to build …	chipx86	July 30, 2015, 6:55 p.m.
'QuerySet' imported but unused	reviewbot	July 31, 2015, 12:24 a.m.
While we're busy making things more correct, we should replace this with a call to email.utils.formataddr(), which will do correct …	david	July 31, 2015, 4:50 p.m.
formataddr will do the right thing if the full name is falsy.	david	Aug. 4, 2015, 8:43 a.m.
This doesn't really work because these sets can now contain Group objects that users can be members of but we …	EL elatt	Sept. 28, 2015, 10:54 a.m.
Why is the cc_field set not being discarded as well? Also, it is unfortunate that we have to duplicate this …	EL elatt	Sept. 28, 2015, 10:50 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py

reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Wow, big change. Looks like some great performance improvements.

Not that I want to add to your plate, but I'm going to want to see more unit tests covering more combinations of things. All the different conditionals we now have for choosing what ends up in sets should be tested.

Since it's broken up now, these should be easier to test. The new utility functions can be individually tested with every combination, and the send e-mail functions can be tested with the knowledge that those utility functions are working.

E-mail is pretty critical, and any bug will affect nearly every install, so it's just absolutely crucial that nothing slips by us here.

reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"through the", here and below.
```
reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
I'd say "site configuration setting".
```
reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing "full name".
```
reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing period.
```

reviewboard/notifications/email.py (Diff revision 1)

The issue has been dropped. Show all issues

I think to link this right, it'll need to be reviewboard.reviews.models.groups.Group.

You'd have to check with a docs build.

reviewboard/notifications/email.py (Diff revision 1)

The issue has been resolved. Show all issues

This is pretty old code. We can just do:

if ',' not in group.mailing_list:

reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```

reviewboard/notifications/email.py (Diff revision 1)

The issue has been resolved. Show all issues

Before, extra_recipients was a list of usernames. Now it's treated as a list of users. Is that correct?

If so, we should make sure to specify exactly what's expected in the docs above. We should also sanity-check the contents and log a suitable error about migrating the call, in case an extension is already calling this manually (I think there are some out there that do).

reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This is checking if the function is present. It needs to be called.
```

reviewboard/notifications/email.py (Diff revision 1)

The issue has been resolved. Show all issues

I know I provided the code snippet earlier, but I think if we do is_active=True first, it'll let the DB engine check that before checking the joins.

reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/notifications/email.py (Diff revision 1)

The issue has been resolved. Show all issues

This will fail if the , is first, but anyway, it should be:

if ',' in recipient.mailing_list:

reviewboard/notifications/email.py (Diff revision 1)

The issue has been dropped. Show all issues

We should specifically check if it's a string, and have an else that logs an error saying it's an unknown value, showing the repr of the value.

brennie July 30, 2015, 6:54 p.m.

This should only ever be a User or a Group becuase its called from within the email module itself.

reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be one line.
```
reviewboard/notifications/email.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing docs.
```

reviewboard/notifications/email.py (Diff revision 1)

The issue has been resolved. Show all issues

Alignment problem.

Also, in this case, you can pass a list or tuple in to issubset(). No need to build a set of the data.

Change Summary:

Added unit tests. Fixed some bugs.

Description:

		The recipient building portion of `send_review_mail` has been
		refactored out into two methods: `build_recipients`, which returns sets
		of `User`s and `Group`s that should receive the e-mail, as well as
		`recipients_to_addresses`, which builds sets of addresses with fewer
		queries than before.

		The logic of determing the recipients of an e-mail has been moved out
		of `send_review_mail` and the signature of the function has changed to
		accept the `TO` and `CC` fields as sets or lists of `User`s and
~		`Group`s.
	~	`Group`s. The logic has also been corrected to no longer send e-mails
	+	to the following:
	+
	+	users who have starred a review request but are no longer in the review request's local site;
	+	users in the `limit_recipients_to` list who are inactive; and
	+	users in the `limit_recipients_to` list who are no longer part of the review request's local site.

		All methods that call into `send_review_mail` to send e-mail now must
		calculate the addresses they will be sending to. This is to allow for
		a future change to allow extensions to filter the `TO` and `CC`
		fields of e-mails.

		A large portion of this change is a style and docstring cleanup to
~		bring it in line with our style guidelines.
	~	bring it in line with our style guidelines as well as adding numerous
	+	new unit tests to cover all the behaviour of the two new functions
	+	and ensure it does not regress.

Diff:

Revision 2 (+1155 -137)

Show changes

	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py

reviewboard/notifications/email.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'QuerySet' imported but unused
```

Change Summary:

PEP8

Diff:

Revision 3 (+1154 -137)

Show changes

	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py

Change Summary:

Fix docs.

Diff:

Revision 4 (+1154 -137)

Show changes

	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py

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

The issue has been resolved. Show all issues

While we're busy making things more correct, we should replace this with a call to email.utils.formataddr(), which will do correct RFC 2047 encoding.

Diff:

Revision 5 (+1156 -138)

Show changes

	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py

reviewboard/notifications/email.py (Diff revisions 4 - 5)
The issue has been resolved. Show all issues
```
formataddr will do the right thing if the full name is falsy.
```

Diff:

Revision 6 (+1156 -141)

Show changes

	reviewboard/notifications/email.py
	reviewboard/notifications/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/notifications/tests.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (0734e87)

The current code doesn't quite work for users that have should_send_own_updates set to false, and a review is assigned to a group that the user is a member of, and this change is by said user. The problem is that the earlier filtering in build_recipients() can't detect this case b/c it just sees opaque Group objects, then in send_review_mail() these lists of User and Group objects get converted to lists of email addrs and we go and process should_send_own_updates again but for some reason ignore the cc_field list which is where the user's email addr is a member of now.

Who are the clients of build_recipients()? Is it possible for build_recipients() to expand the User and Group objects into email addrs before returning its two sets? If we could, we could completely remove all that extra logic from send_review_mail() and it can just worry about sending email to the exact list of email addrs it was given. Otherwise, the only solution I see is that the should_send_own_updates() logic in build_recipients() get removed and the contract for the method should be updated to explicitly state that it is up to the clients of the method to do that processing (for both the to and cc field sets).

brennie Sept. 28, 2015, 10:54 a.m.

The reason that build_recipients operates the way it does is for e-mail extensions, which allow adding/removing users and groups to e-mails before they are sent. See this review request.

This isn't really the place to file issues with the code now that is landed. Can we keep the discussion to the bug report?

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

The issue has been dropped. Show all issues

This doesn't really work because these sets can now contain Group objects that users can be members of but we don't expand the groups until outside of the method.

brennie Sept. 28, 2015, 10:54 a.m.

And in send_review_mail we will again filter out the user's email...

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

The issue has been dropped. Show all issues

Why is the cc_field set not being discarded as well? Also, it is unfortunate that we have to duplicate this logic here. It would be easier to test if this logic was outside of this method

brennie Sept. 28, 2015, 10:54 a.m.

On line 490:

cc_field = recipeints_to_addresses(cc_field) - to_field

This ensures that any address in to_field is not in cc_field, so the user's e-mail address cannot be in cc_field.