Bug 266147: Internal error when Flag::notify() ends up with an invalid or empty CC...

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 9c8a857f70b27a796256bea36f08b00884c2c5cf..760731b28134bd6098b42696b7792cf324d89ea9 100644 (file)
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -19,7 +19,7 @@
 #
 # Contributor(s): Myk Melez <myk@mozilla.org>
 #                 Jouni Heikniemi <jouni@heikniemi.net>
-#                 Frédéric Buclin <LpSolit@gmail.com>
+#                 Frédéric Buclin <LpSolit@gmail.com>
 
 =head1 NAME
 
@@ -538,12 +538,13 @@ sub create {
                         $timestamp)");
     
     # Send an email notifying the relevant parties about the flag creation.
-    if (($flag->{'requestee'} 
-         && $flag->{'requestee'}->wants_mail([EVT_FLAG_REQUESTED]))
-         || $flag->{'type'}->{'cc_list'})
+    if ($flag->{'requestee'} 
+          && $flag->{'requestee'}->wants_mail([EVT_FLAG_REQUESTED]))
     {
-        notify($flag, "request/email.txt.tmpl");
+        $flag->{'addressee'} = $flag->{'requestee'};
     }
+
+    notify($flag, "request/email.txt.tmpl");
 }
 
 =pod
@@ -645,14 +646,25 @@ sub modify {
                                modification_date = $timestamp ,
                                is_active = 1
                         WHERE  id = $flag->{'id'}");
-            
-            # Send an email notifying the relevant parties about the fulfillment.
-            if ($flag->{'setter'}->wants_mail([EVT_REQUESTED_FLAG]) 
-                || $flag->{'type'}->{'cc_list'})
-            {
-                $flag->{'status'} = $status;
-                notify($flag, "request/email.txt.tmpl");
+
+            # If the status of the flag was "?", we have to notify
+            # the requester (if he wants to).
+            my $requester;
+            if ($flag->{'status'} eq '?') {
+                $requester = $flag->{'setter'};
             }
+            # Now update the flag object with its new values.
+            $flag->{'setter'} = Bugzilla->user;
+            $flag->{'requestee'} = undef;
+            $flag->{'status'} = $status;
+
+            # Send an email notifying the relevant parties about the fulfillment,
+            # including the requester.
+            if ($requester && $requester->wants_mail([EVT_REQUESTED_FLAG])) {
+                $flag->{'addressee'} = $requester;
+            }
+
+            notify($flag, "request/email.txt.tmpl");
         }
         elsif ($status eq '?') {
             # Get the requestee, if any.
@@ -661,6 +673,11 @@ sub modify {
                 $requestee_id = login_to_id($requestee_email);
                 $flag->{'requestee'} = new Bugzilla::User($requestee_id);
             }
+            else {
+                # If the status didn't change but we only removed the
+                # requestee, we have to clear the requestee field.
+                $flag->{'requestee'} = undef;
+            }
 
             # Update the database with the changes.
             &::SendSQL("UPDATE flags 
@@ -670,14 +687,19 @@ sub modify {
                                modification_date = $timestamp ,
                                is_active = 1
                         WHERE  id = $flag->{'id'}");
-            
+
+            # Now update the flag object with its new values.
+            $flag->{'setter'} = Bugzilla->user;
+            $flag->{'status'} = $status;
+
             # Send an email notifying the relevant parties about the request.
-            if ($flag->{'requestee'} 
-                && ($flag->{'requestee'}->wants_mail([EVT_FLAG_REQUESTED]) 
-                    || $flag->{'type'}->{'cc_list'}))
+            if ($flag->{'requestee'}
+                  && $flag->{'requestee'}->wants_mail([EVT_FLAG_REQUESTED]))
             {
-                notify($flag, "request/email.txt.tmpl");
+                $flag->{'addressee'} = $flag->{'requestee'};
             }
+
+            notify($flag, "request/email.txt.tmpl");
         }
         # The user unset the flag; set is_active = 0
         elsif ($status eq 'X') {
@@ -711,12 +733,24 @@ sub clear {
     &::SendSQL("UPDATE flags SET is_active = 0 WHERE id = $id");
     &::PopGlobalSQLState();
 
+    # If we cancel a pending request, we have to notify the requester
+    # (if he wants to).
+    my $requester;
+    if ($flag->{'status'} eq '?') {
+        $requester = $flag->{'setter'};
+    }
+
+    # Now update the flag object to its new values. The last
+    # requester/setter and requestee are kept untouched (for the
+    # record). Else we could as well delete the flag completely.
     $flag->{'exists'} = 0;    
-    # Set the flag's status to "cleared" so the email template
-    # knows why email is being sent about the request.
     $flag->{'status'} = "X";
-    
-    notify($flag, "request/email.txt.tmpl") if $flag->{'requestee'};
+
+    if ($requester && $requester->wants_mail([EVT_REQUESTED_FLAG])) {
+        $flag->{'addressee'} = $requester;
+    }
+
+    notify($flag, "request/email.txt.tmpl");
 }
 
 
@@ -891,7 +925,8 @@ sub GetTarget {
 
 =item C<notify($flag, $template_file)>
 
-Sends an email notification about a flag being created or fulfilled.
+Sends an email notification about a flag being created, fulfilled
+or deleted.
 
 =back
 
@@ -900,6 +935,9 @@ Sends an email notification about a flag being created or fulfilled.
 sub notify {
     my ($flag, $template_file) = @_;
 
+    # There is nobody to notify.
+    return unless ($flag->{'addressee'} || $flag->{'type'}->{'cc_list'});
+
     # If the target bug is restricted to one or more groups, then we need
     # to make sure we don't send email about it to unauthorized users
     # on the request type's CC: list, so we have to trawl the list for users
@@ -923,10 +961,11 @@ sub notify {
         $flag->{'type'}->{'cc_list'} = join(", ", @new_cc_list);
     }
 
-    $flag->{'requestee'}->{'email'} .= Param('emailsuffix');
-    $flag->{'setter'}->{'email'} .= Param('emailsuffix');
+    # If there is nobody left to notify, return.
+    return unless ($flag->{'addressee'} || $flag->{'type'}->{'cc_list'});
+
     $::vars->{'flag'} = $flag;
-    
+
     my $message;
     my $rv = 
       $::template->process($template_file, $::vars, \$message);

diff --git a/template/en/default/request/email.txt.tmpl b/template/en/default/request/email.txt.tmpl
index d59cad0ec5907ee7c2fb131b6cfddb62318ac57d..9c3be3387546c040e0cd6e3b7d9c5830fe4c918b 100644 (file)
--- a/template/en/default/request/email.txt.tmpl
+++ b/template/en/default/request/email.txt.tmpl
@@ -18,6 +18,7 @@
   #
   # Contributor(s):     Myk Melez <myk@mozilla.org>
   #                     Jeff Hedlund <jeff.hedlund@matrixsi.com>
+  #                     Frédéric Buclin <LpSolit@gmail.com>
   #%]
 
 [% PROCESS global/variables.none.tmpl %]
@@ -28,18 +29,16 @@
 [% statuses = { '+' => "granted" , '-' => 'denied' , 'X' => "cancelled" ,
                 '?' => "asked" } %]
 [% IF flag.status == '?' %]
-   [% to_email = flag.requestee.email 
-                 IF flag.requestee.wants_mail([constants.EVT_FLAG_REQUESTED]) %]
-   [% to_identity = flag.requestee.identity %]
-   [% subject_status = "requested" %]
+  [% to_identity = flag.addressee.identity _ " for" %]
+  [% subject_status = "requested" %]
 [% ELSE %]
-   [% 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} %]
+  [% IF flag.addressee %]
+    [% to_identity = flag.addressee.identity _ "'s request for" %]
+  [% END %]
+  [% subject_status = statuses.${flag.status} %]
 [% END %]
 From: bugzilla-request-daemon
-To: [% to_email %]
+To: [% flag.addressee.email %]
 CC: [% flag.type.cc_list %]
 Subject: [% flag.type.name %] [%+ subject_status %]: [[% terms.Bug %] [%+ flag.target.bug.id %]] [% flag.target.bug.summary %]
 [%- IF flag.target.attachment.exists %] :
@@ -48,7 +47,7 @@ Subject: [% flag.type.name %] [%+ subject_status %]: [[% terms.Bug %] [%+ flag.t
 [%+ USE wrap -%]
 [%- FILTER bullet = wrap(80) -%]
 
-[% user.identity %] has [% statuses.${flag.status} %] [%+ to_identity %] for [% flag.type.name %]:
+[% user.identity %] has [% statuses.${flag.status} %] [%+ to_identity %] [%+ flag.type.name %]:
 
 [% terms.Bug %] [%+ bugidsummary %]
 [% END %]