308253 - Ability to add select (enum) fields to a bug whose list of values depends on the value of another field

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

It would be nice to have a custom field type of a <select> box whose <option>
values depend on another field.

That is, "I want to make a per-product field," or "I want to make a
per-component" field, or "I want to make a per-version field." This is all
handled by having "a field that depends on another field."

We don't need to get too complex with this, but we will need to genericize the
JavaScript that we have on query.cgi that changes value lists depending on what
product you choose.

Comment 1

[Edward Chan]

This functionality would be most useful and we would use it as soon as it becomes available.  The user should be able to change the default value of the field if they so choose to however.

Comment 2

[Max Kanat-Alexander]

Attachment: Monolithic Patch

Here's a monolithic patch that does it. It needs to be split into several separate bugs and reviewed in those.

Comment 3

[Albert Ting]

Could the select box also support multi-select rather than just a single-select?

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> Could the select box also support multi-select rather than just a
> single-select?

  We could do that in a separate bug. Do you mean the child being a multi-select, or the parent being a multi-select? I have code somewhere for the child being a multi-select, but not for the parent--that's way more complex.

Comment 5

[Albert Ting]

(In reply to comment #4)
> (In reply to comment #3)
> > Could the select box also support multi-select rather than just a
> > single-select?
> 
>   We could do that in a separate bug. Do you mean the child being a
> multi-select, or the parent being a multi-select? I have code somewhere for the
> child being a multi-select, but not for the parent--that's way more complex.

I mean the child being multi-select.  My main interest is I want the list of values to change based on the product selected (or the component selected)

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> I mean the child being multi-select.  My main interest is I want the list of
> values to change based on the product selected (or the component selected)

  Yeah, that's possible. Like I said, I already have code that does it, so maybe I'll even include it in this patch. That would be fixing two things in one patch, though, so I might separate it out into two patches.

Comment 7

[Albert Ting]

(In reply to comment #6)
> (In reply to comment #5)
> > I mean the child being multi-select.  My main interest is I want the list of
> > values to change based on the product selected (or the component selected)
> 
>   Yeah, that's possible. Like I said, I already have code that does it, so
> maybe I'll even include it in this patch. That would be fixing two things in
> one patch, though, so I might separate it out into two patches.

Cool, this means we could migrate Versions and Milestones to use the new custom fields. 

Comment 9

[Max Kanat-Alexander]

Attachment: Work In Progress

Okay, here's what I have so far. I thought about implementing it so that each value could be controlled by a different field, but that got too complex. So we just have one field that controls one field's values, and each value sets which value shows it.

Comment 10

[Max Kanat-Alexander]

Eventually we'll probably want to add the ability to say "this field shows up when the value is X or Y or Z", but that's in the future, if it's needed by enough people.

Comment 11

[Max Kanat-Alexander]

Attachment: v1 (works in non-IE browsers)

Here's a totally complete version of this patch...that doesn't work in IE. It works everywhere else, as far as I know. Apparently IE doesn't support "display: none" on <option> tags, and it also doesn't support the disabled attribute (these facts are even true in IE7).

The solution that I have now is elegant. It just doesn't work in IE. There are hacky workarounds for IE7 to enable the disabled attribute, but they are...well, bad from a user perspective (they let you select the item and then revert it after you've selected it). And anyhow, I want to *hide* the item, not just disable it.

I don't want to have to carry around a data structure like we do for classifications, products, and components, because then I have to add JavaScript all over the place for fields. :-( I may be forced to do that, though, to make this stuff work in IE.

Comment 12

[Max Kanat-Alexander]

One option may be to programatically replace the <option> tag with an empty <optgroup> that contains the name of the option, when the option is disabled, and swap it out again for an <option> when it's enabled. I'd have to probably use DOM methods to do it, but it could be done...

Comment 13

[Max Kanat-Alexander]

Attachment: Work In Progress 3

This one works pretty much on IE, but apparently still needs some bug fixes.

Comment 14

[Max Kanat-Alexander]

Attachment: v2

OK! This works in IE! And I didn't have to re-work everything. :-)

Basically, what I do in IE is I replace hidden <option> tags with a comment node in the DOM, which works in IE6 and above. This is also necessary in Safai/Chrome, apparently (though at least WebKit supports the "disabled" option on <option> tags).

You'll notice that I had to make some changes in Install::DB about some old status_workflow code--that code was using a Bugzilla::Status function, and Install::DB can't use objects. In this case Bugzilla::Status now expects a visibility_value_id field to be in the bug_status table, but it's not there yet at that point of Install::DB, so I had to use manual code instead of using a Bugzilla::Status code.

Comment 15

[Bradley Baetz (:bbaetz)]

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

As discussed on IRC, doesn't seem to work when the page loads initially - 'hidden' fields are hidden

Can't add a new cf (Can't use string ("Bugzilla::Field") as a HASH ref while "strict refs" in use at Bugzilla/Field.pm line 373.) but that may not be this patch - can't trivially test due to the schema change.

Since I can't create new cfs, can't do additional testing.

The code itself looks fine, though

Comment 16

[Max Kanat-Alexander]

Attachment: v3

Ah, okay, I fixed all the problems you found.

Comment 17

[Max Kanat-Alexander]

Attachment: v3 (bzr)

Here's v3 diffed with bzr instead of cvs, so you can interdiff it with the previous patches (at least, I hope!).

Comment 18

[Bradley Baetz (:bbaetz)]

Comment on attachment 346860 [details] [diff] [review]
v3 (bzr)

>=== modified file 'Bugzilla/Field.pm'
>--- Bugzilla/Field.pm	2008-10-25 04:14:56 +0000
>+++ Bugzilla/Field.pm	2008-11-07 10:28:50 +0000

> 
>+    my $type = $params->{type};
>+    $params->{value_field_id} = 
>+        $class->_check_value_field_id($params->{value_field_id},
>+            ($type == FIELD_TYPE_SINGLE_SELECT 
>+             || $type == FIELD_TYPE_MULTI_SELECT) ? 1 : 0);

Use is_select

>=== modified file 'js/field.js'
>--- js/field.js	2008-10-25 04:11:30 +0000
>+++ js/field.js	2008-11-07 10:28:50 +0000
>@@ -359,3 +359,125 @@
>+function showOptionInIE(aNode, aSelect) {
>+    if (browserCanHideOptions(aSelect)) return;
>+    // If aNode is an Option
>+    alert(aNode.value + " type: " + typeof(aNode.value));

and remove this

Tested on FF and IE, and it seems to work.

r=bbaetz with that

Comment 19

[Max Kanat-Alexander]

I can't use is_select there, there's no object in existence to call it on.

I fixed the alert.

Checking in editfields.cgi;
/cvsroot/mozilla/webtools/bugzilla/editfields.cgi,v  <--  editfields.cgi
new revision: 1.11; previous revision: 1.10
done
Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.38; previous revision: 1.37
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.38; previous revision: 1.37
done
Checking in Bugzilla/Status.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Status.pm,v  <--  Status.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.108; previous revision: 1.107
done
Checking in Bugzilla/Field/Choice.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field/Choice.pm,v  <--  Choice.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.58; previous revision: 1.57
done
Checking in js/field.js;
/cvsroot/mozilla/webtools/bugzilla/js/field.js,v  <--  field.js
new revision: 1.12; previous revision: 1.11
done
Checking in js/util.js;
/cvsroot/mozilla/webtools/bugzilla/js/util.js,v  <--  util.js
new revision: 1.6; previous revision: 1.5
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.55; previous revision: 1.54
done
Checking in template/en/default/admin/custom_fields/cf-js.js.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/cf-js.js.tmpl,v  <--  cf-js.js.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/custom_fields/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.12; previous revision: 1.11
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.12; previous revision: 1.11
done
Checking in template/en/default/admin/fieldvalues/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/admin/fieldvalues/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/admin/fieldvalues/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field-events.js.tmpl,v
done
Checking in template/en/default/bug/field-events.js.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field-events.js.tmpl,v  <--  field-events.js.tmpl
initial revision: 1.1
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.21; previous revision: 1.20
done
Checking in template/en/default/bug/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v  <--  knob.html.tmpl
new revision: 1.39; previous revision: 1.38
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.90; previous revision: 1.89
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.79; previous revision: 1.78
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.266; previous revision: 1.265
done

Comment 20

[Frédéric Buclin]

We need to document this. docs/en/html/custom-fields.html says nothing about this feature, and when I selected a field, I wondered "So what? The UI remains the same; no new form appears", till I realized I had to go to editvalues.cgi to see the new UI.

Also, I second what Albert says. Not being able to say "this value should be displayed only when the other field has values value1, or value2, or value3, ..." is annoying (e.g. display only when Product = Core or Firefox). Could someone file a separate bug for this (if not filed yet)?

Comment 21

[Max Kanat-Alexander]

Yes, please do file a separate bug for that. It should be a relatively easy modification now that we have the basic framework in place.

Comment 22

[Albert Ting]

Thanks Max for all the legwork!  Does this patch handle multi-select or just single select enum fields?

Comment 23

[Max Kanat-Alexander]

(In reply to comment #22)
> Thanks Max for all the legwork!  Does this patch handle multi-select or just
> single select enum fields?

  It handles both.

Comment 24

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.

Comment 25

[Frédéric Buclin]

Attachment: Selenium script for 4.2

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
modified t/test_custom_fields.t
Committed revision 216.

Comment 26

[Frédéric Buclin]

Attachment: Selenium script for 4.0

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/test_custom_fields.t
modified t/lib/QA/Util.pm
Committed revision 203.

Comment 27

[Frédéric Buclin]

Documentation added to bug 707170. While writing tests for this feature, I found a bug related to custom field values which depend on the value of another field: when you select such a value and commit your changes, this value is not always selected correctly when you visit the bug again. The source code has selected="selected" for this value, but it's not the one being displayed in the web browser. If I turn off JS in my web browser, then this value is correctly displayed. More information in the regression bug I will file in a minute.