Bug 287330: Multi-Select Custom Fields

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit

webtools/bugzilla/Bugzilla/Bug.pm

@@ -70,6 +70,9 @@ use constant LIST_ORDER => ID_FIELD;
 # This is a sub because it needs to call other subroutines.
 sub DB_COLUMNS {
     my $dbh = Bugzilla->dbh;
+    my @custom = Bugzilla->get_fields({ custom => 1, obsolete => 0});
+    @custom = grep {$_->type != FIELD_TYPE_MULTI_SELECT} @custom;
+    my @custom_names = map {$_->name} @custom;
     return qw(
         alias
         bug_file_loc
@@ -98,7 +101,7 @@ sub DB_COLUMNS {
     'qa_contact  AS qa_contact_id',
     $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ' AS creation_ts',
     $dbh->sql_date_format('deadline', '%Y-%m-%d') . ' AS deadline',
-    Bugzilla->custom_field_names;
+    @custom_names;
 }
 
 use constant REQUIRED_CREATE_FIELDS => qw(
@@ -140,6 +143,9 @@ sub VALIDATORS {
         if ($field->type == FIELD_TYPE_SINGLE_SELECT) {
             $validator = \&_check_select_field;
         }
+        elsif ($field->type == FIELD_TYPE_MULTI_SELECT) {
+            $validator = \&_check_multi_select_field;
+        }
         else {
             $validator = \&_check_freetext_field;
         }
@@ -157,6 +163,9 @@ use constant UPDATE_VALIDATORS => {
 };
 
 sub UPDATE_COLUMNS {
+    my @custom = Bugzilla->get_fields({ custom => 1, obsolete => 0});
+    @custom = grep {$_->type != FIELD_TYPE_MULTI_SELECT} @custom;
+    my @custom_names = map {$_->name} @custom;
     my @columns = qw(
         alias
         cclist_accessible
@@ -175,7 +184,7 @@ sub UPDATE_COLUMNS {
         short_desc
         status_whiteboard
     );
-    push(@columns, Bugzilla->custom_field_names);
+    push(@columns, @custom_names);
     return @columns;
 };
 
@@ -303,14 +312,10 @@ sub create {
 
     # These are not a fields in the bugs table, so we don't pass them to
     # insert_create_data.
-    my $cc_ids = $params->{cc};
+    my $cc_ids     = delete $params->{cc};
-    delete $params->{cc};
+    my $groups     = delete $params->{groups};
-    my $groups = $params->{groups};
+    my $depends_on = delete $params->{dependson};
-    delete $params->{groups};
+    my $blocked    = delete $params->{blocked};
-    my $depends_on = $params->{dependson};
-    delete $params->{dependson};
-    my $blocked = $params->{blocked};
-    delete $params->{blocked};
     my ($comment, $privacy) = ($params->{comment}, $params->{commentprivacy});
     delete $params->{comment};
     delete $params->{commentprivacy};
@@ -322,9 +327,9 @@ sub create {
 
     # We don't want the bug to appear in the system until it's correctly
     # protected by groups.
-    my $timestamp = $params->{creation_ts}; 
+    my $timestamp = delete $params->{creation_ts}; 
-    delete $params->{creation_ts};
 
+    my $ms_values = $class->_extract_multi_selects($params);
     my $bug = $class->insert_create_data($params);
 
     # Add the group restrictions
@@ -372,6 +377,16 @@ sub create {
         $sth_bug_time->execute($timestamp, $blocked_id);
     }
 
+    # Insert the values into the multiselect value tables
+    foreach my $field (keys %$ms_values) {
+        $dbh->do("DELETE FROM bug_$field where bug_id = ?",
+                undef, $bug->bug_id);
+        foreach my $value ( @{$ms_values->{$field}} ) {
+            $dbh->do("INSERT INTO bug_$field (bug_id, value) VALUES (?,?)",
+                    undef, $bug->bug_id, $value);
+        }
+    }
+
     $dbh->bz_commit_transaction();
 
     # Because MySQL doesn't support transactions on the longdescs table,
@@ -469,6 +484,7 @@ sub update {
     my $delta_ts = shift || $dbh->selectrow_array("SELECT NOW()");
     $self->{delta_ts} = $delta_ts;
 
+    my $old_bug = $self->new($self->id);
     my $changes = $self->SUPER::update(@_);
 
     foreach my $comment (@{$self->{added_comments} || []}) {
@@ -480,6 +496,24 @@ sub update {
                  $self->bug_id, Bugzilla->user->id, $delta_ts, @values);
     }
 
+    # Insert the values into the multiselect value tables
+    my @multi_selects = Bugzilla->get_fields(
+        { custom => 1, type => FIELD_TYPE_MULTI_SELECT, obsolete => 0 });
+    foreach my $field (@multi_selects) {
+        my $name = $field->name;
+        my ($removed, $added) = diff_arrays($old_bug->$name, $self->$name);
+        if (scalar @$removed || scalar @$added) {
+            $changes->{$name} = [join(', ', @$removed), join(', ', @$added)];
+
+            $dbh->do("DELETE FROM bug_$name where bug_id = ?",
+                     undef, $self->id);
+            foreach my $value (@{$self->$name}) {
+                $dbh->do("INSERT INTO bug_$name (bug_id, value) VALUES (?,?)",
+                         undef, $self->id, $value);
+            }
+        }
+    }
+
     # Log bugs_activity items
     # XXX Eventually, when bugs_activity is able to track the dupe_id,
     # this code should go below the duplicates-table-updating code below.
@@ -504,6 +538,25 @@ sub update {
     return $changes;
 }
 
+# Used by create().
+# We need to handle multi-select fields differently than normal fields,
+# because they're arrays and don't go into the bugs table.
+sub _extract_multi_selects {
+    my ($invocant, $params) = @_;
+
+    my @multi_selects = Bugzilla->get_fields(
+        { custom => 1, type => FIELD_TYPE_MULTI_SELECT, obsolete => 0 });
+    my %ms_values;
+    foreach my $field (@multi_selects) {
+        my $name = $field->name;
+        if (exists $params->{$name}) {
+            my $array = delete($params->{$name}) || [];
+            $ms_values{$name} = $array;
+        }
+    }
+    return \%ms_values;
+}
+
 # XXX Temporary hack until all of process_bug uses update().
 sub update_cc {
     my $self = shift;
@@ -1171,6 +1224,19 @@ sub _check_work_time {
     return $_[0]->_check_time($_[1], 'work_time');
 }
 
+# Custom Field Validators
+
+sub _check_multi_select_field {
+    my ($invocant, $values, $field) = @_;
+    return [] if !$values;
+    foreach my $value (@$values) {
+        $value = trim($value);
+        check_field($field, $value);
+        trick_taint($value);
+    }
+    return $values;
+}
+
 sub _check_select_field {
     my ($invocant, $value, $field) = @_;
     $value = trim($value);
@@ -1233,6 +1299,9 @@ sub set_alias { $_[0]->set('alias', $_[1]); }
 sub set_cclist_accessible { $_[0]->set('cclist_accessible', $_[1]); }
 sub set_custom_field {
     my ($self, $field, $value) = @_;
+    if (ref $value eq 'ARRAY' && !$field->type == FIELD_TYPE_MULTI_SELECT) {
+        $value = $value->[0];
+    }
     ThrowCodeError('field_not_custom', { field => $field }) if !$field->custom;
     $self->set($field->name, $value);
 }
@@ -2670,6 +2739,9 @@ sub check_can_change_field {
     # Return true if they haven't changed this field at all.
     if ($oldvalue eq $newvalue) {
         return 1;
+    } elsif (ref($newvalue) eq 'ARRAY' && ref($oldvalue) eq 'ARRAY') {
+        my ($removed, $added) = diff_arrays($oldvalue, $newvalue);
+        return 1 if !scalar(@$removed) && !scalar(@$added);
     } elsif (trim($oldvalue) eq trim($newvalue)) {
         return 1;
     # numeric fields need to be compared using ==
@@ -2941,11 +3013,19 @@ sub AUTOLOAD {
   no strict 'refs';
   *$AUTOLOAD = sub {
       my $self = shift;
-      if (defined $self->{$attr}) {
+
+      return $self->{$attr} if defined $self->{$attr};
+
+      $self->{_multi_selects} ||= [Bugzilla->get_fields(
+          {custom => 1, type => FIELD_TYPE_MULTI_SELECT })];
+      if ( grep($_->name eq $attr, @{$self->{_multi_selects}}) ) {
+          $self->{$attr} ||= Bugzilla->dbh->selectcol_arrayref(
+              "SELECT value FROM bug_$attr WHERE bug_id = ? ORDER BY value",
+              undef, $self->id);
           return $self->{$attr};
-      } else {
-          return '';
       }
+
+      return '';
   };
 
   goto &$AUTOLOAD;

webtools/bugzilla/Bugzilla/Constants.pm

@@ -119,6 +119,7 @@ use File::Basename;
     FIELD_TYPE_UNKNOWN
     FIELD_TYPE_FREETEXT
     FIELD_TYPE_SINGLE_SELECT
+    FIELD_TYPE_MULTI_SELECT
 
     USAGE_MODE_BROWSER
     USAGE_MODE_CMDLINE
@@ -340,6 +341,7 @@ use constant SENDMAIL_PATH => '/usr/lib:/usr/sbin:/usr/ucblib';
 use constant FIELD_TYPE_UNKNOWN   => 0;
 use constant FIELD_TYPE_FREETEXT  => 1;
 use constant FIELD_TYPE_SINGLE_SELECT => 2;
+use constant FIELD_TYPE_MULTI_SELECT => 3;
 
 # The maximum number of days a token will remain valid.
 use constant MAX_TOKEN_AGE => 3;

webtools/bugzilla/Bugzilla/DB.pm

@@ -642,9 +642,8 @@ sub _bz_add_table_raw {
     $self->do($_) foreach (@statements);
 }
 
-sub bz_add_field_table {
+sub _bz_add_field_table {
-    my ($self, $name) = @_;
+    my ($self, $name, $table_schema) = @_;
-    my $table_schema = $self->_bz_schema->FIELD_TABLE_SCHEMA;
     # We do nothing if the table already exists.
     return if $self->bz_table_info($name);
     my $indexes      = $table_schema->{INDEXES};
@@ -659,6 +658,19 @@ sub bz_add_field_table {
     $self->bz_add_table($name);
 }
 
+sub bz_add_field_tables {
+    my ($self, $field) = @_;
+    
+    $self->_bz_add_field_table($field->name,
+                                $self->_bz_schema->FIELD_TABLE_SCHEMA);
+    if ( $field->type == FIELD_TYPE_MULTI_SELECT ) {
+        $self->_bz_add_field_table('bug_' . $field->name,
+                $self->_bz_schema->MULTI_SELECT_VALUE_TABLE);
+    }
+
+}
+
+
 sub bz_drop_column {
     my ($self, $table, $column) = @_;
 

webtools/bugzilla/Bugzilla/DB/Schema.pm

@@ -1254,6 +1254,18 @@ use constant FIELD_TABLE_SCHEMA => {
         sortkey_idx => ['sortkey', 'value'],
     ],
 };
+
+use constant MULTI_SELECT_VALUE_TABLE => {
+    FIELDS => [
+        bug_id => {TYPE => 'INT3', NOTNULL => 1},
+        value  => {TYPE => 'varchar(64)', NOTNULL => 1},
+    ],
+    INDEXES => [
+        bug_id_idx => {FIELDS => [qw( bug_id value)], TYPE => 'UNIQUE'},
+    ],
+};
+
+
 #--------------------------------------------------------------------------
 
 =head1 METHODS

webtools/bugzilla/Bugzilla/Field.pm

@@ -252,9 +252,9 @@ sub _check_sortkey {
 sub _check_type {
     my ($invocant, $type) = @_;
     my $saved_type = $type;
-    # FIELD_TYPE_SINGLE_SELECT here should be updated every time a new,
+    # The constant here should be updated every time a new,
     # higher field type is added.
-    (detaint_natural($type) && $type <= FIELD_TYPE_SINGLE_SELECT)
+    (detaint_natural($type) && $type <= FIELD_TYPE_MULTI_SELECT)
       || ThrowCodeError('invalid_customfield_type', { type => $saved_type });
     return $type;
 }
@@ -454,13 +454,20 @@ sub create {
     if ($field->custom) {
         my $name = $field->name;
         my $type = $field->type;
+        if (SQL_DEFINITIONS->{$type}) {
             # Create the database column that stores the data for this field.
             $dbh->bz_add_column('bugs', $name, SQL_DEFINITIONS->{$type});
+        }
+
+        if ($type == FIELD_TYPE_SINGLE_SELECT
+                || $type == FIELD_TYPE_MULTI_SELECT) 
+        {
+            # Create the table that holds the legal values for this field.
+            $dbh->bz_add_field_tables($field);
+        }
 
         if ($type == FIELD_TYPE_SINGLE_SELECT) {
-            # Create the table that holds the legal values for this field.
+            # Insert a default value of "---" into the legal values table.
-            $dbh->bz_add_field_table($name);
-            # And insert a default value of "---" into it.
             $dbh->do("INSERT INTO $name (value) VALUES ('---')");
         }
     }

webtools/bugzilla/colchange.cgi

@@ -80,7 +80,9 @@ if (Bugzilla->user->in_group(Bugzilla->params->{"timetrackinggroup"})) {
 
 push(@masterlist, ("short_desc", "short_short_desc"));
 
-push(@masterlist, Bugzilla->custom_field_names);
+my @custom_fields = grep { $_->type != FIELD_TYPE_MULTI_SELECT }
+                         Bugzilla->get_fields({ custom => 1, obsolete => 0 });
+push(@masterlist, map { $_->name } @custom_fields);
 
 $vars->{'masterlist'} = \@masterlist;
 

webtools/bugzilla/editvalues.cgi

@@ -41,6 +41,8 @@ our @valid_fields = ('op_sys', 'rep_platform', 'priority', 'bug_severity',
 # Add custom select fields.
 my @custom_fields = Bugzilla->get_fields({custom => 1,
                                           type => FIELD_TYPE_SINGLE_SELECT});
+push(@custom_fields, Bugzilla->get_fields({custom => 1,
+                                          type => FIELD_TYPE_MULTI_SELECT}));
 
 push(@valid_fields, map { $_->name } @custom_fields);
 

webtools/bugzilla/post_bug.cgi

@@ -120,8 +120,8 @@ $template->process($format->{'template'}, $vars, \$comment)
     || ThrowTemplateError($template->error());
 
 # Include custom fields editable on bug creation.
-my @custom_bug_fields = Bugzilla->get_fields(
+my @custom_bug_fields = grep {$_->type != FIELD_TYPE_MULTI_SELECT}
-    { custom => 1, obsolete => 0, enter_bug => 1 });
+        Bugzilla->get_fields({ custom => 1, obsolete => 0, enter_bug => 1 });
 
 # Undefined custom fields are ignored to ensure they will get their default
 # value (e.g. "---" for custom single select fields).
@@ -167,6 +167,13 @@ $bug_params{'cc'}          = [$cgi->param('cc')];
 $bug_params{'groups'}      = \@selected_groups;
 $bug_params{'comment'}     = $comment;
 
+my @multi_selects = Bugzilla->get_fields(
+        { type => FIELD_TYPE_MULTI_SELECT, custom => 1, obsolete => 0,
+            enter_bug => 1 });
+foreach my $field (@multi_selects) {
+    $bug_params{$field->name} = [$cgi->param($field->name)];
+}
+
 my $bug = Bugzilla::Bug->create(\%bug_params);
 
 # Get the bug ID back.

webtools/bugzilla/process_bug.cgi

@@ -571,11 +571,11 @@ foreach my $field ("version", "target_milestone") {
 # Add custom fields data to the query that will update the database.
 foreach my $field (Bugzilla->get_fields({custom => 1, obsolete => 0})) {
     my $fname = $field->name;
-    if (defined $cgi->param($fname)
+    if ( (defined $cgi->param($fname) || defined $cgi->param("defined_$fname"))
          && (!$cgi->param('dontchange')
              || $cgi->param($fname) ne $cgi->param('dontchange')))
     {
-        $_->set_custom_field($field, $cgi->param($fname)) foreach @bug_objects;
+        $_->set_custom_field($field, [$cgi->param($fname)]) foreach @bug_objects;
     }
 }
 
@@ -1010,6 +1010,11 @@ foreach my $id (@idlist) {
     my $bug_changed = 0;
     my $write = "WRITE";        # Might want to make a param to control
                                 # whether we do LOW_PRIORITY ...
+
+    my @multi_select_locks  = map {'bug_' . $_->name . " $write"}
+        Bugzilla->get_fields({ custom => 1, type => FIELD_TYPE_MULTI_SELECT,
+                               obsolete => 0 });
+
     $dbh->bz_lock_tables("bugs $write", "bugs_activity $write", "cc $write",
             "profiles READ", "dependencies $write", "votes $write",
             "products READ", "components READ", "milestones READ",
@@ -1020,7 +1025,8 @@ foreach my $id (@idlist) {
             "keyworddefs READ", "groups READ", "attachments READ",
             "bug_status READ", "group_control_map AS oldcontrolmap READ",
             "group_control_map AS newcontrolmap READ",
-            "group_control_map READ", "email_setting READ", "classifications READ");
+            "group_control_map READ", "email_setting READ", 
+            "classifications READ", @multi_select_locks);
 
     # It may sound crazy to set %formhash for each bug as $cgi->param()
     # will not change, but %formhash is modified below and we prefer

webtools/bugzilla/query.cgi

@@ -253,10 +253,21 @@ $vars->{'priority'} = get_legal_field_values('priority');
 $vars->{'bug_severity'} = get_legal_field_values('bug_severity');
 
 # Boolean charts
-my @fields;
+my @fields = Bugzilla->get_fields({ obsolete => 0 });
-push(@fields, { name => "noop", description => "---" });
+# Multi-selects aren't searchable, currently.
-push(@fields, $dbh->bz_get_field_defs());
+@fields = grep($_->type != FIELD_TYPE_MULTI_SELECT, @fields);
-@fields = sort {lc($a->{'description'}) cmp lc($b->{'description'})} @fields;
+
+# If we're not in the time-tracking group, exclude time-tracking fields.
+if (!Bugzilla->user->in_group(Bugzilla->params->{'timetrackinggroup'})) {
+    foreach my $tt_field (qw(estimated_time remaining_time work_time
+                             percentage_complete deadline))
+    {
+        @fields = grep($_->name ne $tt_field, @fields);
+    }
+}
+
+@fields = sort {lc($a->description) cmp lc($b->description)} @fields;
+unshift(@fields, { name => "noop", description => "---" });
 $vars->{'fields'} = \@fields;
 
 # Creating new charts - if the cmd-add value is there, we define the field

webtools/bugzilla/template/en/default/admin/custom_fields/edit.html.tmpl

@@ -84,7 +84,8 @@
       <th>&nbsp;</th>
       <td>&nbsp;</td>
     </tr>
-    [% IF field.type == constants.FIELD_TYPE_SINGLE_SELECT %]
+    [% IF field.type == constants.FIELD_TYPE_SINGLE_SELECT
+          || field.type == constants.FIELD_TYPE_MULTI_SELECT %]
       <tr>
         <th>&nbsp;</th>
         <td colspan="3">

webtools/bugzilla/template/en/default/bug/field.html.tmpl

@@ -42,21 +42,42 @@
   [% SWITCH field.type %]
     [% CASE constants.FIELD_TYPE_FREETEXT %]
         <input name="[% field.name FILTER html %]" value="[% value FILTER html %]" size="60">
-    [% CASE constants.FIELD_TYPE_SINGLE_SELECT %]
+    [% CASE [ constants.FIELD_TYPE_SINGLE_SELECT 
-        <select id="[% field.name FILTER html %]" name="[% field.name FILTER html %]">
+              constants.FIELD_TYPE_MULTI_SELECT ] %]
+        <select id="[% field.name FILTER html %]" 
+                name="[% field.name FILTER html %]" 
+                [% IF field.type == constants.FIELD_TYPE_MULTI_SELECT %]
+                    [% SET field_size = 5 %]
+                    [% IF field.legal_values.size < 5 %]
+                        [% SET field_size = field.legal_values.size %]
+                    [% END %]
+                    size="[% field_size FILTER html %]" multiple="multiple"
+                [% END %]
+                >
           [% IF allow_dont_change %]
-            <option value="[% dontchange FILTER html %]">
+            <option value="[% dontchange FILTER html %]"
+                   [% ' selected="selected"' IF value == dontchange %]>
               [% dontchange FILTER html %]
             </option>
           [% END %]
           [% FOREACH legal_value = field.legal_values %]
             <option value="[% legal_value FILTER html %]"
-                [%- " selected=\"selected\"" IF value == legal_value %]>
+                [%- " selected=\"selected\"" IF value.contains(legal_value).size %]>
                 [%- legal_value FILTER html %]</option>
           [% END %]
         </select>
+        [%# When you pass an empty multi-select in the web interface,
+          # it doesn't appear at all in the CGI object. Instead of
+          # forcing all users of process_bug to always specify every
+          # multi-select, we have this field defined if the multi-select
+          # field is defined, and then if this is passed but the multi-select
+          # isn't, we know that the multi-select was emptied.
+        %]
+        [% IF field.type == constants.FIELD_TYPE_MULTI_SELECT %]
+          <input type="hidden" name="defined_[% field.name FILTER html %]">
+        [% END %]
   [% END %]
 [% ELSE %]
-  [% value FILTER html %]
+  [% value.join(', ') FILTER html %]
 [% END %]
 </td>

webtools/bugzilla/template/en/default/global/field-descs.none.tmpl

@@ -80,7 +80,9 @@
 
 [% field_types = { ${constants.FIELD_TYPE_UNKNOWN}       => "Unknown Type",
                    ${constants.FIELD_TYPE_FREETEXT}      => "Free Text",
-                   ${constants.FIELD_TYPE_SINGLE_SELECT} => "Drop Down" } %]
+                   ${constants.FIELD_TYPE_SINGLE_SELECT} => "Drop Down",
+                   ${constants.FIELD_TYPE_MULTI_SELECT}  => "Multiple-Selection Box",
+                } %]
 
 [% status_descs = { "UNCONFIRMED" => "UNCONFIRMED",
                     "NEW"         => "NEW",

webtools/bugzilla/template/en/default/global/user-error.html.tmpl

@@ -661,10 +661,10 @@
     You tried to change the
     <strong>[% field_descs.$field FILTER html %]</strong> field 
     [% IF oldvalue.defined %]
-      from <em>[% oldvalue FILTER html %]</em>
+      from <em>[% oldvalue.join(', ') FILTER html %]</em>
     [% END %]
     [% IF newvalue.defined %]
-      to <em>[% newvalue FILTER html %]</em>
+      to <em>[% newvalue.join(', ') FILTER html %]</em>
     [% END %]
     , but only
     [% IF privs < 3 %]