Fix for bug 179334: updates the setter consistently. also fixes numerous other bugs in the RT code.

r=bbaetz
a=myk

5 files changed, 108 insertions, 45 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 7395be09d..914251968 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -183,6 +183,7 @@ sub process {
     my @old_summaries;
     foreach my $flag (@$flags) {
         my $summary = $flag->{'type'}->{'name'} . $flag->{'status'};
+        $summary .= "($flag->{'requestee'}->{'nick'})" if $flag->{'requestee'};
         push(@old_summaries, $summary);
     }
     
@@ -221,6 +222,7 @@ sub process {
     my @new_summaries;
     foreach my $flag (@$flags) {
         my $summary = $flag->{'type'}->{'name'} . $flag->{'status'};
+        $summary .= "($flag->{'requestee'}->{'nick'})" if $flag->{'requestee'};
         push(@new_summaries, $summary);
     }
 
@@ -268,8 +270,10 @@ sub create {
                         $timestamp)");
     
     # Send an email notifying the relevant parties about the flag creation.
-    if ($flag->{'requestee'} && $flag->{'requestee'}->email_prefs->{'FlagRequestee'} 
-        || $flag->{'type'}->{'cc_list'}) {
+    if ($flag->{'requestee'} 
+        && ($flag->{'requestee'}->email_prefs->{'FlagRequestee'} 
+            || $flag->{'type'}->{'cc_list'}))
+    {
         notify($flag, "request/created-email.txt.tmpl");
     }
 }
@@ -299,15 +303,21 @@ sub modify {
     # Extract a list of flags from the form data.
     my @ids = map(/^flag-(\d+)$/ ? $1 : (), keys %$data);
     
-    # Loop over flags and update their record in the database.
+    # Loop over flags and update their record in the database if necessary.
+    # Two kinds of changes can happen to a flag: it can be set to a different
+    # state, and someone else can be asked to set it.  We take care of both
+    # those changes.
     my @flags;
     foreach my $id (@ids) {
         my $flag = get($id);
+
         my $status = $data->{"flag-$id"};
-        
+        my $requestee_email = $data->{"requestee-$id"};
+
         # Ignore flags the user didn't change.
-        next if $status eq $flag->{'status'};
-        
+        next if ($status eq $flag->{'status'} && $flag->{'requestee'}
+                 && $requestee_email eq $flag->{'requestee'}->{'email'});
+
         # Since the status is validated, we know it's safe, but it's still
         # tainted, so we have to detaint it before using it in a query.
         &::trick_taint($status);
@@ -315,6 +325,7 @@ sub modify {
         if ($status eq '+' || $status eq '-') {
             &::SendSQL("UPDATE flags 
                         SET    setter_id = $::userid , 
+                               requestee_id = NULL , 
                                status = '$status' , 
                                modification_date = $timestamp
                         WHERE  id = $flag->{'id'}");
@@ -328,10 +339,28 @@ sub modify {
             }
         }
         elsif ($status eq '?') {
+            # Get the requestee, if any.
+            my $requestee_id = "NULL";
+            if ($requestee_email) {
+                $requestee_id = &::DBname_to_id($requestee_email);
+                $flag->{'requestee'} = new Bugzilla::User($requestee_id);
+            }
+
+            # Update the database with the changes.
             &::SendSQL("UPDATE flags 
-                        SET    status = '$status' , 
+                        SET    setter_id = $::userid , 
+                               requestee_id = $requestee_id , 
+                               status = '$status' , 
                                modification_date = $timestamp
                         WHERE  id = $flag->{'id'}");
+            
+            # Send an email notifying the relevant parties about the request.
+            if ($flag->{'requestee'} 
+                && ($flag->{'requestee'}->email_prefs->{'FlagRequestee'} 
+                    || $flag->{'type'}->{'cc_list'}))
+            {
+                notify($flag, "request/created-email.txt.tmpl");
+            }
         }
         # The user unset the flag, so delete it from the database.
         elsif ($status eq 'X') {
@@ -390,12 +419,12 @@ sub FormToNewFlags {
             status => $status 
         };
 
-        my $requestee_str = $data->{"requestee-$type_id"} || $data->{'requestee'};
-        if ($requestee_str) {
-            my $requestee_id = &::DBname_to_id($requestee_str);
-            $requestee_id 
-              || &::ThrowUserError("invalid_username", {name => $requestee_str});
-            $flag->{'requestee'} = new Bugzilla::User($requestee_id);
+        if ($status eq "?") {
+            my $requestee = $data->{"requestee_type-$type_id"};
+            if ($requestee) {
+                my $requestee_id = &::DBname_to_id($requestee);
+                $flag->{'requestee'} = new Bugzilla::User($requestee_id);
+            }
         }
         
         # Add the flag to the array of flags.
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 3bc02c723..7cf05d935 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -234,10 +234,15 @@ sub match_field {
                 $expanded_fields->{$field_name} = 
                   { type => $fields->{$field_pattern}->{'type'} };
                 
-                # The field is a requestee field; in order for its name to show
-                # up correctly on the confirmation page, we need to find out
-                # the name of its flag type.
+                # The field is a requestee field; in order for its name 
+                # to show up correctly on the confirmation page, we need 
+                # to find out the name of its flag type.
                 if ($field_name =~ /^requestee-(\d+)$/) {
+                    my $flag = Bugzilla::Flag::get($1);
+                    $expanded_fields->{$field_name}->{'flag_type'} = 
+                      $flag->{'type'};
+                }
+                elsif ($field_name =~ /^requestee_type-(\d+)$/) {
                     $expanded_fields->{$field_name}->{'flag_type'} = 
                       Bugzilla::FlagType::get($1);
                 }
diff --git a/attachment.cgi b/attachment.cgi
index 04b86dc33..4c5737156 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -110,7 +110,8 @@ elsif ($action eq "update")
   validateContentType() unless $::FORM{'ispatch'};
   validateIsObsolete();
   validatePrivate();
-  Bugzilla::User::match_field({ '^requestee-(\d+)$' => { 'type' => 'single' } });
+  Bugzilla::User::match_field({ '^requestee(_type)?-(\d+)$' => 
+                                    { 'type' => 'single' } });
   Bugzilla::Flag::validate(\%::FORM);
   Bugzilla::FlagType::validate(\%::FORM);
   update();
diff --git a/process_bug.cgi b/process_bug.cgi
index a62320fd2..a0ed799c3 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -91,10 +91,10 @@ scalar(@idlist) || ThrowUserError("no_bugs_chosen");
 # do a match on the fields if applicable
 
 &Bugzilla::User::match_field({
-    'qa_contact'        => { 'type' => 'single' },
-    'newcc'             => { 'type' => 'multi'  },
-    'assigned_to'       => { 'type' => 'single' },
-    '^requestee-(\d+)$' => { 'type' => 'single' },
+    'qa_contact'                => { 'type' => 'single' },
+    'newcc'                     => { 'type' => 'multi'  },
+    'assigned_to'               => { 'type' => 'single' },
+    '^requestee(_type)?-(\d+)$' => { 'type' => 'single' },
 });
 
 # If we are duping bugs, let's also make sure that we can change 
diff --git a/template/en/default/flag/list.html.tmpl b/template/en/default/flag/list.html.tmpl
index c3d760eed..67cb0b815 100644
--- a/template/en/default/flag/list.html.tmpl
+++ b/template/en/default/flag/list.html.tmpl
@@ -20,33 +20,53 @@
   #%]
 
 <script type="text/javascript">
-  // Enables or disables a requestee field depending on whether
-  // the user is requesting that flag type.
-  function toggleRequesteeField(selectField)
+  // Enables or disables a requestee field depending on whether or not
+  // the user is requesting the corresponding flag.
+  function toggleRequesteeField(flagField)
   {
-    var flagID = selectField.name.replace(/flag_type-(\d+)/, "$1");
-    var requesteeField = document.getElementById("requestee-" + flagID);
+    // Convert the ID of the flag field into the ID of its corresponding
+    // requestee field and then use the ID to get the field.
+    var id = flagField.name.replace(/flag(_type)?-(\d+)/, "requestee$1-$2");
+    var requesteeField = document.getElementById(id);
     if (!requesteeField) return;
-    if   (selectField.value == "?") requesteeField.disabled = false;
-    else                            requesteeField.disabled = true;
+    
+    // Enable or disable the requestee field based on the value
+    // of the flag field.
+    if (flagField.value == "?") requesteeField.disabled = false;
+    else                        requesteeField.disabled = true;
   }
   
-  // Disables requestee fields when the window is loaded since they
-  // shouldn't be enabled until the user requests that flag type.
+  // Disables requestee fields when the window is loaded since they shouldn't
+  // be enabled until the user requests that flag type.
   function disableRequesteeFields()
   {
     var inputElements = document.getElementsByTagName("input");
-    var inputElement;
+    var inputElement, id, flagField;
     for ( var i=0 ; i<inputElements.length ; i++ )
     {
       inputElement = inputElements.item(i);
-      if (inputElement.name.search(/^requestee-(\d+)$/) != -1)
-        inputElement.disabled = true;
+      if (inputElement.name.search(/^requestee(_type)?-(\d+)$/) != -1)
+      {
+        // Convert the ID of the requestee field into the ID of its corresponding
+        // flag field and then use the ID to get the field.
+        id = inputElement.name.replace(/requestee(_type)?-(\d+)/, "flag$1-$2");
+        flagField = document.getElementById(id);
+        if (flagField && flagField.value != "?")
+            inputElement.disabled = true;
+      }
     }
   }
   window.onload = disableRequesteeFields;
 </script>
 
+[%# We list flags by looping twice over the flag types relevant for the bug.
+  # In the first loop, we display existing flags and then, for active types,
+  # we display UI for adding new flags.  In the second loop, we display UI 
+  # for adding additional new flags for those types for which a flag already
+  # exists but which are multiplicable (can have multiple flags of the type
+  # on a single bug/attachment).
+  #%]
+
 <table id="flags">
 
   [% FOREACH type = flag_types %]
@@ -59,7 +79,8 @@
           [% type.name FILTER html %]
         </td>
         <td>
-          <select name="flag-[% flag.id %]" onchange="toggleRequesteeField(this);">
+          <select id="flag-[% flag.id %]" name="flag-[% flag.id %]" 
+                  onchange="toggleRequesteeField(this);">
             <option value="X"></option>
             [% IF type.is_active %]
               <option value="+" [% "selected" IF flag.status == "+" %]>+</option>
@@ -73,12 +94,15 @@
           </select>
         </td>
         <td>
-          [% IF flag.status == "?" %]
-            [% IF flag.requestee %]([% flag.requestee.nick FILTER html %])[% END %]
-          [% ELSIF type.is_requestable && type.is_active %]
+          [% IF type.is_active && type.is_requestable %]
             <span style="white-space: nowrap;">
-              (<input type="text" id="requestee-[% type.id %]" 
-                      name="requestee-[% type.id %]" size="8" maxlength="255">)
+              (<input type="text" size="8" maxlength="255"
+                      id="requestee-[% flag.id %]" 
+                      name="requestee-[% flag.id %]"
+                      [% IF flag.status == "?" && flag.requestee %]
+                        value="[% flag.requestee.email FILTER html %]"
+                      [% END %]
+               >)
             </span>
           [% END %]
         </td>
@@ -89,7 +113,8 @@
         <td>&nbsp;</td>
         <td>[% type.name %]</td>
         <td>
-          <select name="flag_type-[% type.id %]" onchange="toggleRequesteeField(this);">
+          <select id="flag_type-[% type.id %]" name="flag_type-[% type.id %]" 
+                  onchange="toggleRequesteeField(this);">
             <option value="X"></option>
             <option value="+">+</option>
             <option value="-">-</option>
@@ -100,8 +125,9 @@
         </td>
         <td>
             <span style="white-space: nowrap;">
-              (<input type="text" id="requestee-[% type.id %]" 
-                      name="requestee-[% type.id %]" size="8" maxlength="255">)
+              (<input type="text" size="8" maxlength="255"
+                      id="requestee_type-[% type.id %]" 
+                      name="requestee_type-[% type.id %]">)
             </span>
         </td>
       </tr>
@@ -117,7 +143,8 @@
     <tr>
       <td colspan="2">addl. [% type.name %]</td>
       <td>
-        <select name="flag_type-[% type.id %]" onchange="toggleRequesteeField(this);">
+        <select id="flag_type-[% type.id %]" name="flag_type-[% type.id %]" 
+                onchange="toggleRequesteeField(this);">
           <option value="X"></option>
           <option value="+">+</option>
           <option value="-">-</option>
@@ -129,8 +156,9 @@
       <td>
         [% IF type.is_requestable && type.is_requesteeble %]
             <span style="white-space: nowrap;">
-              (<input type="text" id="requestee-[% type.id %]" 
-                      name="requestee-[% type.id %]" size="8" maxlength="255">)
+              (<input type="text" size="8" maxlength="255"
+                      id="requestee_type-[% type.id %]" 
+                      name="requestee_type-[% type.id %]">)
             </span>
         [% END %]
       </td>