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

Review Request #5828 — Created May 14, 2014 and submitted

Information

Review Board
master
7dc3e56...

Reviewers

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.

  • 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.
Description From Last Updated

Blank line after.

chipx86chipx86

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 …

chipx86chipx86

"list" is a bit vague when looking at the page, since there's the mailing list, lists of users, etc. I …

chipx86chipx86

Is it? I'm still not convinced. I think whether it's highly recommended very much depends on the organization. I'd prefer …

chipx86chipx86
chipx86
  1. 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.

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

      1. Email addresses are already visible in the web UI for almost everyone
      2. As people comment on things, the emails that are sent out accumulate To: email addresses already
      3. 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.

  2. 
      
david
reviewbot
  1. 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:
    
    
  2. 
      
reviewbot
  1. 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:
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 2)
     
     
    Show all issues

    Blank line after.

  3. 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).

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

  4. 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".

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

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

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

  6. 
      
david
reviewbot
  1. 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:
    
    
  2. 
      
reviewbot
  1. 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:
    
    
  2. 
      
chipx86
  1. Ship It!

  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (c3c11cf)