Bug 73665 - migrate email preferences to their own table, and rearchitect email internals. Patch by gerv; r=jake, a=justdave.

1 files changed, 196 insertions, 465 deletions

diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index ba7c641df..fef959087 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -25,6 +25,7 @@
 #                 Matthew Tuck <matty@chariot.net.au>
 #                 Bradley Baetz <bbaetz@student.usyd.edu.au>
 #                 J. Paul Reed <preed@sigkill.com>
+#                 Gervase Markham <gerv@gerv.net>
 
 use strict;
 
@@ -35,22 +36,25 @@ use base qw(Exporter);
     PerformSubsts
 );
 
+use Bugzilla::Constants;
 use Bugzilla::Config qw(:DEFAULT $datadir);
 use Bugzilla::Util;
 
 use Mail::Mailer;
 use Mail::Header;
 
-# This code is really ugly. It was a commandline interface, then it was moved
-# There are package-global variables which we rely on ProcessOneBug to clean
-# up each time, and other sorts of fun.
+# We need these strings for the X-Bugzilla-Reasons header
+# Note: this hash uses "," rather than "=>" to avoid auto-quoting of the LHS.
+my %rel_names = (REL_ASSIGNEE          , "AssignedTo", 
+                 REL_REPORTER          , "Reporter",
+                 REL_QA                , "QAcontact",
+                 REL_CC                , "CC",
+                 REL_VOTER             , "Voter");
+
+# This code is really ugly. It was a commandline interface, then it was moved.
 # This really needs to be cleaned at some point.
 
-my $nametoexclude = "";
 my %nomail;
-my $last_changed;
-
-my @excludedAddresses = ();
 
 my $sitespec = '@'.Param('urlbase');
 $sitespec =~ s/:\/\//\./; # Make the protocol look like part of the domain
@@ -59,11 +63,6 @@ if ($2) {
     $sitespec = "-$2$sitespec"; # Put the port number back in, before the '@'
 }
 
-my %force;
-
-my %seen;
-my @sentlist;
-
 # I got sick of adding &:: to everything.
 # However, 'Yuck!'
 # I can't require, cause that pulls it in only once, so it won't then be
@@ -86,7 +85,6 @@ if (open(NOMAIL, '<', "$datadir/nomail")) {
     close(NOMAIL);
 }
 
-
 sub FormatTriple {
     my ($a, $b, $c) = (@_);
     $^A = "";
@@ -117,35 +115,19 @@ END
 # roles when the email is sent.
 # All the names are email addresses, not userids
 # values are scalars, except for cc, which is a list
+# This hash usually comes from the "mailrecipients" var in a template call.
 sub Send($;$) {
-    my ($id, $recipients) = (@_);
+    my ($id, $forced) = (@_);
 
     # This only works in a sub. Probably something to do with the
     # require abuse we do.
     GetVersionTable();
 
-    # Make sure to clean up _all_ package vars here. Yuck...
-    $nametoexclude = $recipients->{'changer'} || "";
-    @{$force{'CClist'}} = (exists $recipients->{'cc'} && 
-     scalar($recipients->{'cc'}) > 0) ? map(trim($_), 
-     @{$recipients->{'cc'}}) : ();
-    @{$force{'Owner'}} = $recipients->{'owner'} ? 
-     (trim($recipients->{'owner'})) : ();
-    @{$force{'QAcontact'}} = $recipients->{'qacontact'} ? 
-     (trim($recipients->{'qacontact'})) : ();
-    @{$force{'Reporter'}} = $recipients->{'reporter'} ? 
-     (trim($recipients->{'reporter'})) : ();
-    @{$force{'Voter'}} = ();
-
-    %seen = ();
-    @excludedAddresses = ();
-    @sentlist = ();
-
-    return ProcessOneBug($id);
+    return ProcessOneBug($id, $forced);
 }
 
-sub ProcessOneBug($) {
-    my ($id) = (@_);
+sub ProcessOneBug($$) {
+    my ($id, $forced) = (@_);
 
     my @headerlist;
     my %values;
@@ -154,6 +136,8 @@ sub ProcessOneBug($) {
 
     my $msg = "";
 
+    my $dbh = Bugzilla->dbh;
+     
     SendSQL("SELECT name, description, mailhead FROM fielddefs " .
             "ORDER BY sortkey");
     while (MoreSQLData()) {
@@ -173,21 +157,33 @@ sub ProcessOneBug($) {
 
     my ($start, $end) = (@row);
 
-    my $cc_ref = Bugzilla->dbh->selectcol_arrayref(
-        q{SELECT profiles.login_name FROM cc, profiles
-           WHERE bug_id = ?
-             AND cc.who = profiles.userid
-        ORDER BY profiles.login_name},
-        undef, $id);
-    $values{'cc'} = join(',', @$cc_ref); 
+    # User IDs of people in various roles. More than one person can 'have' a 
+    # role, if the person in that role has changed, or people are watching.
+    my $reporter = $values{'reporter'};
+    my @assignees = ($values{'assigned_to'});
+    my @qa_contacts = ($values{'qa_contact'});
+    my @ccs = @{$dbh->selectcol_arrayref("SELECT who 
+                                          FROM cc WHERE bug_id = $id")};
+
+    # Include the people passed in as being in particular roles.
+    # This can include people who used to hold those roles.
+    # At this point, we don't care if there are duplicates in these arrays.
+    my $changer = $forced->{'changer'};
+    if ($forced->{'owner'}) {
+        push (@assignees, DBNameToIdAndCheck($forced->{'owner'}));
+    }
     
-    my @voterList;
-    SendSQL("SELECT profiles.login_name FROM votes, profiles " .
-            "WHERE votes.bug_id = $id AND profiles.userid = votes.who");
-    while (MoreSQLData()) {
-        push(@voterList, FetchOneColumn());
+    if ($forced->{'qacontact'}) {
+        push (@qa_contacts, DBNameToIdAndCheck($forced->{'qacontact'}));
     }
-
+    
+    if ($forced->{'cc'}) {
+        foreach my $cc (@{$forced->{'cc'}}) {
+            push(@ccs, DBNameToIdAndCheck($cc));
+        }
+    }
+    
+    # Convert to names, for later display
     $values{'assigned_to'} = DBID_to_name($values{'assigned_to'});
     $values{'reporter'} = DBID_to_name($values{'reporter'});
     if ($values{'qa_contact'}) {
@@ -266,7 +262,6 @@ sub ProcessOneBug($) {
         push(@diffparts, $diffpart);
     }
 
-
     my $deptext = "";
 
     SendSQL("SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name, " .
@@ -326,425 +321,170 @@ sub ProcessOneBug($) {
 
     my ($newcomments, $anyprivate) = GetLongDescriptionAsText($id, $start, $end);
 
-    #
+    ###########################################################################
     # Start of email filtering code
-    #
-    my $count = 0;
-
-    # Get a list of the reasons a user might receive email about the bug.
-    my @currentEmailAttributes = 
-      getEmailAttributes(\%values, \@diffs, $newcomments);
+    ###########################################################################
     
-    my (@assigned_toList,@reporterList,@qa_contactList,@ccList) = ();
-
-    #open(LOG, ">>/tmp/maillog");
-    #print LOG "\nBug ID: $id   CurrentEmailAttributes:";
-    #print LOG join(',', @currentEmailAttributes) . "\n";
-
-    @excludedAddresses = (); # zero out global list 
-
-    @assigned_toList = filterEmailGroup('Owner',
-                                        \@currentEmailAttributes,
-                                        $values{'assigned_to'});
-    @reporterList = filterEmailGroup('Reporter', 
-                                     \@currentEmailAttributes,
-                                     $values{'reporter'});
-    if (Param('useqacontact') && $values{'qa_contact'}) {
-        @qa_contactList = filterEmailGroup('QAcontact',
-                                           \@currentEmailAttributes,
-                                           $values{'qa_contact'});
-    } else { 
-        @qa_contactList = (); 
-    }
-
-    @ccList = filterEmailGroup('CClist', \@currentEmailAttributes,
-                               $values{'cc'});
-
-    @voterList = filterEmailGroup('Voter', \@currentEmailAttributes,
-                                  join(',',@voterList));
-
-    my @emailList = (@assigned_toList, @reporterList, 
-                     @qa_contactList, @ccList, @voterList);
-
-    # only need one entry per person
-    my @allEmail = ();
-    my %AlreadySeen = ();
-    my $checkperson = "";
-    foreach my $person (@emailList) {
-        # don't modify the original so it sends out with the right case
-        # based on who came first.
-        $checkperson = lc($person);
-        if ( !($AlreadySeen{$checkperson}) ) {
-            push(@allEmail,$person);
-            $AlreadySeen{$checkperson}++;
-        }
-    }
-
-    #print LOG "\nbug $id email sent: " . join(',', @allEmail) . "\n";
+    # A user_id => roles hash to keep track of people.
+    my %recipients;
+    
+    # Now we work out all the people involved with this bug, and note all of
+    # the relationships in a hash. The keys are userids, the values are an
+    # array of role constants.
+    
+    # Voters
+    my $voters = 
+          $dbh->selectcol_arrayref("SELECT who FROM votes WHERE bug_id = $id");
         
-    @excludedAddresses = filterExcludeList(\@excludedAddresses,
-                                           \@allEmail);
-
-    # print LOG "excluded: " . join(',',@excludedAddresses) . "\n\n";
-
-    foreach my $person ( @allEmail ) {
-        my @reasons;
-
-        $count++;
-
-        push(@reasons, 'AssignedTo') if lsearch(\@assigned_toList, $person) != -1;
-        push(@reasons, 'Reporter') if lsearch(\@reporterList, $person) != -1;
-        push(@reasons, 'QAcontact') if lsearch(\@qa_contactList, $person) != -1;
-        push(@reasons, 'CC') if lsearch(\@ccList, $person) != -1;
-        push(@reasons, 'Voter') if lsearch(\@voterList, $person) != -1;
-
-        if ( !defined(NewProcessOnePerson($person, $count, \@headerlist,
-                                          \@reasons, \%values,
-                                          \%defmailhead, 
-                                          \%fielddescription, \@diffparts,
-                                          $newcomments, 
-                                          $anyprivate, $start, $id, 
-                                          \@depbugs))) 
-        {
-
-            # if a value is not returned, this means that the person
-            # was not sent mail.  add them to the excludedAddresses list.
-            # it will be filtered later for dups.
-            #
-            push @excludedAddresses, $person;
+    push(@{$recipients{$_}}, REL_VOTER) foreach (@$voters);
 
+    # CCs
+    push(@{$recipients{$_}}, REL_CC) foreach (@ccs);
+    
+    # Reporter (there's only ever one)
+    push(@{$recipients{$reporter}}, REL_REPORTER);
+    
+    # QA Contact
+    if (Param('useqacontact')) {
+        foreach (@qa_contacts) {
+            # QA Contact can be blank; ignore it if so.
+            push(@{$recipients{$_}}, REL_QA) if $_;
         }
     }
 
+    # Assignee
+    push(@{$recipients{$_}}, REL_ASSIGNEE) foreach (@assignees);
 
-    SendSQL("UPDATE bugs SET lastdiffed = '$end' WHERE bug_id = $id");
-
-    # Filter the exclude list for dupes one last time
-    @excludedAddresses = filterExcludeList(\@excludedAddresses,
-                                           \@sentlist);
-
-    return { sent => \@sentlist, excluded => \@excludedAddresses };
-}
-
-# When one person is in different fields on one bug, they may be
-# excluded from email because of one relationship to the bug (eg
-# they're the QA contact) but included because of another (eg they
-# also reported the bug).  Inclusion takes precedence, so this
-# function looks for and removes any users from the exclude list who
-# are also on the include list.  Additionally, it removes duplicate
-# entries from the exclude list.  
-#
-# Arguments are the exclude list and the include list; the cleaned up
-# exclude list is returned.
-#
-sub filterExcludeList ($$) {
-
-    if ($#_ != 1) {
-        die ("filterExcludeList called with wrong number of args");
-    }
-
-    my ($refExcluded, $refAll) = @_;
-
-    my @excludedAddrs = @$refExcluded;
-    my @allEmail = @$refAll;
-    my @tmpList = @excludedAddrs;
-    my (@result,@uniqueResult) = ();
-    my %alreadySeen;
-
-    foreach my $excluded (@tmpList) {
-
-        push (@result,$excluded);
-        foreach my $included (@allEmail) {
-
-            # match found, so we remove the entry
-            if (lc($included) eq lc($excluded)) {
-                pop(@result);
-                last;
+    # The last relevant set of people are those who are being removed from 
+    # their roles in this change. We get their names out of the diffs.
+    foreach my $ref (@diffs) {
+        my ($who, $what, $when, $old, $new) = (@$ref);
+        if ($old) {
+            # You can't stop being the reporter, and mail isn't sent if you
+            # remove your vote.
+            if ($what eq "CC") {
+                push(@{$recipients{DBNameToIdAndCheck($old)}}, REL_CC);
+            }
+            elsif ($what eq "QAContact") {
+                push(@{$recipients{DBNameToIdAndCheck($old)}}, REL_QA);
+            }
+            elsif ($what eq "AssignedTo") {
+                push(@{$recipients{DBNameToIdAndCheck($old)}}, REL_ASSIGNEE);
             }
-        }
-    }
-
-    # only need one entry per person
-    my $checkperson = "";
-
-    foreach my $person (@result) {
-        $checkperson = lc($person);
-        if ( !($alreadySeen{$checkperson}) ) {
-            push(@uniqueResult,$person);
-            $alreadySeen{$checkperson}++;
-        }
-    }
-
-    return @uniqueResult;
-}
-
-# if the Status was changed to Resolved or Verified
-#       set the Resolved flag
-#
-# else if Severity, Status, Target Milestone OR Priority fields have any change
-#       set the Status flag
-#
-# else if Keywords has changed
-#       set the Keywords flag
-#
-# else if CC has changed
-#       set the CC flag
-#
-# if the Comments field shows an attachment
-#       set the Attachment flag
-#
-# else if Comments exist
-#       set the Comments flag
-#
-# if no flags are set and there was some other field change
-#       set the Status flag
-#
-sub getEmailAttributes (\%\@$) {
-    
-    my ($bug, $fieldDiffs, $commentField) = @_;
-    my (@flags,@uniqueFlags,%alreadySeen) = ();
-    
-    # Add a flag if the status of the bug is "unconfirmed".
-    if ($bug->{'bug_status'} eq 'UNCONFIRMED') {
-        push (@flags, 'Unconfirmed')
-    };
-    
-    foreach my $ref (@$fieldDiffs) {
-        my ($who, $fieldName, $when, $old, $new) = (@$ref);
-        
-        #print qq{field: $fieldName $new<br>};
-        
-        # the STATUS will be flagged for Severity, Status, Target Milestone and 
-        # Priority changes
-        #
-        if ( $fieldName eq 'Status' && ($new eq 'RESOLVED' || $new eq 'VERIFIED')) {
-            push (@flags, 'Resolved');
-        }
-        elsif ( $fieldName eq 'Severity' || $fieldName eq 'Status' ||
-                $fieldName eq 'Priority' || $fieldName eq 'Target Milestone') {
-            push (@flags, 'Status');
-        } elsif ( $fieldName eq 'Keywords') {
-            push (@flags, 'Keywords');
-        } elsif ( $fieldName eq 'CC') {
-            push (@flags, 'CC');
-        }
-
-        # These next few lines find out who has been added
-        # to the Owner, QA, CC, etc. fields.  They do not affect
-        # the @flags array at all, but are run here because they
-        # affect filtering later and we're already in the loop.
-        if ($fieldName eq 'AssignedTo') {
-            push (@{$force{'Owner'}}, $new);
-        } elsif ($fieldName eq 'QAcontact') {
-           push (@{$force{'QAcontact'}}, $new);
-        } elsif ($fieldName eq 'CC') {
-            my @added = split (/[ ,]/, $new);
-            push (@{$force{'CClist'}}, @added);
         }
     }
     
-    if ( $commentField =~ /Created an attachment \(/ ) {
-        push (@flags, 'Attachments');
-    }
-    elsif ( ($commentField ne '') && !(scalar(@flags) == 1 && $flags[0] eq 'Resolved')) {
-        push (@flags, 'Comments');
-    }
-    
-    # default fallthrough for any unflagged change is 'Other'
-    if ( @flags == 0 && @$fieldDiffs != 0 ) {
-        push (@flags, 'Other');
-    }
-    
-    # only need one flag per attribute type
-    foreach my $flag (@flags) {
-        if ( !($alreadySeen{$flag}) ) {
-            push(@uniqueFlags,$flag);
-            $alreadySeen{$flag}++;
+    if (Param("supportwatchers")) {
+        # Find all those user-watching anyone on the current list, who is not 
+        # on it already themselves.
+        my $involved = join(",", keys %recipients);
+
+        my $userwatchers = 
+            $dbh->selectall_arrayref("SELECT watcher, watched FROM watch 
+                                      WHERE watched IN ($involved)
+                                      AND watcher NOT IN ($involved)");
+
+        # Mark these people as having the role of the person they are watching
+        foreach my $watch (@$userwatchers) {
+            $recipients{$watch->[0]} = $recipients{$watch->[1]};
         }
     }
-    #print "\nEmail Attributes: ", join(' ,',@uniqueFlags), "<br>\n";
-    
-    # catch-all default, just in case the above logic is faulty
-    if ( @uniqueFlags == 0) {
-        push (@uniqueFlags, 'Comments');
-    }
-    
-    return @uniqueFlags;
-}
-
-sub filterEmailGroup ($$$) {
-    # This function figures out who should receive email about the bug
-    # based on the user's role with respect to the bug (assignee, reporter, 
-    # etc.), the changes that occurred (new comments, attachment added, 
-    # status changed, etc.) and the user's email preferences.
-    
-    # Returns a filtered list of those users who would receive email
-    # about these changes, and adds the names of those users who would
-    # not receive email about them to the global @excludedAddresses list.
-    
-    my ($role, $reasons, $users) = @_;
-    
-    # Make a list of users to filter.
-    my @users = split( /,/ , $users );
-    
-    # Treat users who are in the process of being removed from this role
-    # as if they still have it.
-    push @users, @{$force{$role}};
-
-    # If this installation supports user watching, add to the list those
-    # users who are watching other users already on the list.  By doing
-    # this here, we allow watchers to inherit the roles of the people 
-    # they are watching while at the same time filtering email for watchers
-    # based on their own preferences.
-    if (Param("supportwatchers") && scalar(@users)) {
-        # Convert the unfiltered list of users into an SQL-quoted, 
-        # comma-separated string suitable for use in an SQL query.
-        my $watched = join(",", map(SqlQuote($_), @users));
-        SendSQL("SELECT watchers.login_name 
-                   FROM watch, profiles AS watchers, profiles AS watched
-                  WHERE watched.login_name IN ($watched)
-                    AND watched.userid = watch.watched
-                    AND watch.watcher = watchers.userid");
-        push (@users, FetchOneColumn()) while MoreSQLData();
-    }
+        
+    # We now have a complete set of all the users, and their relationships to
+    # the bug in question. However, we are not necessarily going to mail them
+    # all - there are preferences, permissions checks and all sorts to do yet.
+    my @sent;
+    my @excluded;
 
-    # Initialize the list of recipients.
-    my @recipients = ();
+    foreach my $user_id (keys %recipients) {
+        my @rels_which_want;
+        my $sent_mail = 0;
 
-    USER: foreach my $user (@users) {
-        next unless $user;
-        
-        # Get the user's unique ID, and if the user is not registered
-        # (no idea why unregistered users should even be on this list,
-        # but the code that was here before I re-wrote it allows this),
-        # then we do not have any preferences for them, so assume the
-        # default preference is to receive all mail.
-        my $userid = login_to_id($user);
-        if (!$userid) {
-            push(@recipients, $user);
-            next;
-        }
-        
-        # Get the user's email preferences from the database.
-        SendSQL("SELECT emailflags FROM profiles WHERE userid = $userid");
-        my $prefs = FetchOneColumn();
-        
-        # Write the user's preferences into a Perl record indexed by 
-        # preference name.  We pass the value "255" to the split function 
-        # because otherwise split will trim trailing null fields, causing 
-        # Perl to generate lots of warnings.  Any suitably large number 
-        # would do.
-        my %prefs = split(/~/, $prefs, 255);
-        
-        # If this user is the one who made the change in the first place,
-        # and they prefer not to receive mail about their own changes,
-        # filter them from the list.
-        if (lc($user) eq lc($nametoexclude) && $prefs{'ExcludeSelf'} eq 'on') {
-            push(@excludedAddresses, $user);
-            next;
-        }
-        
-        # If the user doesn't want to receive email about unconfirmed
-        # bugs, that setting overrides their other preferences, including
-        # the preference to receive email when they are added to or removed
-        # from a role, so remove them from the list before checking their
-        # other preferences.
-        if (grep(/Unconfirmed/, @$reasons)
-            && exists($prefs{"email${role}Unconfirmed"})
-            && $prefs{"email${role}Unconfirmed"} eq '')
-        {
-            push(@excludedAddresses, $user);
-            next;
-        }
+        my $user = new Bugzilla::User($user_id);
         
-        # If the user was added to or removed from this role, and they
-        # prefer to receive email when that happens, send them mail.
-        # Note: This was originally written to send email when users
-        # were removed from roles and was later enhanced for additions,
-        # but for simplicity's sake the name "Removeme" was retained.
-        if (grep($_ eq $user, @{$force{$role}})
-            && $prefs{"email${role}Removeme"} eq 'on')
+        if ($user->can_see_bug($id))
         {
-            push (@recipients, $user);
-            next;
+            # Go through each role the user has and see if they want mail in
+            # that role.
+            foreach my $relationship (@{$recipients{$user_id}}) {
+                if ($user->wants_bug_mail($id,
+                                          $relationship, 
+                                          \@diffs, 
+                                          $newcomments, 
+                                          $changer))
+                {
+                    push(@rels_which_want, $relationship);
+                }
+            }
         }
         
-        # If the user prefers to be included in mail about this change,
-        # add them to the list of recipients.
-        foreach my $reason (@$reasons) {
-            my $pref = "email$role$reason";
-            if (!exists($prefs{$pref}) || $prefs{$pref} eq 'on') {
-                push(@recipients, $user);
-                next USER;
+        if (scalar(@rels_which_want)) {
+            # So the user exists, can see the bug, and wants mail in at least
+            # one role. But do we want to send it to them?
+
+            # If we are using insiders, and the comment is private, only send 
+            # to insiders
+            my $insider_ok = 1;
+            $insider_ok = 0 if (Param("insidergroup") && 
+                                ($anyprivate != 0) && 
+                                (!$user->groups->{Param("insidergroup")}));
+
+            # We shouldn't send mail if this is a dependency mail (i.e. there 
+            # is something in @depbugs), and any of the depending bugs are not 
+            # visible to the user. This is to avoid leaking the summaries of 
+            # confidential bugs.
+            my $dep_ok = 1;
+            foreach my $dep_id (@depbugs) {
+                if (!$user->can_see_bug($dep_id)) {
+                   $dep_ok = 0;
+                   last;
+                }
+            }
+
+            # Make sure the user isn't in the nomail list, and the insider and 
+            # dep checks passed.
+            if ((!$nomail{$user->login}) &&
+                $insider_ok &&
+                $dep_ok)
+            {
+                # OK, OK, if we must. Email the user.
+                $sent_mail = sendMail($user, 
+                                      \@headerlist,
+                                      \@rels_which_want, 
+                                      \%values,
+                                      \%defmailhead, 
+                                      \%fielddescription, 
+                                      \@diffparts,
+                                      $newcomments, 
+                                      $anyprivate, 
+                                      $start, 
+                                      $id);
             }
         }
+       
+        if ($sent_mail) {
+            push(@sent, $user->login); 
+        } 
+        else {
+            push(@excluded, $user->login); 
+        } 
+    }
     
-        # At this point there's no way the user wants to receive email
-        # about this change, so exclude them from the list of recipients.
-        push(@excludedAddresses, $user);
-    
-    } # for each user on the unfiltered list
+    $dbh->do("UPDATE bugs SET lastdiffed = '$end' WHERE bug_id = $id");
 
-    return @recipients;
+    return {'sent' => \@sent, 'excluded' => \@excluded};
 }
 
-sub NewProcessOnePerson ($$$$$$$$$$$$$) {
-    my ($person, $count, $hlRef, $reasonsRef, $valueRef, $dmhRef, $fdRef,  
+sub sendMail($$$$$$$$$$$$) {
+    my ($user, $hlRef, $relRef, $valueRef, $dmhRef, $fdRef,  
         $diffRef, $newcomments, $anyprivate, $start, 
-        $id, $depbugsRef) = @_;
+        $id) = @_;
 
     my %values = %$valueRef;
     my @headerlist = @$hlRef;
-    my @reasons = @$reasonsRef;
-    my %defmailhead = %$dmhRef;
+    my %mailhead = %$dmhRef;
     my %fielddescription = %$fdRef;
-    my @diffparts = @$diffRef;
-    my @depbugs = @$depbugsRef;
-    
-    if ($seen{$person}) {
-      return;
-    }
-
-    if ($nomail{$person}) {
-      return;
-    }
-
-    # This routine should really get passed a userid
-    # This rederives groups as a side effect
-    my $user = Bugzilla::User->new_from_login($person);
-    if (!$user) { # person doesn't exist, probably changed email
-      return;
-    }
-    my $userid = $user->id;
-
-    $seen{$person} = 1;
-
-    # if this person doesn't have permission to see info on this bug, 
-    # return.
-    #
-    # XXX - This currently means that if a bug is suddenly given
-    # more restrictive permissions, people without those permissions won't
-    # see the action of restricting the bug itself; the bug will just 
-    # quietly disappear from their radar.
-    #
-    return unless $user->can_see_bug($id);
-
-    #  Drop any non-insiders if the comment is private
-    return if (Param("insidergroup") && 
-               ($anyprivate != 0) && 
-               (!$user->groups->{Param("insidergroup")}));
-
-    # We shouldn't send changedmail if this is a dependency mail, and any of 
-    # the depending bugs are not visible to the user.
-    foreach my $dep_id (@depbugs) {
-        my $save_id = $dep_id;
-        detaint_natural($dep_id) || warn("Unexpected Error: \@depbugs contains a non-numeric value: '$save_id'")
-                                 && return;
-        return unless $user->can_see_bug($dep_id);
-    }
-
-    my %mailhead = %defmailhead;
-    
+    my @diffparts = @$diffRef;    
     my $head = "";
     
     foreach my $f (@headerlist) {
@@ -801,28 +541,24 @@ sub NewProcessOnePerson ($$$$$$$$$$$$$) {
  
     if ($difftext eq "" && $newcomments eq "") {
       # Whoops, no differences!
-      return;
+      return 0;
     }
     
+    # XXX: This needs making localisable, probably by passing the role to
+    # the email template and letting it choose the text.
     my $reasonsbody = "------- You are receiving this mail because: -------\n";
 
-    if (scalar(@reasons) == 0) {
-        $reasonsbody .= "Whoops!  I have no idea!\n";
-    } else {
-        foreach my $reason (@reasons) {
-            if ($reason eq 'AssignedTo') {
-                $reasonsbody .= "You are the assignee for the bug, or are watching the assignee.\n";
-            } elsif ($reason eq 'Reporter') {
-                $reasonsbody .= "You reported the bug, or are watching the reporter.\n";
-            } elsif ($reason eq 'QAcontact') {
-                $reasonsbody .= "You are the QA contact for the bug, or are watching the QA contact.\n";
-            } elsif ($reason eq 'CC') {
-                $reasonsbody .= "You are on the CC list for the bug, or are watching someone who is.\n";
-            } elsif ($reason eq 'Voter') {
-                $reasonsbody .= "You are a voter for the bug, or are watching someone who is.\n";
-            } else {
-                $reasonsbody .= "Whoops!  There is an unknown reason!\n";
-            }
+    foreach my $relationship (@$relRef) {
+        if ($relationship == REL_ASSIGNEE) {
+            $reasonsbody .= "You are the assignee for the bug, or are watching the assignee.\n";
+        } elsif ($relationship == REL_REPORTER) {
+            $reasonsbody .= "You reported the bug, or are watching the reporter.\n";
+        } elsif ($relationship == REL_QA) {
+            $reasonsbody .= "You are the QA contact for the bug, or are watching the QA contact.\n";
+        } elsif ($relationship == REL_CC) {
+            $reasonsbody .= "You are on the CC list for the bug, or are watching someone who is.\n";
+        } elsif ($relationship == REL_VOTER) {
+            $reasonsbody .= "You are a voter for the bug, or are watching someone who is.\n";
         }
     }
 
@@ -841,14 +577,8 @@ sub NewProcessOnePerson ($$$$$$$$$$$$$) {
         $newcomments =~ s/(Created an attachment \(id=([0-9]+)\))/$1\n --> \(${showattachurlbase}$2&action=view\)/g;
     }
 
-    $person .= Param('emailsuffix');
-# 09/13/2000 cyeh@bluemartini.com
-# If a bug is changed, don't put the word "Changed" in the subject mail
-# since if the bug didn't change, you wouldn't be getting mail
-# in the first place! see http://bugzilla.mozilla.org/show_bug.cgi?id=29820 
-# for details.
     $substs{"neworchanged"} = $isnew ? ' New: ' : '';
-    $substs{"to"} = $person;
+    $substs{"to"} = $user->email;
     $substs{"cc"} = '';
     $substs{"bugid"} = $id;
     if ($isnew) {
@@ -859,13 +589,15 @@ sub NewProcessOnePerson ($$$$$$$$$$$$$) {
     $substs{"product"} = $values{'product'};
     $substs{"component"} = $values{'component'};
     $substs{"summary"} = $values{'short_desc'};
-    $substs{"reasonsheader"} = join(" ", @reasons);
+    $substs{"reasonsheader"} = join(" ", map { $rel_names{$_} } @$relRef);
     $substs{"reasonsbody"} = $reasonsbody;
     $substs{"space"} = " ";
     if ($isnew) {
-        $substs{'threadingmarker'} = "Message-ID: <bug-$id-$userid$sitespec>";
+        $substs{'threadingmarker'} = "Message-ID: <bug-$id-" . 
+                                     $user->id . "$sitespec>";
     } else {
-        $substs{'threadingmarker'} = "In-Reply-To: <bug-$id-$userid$sitespec>";
+        $substs{'threadingmarker'} = "In-Reply-To: <bug-$id-" . 
+                                     $user->id . "$sitespec>";
     }
     
     my $template = Param("newchangedmail");
@@ -874,7 +606,6 @@ sub NewProcessOnePerson ($$$$$$$$$$$$$) {
 
     MessageToMTA($msg);
 
-    push(@sentlist, $person);
     return 1;
 }