Summary

Send emails to both users and mailing lists for review groups.

Review Request #5828 — Created May 14, 2014 and submitted May 28, 2014, 12:17 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

7dc3e56...

Reviewers

Groups

reviewboard

People

Description

When we initially created review groups, we made the assumption that all groups
would have an associated mailing list. This made a lot of sense when
transitioning from an email based code review system. Shortly after, we made
it so that any review group that didn't have a mailing list would instead send
email directly to all the members. This creates a confusing situation whereby
users are expected to subscribe to mailing lists if they're present but
otherwise get emails.

This change adds an option to review groups to specify whether emails should go
only to the list, or to the list plus every subscriber. The default for this is
to keep the current behavior.

Testing Done


Tested evolution.
Verified that with this check-box off, the recipient list for a review

  request assigned to a group with a mailing list contained both the mailing

  list and the group members.

Issues

Description	From	Last Updated
Blank line after.	chipx86	May 27, 2014, 5:38 p.m.
Can we change this to not g.mailing_list or not g.email_list_only? I find it a lot more readable (no mental parsing …	chipx86	May 27, 2014, 5:34 p.m.
"list" is a bit vague when looking at the page, since there's the mailing list, lists of users, etc. I …	chipx86	May 27, 2014, 5:38 p.m.
Is it? I'm still not convinced. I think whether it's highly recommended very much depends on the organization. I'd prefer …	chipx86	May 27, 2014, 5:47 p.m.

I don't think we should do this by default. It's a security leak.

This makes it really easy for any user to find out the e-mail addresses of all other users on a group, which may be substantial.

I'd much rather we have some opt-in checkbox for this. We've only received the one request for it, and so I feel it's more of a niche requirement, and not one we should ever default to, given the security issues.

david May 15, 2014, 2:57 p.m.

Well, I don't really think that it's a security leak for several reasons:

Email addresses are already visible in the web UI for almost everyone
As people comment on things, the emails that are sent out accumulate To: email addresses already
Review Board is almost always used in a corporate environment where email addresses aren't secret

That said, I do agree that it should be a check-box, because some review groups may be very very large. I still don't know how to communicate to users that they need to subscribe to a mailing list in order to get emails.

Change Summary:

Make it optional.

Description:

		When we initially created review groups, we made the assumption that all groups
		would have an associated mailing list. This made a lot of sense when
		transitioning from an email based code review system. Shortly after, we made
		it so that any review group that didn't have a mailing list would instead send
		email directly to all the members. This creates a confusing situation whereby
		users are expected to subscribe to mailing lists if they're present but
		otherwise get emails.

~		Because email clients are pretty darned good at de-duping when lists are
~		involved, and users can already get duplicates when they're directly assigned
~		to something and/or are the sender, I'm changing it from OR to AND. This means
	~	This change adds an option to review groups to specify whether emails should go
	~	only to the list, or to the list plus every subscriber. The default for this is
	~	to keep the current behavior.
-		that users can go around subscribing to groups, and they don't have to
-		magically know whether or not there's a mailing list.

Testing Done:

~		Verified that the recipient list for a review request assigned to a group
~		with a mailing list contained both the mailing list and the group members.
	~	Tested evolution.
	~	Verified that with this check-box off, the recipient list for a review request assigned to a group with a mailing list contained both the mailing list and the group members.

Branch:

-	release-1.7.x
+	master

Commit:

-	7df0cce75a144551117ad39846e8897fce5af6aa
+	5a74fc498f9fb8e916d8c5d9315b7ca3f797c408

Diff:

Revision 2 (+30 -9)

Show changes

	reviewboard/notifications/email.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/group_email_list_only.py
	reviewboard/reviews/models/group.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/notifications/email.py
    reviewboard/reviews/models/group.py
    reviewboard/reviews/evolutions/group_email_list_only.py
    reviewboard/reviews/evolutions/__init__.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/notifications/email.py
    reviewboard/reviews/models/group.py
    reviewboard/reviews/evolutions/group_email_list_only.py
    reviewboard/reviews/evolutions/__init__.py
  Ignored Files:

reviewboard/notifications/email.py (Diff revision 2)
Show all issues
```
Blank line after.
```

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

Show all issues

Can we change this to not g.mailing_list or not g.email_list_only? I find it a lot more readable (no mental parsing required).

david May 27, 2014, 5:38 p.m.

I completely disagree in this case. email_list_only only matters if mailing_list is present. The or form makes it seem like the two are unrelated.

reviewboard/reviews/models/group.py (Diff revision 2)

Show all issues

"list" is a bit vague when looking at the page, since there's the mailing list, lists of users, etc. I know there's help text, but I think the label can be better.

How about "send e-mail only to the mailing list"?

I also want to be consistent with "email" vs. "e-mail".

reviewboard/reviews/models/group.py (Diff revision 2)

Show all issues

Is it? I'm still not convinced. I think whether it's highly recommended very much depends on the organization. I'd prefer we just nuke that part of the sentence.

david May 27, 2014, 5:38 p.m.

I think it is. Large groups opting to send mail to everyone will result in very long lists of addressees. Remember what it was like when some person who didn't know how to use email would include thousands of email addresses on a message? Some clients do okay with it, but a lot force you to scroll down past all the addressees before you get to the message.

chipx86 May 27, 2014, 5:47 p.m.

I read this backwards, and was thinking we were recommending turning it off. Brain no worky.

Commit:

-	5a74fc498f9fb8e916d8c5d9315b7ca3f797c408
+	7dc3e5674e751a9d70ee5866dbd198620724f7d0

Diff:

Revision 3 (+31 -9)

Show changes

	reviewboard/notifications/email.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/group_email_list_only.py
	reviewboard/reviews/models/group.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/notifications/email.py
    reviewboard/reviews/models/group.py
    reviewboard/reviews/evolutions/group_email_list_only.py
    reviewboard/reviews/evolutions/__init__.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/notifications/email.py
    reviewboard/reviews/models/group.py
    reviewboard/reviews/evolutions/group_email_list_only.py
    reviewboard/reviews/evolutions/__init__.py
  Ignored Files:

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (c3c11cf)