Escape commas in multi-select activity log, detect changes for multi-selects during mid-air collisions

Bugzilla/Bug.pm

@@ -1401,8 +1401,8 @@ sub save_multiselects
         if (@$removed || @$added)
         {
             $changes->{$name} = [
-                join(', ', map { $old{$_}->name } @$removed),
+                join_escaped(', ', ',', map { $old{$_}->name } @$removed),
-                join(', ', map { $new{$_}->name } @$added)
+                join_escaped(', ', ',', map { $new{$_}->name } @$added),
             ];
             Bugzilla->dbh->do("DELETE FROM ".$field->value_type->REL_TABLE." WHERE bug_id = ?", undef, $self->id);
             if (@{$self->$name})
@@ -2771,22 +2771,22 @@ sub set_comment_worktimeonly
 sub modify_keywords
 {
     my ($self, $keywords, $descriptions, $action) = @_;
-
+    $keywords = [ split /[\s,]*,[\s,]*/, $keywords ] if !ref $keywords;
     if ($action eq 'delete')
     {
         my $old_kw = $self->keywords_obj;
         my $kw = { map { lc($_->name) => $_ } @$old_kw };
-        delete $kw->{lc $_} for split /[\s,]*,[\s,]*/, $keywords;
+        delete $kw->{lc $_} for @$keywords;
         $self->set('keywords', { keyword_objects => [ values %$kw ] });
     }
     else
     {
         if ($action eq 'add')
         {
-            $keywords .= ', '.$self->get_string('keywords');
+            push @$keywords, map { $_->name } $self->get_object('keywords');
         }
         $self->set('keywords', {
-            keywords => $keywords,
+            keywords => join(', ', @$keywords),
             descriptions => http_decode_query($descriptions),
         });
     }

Bugzilla/Util.pm

@@ -46,7 +46,7 @@ use base qw(Exporter);
     stem_text intersect union
     get_text disable_utf8 bz_encode_json
     xml_element xml_element_quote xml_dump_simple xml_simple
-    Dumper http_build_query http_decode_query
+    Dumper http_build_query http_decode_query join_escaped split_escaped
 );
 
 use Bugzilla::Constants;
@@ -1014,6 +1014,26 @@ sub xml_simple_char
     $frame->{char} .= $text;
 }
 
+sub join_escaped
+{
+    my ($str, $re, @array) = @_;
+    s/($re|\\)/\\$1/gs for @array;
+    return join $str, @array;
+}
+
+sub split_escaped
+{
+    my ($re, $s, $limit) = @_;
+    my @r;
+    while ($s =~ s/^(.*(?:^|[^\\])(?:\\\\)*)$re//gs)
+    {
+        push @r, $1;
+    }
+    push @r, $s if length $s;
+    s/\\(.)/$1/gso for @r;
+    return @r;
+}
+
 1;
 __END__
 

process_bug.cgi

@@ -144,40 +144,80 @@ if ($ARGS->{delta_ts})
     {
         ($vars->{operations}) = Bugzilla::Bug::GetBugActivity($first_bug->id, undef, $ARGS->{delta_ts});
 
-        # CustIS Bug 56327 - Change only fields the user wanted to change
+        ## Change only fields the user wanted to change (Originally CustIS Bug 56327)
+
+        # Merge all changes into a single hash
+        my $add_rm = {};
         for my $op (@{$vars->{operations}})
         {
-            for (@{$op->{changes}})
+            for my $chg (@{$op->{changes}})
             {
-                # FIXME similar detection is needed for other multi-selects
+                if ($chg->{fieldname} eq 'dependson' || $chg->{fieldname} eq 'blocked' ||
-                if ($_->{fieldname} eq 'dependson' || $_->{fieldname} eq 'blocked')
+                    Bugzilla->get_field($chg->{fieldname})->type == FIELD_TYPE_MULTI_SELECT)
                 {
-                    # Calculate old value from current value and activity log
+                    my @rm = split_escaped(',\s*', $chg->{removed});
-                    my $cur = $_->{fieldname};
+                    my @add = split_escaped(',\s*', $chg->{added});
-                    $cur = { map { $_ => 1 } @{ $first_bug->$cur() } };
+                    my $h = ($add_rm->{$chg->{fieldname}} ||= [ {}, {} ]);
-                    my $new = join ', ', keys %$cur;
+                    for (@rm)
-                    delete $cur->{$_} for split /[\s,]*,[\s,]*/, $_->{added};
-                    $cur->{$_} = 1 for split /[\s,]*,[\s,]*/, $_->{removed};
-                    # Compare the old value with submitted one
-                    my $equal = 1;
-                    for (split /[\s,]*,[\s,]*/, $ARGS->{$_->{fieldname}})
                     {
-                        if (!$cur->{$_})
+                        delete $h->[1]->{$_} or $h->[0]->{$_} = 1;
-                        {
+                    }
-                            $equal = 0;
+                    for (@add)
-                            last;
+                    {
-                        }
+                        delete $h->[0]->{$_} or $h->[1]->{$_} = 1;
-                        delete $cur->{$_};
                     }
-                    $equal = 0 if %$cur;
-                    # If equal to old value -> change to the new value
-                    $ARGS->{$_->{fieldname}} = $new if $equal;
                 }
-                elsif ($ARGS->{$_->{fieldname}} eq $_->{removed})
+                elsif (!defined $add_rm->{$chg->{fieldname}})
                 {
-                    # If equal to old value -> change to the new value
+                    $add_rm->{$chg->{fieldname}} = [ $chg->{removed}, $chg->{added} ];
-                    $ARGS->{$_->{fieldname}} = $_->{added};
                 }
+                else
+                {
+                    $add_rm->{$chg->{fieldname}}->[1] = $chg->{added};
+                }
+            }
+        }
+
+        for my $field (keys %$add_rm)
+        {
+            my ($removed, $added) = @{$add_rm->{$field}};
+            # FIXME Also detect bug_group changes?
+            if ($field eq 'dependson' || $field eq 'blocked' ||
+                Bugzilla->get_field($field)->type == FIELD_TYPE_MULTI_SELECT)
+            {
+                # Restore old value by rolling back the activity
+                my %new;
+                if ($field eq 'dependson' || $field eq 'blocked')
+                {
+                    %new = (map { $_ => 1 } @{ $first_bug->$field() });
+                }
+                else
+                {
+                    %new = (map { $_->name => 1 } @{ $first_bug->get_object($field) });
+                }
+                my %old = %new;
+                delete $old{$_} for keys %$added;
+                $old{$_} = 1 for keys %$removed;
+                # Compare old value with the submitted one
+                my $equal = 1;
+                $ARGS->{$field} = '' if !defined $ARGS->{$field};
+                for (ref $ARGS->{$field} ? @{$ARGS->{$field}} : split /[\s,]*,[\s,]*/, $ARGS->{$field})
+                {
+                    if (!$old{$_})
+                    {
+                        $equal = 0;
+                        last;
+                    }
+                    delete $old{$_};
+                }
+                $equal = 0 if %old;
+                # If equal to old value -> change to the new value
+                $ARGS->{$field} = [ keys %new ] if $equal;
+            }
+            elsif ($ARGS->{$field} eq $removed)
+            {
+                # If equal to old value -> change to the new value
+                $ARGS->{$field} = $added;
             }
         }