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

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
 
@@ -552,12 +552,13 @@ sub create {
                         $timestamp)");
     
     # Send an email notifying the relevant parties about the flag creation.
-    if (($flag->{'requestee'} 
+    if ($flag->{'requestee'} 
           && $flag->{'requestee'}->wants_mail([EVT_FLAG_REQUESTED]))
-         || $flag->{'type'}->{'cc_list'})
     {
-        notify($flag, "request/email.txt.tmpl");
+        $flag->{'addressee'} = $flag->{'requestee'};
     }
+
+    notify($flag, "request/email.txt.tmpl");
 }
 
 =pod
@@ -686,13 +687,24 @@ sub modify {
                                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'})
-            {
+            # 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'} = $setter;
+            $flag->{'requestee'} = undef;
             $flag->{'status'} = $status;
-                notify($flag, "request/email.txt.tmpl");
+
+            # 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.
@@ -701,6 +713,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 
@@ -711,13 +728,18 @@ sub modify {
                                is_active = 1
                         WHERE  id = $flag->{'id'}");
 
+            # Now update the flag object with its new values.
+            $flag->{'setter'} = $setter;
+            $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'}))
+                  && $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') {
@@ -751,12 +773,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");
 }
 
 
@@ -932,7 +966,8 @@ sub get_target {
 
 =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
 
@@ -944,6 +979,9 @@ sub notify {
     my $template = Bugzilla->template;
     my $vars = {};
 
+    # There is nobody to notify.
+    return unless ($flag->{'addressee'} || $flag->{'type'}->{'cc_list'});
+
     my $attachment_is_private = $flag->{'target'}->{'attachment'} ?
       $flag->{'target'}->{'attachment'}->{'isprivate'} : undef;
 
@@ -966,8 +1004,9 @@ 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;

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 %]
+  [% 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" %]
+  [% 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 %]