From aa888f2218179d59b4f0b8e51e43b863f1da3e43 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Sat, 8 Sep 2007 05:14:25 +0000 Subject: Bug 287330: Multi-Select Custom Fields Patch By Max Kanat-Alexander r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 110 ++++++++++++++++++--- Bugzilla/Constants.pm | 2 + Bugzilla/DB.pm | 18 +++- Bugzilla/DB/Schema.pm | 12 +++ Bugzilla/Field.pm | 21 ++-- colchange.cgi | 4 +- editvalues.cgi | 2 + post_bug.cgi | 11 ++- process_bug.cgi | 16 ++- query.cgi | 19 +++- .../en/default/admin/custom_fields/edit.html.tmpl | 3 +- template/en/default/bug/field.html.tmpl | 31 +++++- template/en/default/global/field-descs.none.tmpl | 4 +- template/en/default/global/user-error.html.tmpl | 6 +- 14 files changed, 212 insertions(+), 47 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 ('---')"); } } diff --git a/colchange.cgi b/colchange.cgi index b52433924..b2deb3274 100755 --- a/colchange.cgi +++ b/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; diff --git a/editvalues.cgi b/editvalues.cgi index 951078389..a9d5878c0 100755 --- a/editvalues.cgi +++ b/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); diff --git a/post_bug.cgi b/post_bug.cgi index b873d8f72..ddc12fd64 100755 --- a/post_bug.cgi +++ b/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( - { custom => 1, obsolete => 0, enter_bug => 1 }); +my @custom_bug_fields = grep {$_->type != FIELD_TYPE_MULTI_SELECT} + 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. diff --git a/process_bug.cgi b/process_bug.cgi index e5e873e86..d4bf213ad 100755 --- a/process_bug.cgi +++ b/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) - && (!$cgi->param('dontchange') - || $cgi->param($fname) ne $cgi->param('dontchange'))) + 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 diff --git a/query.cgi b/query.cgi index 3d18606ba..5ca04d81d 100755 --- a/query.cgi +++ b/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; -push(@fields, { name => "noop", description => "---" }); -push(@fields, $dbh->bz_get_field_defs()); -@fields = sort {lc($a->{'description'}) cmp lc($b->{'description'})} @fields; +my @fields = Bugzilla->get_fields({ obsolete => 0 }); +# Multi-selects aren't searchable, currently. +@fields = grep($_->type != FIELD_TYPE_MULTI_SELECT, @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 diff --git a/template/en/default/admin/custom_fields/edit.html.tmpl b/template/en/default/admin/custom_fields/edit.html.tmpl index 9199eb62a..b983bbcc6 100644 --- a/template/en/default/admin/custom_fields/edit.html.tmpl +++ b/template/en/default/admin/custom_fields/edit.html.tmpl @@ -84,7 +84,8 @@     - [% IF field.type == constants.FIELD_TYPE_SINGLE_SELECT %] + [% IF field.type == constants.FIELD_TYPE_SINGLE_SELECT + || field.type == constants.FIELD_TYPE_MULTI_SELECT %]   diff --git a/template/en/default/bug/field.html.tmpl b/template/en/default/bug/field.html.tmpl index af1233097..1652ffb0b 100644 --- a/template/en/default/bug/field.html.tmpl +++ b/template/en/default/bug/field.html.tmpl @@ -42,21 +42,42 @@ [% SWITCH field.type %] [% CASE constants.FIELD_TYPE_FREETEXT %] - [% CASE constants.FIELD_TYPE_SINGLE_SELECT %] - [% IF allow_dont_change %] - [% END %] [% FOREACH legal_value = field.legal_values %] [% END %] + [%# 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 %] + + [% END %] [% END %] [% ELSE %] - [% value FILTER html %] + [% value.join(', ') FILTER html %] [% END %] diff --git a/template/en/default/global/field-descs.none.tmpl b/template/en/default/global/field-descs.none.tmpl index 2d577bb1a..dc6887707 100644 --- a/template/en/default/global/field-descs.none.tmpl +++ b/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", diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 5a33b75c5..ff9c17497 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -658,13 +658,13 @@ [% ELSIF error == "illegal_change" %] [% title = "Not allowed" %] - You tried to change the + You tried to change the [% field_descs.$field FILTER html %] field [% IF oldvalue.defined %] - from [% oldvalue FILTER html %] + from [% oldvalue.join(', ') FILTER html %] [% END %] [% IF newvalue.defined %] - to [% newvalue FILTER html %] + to [% newvalue.join(', ') FILTER html %] [% END %] , but only [% IF privs < 3 %] -- cgit v1.2.3-24-g4f1b