From 882dcc873146d665f2d1257b89e588ae6e6356f0 Mon Sep 17 00:00:00 2001 From: "gerv%gerv.net" <> Date: Wed, 30 Mar 2005 05:42:53 +0000 Subject: Bug 73665 - migrate email preferences to their own table, and rearchitect email internals. Patch by gerv; r=jake, a=justdave. --- Bugzilla/BugMail.pm | 661 +++++++--------------- Bugzilla/Constants.pm | 129 ++--- Bugzilla/DB/Schema.pm | 15 +- Bugzilla/Flag.pm | 7 +- Bugzilla/User.pm | 211 +++++-- checksetup.pl | 155 +++-- contrib/sendunsentbugmail.pl | 19 +- post_bug.cgi | 9 +- template/en/default/account/prefs/email.html.tmpl | 319 ++++++----- template/en/default/filterexceptions.pl | 6 +- template/en/default/request/email.txt.tmpl | 6 +- userprefs.cgi | 130 ++--- 12 files changed, 816 insertions(+), 851 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 # Bradley Baetz # J. Paul Reed +# Gervase Markham 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
}; - - # 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), "
\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: "; + $substs{'threadingmarker'} = "Message-ID: id . "$sitespec>"; } else { - $substs{'threadingmarker'} = "In-Reply-To: "; + $substs{'threadingmarker'} = "In-Reply-To: id . "$sitespec>"; } my $template = Param("newchangedmail"); @@ -874,7 +606,6 @@ sub NewProcessOnePerson ($$$$$$$$$$$$$) { MessageToMTA($msg); - push(@sentlist, $person); return 1; } diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 45897a332..9946be3f3 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -51,9 +51,6 @@ use base qw(Exporter); LOGOUT_CURRENT LOGOUT_KEEP_CURRENT - DEFAULT_FLAG_EMAIL_SETTINGS - DEFAULT_EMAIL_SETTINGS - GRANT_DIRECT GRANT_DERIVED GRANT_REGEXP @@ -71,6 +68,20 @@ use base qw(Exporter); COMMENT_COLS UNLOCK_ABORT + + RELATIONSHIPS + REL_ASSIGNEE REL_QA REL_REPORTER REL_CC REL_VOTER + REL_ANY + + POS_EVENTS + EVT_OTHER EVT_ADDED_REMOVED EVT_COMMENT EVT_ATTACHMENT EVT_ATTACHMENT_DATA + EVT_PROJ_MANAGEMENT EVT_OPENED_CLOSED EVT_KEYWORD EVT_CC + + NEG_EVENTS + EVT_UNCONFIRMED EVT_CHANGED_BY_ME + + GLOBAL_EVENTS + EVT_FLAG_REQUESTED EVT_REQUESTED_FLAG ); @Bugzilla::Constants::EXPORT_OK = qw(contenttypes); @@ -136,72 +147,6 @@ use constant contenttypes => "ics" => "text/calendar" , }; -use constant DEFAULT_FLAG_EMAIL_SETTINGS => - "~FlagRequestee~on" . - "~FlagRequester~on"; - -# By default, almost all bugmail is turned on, with the exception -# of CC list additions for anyone except the Assignee/Owner. -# If you want to customize the default settings for new users at -# your own site, ensure that each of the lines ends with either -# "~on" or just "~" (for off). - -use constant DEFAULT_EMAIL_SETTINGS => - "ExcludeSelf~on" . - - "~FlagRequestee~on" . - "~FlagRequester~on" . - - "~emailOwnerRemoveme~on" . - "~emailOwnerComments~on" . - "~emailOwnerAttachments~on" . - "~emailOwnerStatus~on" . - "~emailOwnerResolved~on" . - "~emailOwnerKeywords~on" . - "~emailOwnerCC~on" . - "~emailOwnerOther~on" . - "~emailOwnerUnconfirmed~on" . - - "~emailReporterRemoveme~on" . - "~emailReporterComments~on" . - "~emailReporterAttachments~on" . - "~emailReporterStatus~on" . - "~emailReporterResolved~on" . - "~emailReporterKeywords~on" . - "~emailReporterCC~" . - "~emailReporterOther~on" . - "~emailReporterUnconfirmed~on" . - - "~emailQAcontactRemoveme~on" . - "~emailQAcontactComments~on" . - "~emailQAcontactAttachments~on" . - "~emailQAcontactStatus~on" . - "~emailQAcontactResolved~on" . - "~emailQAcontactKeywords~on" . - "~emailQAcontactCC~" . - "~emailQAcontactOther~on" . - "~emailQAcontactUnconfirmed~on" . - - "~emailCClistRemoveme~on" . - "~emailCClistComments~on" . - "~emailCClistAttachments~on" . - "~emailCClistStatus~on" . - "~emailCClistResolved~on" . - "~emailCClistKeywords~on" . - "~emailCClistCC~" . - "~emailCClistOther~on" . - "~emailCClistUnconfirmed~on" . - - "~emailVoterRemoveme~on" . - "~emailVoterComments~" . - "~emailVoterAttachments~" . - "~emailVoterStatus~" . - "~emailVoterResolved~on" . - "~emailVoterKeywords~" . - "~emailVoterCC~" . - "~emailVoterOther~" . - "~emailVoterUnconfirmed~"; - use constant GRANT_DIRECT => 0; use constant GRANT_DERIVED => 1; use constant GRANT_REGEXP => 2; @@ -230,4 +175,50 @@ use constant COMMENT_COLS => 80; # because of error use constant UNLOCK_ABORT => 1; +use constant REL_ASSIGNEE => 0; +use constant REL_QA => 1; +use constant REL_REPORTER => 2; +use constant REL_CC => 3; +use constant REL_VOTER => 4; + +use constant RELATIONSHIPS => REL_ASSIGNEE, REL_QA, REL_REPORTER, REL_CC, + REL_VOTER; + +# Used for global events like EVT_FLAG_REQUESTED +use constant REL_ANY => 100; + +# There are two sorts of event - positive and negative. Positive events are +# those for which the user says "I want mail if this happens." Negative events +# are those for which the user says "I don't want mail if this happens." +# +# Exactly when each event fires is defined in wants_bug_mail() in User.pm; I'm +# not commenting them here in case the comments and the code get out of sync. +use constant EVT_OTHER => 0; +use constant EVT_ADDED_REMOVED => 1; +use constant EVT_COMMENT => 2; +use constant EVT_ATTACHMENT => 3; +use constant EVT_ATTACHMENT_DATA => 4; +use constant EVT_PROJ_MANAGEMENT => 5; +use constant EVT_OPENED_CLOSED => 6; +use constant EVT_KEYWORD => 7; +use constant EVT_CC => 8; + +use constant POS_EVENTS => EVT_OTHER, EVT_ADDED_REMOVED, EVT_COMMENT, + EVT_ATTACHMENT, EVT_ATTACHMENT_DATA, + EVT_PROJ_MANAGEMENT, EVT_OPENED_CLOSED, EVT_KEYWORD, + EVT_CC; + +use constant EVT_UNCONFIRMED => 50; +use constant EVT_CHANGED_BY_ME => 51; + +use constant NEG_EVENTS => EVT_UNCONFIRMED, EVT_CHANGED_BY_ME; + +# These are the "global" flags, which aren't tied to a particular relationship. +# and so use REL_ANY. +use constant EVT_FLAG_REQUESTED => 100; # Flag has been requested of me +use constant EVT_REQUESTED_FLAG => 101; # I have requested a flag + +use constant GLOBAL_EVENTS => EVT_FLAG_REQUESTED, EVT_REQUESTED_FLAG; + + 1; diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm index 801f353d7..564d8c7a0 100644 --- a/Bugzilla/DB/Schema.pm +++ b/Bugzilla/DB/Schema.pm @@ -584,7 +584,6 @@ use constant ABSTRACT_SCHEMA => { disabledtext => {TYPE => 'MEDIUMTEXT', NOTNULL => 1}, mybugslink => {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'}, - emailflags => {TYPE => 'MEDIUMTEXT'}, refreshed_when => {TYPE => 'DATETIME', NOTNULL => 1}, extern_id => {TYPE => 'varchar(64)'}, ], @@ -610,6 +609,20 @@ use constant ABSTRACT_SCHEMA => { ], }, + email_setting => { + FIELDS => [ + user_id => {TYPE => 'INT3', NOTNULL => 1}, + relationship => {TYPE => 'INT1', NOTNULL => 1}, + event => {TYPE => 'INT1', NOTNULL => 1}, + ], + INDEXES => [ + email_settings_user_id_idx => ['user_id'], + email_settings_unique_idx => + {FIELDS => [qw(user_id relationship event)], + TYPE => 'UNIQUE'}, + ], + }, + watch => { FIELDS => [ watcher => {TYPE => 'INT3', NOTNULL => 1}, diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm index 5f6384487..961957278 100644 --- a/Bugzilla/Flag.pm +++ b/Bugzilla/Flag.pm @@ -72,6 +72,7 @@ use Bugzilla::Util; use Bugzilla::Error; use Bugzilla::Attachment; use Bugzilla::BugMail; +use Bugzilla::Constants; use constant TABLES_ALREADY_LOCKED => 1; @@ -482,7 +483,7 @@ sub create { # Send an email notifying the relevant parties about the flag creation. if (($flag->{'requestee'} - && $flag->{'requestee'}->email_prefs->{'FlagRequestee'}) + && $flag->{'requestee'}->wants_mail([EVT_FLAG_REQUESTED])) || $flag->{'type'}->{'cc_list'}) { notify($flag, "request/email.txt.tmpl"); @@ -590,7 +591,7 @@ sub modify { WHERE id = $flag->{'id'}"); # Send an email notifying the relevant parties about the fulfillment. - if ($flag->{'setter'}->email_prefs->{'FlagRequester'} + if ($flag->{'setter'}->wants_mail([EVT_REQUESTED_FLAG]) || $flag->{'type'}->{'cc_list'}) { $flag->{'status'} = $status; @@ -616,7 +617,7 @@ sub modify { # Send an email notifying the relevant parties about the request. if ($flag->{'requestee'} - && ($flag->{'requestee'}->email_prefs->{'FlagRequestee'} + && ($flag->{'requestee'}->wants_mail([EVT_FLAG_REQUESTED]) || $flag->{'type'}->{'cc_list'})) { notify($flag, "request/email.txt.tmpl"); diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 6f2c31d60..0d1b815c0 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -24,6 +24,7 @@ # Byron Jones # Shane H. W. Travis # Max Kanat-Alexander +# Gervase Markham ################################################################################ # Module Initialization @@ -41,6 +42,7 @@ use Bugzilla::Util; use Bugzilla::Constants; use Bugzilla::User::Setting; use Bugzilla::Auth; +use Bugzilla::BugMail; use base qw(Exporter); @Bugzilla::User::EXPORT = qw(insert_new_user is_available_username @@ -111,7 +113,7 @@ sub _create { realname, disabledtext, mybugslink - FROM profiles + FROM profiles WHERE $cond}, undef, $val); @@ -892,60 +894,146 @@ sub match_field { } -sub email_prefs { - # Get or set (not implemented) the user's email notification preferences. - +# Changes in some fields automatically trigger events. The 'field names' are +# from the fielddefs table. We really should be using proper field names +# throughout. +our %names_to_events = ( + 'Resolution' => EVT_OPENED_CLOSED, + 'Keywords' => EVT_KEYWORD, + 'CC' => EVT_CC, + 'Severity' => EVT_PROJ_MANAGEMENT, + 'Priority' => EVT_PROJ_MANAGEMENT, + 'Status' => EVT_PROJ_MANAGEMENT, + 'Target Milestone' => EVT_PROJ_MANAGEMENT, + 'Attachment description' => EVT_ATTACHMENT_DATA, + 'Attachment mime type' => EVT_ATTACHMENT_DATA, + 'Attachment is patch' => EVT_ATTACHMENT_DATA); + +# Returns true if the user wants mail for a given bug change. +# Note: the "+" signs before the constants suppress bareword quoting. +sub wants_bug_mail { my $self = shift; - return {} unless $self->id; - - # If the calling code is setting the email preferences, update the object - # but don't do anything else. This needs to write email preferences back - # to the database. - if (@_) { $self->{email_prefs} = shift; return; } - - # If we already got them from the database, return the existing values. - return $self->{email_prefs} if $self->{email_prefs}; + my ($bug_id, $relationship, $fieldDiffs, $commentField, $changer) = @_; + + # Don't send any mail, ever, if account is disabled + # XXX Temporary Compatibility Change 1 of 2: + # This code is disabled for the moment to make the behaviour like the old + # system, which sent bugmail to disabled accounts. + # return 0 if $self->{'disabledtext'}; - # Retrieve the values from the database. - &::SendSQL("SELECT emailflags FROM profiles WHERE userid = $self->{id}"); - my ($flags) = &::FetchSQLData(); - - my @roles = qw(Owner Reporter QAcontact CClist Voter); - my @reasons = qw(Removeme Comments Attachments Status Resolved Keywords - CC Other Unconfirmed); - - # Convert the prefs from the flags string from the database into - # a Perl record. The 255 param is here because split will trim - # any trailing null fields without a third param, which causes Perl - # to eject lots of warnings. Any suitably large number would do. - my $prefs = { split(/~/, $flags, 255) }; + # Make a list of the events which have happened during this bug change, + # from the point of view of this user. + my %events; + foreach my $ref (@$fieldDiffs) { + my ($who, $fieldName, $when, $old, $new) = @$ref; + # A change to any of the above fields sets the corresponding event + if (defined($names_to_events{$fieldName})) { + $events{$names_to_events{$fieldName}} = 1; + } + else { + # Catch-all for any change not caught by a more specific event + # XXX: Temporary Compatibility Change 2 of 2: + # This code is disabled, and replaced with the code a few lines + # below, in order to make the behaviour more like the original, + # which only added this event if _all_ changes were of "other" type. + # $events{+EVT_OTHER} = 1; + } + + # If the user is in a particular role and the value of that role + # changed, we need the ADDED_REMOVED event. + if (($fieldName eq "AssignedTo" && $relationship == REL_ASSIGNEE) || + ($fieldName eq "QAContact" && $relationship == REL_QA)) + { + $events{+EVT_ADDED_REMOVED} = 1; + } + + if ($fieldName eq "CC") { + my $login = $self->login; + my $inold = ($old =~ /^(.*,)?\Q$login\E(,.*)?$/); + my $innew = ($new =~ /^(.*,)?\Q$login\E(,.*)?$/); + if ($inold != $innew) + { + $events{+EVT_ADDED_REMOVED} = 1; + } + } + } + + if ($commentField =~ /Created an attachment \(/) { + $events{+EVT_ATTACHMENT} = 1; + } + elsif ($commentField ne '') { + $events{+EVT_COMMENT} = 1; + } - # Determine the value of the "excludeself" global email preference. - # Note that the value of "excludeself" is assumed to be off if the - # preference does not exist in the user's list, unlike other - # preferences whose value is assumed to be on if they do not exist. - $prefs->{ExcludeSelf} = - exists($prefs->{ExcludeSelf}) && $prefs->{ExcludeSelf} eq "on"; + my @event_list = keys %events; - # Determine the value of the global request preferences. - foreach my $pref (qw(FlagRequestee FlagRequester)) { - $prefs->{$pref} = !exists($prefs->{$pref}) || $prefs->{$pref} eq "on"; + # XXX Temporary Compatibility Change 2 of 2: + # See above comment. + if (!scalar(@event_list)) { + @event_list = (EVT_OTHER); } - # Determine the value of the rest of the preferences by looping over - # all roles and reasons and converting their values to Perl booleans. - foreach my $role (@roles) { - foreach my $reason (@reasons) { - my $key = "email$role$reason"; - $prefs->{$key} = !exists($prefs->{$key}) || $prefs->{$key} eq "on"; + my $wants_mail = $self->wants_mail(\@event_list, $relationship); + + # The negative events are handled separately - they can't be incorporated + # into the first wants_mail call, because they are of the opposite sense. + # + # We do them separately because if _any_ of them are set, we don't want + # the mail. + if ($wants_mail && $changer && ($self->{'login'} eq $changer)) { + $wants_mail &= $self->wants_mail([EVT_CHANGED_BY_ME], $relationship); + } + + if ($wants_mail) { + my $dbh = Bugzilla->dbh; + # We don't create a Bug object from the bug_id here because we only + # need one piece of information, and doing so (as of 2004-11-23) slows + # down bugmail sending by a factor of 2. If Bug creation was more + # lazy, this might not be so bad. + my $bug_status = $dbh->selectrow_array("SELECT bug_status + FROM bugs + WHERE bug_id = $bug_id"); + + if ($bug_status eq "UNCONFIRMED") { + $wants_mail &= $self->wants_mail([EVT_UNCONFIRMED], $relationship); } } - - $self->{email_prefs} = $prefs; - return $self->{email_prefs}; + return $wants_mail; } +# Returns true if the user wants mail for a given set of events. +sub wants_mail { + my $self = shift; + my ($events, $relationship) = @_; + + # Don't send any mail, ever, if account is disabled + # XXX Temporary Compatibility Change 1 of 2: + # This code is disabled for the moment to make the behaviour like the old + # system, which sent bugmail to disabled accounts. + # return 0 if $self->{'disabledtext'}; + + # No mail if there are no events + return 0 if !scalar(@$events); + + my $dbh = Bugzilla->dbh; + + # If a relationship isn't given, default to REL_ANY. + if (!defined($relationship)) { + $relationship = REL_ANY; + } + + my $wants_mail = + $dbh->selectrow_array("SELECT * + FROM email_setting + WHERE user_id = $self->{'id'} + AND relationship = $relationship + AND event IN (" . join(",", @$events) . ") + LIMIT 1"); + + return($wants_mail); +} + sub get_userlist { my $self = shift; @@ -1003,13 +1091,28 @@ sub insert_new_user ($$;$$) { # Insert the new user record into the database. $dbh->do("INSERT INTO profiles - (login_name, realname, cryptpassword, emailflags, - disabledtext) - VALUES (?, ?, ?, ?, ?)", + (login_name, realname, cryptpassword, disabledtext) + VALUES (?, ?, ?, ?)", undef, - ($username, $realname, $cryptpassword, DEFAULT_EMAIL_SETTINGS, - $disabledtext)); + ($username, $realname, $cryptpassword, $disabledtext)); + # Turn on all email for the new user + my $userid = $dbh->bz_last_key('profiles', 'userid'); + + foreach my $rel (RELATIONSHIPS) { + foreach my $event (POS_EVENTS, NEG_EVENTS) { + $dbh->do("INSERT INTO email_setting " . + "(user_id, relationship, event) " . + "VALUES ($userid, $rel, $event)"); + } + } + + foreach my $event (GLOBAL_EVENTS) { + $dbh->do("INSERT INTO email_setting " . + "(user_id, relationship, event) " . + "VALUES ($userid, " . REL_ANY . ", $event)"); + } + # Return the password to the calling code so it can be included # in an email sent to the user. return $password; @@ -1304,6 +1407,16 @@ will be set to the specified value. C is called with a single key name, which returns the associated value. +=item C + +Returns true if the user wants mail for a given bug change. + +=item C + +Returns true if the user wants mail for a given set of events. This method is +more general than C, allowing you to check e.g. permissions +for flag mail. + =back =head1 CLASS FUNCTIONS diff --git a/checksetup.pl b/checksetup.pl index f75f6a928..0d12c8256 100755 --- a/checksetup.pl +++ b/checksetup.pl @@ -2485,7 +2485,8 @@ if (!($sth->fetchrow_arrayref()->[0])) { # 2000-12-18. Added an 'emailflags' field for storing preferences about # when email gets sent on a per-user basis. -if (!$dbh->bz_get_field_def('profiles', 'emailflags')) { +if (!$dbh->bz_get_field_def('profiles', 'emailflags') && + !$dbh->bz_get_field_def('email_setting', 'user_id')) { $dbh->bz_add_field('profiles', 'emailflags', 'mediumtext'); } @@ -3756,47 +3757,6 @@ if ($dbh->bz_get_field_def('bugs', 'short_desc')->[2]) { # if it allows nulls $dbh->bz_change_field_type('bugs', 'short_desc', 'mediumtext not null'); } -# 2004-12-29 - Flag email code is broke somewhere, and doesn't treat a lack -# of FlagRequestee/er emailflags as 'on' like it's supposed to. Easiest way -# to fix this is to make sure that everyone has these set. (bug 275599). -# While we're at it, let's make sure everyone has some emailprefs set, -# whether or not they've ever visited userprefs.cgi (bug 108870). In fact, -# do this first so that the second check gets fewer hits. -# -my $emailflags_count = 0; -$sth = $dbh->prepare("SELECT userid FROM profiles " . - "WHERE emailflags LIKE '' " . - "OR emailflags IS NULL"); -$sth->execute(); -while (my ($userid) = $sth->fetchrow_array()) { - $dbh->do("UPDATE profiles SET emailflags = " . - $dbh->quote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS) . - "WHERE userid = $userid"); - $emailflags_count++; -} - -if ($emailflags_count) { - print "Added default email prefs to $emailflags_count users who had none.\n" unless $silent; - $emailflags_count = 0; -} - - -$sth = $dbh->prepare("SELECT userid, emailflags FROM profiles " . - "WHERE emailflags NOT LIKE '%Flagrequeste%' "); -$sth->execute(); -while (my ($userid, $emailflags) = $sth->fetchrow_array()) { - $emailflags .= Bugzilla::Constants::DEFAULT_FLAG_EMAIL_SETTINGS; - $emailflags = $dbh->quote($emailflags); - $dbh->do("UPDATE profiles SET emailflags = $emailflags " . - "WHERE userid = $userid"); - $emailflags_count++; -} - -if ($emailflags_count) { - print "Added default Flagrequester/ee email prefs to $emailflags_count users who had none.\n" unless $silent; - $emailflags_count = 0; -} - # 2003-10-24 - alt@sonic.net, bug 224208 # Support classification level $dbh->bz_add_field('products', 'classification_id', 'smallint DEFAULT 1'); @@ -3882,8 +3842,113 @@ if (!$dbh->bz_get_field_def('bugs', 'qa_contact')->[2]) { # if it's NOT NULL WHERE initialqacontact = 0"); } -} # END LEGACY CHECKS +# 2005-03-29 - gerv@gerv.net - bug 73665. +# Migrate email preferences to new email prefs table. +if ($dbh->bz_get_field_def("profiles", "emailflags")) { + print "Migrating email preferences to new table ...\n" unless $silent; + + # These are the "roles" and "reasons" from the original code, mapped to + # the new terminology of relationships and events. + my %relationships = ("Owner" => REL_ASSIGNEE, + "Reporter" => REL_REPORTER, + "QAcontact" => REL_QA, + "CClist" => REL_CC, + "Voter" => REL_VOTER); + + my %events = ("Removeme" => EVT_ADDED_REMOVED, + "Comments" => EVT_COMMENT, + "Attachments" => EVT_ATTACHMENT, + "Status" => EVT_PROJ_MANAGEMENT, + "Resolved" => EVT_OPENED_CLOSED, + "Keywords" => EVT_KEYWORD, + "CC" => EVT_CC, + "Other" => EVT_OTHER, + "Unconfirmed" => EVT_UNCONFIRMED); + + # Request preferences + my %requestprefs = ("FlagRequestee" => EVT_FLAG_REQUESTED, + "FlagRequester" => EVT_REQUESTED_FLAG); + + # Select all emailflags flag strings + my $sth = $dbh->prepare("SELECT userid, emailflags FROM profiles"); + $sth->execute(); + while (my ($userid, $flagstring) = $sth->fetchrow_array()) { + # If the user has never logged in since emailprefs arrived, and the + # temporary code to give them a default string never ran, then + # $flagstring will be null. In this case, they just get all mail. + $flagstring ||= ""; + + # The 255 param is here, because without a third param, split will + # trim any trailing null fields, which causes Perl to eject lots of + # warnings. Any suitably large number would do. + my %emailflags = split(/~/, $flagstring, 255); # Appease my editor: / + + my $sth2 = $dbh->prepare("INSERT into email_setting " . + "(user_id, relationship, event) VALUES (" . + "$userid, ?, ?)"); + foreach my $relationship (keys %relationships) { + foreach my $event (keys %events) { + my $key = "email$relationship$event"; + if (!exists($emailflags{$key}) || $emailflags{$key} eq 'on') { + $sth2->execute($relationships{$relationship}, + $events{$event}); + } + } + } + + # Note that in the old system, the value of "excludeself" is assumed to + # be off if the preference does not exist in the user's list, unlike + # other preferences whose value is assumed to be on if they do not + # exist. + # + # This preference has changed from global to per-relationship. + if (!exists($emailflags{'ExcludeSelf'}) + || $emailflags{'ExcludeSelf'} ne 'on') + { + foreach my $relationship (keys %relationships) { + $dbh->do("INSERT into email_setting " . + "(user_id, relationship, event) VALUES (" . + $userid . ", " . + $relationships{$relationship}. ", " . + EVT_CHANGED_BY_ME . ")"); + } + } + + foreach my $key (keys %requestprefs) { + if (!exists($emailflags{$key}) || $emailflags{$key} eq 'on') { + $dbh->do("INSERT into email_setting " . + "(user_id, relationship, event) VALUES (" . + $userid . ", " . REL_ANY . ", " . + $requestprefs{$key} . ")"); + } + } + } + + # EVT_ATTACHMENT_DATA should initially have identical settings to + # EVT_ATTACHMENT. + CloneEmailEvent(EVT_ATTACHMENT, EVT_ATTACHMENT_DATA); + + $dbh->bz_drop_field("profiles", "emailflags"); +} + +sub CloneEmailEvent { + my ($source, $target) = @_; + + my $sth1 = $dbh->prepare("SELECT user_id, relationship FROM email_setting + WHERE event = $source"); + my $sth2 = $dbh->prepare("INSERT into email_setting " . + "(user_id, relationship, event) VALUES (" . + "?, ?, $target)"); + + $sth1->execute(); + + while (my ($userid, $relationship) = $sth1->fetchrow_array()) { + $sth2->execute($userid, $relationship); + } +} + +} # END LEGACY CHECKS # If you had to change the --TABLE-- definition in any way, then add your # differential change code *** A B O V E *** this comment. @@ -4156,10 +4221,10 @@ if ($sth->rows == 0) { $dbh->do( q{INSERT INTO profiles (login_name, realname, cryptpassword, - emailflags, disabledtext, refreshed_when) + disabledtext, refreshed_when) VALUES (?, ?, ?, ?, ?, ?)}, undef, $login, $realname, $cryptedpassword, - Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS, '', '1900-01-01 00:00:00'); + '', '1900-01-01 00:00:00'); } # Put the admin in each group if not already diff --git a/contrib/sendunsentbugmail.pl b/contrib/sendunsentbugmail.pl index 9184e0be2..c82c0ae30 100644 --- a/contrib/sendunsentbugmail.pl +++ b/contrib/sendunsentbugmail.pl @@ -47,9 +47,22 @@ if (scalar(@list) > 0) { my $start_time = time; print "Sending mail for bug $bugid...\n"; my $outputref = Bugzilla::BugMail::Send($bugid); - my ($sent, $excluded) = (scalar(@{$outputref->{sent}}),scalar(@{$outputref->{excluded}})); - print "$sent mails sent, $excluded people excluded.\n"; - print "Took " . (time - $start_time) . " seconds.\n\n"; + if ($ARGV[0] && $ARGV[0] eq "--report") { + print "Mail sent to:\n"; + foreach (sort @{$outputref->{sent}}) { + print $_ . "\n"; + } + + print "Excluded:\n"; + foreach (sort @{$outputref->{excluded}}) { + print $_ . "\n"; + } + } + else { + my ($sent, $excluded) = (scalar(@{$outputref->{sent}}),scalar(@{$outputref->{excluded}})); + print "$sent mails sent, $excluded people excluded.\n"; + print "Took " . (time - $start_time) . " seconds.\n\n"; + } } print "Unsent mail has been sent.\n"; } diff --git a/post_bug.cgi b/post_bug.cgi index 22eca5b9d..7f05dc7bc 100755 --- a/post_bug.cgi +++ b/post_bug.cgi @@ -486,14 +486,7 @@ if (UserInGroup("editbugs")) { } # Email everyone the details of the new bug -$vars->{'mailrecipients'} = { 'cc' => \@cc, - 'owner' => DBID_to_name($::FORM{'assigned_to'}), - 'reporter' => Bugzilla->user->login, - 'changer' => Bugzilla->user->login }; - -if (defined $::FORM{'qa_contact'}) { - $vars->{'mailrecipients'}->{'qacontact'} = DBID_to_name($::FORM{'qa_contact'}); -} +$vars->{'mailrecipients'} = {'changer' => Bugzilla->user->login}; $vars->{'id'} = $id; my $bug = new Bugzilla::Bug($id, $::userid); diff --git a/template/en/default/account/prefs/email.html.tmpl b/template/en/default/account/prefs/email.html.tmpl index ba07fb155..f712bd8fa 100644 --- a/template/en/default/account/prefs/email.html.tmpl +++ b/template/en/default/account/prefs/email.html.tmpl @@ -40,56 +40,32 @@ [% useqacontact = Param('useqacontact') %] [% usevotes = Param('usevotes') %] - - [% IF Param('supportwatchers') %] - - - - - - - +

+ If you don't like getting a notification for "trivial" + changes to [% terms.bugs %], you can use the settings below to + filter some or all notifications. +

- - - - - - - - - - [% END %] +
@@ -103,129 +79,204 @@ - - - -
-
-
- If you want to help cover for someone when they're on vacation, or if - you need to do the QA related to all of their [% terms.bugs %], you can tell - [%+ terms.Bugzilla %] to send mail related to their [% terms.bugs %] to you, too. List the - email addresses of any accounts you wish to watch here, separated by - commas. -
Users to watch: - -
Users watching you: - [% IF watchers.size %] - [% FOREACH watcher = watchers %] - [% watcher FILTER html %]
- [% END %] - [% ELSE %] - Nobody is currently watching your account. - [% END %] -
- - -
-
- - + [% prefname = "email-$constants.REL_ANY-$constants.EVT_FLAG_REQUESTED" %] + +
- - + [% prefname = "email-$constants.REL_ANY-$constants.EVT_REQUESTED_FLAG" %] + +
- -
Field/recipient specific options:

+[% events = [ + { id = constants.EVT_ADDED_REMOVED, + description = "I'm added to or removed from this capacity" }, + { id = constants.EVT_OPENED_CLOSED, + description = "The $terms.bug is resolved or reopened" }, + { id = constants.EVT_PROJ_MANAGEMENT, + description = "The priority, status, severity, or milestone changes" }, + { id = constants.EVT_COMMENT, + description = "New comments are added" }, + { id = constants.EVT_ATTACHMENT, + description = "New attachments are added" }, + { id = constants.EVT_ATTACHMENT_DATA, + description = "Some attachment data changes" }, + { id = constants.EVT_KEYWORD, + description = "The keywords field changes" }, + { id = constants.EVT_CC, + description = "The CC field changes" }, + { id = constants.EVT_OTHER, + description = "Any field not mentioned above changes" }, +] %] + +[% neg_events = [ + { id = constants.EVT_UNCONFIRMED, + description = "The $terms.bug is in the UNCONFIRMED state" }, + { id = constants.EVT_CHANGED_BY_ME, + description = "The change was made by me" }, +] %] + +[% relationships = [ + { id = constants.REL_ASSIGNEE, + description = "Assignee" }, + { id = constants.REL_QA, + description = "QA Contact" }, + { id = constants.REL_REPORTER, + description = "Reporter" }, + { id = constants.REL_CC, + description = "CCed" }, + { id = constants.REL_VOTER, + description = "Voter" }, +] %] - - - - - [% IF useqacontact %] - + [% FOREACH relationship = relationships %] + [% NEXT IF (relationship.id == constants.REL_QA AND NOT useqacontact) OR + (relationship.id == constants.REL_VOTER AND NOT usevotes) %] + [% END %] - - [% IF usevotes %] - + + [% FOREACH event = events %] + + [% FOREACH relationship = relationships %] + [% NEXT IF (relationship.id == constants.REL_QA AND NOT useqacontact) OR + (relationship.id == constants.REL_VOTER AND NOT usevotes) %] + + [% END %] + - [% END %] + + [% END %] + + + + -[% bugLabelLower = BLOCK %] -[% terms.bug %] -[% END %] - - [% FOREACH reason = [ - { name = 'Removeme', - description = "I'm added to or removed from this capacity" }, - { name = 'Comments', - description = "New Comments are added" }, - { name = 'Attachments', - description = "New Attachments are added" }, - { name = 'Status', - description = "Priority, status, severity, and/or milestone changes" }, - { name = 'Resolved', - description = "The ${bugLabelLower} is resolved or verified" }, - { name = 'Keywords', - description = "Keywords field changes" }, - { name = 'CC', - description = "CC field changes" }, - { name = 'Other', - description = "Any field not mentioned above changes" }, - { name = 'Unconfirmed', - description = "The ${bugLabelLower} is in the unconfirmed state" }, - ] %] + [% FOREACH event = neg_events %] - [% FOREACH role = [ "Reporter", "Owner", "QAcontact", "CClist", "Voter" ] - %] - [% NEXT IF role == "QAcontact" AND NOT useqacontact %] - [% NEXT IF role == "Voter" AND NOT usevotes %] - [% END %] [% END %] +
+ When my relationship to this [% terms.bug %] is: + I want to receive mail when:
- Reporter - - Assignee - - QA Contact - + [% relationship.description FILTER html %] + - CC - - Voter +
+ + + [% event.description FILTER html %]
+   + + but not when (overrides above): +
- + [% FOREACH relationship = relationships %] + [% NEXT IF (relationship.id == constants.REL_QA AND NOT useqacontact) OR + (relationship.id == constants.REL_VOTER AND NOT usevotes) %] + - [% reason.description %] + [% event.description FILTER html %]
+[%# Add hidden form fields for fields not used %] +[% FOREACH event = events %] + [% FOREACH relationship = relationships %] + [% IF (relationship.id == constants.REL_QA AND NOT useqacontact) OR + (relationship.id == constants.REL_VOTER AND NOT usevotes) %] + + [% END %] + [% END %] +[% END %] + +[% FOREACH event = neg_events %] + [% FOREACH relationship = relationships %] + [% IF (relationship.id == constants.REL_QA AND NOT useqacontact) OR + (relationship.id == constants.REL_VOTER AND NOT usevotes) %] + + [% END %] + [% END %] +[% END %] + +[% IF Param('supportwatchers') %] +
+User Watching + +

+If you watch a user, it is as if you are standing in their shoes for the +purposes of getting email. Email is sent or not according to your +preferences for their relationship to the [% terms.bug %] +(e.g. Assignee). You are watching anyone on the following comma-separated list: +

+ +

Users to watch: + +

+ +

Users watching you:
+ [% IF watchers.size %] + [% FOREACH watcher = watchers %] + [% watcher FILTER html %]
+ [% END %] + [% ELSE %] + None + [% END %] +

+ +[% END %] + +
+
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl index 0b3eac943..19625ea72 100644 --- a/template/en/default/filterexceptions.pl +++ b/template/en/default/filterexceptions.pl @@ -645,9 +645,9 @@ ], 'account/prefs/email.html.tmpl' => [ - 'role', - 'reason.name', - 'reason.description', + 'relationship.id', + 'event.id', + 'prefname', ], 'account/prefs/permissions.html.tmpl' => [ diff --git a/template/en/default/request/email.txt.tmpl b/template/en/default/request/email.txt.tmpl index 719f9d869..53af6e5d7 100644 --- a/template/en/default/request/email.txt.tmpl +++ b/template/en/default/request/email.txt.tmpl @@ -28,11 +28,13 @@ [% statuses = { '+' => "granted" , '-' => 'denied' , 'X' => "cancelled" , '?' => "asked" } %] [% IF flag.status == '?' %] - [% to_email = flag.requestee.email IF flag.requestee.email_prefs.FlagRequestee %] + [% to_email = flag.requestee.email + IF flag.requestee.wants_mail(constants.EVT_FLAG_REQUESTED) %] [% to_identity = flag.requestee.identity %] [% subject_status = "requested" %] [% ELSE %] - [% to_email = flag.setter.email IF flag.setter.email_prefs.FlagRequester %] + [% to_email = flag.setter.email + IF flag.setter.wants_mail(constants.EVT_REQUESTED_FLAG) %] [% to_identity = flag.setter.identity _ "'s request" %] [% subject_status = statuses.${flag.status} %] [% END %] diff --git a/userprefs.cgi b/userprefs.cgi index d369660e2..eac0bb108 100755 --- a/userprefs.cgi +++ b/userprefs.cgi @@ -37,10 +37,6 @@ require "CGI.pl"; # Use global template variables. use vars qw($template $vars $userid); -my @roles = ("Owner", "Reporter", "QAcontact", "CClist", "Voter"); -my @reasons = ("Removeme", "Comments", "Attachments", "Status", "Resolved", - "Keywords", "CC", "Other", "Unconfirmed"); - ############################################################################### # Each panel has two functions - panel Foo has a DoFoo, to get the data # necessary for displaying the panel, and a SaveFoo, to save the panel's @@ -175,7 +171,10 @@ sub SaveSettings { sub DoEmail { my $dbh = Bugzilla->dbh; - + + ########################################################################### + # User watching + ########################################################################### if (Param("supportwatchers")) { my $watched_ref = $dbh->selectcol_arrayref( "SELECT profiles.login_name FROM watch, profiles" @@ -197,81 +196,74 @@ sub DoEmail { $vars->{'watchers'} = \@watchers; } - SendSQL("SELECT emailflags FROM profiles WHERE userid = $userid"); - - my ($flagstring) = FetchSQLData(); + ########################################################################### + # Role-based preferences + ########################################################################### + my $sth = Bugzilla->dbh->prepare("SELECT relationship, event " . + "FROM email_setting " . + "WHERE user_id = $userid"); + $sth->execute(); + + my %mail; + while (my ($relationship, $event) = $sth->fetchrow_array()) { + $mail{$relationship}{$event} = 1; + } - # The 255 param is here, because without a third param, split will - # trim any trailing null fields, which causes Perl to eject lots of - # warnings. Any suitably large number would do. - my %emailflags = split(/~/, $flagstring, 255); + $vars->{'mail'} = \%mail; +} - # Determine the value of the "excludeself" global email preference. - # Note that the value of "excludeself" is assumed to be off if the - # preference does not exist in the user's list, unlike other - # preferences whose value is assumed to be on if they do not exist. - if (exists($emailflags{'ExcludeSelf'}) - && $emailflags{'ExcludeSelf'} eq 'on') - { - $vars->{'excludeself'} = 1; - } - else { - $vars->{'excludeself'} = 0; - } - - foreach my $flag (qw(FlagRequestee FlagRequester)) { - $vars->{$flag} = - !exists($emailflags{$flag}) || $emailflags{$flag} eq 'on'; - } +sub SaveEmail { + my $dbh = Bugzilla->dbh; + my $cgi = Bugzilla->cgi; - # Parse the info into a hash of hashes; the first hash keyed by role, - # the second by reason, and the value being 1 or 0 for (on or off). - # Preferences not existing in the user's list are assumed to be on. - foreach my $role (@roles) { - $vars->{$role} = {}; - foreach my $reason (@reasons) { - my $key = "email$role$reason"; - if (!exists($emailflags{$key}) || $emailflags{$key} eq 'on') { - $vars->{$role}{$reason} = 1; + ########################################################################### + # Role-based preferences + ########################################################################### + $dbh->bz_lock_tables("email_setting WRITE"); + + # Delete all the user's current preferences + $dbh->do("DELETE FROM email_setting WHERE user_id = $userid"); + + # Repopulate the table - first, with normal events in the + # relationship/event matrix. + # Note: the database holds only "off" email preferences, as can be implied + # from the name of the table - profiles_nomail. + foreach my $rel (RELATIONSHIPS) { + # Positive events: a ticked box means "send me mail." + foreach my $event (POS_EVENTS) { + if (1 == $cgi->param("email-$rel-$event")) { + $dbh->do("INSERT INTO email_setting " . + "(user_id, relationship, event) " . + "VALUES ($userid, $rel, $event)"); } - else { - $vars->{$role}{$reason} = 0; + } + + # Negative events: a ticked box means "don't send me mail." + foreach my $event (NEG_EVENTS) { + if (!defined($cgi->param("neg-email-$rel-$event")) || + $cgi->param("neg-email-$rel-$event") != 1) + { + $dbh->do("INSERT INTO email_setting " . + "(user_id, relationship, event) " . + "VALUES ($userid, $rel, $event)"); } } } -} - -# Note: we no longer store "off" values in the database. -sub SaveEmail { - my $updateString = ""; - my $cgi = Bugzilla->cgi; - my $dbh = Bugzilla->dbh; - if (defined $cgi->param('ExcludeSelf')) { - $updateString .= 'ExcludeSelf~on'; - } else { - $updateString .= 'ExcludeSelf~'; - } - - foreach my $flag (qw(FlagRequestee FlagRequester)) { - $updateString .= "~$flag~" . (defined $cgi->param($flag) ? "on" : ""); - } - - foreach my $role (@roles) { - foreach my $reason (@reasons) { - # Add this preference to the list without giving it a value, - # which is the equivalent of setting the value to "off." - $updateString .= "~email$role$reason~"; - - # If the form field for this preference is defined, then we - # know the checkbox was checked, so set the value to "on". - $updateString .= "on" if defined $cgi->param("email$role$reason"); + # Global positive events: a ticked box means "send me mail." + foreach my $event (GLOBAL_EVENTS) { + if (1 == $cgi->param("email-" . REL_ANY . "-$event")) { + $dbh->do("INSERT INTO email_setting " . + "(user_id, relationship, event) " . + "VALUES ($userid, " . REL_ANY . ", $event)"); } } - - SendSQL("UPDATE profiles SET emailflags = " . SqlQuote($updateString) . - " WHERE userid = $userid"); + $dbh->bz_unlock_tables(); + + ########################################################################### + # User watching + ########################################################################### if (Param("supportwatchers") && defined $cgi->param('watchedusers')) { # Just in case. Note that this much locking is actually overkill: # we don't really care if anyone reads the watch table. So -- cgit v1.2.3-24-g4f1b