Fix for bugs 55161 and 12819. The activity log now stores only what's changed in multi-value fields.

r= justdave@syndicomm.com

7 files changed, 203 insertions, 80 deletions

diff --git a/CGI.pl b/CGI.pl
index f248b2599..06d32d05f 100644
--- a/CGI.pl
+++ b/CGI.pl
@@ -1105,12 +1105,12 @@ sub CheckIfVotedConfirmed {
                 "WHERE bug_id = $id");
         my $fieldid = GetFieldID("bug_status");
         SendSQL("INSERT INTO bugs_activity " .
-                "(bug_id,who,bug_when,fieldid,oldvalue,newvalue) VALUES " .
+                "(bug_id,who,bug_when,fieldid,removed,added) VALUES " .
                 "($id,$who,now(),$fieldid,'$::unconfirmedstate','NEW')");
         if (!$everconfirmed) {
             $fieldid = GetFieldID("everconfirmed");
             SendSQL("INSERT INTO bugs_activity " .
-                    "(bug_id,who,bug_when,fieldid,oldvalue,newvalue) VALUES " .
+                    "(bug_id,who,bug_when,fieldid,removed,added) VALUES " .
                     "($id,$who,now(),$fieldid,'0','1')");
         }
         AppendComment($id, DBID_to_name($who),
@@ -1136,7 +1136,7 @@ sub DumpBugActivity {
     my $query = "
         SELECT IFNULL(fielddefs.name, bugs_activity.fieldid),
                 bugs_activity.bug_when,
-                bugs_activity.oldvalue, bugs_activity.newvalue,
+                bugs_activity.removed, bugs_activity.added,
                 profiles.login_name
         FROM bugs_activity LEFT JOIN fielddefs ON 
                                      bugs_activity.fieldid = fielddefs.fieldid,
@@ -1149,25 +1149,21 @@ sub DumpBugActivity {
     
     print "<table border cellpadding=4>\n";
     print "<tr>\n";
-    print "    <th>Who</th><th>What</th><th>Old value</th><th>New value</th><th>When</th>\n";
+    print "    <th>Who</th><th>What</th><th>Removed</th><th>Added</th><th>When</th>\n";
     print "</tr>\n";
     
     my @row;
     while (@row = FetchSQLData()) {
-        my ($field,$when,$old,$new,$who) = (@row);
-        $old = value_quote($old);
-        $new = value_quote($new);
-        if ($old eq "") {
-            $old = "&nbsp;";
-        }
-        if ($new eq "") {
-            $new = "&nbsp;";
-        }
+        my ($field,$when,$removed,$added,$who) = (@row);
+        $removed = html_quote($removed);
+        $added = html_quote($added);
+        $removed ||= "&nbsp;";
+        $added ||= "&nbsp;";
         print "<tr>\n";
         print "<td>$who</td>\n";
         print "<td>$field</td>\n";
-        print "<td>$old</td>\n";
-        print "<td>$new</td>\n";
+        print "<td>$removed</td>\n";
+        print "<td>$added</td>\n";
         print "<td>$when</td>\n";
         print "</tr>\n";
     }
diff --git a/buglist.cgi b/buglist.cgi
index 6c0ff1e7f..ea3ec2049 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -1426,6 +1426,17 @@ document.write(\" <input type=button value=\\\"Uncheck All\\\" onclick=\\\"SetCh
 </TR>";
     }
 
+   print qq{
+<TR><TD ALIGN="RIGHT"><B>CC List:</B></TD>
+<TD COLSPAN=3><INPUT NAME="masscc" SIZE=32 VALUE="">
+<SELECT NAME="ccaction">
+<OPTION VALUE="add">Add these to the CC List
+<OPTION VALUE="remove">Remove these from the CC List
+</SELECT>
+</TD>
+</TR>
+};
+
     if (@::legal_keywords) {
         print qq{
 <TR><TD><B><A HREF="describekeywords.cgi">Keywords</A>:</TD>
@@ -1541,8 +1552,8 @@ if ($::usergroupset ne '0' && $buggroupset =~ /^\d+$/) {
 To make changes to a bunch of bugs at once:
 <ol>
 <li> Put check boxes next to the bugs you want to change.
-<li> Adjust above form elements.  (It's <b>always</b> a good idea to add some
-     comment explaining what you're doing.)
+<li> Adjust above form elements.  (If the change you are making requires
+       an explanation, include it in the comments box).
 <li> Click the below \"Commit\" button.
 </ol></font>
 <INPUT TYPE=SUBMIT VALUE=Commit>";
diff --git a/checksetup.pl b/checksetup.pl
index 8ce8bf6f7..bcbebff6d 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -768,8 +768,8 @@ $table{bugs_activity} =
     who mediumint not null,
     bug_when datetime not null,
     fieldid mediumint not null,
-    oldvalue tinytext,
-    newvalue tinytext,
+    added tinytext,
+    removed tinytext,
 
     index (bug_id),
     index (bug_when),
@@ -2307,6 +2307,72 @@ if (!defined GetIndexDef('longdescs','who')) {
 # truncates re http://bugzilla.mozilla.org/show_bug.cgi?id=9352
 ChangeFieldType('bugs', 'version','varchar(64) not null');
 
+# 2001-07-20 jake@acutex.net - Change bugs_activity to only record changes
+#  http://bugzilla.mozilla.org/show_bug.cgi?id=55161
+if (GetFieldDef('bugs_activity', 'oldvalue')) {
+    AddField("bugs_activity", "removed", "tinytext");
+    AddField("bugs_activity", "added", "tinytext");
+
+    # Need to get fieldid's for the fields that have multipule values
+    my @multi = ();
+    foreach my $f ("cc", "dependson", "blocked", "keywords") {
+        my $sth = $dbh->prepare("SELECT fieldid FROM fielddefs WHERE name = '$f'");
+        $sth->execute();
+        my ($fid) = $sth->fetchrow_array();
+        push (@multi, $fid);
+    } 
+
+    # Now we need to process the bugs_activity table and reformat the data
+    my $i = 0;
+    print "Fixing activity log ";
+    my $sth = $dbh->prepare("SELECT bug_id, who, bug_when, fieldid,
+                            oldvalue, newvalue FROM bugs_activity");
+    $sth->execute;
+    while (my ($bug_id, $who, $bug_when, $fieldid, $oldvalue, $newvalue) = $sth->fetchrow_array()) {
+        # print a "." every 500 records so the user knows we didn't die
+        print "." if !($i++ % 500); 
+        # Make sure (old|new)value isn't null (to suppress warnings)
+        $oldvalue ||= "";
+        $newvalue ||= "";
+        my ($added, $removed) = "";
+        if (grep /^$fieldid$/, @multi) {
+            my (@add, @remove) = ();
+            my @old = split(/[ ,]/, $oldvalue);
+            my @new = split(/[ ,]/, $newvalue);
+            # Find values that were "added"
+            foreach my $value(@new) {
+                if (! grep /^$value$/, @old) {
+                    push (@add, $value);
+                }
+            }
+            # Find values that were removed
+            foreach my $value(@old) {
+                if (! grep /^$value$/, @new) {
+                    push (@remove, $value);
+                }
+            }
+            $added = join (", ", @add);
+            $removed = join (", ", @remove);
+            # If we can't determine what changed, put a ? in both fields
+            unless ($added || $removed) {
+                $added = "?";
+                $removed = "?";
+            }
+        } else {
+            $removed = $oldvalue;
+            $added = $newvalue;
+        }
+        $added = $dbh->quote($added);
+        $removed = $dbh->quote($removed);
+        $dbh->do("UPDATE bugs_activity SET removed = $removed, added = $added
+                  WHERE bug_id = $bug_id AND who = $who
+                   AND bug_when = '$bug_when' AND fieldid = $fieldid");
+    }
+    print ". Done.\n";
+    DropField("bugs_activity", "oldvalue");
+    DropField("bugs_activity", "newvalue");
+} 
+
 # If you had to change the --TABLE-- definition in any way, then add your
 # differential change code *** A B O V E *** this comment.
 #
diff --git a/globals.pl b/globals.pl
index 302d9b8b7..51fe1ddee 100644
--- a/globals.pl
+++ b/globals.pl
@@ -1224,6 +1224,37 @@ sub Param ($) {
     die "Can't find param named $value";
 }
 
+# Take two comma or space separated strings and return what
+# values were removed from or added to the new one.
+sub DiffStrings {
+    my ($oldstr, $newstr) = @_;
+
+    my (@remove, @add) = ();
+    my @old = split(/[ ,]/, $oldstr);
+    my @new = split(/[ ,]/, $newstr);
+
+    # Find values that were removed
+    foreach my $value(@old) {
+        next if $value =~ /^\s*$/;
+        if (! grep /^$value$/, @new) {
+            push (@remove, $value);
+        }
+    }
+
+    # Find values that were added
+    foreach my $value(@new) {
+        next if $value =~ /^\s*$/;
+        if (! grep /^$value$/, @old) {
+            push (@add, $value);
+        }
+    }
+
+    my $removed = join (", ", @remove);
+    my $added = join (", ", @add);
+
+    return ($removed, $added);
+}
+
 sub PerformSubsts {
     my ($str, $substs) = (@_);
     $str =~ s/%([a-z]*)%/(defined $substs->{$1} ? $substs->{$1} : Param($1))/eg;
diff --git a/move.pl b/move.pl
index 42eb96351..37b8cb7ef 100755
--- a/move.pl
+++ b/move.pl
@@ -107,12 +107,12 @@ foreach my $id (split(/:/, $::FORM{'buglist'})) {
     my $fieldid = GetFieldID("bug_status");
     my $cur_status= $bug->bug_status;
     SendSQL("INSERT INTO bugs_activity " .
-            "(bug_id,who,bug_when,fieldid,oldvalue,newvalue) VALUES " .
+            "(bug_id,who,bug_when,fieldid,removed,added) VALUES " .
             "($id,$exporterid,now(),$fieldid,'$cur_status','RESOLVED')");
     $fieldid = GetFieldID("resolution");
     my $cur_res= $bug->resolution;
     SendSQL("INSERT INTO bugs_activity " .
-            "(bug_id,who,bug_when,fieldid,oldvalue,newvalue) VALUES " .
+            "(bug_id,who,bug_when,fieldid,removed,added) VALUES " .
             "($id,$exporterid,now(),$fieldid,'$cur_res','MOVED')");
 
     SendSQL("UPDATE bugs SET bug_status =\"RESOLVED\" where bug_id=\"$id\"");
diff --git a/process_bug.cgi b/process_bug.cgi
index 26ea412c1..b5ea3c782 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -456,32 +456,9 @@ if (defined $::FORM{'qa_contact'}) {
 
 ConnectToDatabase();
 
-my $formCcSet = new RelationSet;
-my $origCcSet = new RelationSet;
-my $origCcString;
 my $removedCcString = "";
 my $duplicate = 0;
 
-# We make sure to check out the CC list before we actually start touching any
-# bugs.  mergeFromString() ultimately searches the database using a quoted
-# form of the data it gets from $::FORM{'cc'}, so anything bogus from a 
-# security standpoint should trigger an abort there.
-#
-if (defined $::FORM{'newcc'} && defined $::FORM{'id'}) {
-    $origCcSet->mergeFromDB("select who from cc where bug_id = $::FORM{'id'}");
-    $formCcSet->mergeFromDB("select who from cc where bug_id = $::FORM{'id'}");
-    $origCcString = $origCcSet->toString();  # cache a copy of the string vers
-    if ((exists $::FORM{'removecc'}) && (exists $::FORM{'cc'})) {
-
-      # save off the folks removed from the CC list so they can be given to 
-      # the processmaill command line so they can be sent mail about it.
-      #
-      $removedCcString = join (',', @{$::MFORM{'cc'}});
-      $formCcSet->removeItemsInArray(@{$::MFORM{'cc'}});
-    }
-    $formCcSet->mergeFromString($::FORM{'newcc'});
-}
-
 if ( Param('strictvaluechecks') ) {
     CheckFormFieldDefined(\%::FORM, 'knob');
 }
@@ -686,10 +663,14 @@ sub LogDependencyActivity {
     my ($i, $oldstr, $target, $me) = (@_);
     my $newstr = SnapShotDeps($i, $target, $me);
     if ($oldstr ne $newstr) {
+        # Figure out what's really different...
+        my ($removed, $added) = DiffStrings($oldstr, $newstr);
+        $added = SqlQuote($added);
+        $removed = SqlQuote($removed);
         my $fieldid = GetFieldID($target);
         SendSQL("INSERT INTO bugs_activity " .
-                "(bug_id,who,bug_when,fieldid,oldvalue,newvalue) VALUES " .
-                "($i,$whoid,$timestamp,$fieldid,'$oldstr','$newstr')");
+                "(bug_id,who,bug_when,fieldid,removed,added) VALUES " .
+                "($i,$whoid,$timestamp,$fieldid,$removed,$added)");
         return 1;
     }
     return 0;
@@ -899,25 +880,67 @@ The changes made were:
         AppendComment($id, $::FORM{'who'}, $::FORM{'comment'});
     }
     
-    if (defined $::FORM{'newcc'} && defined $::FORM{'id'}
-        && ! $origCcSet->isEqual($formCcSet) ) {
-
-        # update the database to look like the form
-        #
-        my @CCDELTAS = $origCcSet->generateSqlDeltas($formCcSet, "cc", 
-                                                     "bug_id", $::FORM{'id'},
-                                                     "who");
-        $CCDELTAS[0] eq "" || SendSQL($CCDELTAS[0]);
-        $CCDELTAS[1] eq "" || SendSQL($CCDELTAS[1]);
-
-        my $col = GetFieldID('cc');
-        my $origq = SqlQuote($origCcString);
-        my $newq = SqlQuote($formCcSet->toString());
-        SendSQL("INSERT INTO bugs_activity " . 
-                "(bug_id,who,bug_when,fieldid,oldvalue,newvalue) VALUES " . 
-                "($id,$whoid,'$timestamp',$col,$origq,$newq)");
+    if (defined $::FORM{newcc} || defined $::FORM{removecc} || defined $::FORM{masscc}) {
+        # Get the current CC list for this bug
+        my %oncc;
+        SendSQL("SELECT who FROM cc WHERE bug_id = $id");
+        while (MoreSQLData()) {
+            $oncc{FetchOneColumn()} = 1;
+        }
+
+        # If masscc is defined, then we came from buglist and need to either add or
+        # remove cc's... otherwise, we came from bugform and may need to do both.
+        my ($cc_add, $cc_remove) = "";
+        if (defined $::FORM{masscc}) {
+            if ($::FORM{ccaction} eq 'add') {
+                $cc_add = $::FORM{masscc};
+            } elsif ($::FORM{ccaction} eq 'remove') {
+                $cc_remove = $::FORM{masscc};
+            }
+        } else {
+            $cc_add = $::FORM{newcc};
+            # We came from bug_form which uses a select box to determine what cc's
+            # need to be removed...
+            if (defined $::FORM{removecc}) {
+                $cc_remove = join (",", @{$::MFORM{cc}});
+            }
+        }
+
+        my (@added, @removed) = ();
+        if ($cc_add) {
+            my @new = split(/[ ,]/, $cc_add);
+            foreach my $person (@new) {
+                my $pid = DBNameToIdAndCheck($person);
+                # If this person isn't already on the cc list, add them
+                if (! $oncc{$pid}) {
+                    SendSQL("INSERT INTO cc (bug_id, who) VALUES ($id, $pid)");
+                    push (@added, $person);
+                }
+            }
+        }
+        if ($cc_remove) {
+            my @old = split (/[ ,]/, $cc_remove);
+            foreach my $person (@old) {
+                my $pid = DBNameToIdAndCheck($person);
+                # If the person is on the cc list, remove them
+                if ($oncc{$pid}) {
+                    SendSQL("DELETE FROM cc WHERE bug_id = $id AND who = $pid");
+                    push (@removed, $person);
+                }
+            }
+            # Save off the removedCcString so it can be fed to processmail
+            $removedCcString = join (",", @removed);
+        }
+        # If any changes were found, record it in the activity log
+        if (scalar(@removed) || scalar(@added)) {
+            my $col = GetFieldID('cc');
+            my $removed = SqlQuote(join(", ", @removed));
+            my $added = SqlQuote(join(", ", @added));
+            SendSQL("INSERT INTO bugs_activity " .
+                    "(bug_id,who,bug_when,fieldid,removed,added) VALUES " .
+                    "($id,$whoid,'$timestamp',$col,$removed,$added)");
+        }
     }
-  
 
     if (defined $::FORM{'dependson'}) {
         my $me = "blocked";
@@ -1013,6 +1036,11 @@ The changes made were:
                 $origQaContact = $old;
             }
 
+            # If this is the keyword field, only record the changes, not everything.
+            if ($col eq 'keywords') {
+                ($old, $new) = DiffStrings($old, $new);
+            }
+
             if ($col eq 'product') {
                 RemoveVotes($id, 0,
                             "This bug has been moved to a different product");
@@ -1020,7 +1048,7 @@ The changes made were:
             $col = GetFieldID($col);
             $old = SqlQuote($old);
             $new = SqlQuote($new);
-            my $q = "insert into bugs_activity (bug_id,who,bug_when,fieldid,oldvalue,newvalue) values ($id,$whoid,'$timestamp',$col,$old,$new)";
+            my $q = "insert into bugs_activity (bug_id,who,bug_when,fieldid,removed,added) values ($id,$whoid,'$timestamp',$col,$old,$new)";
             # puts "<pre>$q</pre>"
             SendSQL($q);
         }
@@ -1054,17 +1082,10 @@ The changes made were:
         my $isoncc = FetchOneColumn();
         unless ($isreporter || $isoncc) {
             # The reporter is oblivious to the existance of the new bug... add 'em to the cc (and record activity)
-            SendSQL("SELECT who FROM cc WHERE bug_id = " . SqlQuote($duplicate));
-            my @dupecc;
-            while (MoreSQLData()) {
-                push (@dupecc, DBID_to_name(FetchOneColumn()));
-            }
-            my @newdupecc = @dupecc;
-            push (@newdupecc, DBID_to_name($reporter));
             my $ccid = GetFieldID("cc");
             my $whochange = DBNameToIdAndCheck($::FORM{'who'});
-            SendSQL("INSERT INTO bugs_activity (bug_id,who,bug_when,fieldid,oldvalue,newvalue) VALUES " .
-                    "('$duplicate','$whochange',now(),$ccid,'" . join (",", sort @dupecc) . "','" . join (",", sort @newdupecc) . "')"); 
+            SendSQL("INSERT INTO bugs_activity (bug_id,who,bug_when,fieldid,removed,added) VALUES " .
+                    "('$duplicate','$whochange',now(),$ccid,'','" . DBID_to_name($reporter) . "')"); 
             SendSQL("INSERT INTO cc (who, bug_id) VALUES ($reporter, " . SqlQuote($duplicate) . ")");
         }
         AppendComment($duplicate, $::FORM{'who'}, "*** Bug $::FORM{'id'} has been marked as a duplicate of this bug. ***");
diff --git a/processmail b/processmail
index 0fcdbbdde..0f2dac947 100755
--- a/processmail
+++ b/processmail
@@ -135,7 +135,7 @@ sub ProcessOneBug {
 
 
     SendSQL("SELECT profiles.login_name, fielddefs.description, " .
-            "       bug_when, oldvalue, newvalue " .
+            "       bug_when, removed, added " .
             "FROM bugs_activity, fielddefs, profiles " .
             "WHERE bug_id = $id " .
             "  AND fielddefs.fieldid = bugs_activity.fieldid " .
@@ -157,7 +157,7 @@ sub ProcessOneBug {
         if ($who ne $lastwho) {
             $lastwho = $who;
             $difftext .= "\n$who" . Param('emailsuffix') . " changed:\n\n";
-            $difftext .= FormatTriple("What    ", "Old Value", "New Value");
+            $difftext .= FormatTriple("What    ", "Removed", "Added");
             $difftext .= ('-' x 76) . "\n";
         }
         $difftext .= FormatTriple($what, $old, $new);
@@ -171,7 +171,7 @@ sub ProcessOneBug {
     my $resid = 
 
     SendSQL("SELECT bugs_activity.bug_id, fielddefs.name, " .
-            "       oldvalue, newvalue " .
+            "       removed, added " .
             "FROM bugs_activity, dependencies, fielddefs ".
             "WHERE bugs_activity.bug_id = dependencies.dependson " .
             "  AND dependencies.blocked = $id " .
@@ -414,9 +414,7 @@ sub getEmailAttributes ($@) {
         } elsif ($fieldName eq 'QAContact') {
            push (@{$force{'QAContact'}}, $new);
         } elsif ($fieldName eq 'CC') {
-            my @oldVal = split (/,/, $old);
-            my @newVal = split (/,/, $new);
-            my @added = filterExcludeList(\@newVal, \@oldVal);
+            my @added = split (/[ ,]/, $new);
             push (@{$force{'CClist'}}, @added);
         }
     }