Bug 287330: Multi-Select Custom Fields

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

5 files changed, 138 insertions, 25 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 7ed76311f..631c77caf 100755
--- a/Bugzilla/Bug.pm
+++ b/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};
-    delete $params->{cc};
-    my $groups = $params->{groups};
-    delete $params->{groups};
-    my $depends_on = $params->{dependson};
-    delete $params->{dependson};
-    my $blocked = $params->{blocked};
-    delete $params->{blocked};
+    my $cc_ids     = delete $params->{cc};
+    my $groups     = delete $params->{groups};
+    my $depends_on = delete $params->{dependson};
+    my $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}; 
-    delete $params->{creation_ts};
+    my $timestamp = 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;
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index a5fbba61d..4406d7415 100644
--- a/Bugzilla/Constants.pm
+++ b/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;
diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index 7384e7b5f..4aad803c6 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -642,9 +642,8 @@ sub _bz_add_table_raw {
     $self->do($_) foreach (@statements);
 }
 
-sub bz_add_field_table {
-    my ($self, $name) = @_;
-    my $table_schema = $self->_bz_schema->FIELD_TABLE_SCHEMA;
+sub _bz_add_field_table {
+    my ($self, $name, $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) = @_;
 
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index f8c1588e4..1c6ee352e 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/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
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index 1830784a9..6555bba96 100644
--- a/Bugzilla/Field.pm
+++ b/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;
-        # Create the database column that stores the data for this field.
-        $dbh->bz_add_column('bugs', $name, SQL_DEFINITIONS->{$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) {
+        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_table($name);
-            # And insert a default value of "---" into it.
+            $dbh->bz_add_field_tables($field);
+        }
+
+        if ($type == FIELD_TYPE_SINGLE_SELECT) {
+            # Insert a default value of "---" into the legal values table.
             $dbh->do("INSERT INTO $name (value) VALUES ('---')");
         }
     }