287330 - Ability to add custom multi-select fields to a bug

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

A multi-select is a field that has a list, but you can select more than one of
the options in the list at the same time. This is a less common type of custom
field, and also is more difficult to implement.

Comment 1

[P P N]

I am new to bugzilla and hence would require some help from you all here.

Could anyone let me know if these enhancements for custom fields completed? If yes, how to get the current updates / patches for bugzilla 2.20? In such a case will all the new customization [custom field] etc get merged with bugzilla 2.20 release?

The customizations mentioned here would really come handy for us too. Please
check BUG #344086 about the same. Looking forward to your replies on the same.

Thanks,

Comment 3

[Max Kanat-Alexander]

Attachment: Monolithic Patch containing Multi-Select Code

I wrote some code for a client that implemented multi-select fields, among a lot of other things. Here's the whole patch, which also includes various other code. The multi-select code needs to be taken out of here and made into a separate patch.

Also, we need to not use the "cache" in the bugs table of the multi-select field, but should should instead be able to search the multi-select table itself, and display that in the bug list (although that really is hard).

Comment 4

[Corris]

Attachment: just the multi-select code

The multiselect functionality ripped out from mkanats patch ( above ).   This is just the multi-selects, not the datetime, textarea, controlled fields, or fieldsets that are present in that patch too. 

I didn't implement searching through the actual multi-select field tables ( it uses the bugs.cf_  "cache" still.

I copied over some cleanup code ( using delete returns the hash value deleted, so you can reduce # of lines by 1 per delete ).

the only thing different I had to do was call trick_taint in Bugzilla::Bug on new bug entry of the "cache" multiselect field into the bugs table.

Comment 5

[Frédéric Buclin]

The CC list doesn't use any cache. Why not doing something similar to this?

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> The CC list doesn't use any cache.

  You also can't display the CC list on the buglist. (That's the only reason for a cache.)

Comment 7

[Max Kanat-Alexander]

Comment on attachment 263917 [details] [diff] [review]
just the multi-select code

>+my ( @multi_select_fields, @multi_select_values);

  I don't know why I didn't do this originally, but we should clearly be using a hash here instead of two arrays.

>Index: Bugzilla/Bug.pm
> [snip]
>+    my @multi = Bugzilla->get_fields({ custom => 1,
>+                                        type => FIELD_TYPE_MULTI_SELECT });
>+    $validators->{$_->name} = \&_check_multi_select_field foreach @multi;
>+

  Nit: For consistency with the above code, don't use the single-line form of foreach.

>+    # and we need to handle multi-select fields differently than normal.
>+    #  ( store them as a CSV in the bugs table, so searches, bug_activity, et al. work.

  No "and" necessary. That second line should say "as a comma-separated string" instead of "as a CSV."

  Also, LpSolit is right in one respect: nothing should depend on the cache except the buglist. That is, bug activity should reference the table directly, if possible. (Whatever we do for the CC field is what we should generally do, except in the buglist where CC can't be displayed.)

>@@ -342,6 +343,7 @@
> 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 => 300;

  That should be 3, not 300. (I used 300 because it was a customization for a client.)

>+sub bz_add_field_tables {

  This is going to need POD.

>+        if ($type == FIELD_TYPE_SINGLE_SELECT
>+                || $type == FIELD_TYPE_MULTI_SELECT) 

  Nit: The || should be aligned with the $type above, not indented further.

>Index: Bugzilla/Object.pm

  This file doesn't need to be patched at all.

>Index: Bugzilla/DB/Schema.pm

>+    INDEXES => [
>+        bug_id_idx => {FIELDS => [qw( bug_id value)], TYPE => 'UNIQUE'},

  Nit: No space in front of bug_id.

>Index: template/en/default/global/field-descs.none.tmpl
> [snip]
>+                   ${constants.FIELD_TYPE_MULTI_SELECT}  => "Multiple-Selection Box",

  Make sure that line isn't longer than 80 characters.


  Sorry the review took so long! Future ones will be *much* faster.

Comment 8

[Max Kanat-Alexander]

All right--no response from the patch author for a while, so I'm going to be taking this bug.

Comment 9

[Max Kanat-Alexander]

Attachment: v2

Okay, here we go!

Bringing this upstream is sponsored by NASA, and so we may be able to work out compensation for any time spent reviewing this patch. Just contact me by email. (LpSolit, you already have a contract with Everything Solved, so you can just bill us for hours.)

Comment 10

[Max Kanat-Alexander]

Oh, and note that multi-select fields aren't searchable in this patch--I plan to work on that in a future patch. Search.pm is really a monster.

Comment 11

[Frédéric Buclin]

Comment on attachment 274116 [details] [diff] [review]
v2

>Index: query.cgi

>+    foreach my $tt_field (qw(estimated_time remaining_time work_time
>+                             percentage_complete deadline))
>+    {
>+        @fields = grep($_->name ne $tt_field, @fields);
>+    }

It would be faster to join all TT fields with "|" and then use a regexp to filter the list, i.e.:
  grep { $_->name !~ /^(estimated_time|remaining_time|...)$/ } @fields;



>Index: Bugzilla/Bug.pm

>+sub _extract_multi_selects {

>+            my $array = delete $params->{$name} || [];

I'm surprised that no error is thrown. When I write:

perl -we 'my $a = [1,2,3]; my $b = delete $a;'
I get:
delete argument is not a HASH or ARRAY element or slice at -e line 1.

What am I missing?


>+sub _check_multi_select_field {

Not related to _check_multi_select_field() directly, but related to field validation: check_can_change_field() is not configured to handle an arrayref as arguments, e.g. when it checks that the old and new values are the same, or the trimmed values are the same. (how could you trim() arrayrefs??).

Also, still related to check_can_change_field(), I get, if I'm not allowed to edit the bug:

You tried to change the Affected Versions field from ARRAY(0x8d317bc)  to ARRAY(0x8d31c3c)  , but only the assignee or reporter of the bug, or a sufficiently empowered user may change that field.

The error message has to be fixed to accept arrays.


>   *$AUTOLOAD = sub {
>       my $self = shift;
>+
>+      $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);
>+      }
>+
>       if (defined $self->{$attr}) {
>           return $self->{$attr};
>       }

For performance reasons, you should first check if $self->{$attr} is defined, and if not, test if it's a custom field. But please don't do the opposite.



>Index: Bugzilla/Object.pm

>     my (@field_names, @values);
>+
>     while (my ($field, $value) = each %$field_values) {

Nit: no reason to include this change in this patch.



>Index: template/en/default/bug/field.html.tmpl

>+                    [% IF field.legal_values.size < 5 %]
>+                        [% SET field_size = field.legal_values.size %]
>+                    [% END %]

Nit: I don't think we need this. We could as well set the size to 5 in all cases. Are you sure the UI and the logic won't fail if a multi-select field is defined but no value is added yet? You would get size=0 here and probably some "undefined" errors when validating fields too.


>             <option value="[% legal_value FILTER html %]"
>+                [%- " selected=\"selected\"" IF value.grep(legal_value).size %]>

This regexp is wrong. If legal values are "old stable" and "stable" (among others), and you select "old stable" in show_bug.cgi, "stable" is also selected because it matches the regexp. So the UI makes you think that "stable" is also selected, and editing the bug again will add "stable" as being really selected. You probably have to write "^$legal_value$". Also, if a legal field value is ".*", this one is always selected, which makes me think you should use \Q \E as well. One more thing about this regexp: "Old (deprecated)" never keeps selected, despite I highlighted it. I don't know why the regexp doesn't like parens in field values (probably because parens are not escaped), but we should fix this.


Some more comments:

- There is no link in editfields.cgi pointing to editvalues.cgi to set legal values of multi-select fields.

- Users with no privs see the multi-select fields in show_bug.cgi as: ARRAY(0x8de9f7c) instead of the values as plain text themselves.

Comment 12

[Frédéric Buclin]

One more thing. You have to drop multi-select fields from the list of valid columns colchange.cgi. Else you get:

DBD::mysql::st execute failed: Unknown column 'bugs.cf_affected_versions' in 'field list' at /var/www/html/bugzilla-tmp/buglist.cgi line 1004

Comment 13

[Max Kanat-Alexander]

(In reply to comment #11)
> It would be faster to join all TT fields with "|" and then use a regexp to
> filter the list, i.e.:
>   grep { $_->name !~ /^(estimated_time|remaining_time|...)$/ } @fields;

  Yes, and for the added complexity of reading that you might even get 5 nanoseconds of improved performance...

> >+            my $array = delete $params->{$name} || [];
> 
> What am I missing?

  That I'm doing delete on a hash item, not an arrayref.

> Nit: I don't think we need this. We could as well set the size to 5 in all
> cases. Are you sure the UI and the logic won't fail if a multi-select field is
> defined but no value is added yet? You would get size=0 here and probably some
> "undefined" errors when validating fields too.

  size=0 is fine, and a multi-select with no values is exactly identical to a multi-select with nothing selected, so we're fine there.

  I think it's nice for the cases where the multi-select doesn't have 5 items.


  I have a patch that fixes everything else, it should be coming up soon.

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
>   Yes, and for the added complexity of reading that you might even get 5
> nanoseconds of improved performance...

It's not complex to read at all.


>   size=0 is fine, and a multi-select with no values is exactly identical to a
> multi-select with nothing selected, so we're fine there.

That's not what I meant. I mean if the legal value list is empty, could there be a code somewhere which would end with SELECT ... WHERE value IN ()?

Comment 15

[Max Kanat-Alexander]

Attachment: v3

Okay, I fixed your issues, and I also fixed a few others that I discovered.

Note that multi-select fields don't currently appear in "New: " bugmail. We should fix bug 395451 after this, which will fix that.

Comment 16

[Frédéric Buclin]

Comment on attachment 280124 [details] [diff] [review]
v3

Everything looks good. Before committing this patch, make sure to fix bug/field.html.tmpl at line 58 to select do_not_change by default when this option is defined. Else you may clear multi-select fields by accident.

r=LpSolit with this comment fixed.

Comment 17

[Max Kanat-Alexander]

I fixed the edit-multiple thing just like you said.

I tested things with an empty multi-select, and it seemed fine.

Checking in colchange.cgi;
/cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v  <--  colchange.cgi
new revision: 1.60; previous revision: 1.59
done
Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.23; previous revision: 1.22
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.188; previous revision: 1.187
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.384; previous revision: 1.383
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.174; previous revision: 1.173
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.200; previous revision: 1.199
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.77; previous revision: 1.76
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.103; previous revision: 1.102
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.27; previous revision: 1.26
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.89; previous revision: 1.88
done
Checking in template/en/default/admin/custom_fields/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.23; previous revision: 1.22
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.230; previous revision: 1.229
done

Comment 18

[Frédéric Buclin]

I guess relnoting this new custom field type is useful. :)

Comment 19

[Frédéric Buclin]

Attachment: fix failure when editing a bug

I saw this mistake while reviewing mkanat's patch, but I forgot to mention it in my review comment, and now process_bug.cgi fails when editing a bug which has custom fields. The fix is trivial, I'm committing it without asking for review.

Comment 20

[Frédéric Buclin]

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.201; previous revision: 1.200
done

Comment 21

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.