514618 - Filter (restrict) the visibility of a custom field by classification

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[SKropp]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 

Hi, for me it would be very interesting to get the possibility to map the cf_ custom field to one classification or multiple classifications. At the moment you can just select a single product one some other single options. However, for the configuration it would be nice to map a custom field to classifications.

Reproducible: Always

Steps to Reproduce:
1.See http://servername/bugzilla/editfields.cgi?action=edit&name=cf_customfield.
2.Under the menu "Field only appears when:" should be the classifications selectable.
3.


Expected Results:  
Under http://servername/bugzilla/editfields.cgi?action=edit&name=cf_customfield the classifications should be selectable for custom fields.

I've tried to implement the mapping of a custom field to a classification about the database mapping table. But this just caused an error.

Comment 1

[Frédéric Buclin]

We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.

Comment 2

[Kent Rogers [:mrbball]]

Attachment: Patch for HEAD, v1

Patch allows Classification to be used as a visibility control for custom
fields.  Switched the Classification class to be based on Field::Choice,
in the same manner as the Product class; just enough of Field::Choice is
implemented to allow visibility control.

The classification field was switched to type FIELD_TYPE_SINGLE_SELECT,
and enter_bug.cgi now passes a bit more information to the create
template in case a custom field whose visibility depends on
classification is allowed to be set on bug creation.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 422962 [details] [diff] [review]
Patch for HEAD, v1

 Nice! :-)

> foreach my $field (@enter_bug_fields) {
>     $vars->{$field->name} = formvalue($field->name);
>+    $vars->{$field->visibility_field->name} = formvalue($field->visibility_field->name);

  I'm not sure that's entirely necessary? What would that be catching that the line above doesn't catch?

>+# Classification and Product fields as a parent.
>+$default{'classification'} = new Bugzilla::Classification($product->classification_id)->name;

  I'd like to add a "classification" accessor to Bugzilla::Product instead. It'd be better than doing this everywhere that we need the info in the future.

>Index: Bugzilla/Classification.pm
>+use base qw(Bugzilla::Field::Choice);

  I think you're going to want to wait until the control-by-component patch is checked in, because it splits out the "interface" of Field::Choice into Field::ChoiceInterface, which you can use here without having to specify things like NAME_FIELD as an override.

  Otherwise, this looks pretty good! :-)

Comment 4

[Kent Rogers [:mrbball]]

(In reply to comment #3)
> (From update of attachment 422962 [details] [diff] [review])
>  Nice! :-)
> 
> > foreach my $field (@enter_bug_fields) {
> >     $vars->{$field->name} = formvalue($field->name);
> >+    $vars->{$field->visibility_field->name} = formvalue($field->visibility_field->name);
> 
>   I'm not sure that's entirely necessary? What would that be catching that the
> line above doesn't catch?
> 
You're right, it doesn't look like it's necessary.  I thought it was in
order to always ensure the template had access to the visibility field,
and I thought I verified that with some testing, but additional testing 
seems to show otherwise.

> >+# Classification and Product fields as a parent.
> >+$default{'classification'} = new Bugzilla::Classification($product->classification_id)->name;
> 
>   I'd like to add a "classification" accessor to Bugzilla::Product instead.
> It'd be better than doing this everywhere that we need the info in the future.
> 
Sounds good.

> >Index: Bugzilla/Classification.pm
> >+use base qw(Bugzilla::Field::Choice);
> 
>   I think you're going to want to wait until the control-by-component patch is
> checked in, because it splits out the "interface" of Field::Choice into
> Field::ChoiceInterface, which you can use here without having to specify things
> like NAME_FIELD as an override.
> 
That patch is in now, right (bug 487508)?  I will try to adapt my change ...

>   Otherwise, this looks pretty good! :-)
Thanks.

Comment 5

[Max Kanat-Alexander]

Yeah, the patch is in now! :-) Yeah, that was the bug.

Comment 6

[Kent Rogers [:mrbball]]

Attachment: v2

Use Bugzilla::Field::ChoiceInterface

Comment 7

[Max Kanat-Alexander]

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

This looks great! :-) I'll test it before checkin, but everything here looks right.

Comment 8

[Max Kanat-Alexander]

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

Okay, in testing, there's one problem with this patch: when I change a bug's product on show_bug.cgi, fields that are controlled by Classification are not properly updated.

Comment 9

[Kent Rogers [:mrbball]]

Can you give more details?  Are you saying you'd expect the fields to
appear/disappear at the time a product is changed on the show_bug.cgi
form (via javascript)?

I don't expect it to work that way, and for me the fields that are
controlled by Classification appear/disappear correctly *after* a product
change is committed.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> Can you give more details?  Are you saying you'd expect the fields to
> appear/disappear at the time a product is changed on the show_bug.cgi
> form (via javascript)?

  Yeah, because that's how all the other controllers work.

Comment 11

[Kent Rogers [:mrbball]]

That's what I came to realize, but I'm stuck.  Classification doesn't
change like all the other controllers do (i.e. a bug doesn't have a
classification) and after hacking around with it for a few hours, I
can't figure out a good way (or any way, really) to get it to work
similarly.

Do you have any guidance?

Comment 12

[Max Kanat-Alexander]

Well, I think that perhaps, after the product field, you should do some sort of magical include of bug/field-events.js.tmpl in a way that's going to work to "change" the classification field when the product field is updated.

Perhaps you could insert a hidden classification field and give it an onchange. I have no idea if that works, or, if it does, if it works in all browsers.

If you want some help, feel free to ask me in IRC.

Comment 13

[Kent Rogers [:mrbball]]

This is the direction I was heading, but it felt like more of a hack than
a real solution.  I will continue down this path ...

Comment 14

[Kent Rogers [:mrbball]]

Attachment: v3

Ok, another try.

I added a classification field to the bug edit form in a
"bz_default_hidden" row.  This magically included bug/field-events.js.tmpl
to call showFieldWhen() for visibility control.

Then I added an onchange to the product element which resets the
classification value.  However, since changing a field's value
programmatically does not fire its own onchange, a call to bz_fireEvent()
was also necessary to trigger the visibility control logic for the
classification field.

The only part I really don't like is the [% IF field.name == 'product' %]
in bug/field.html.tmpl.  I wish there was a less hacky way to set the
onchange event for the product field, but I couldn't come up with one,
short of adding an onchange argument to the interface or adding something
to the interface to specify events in general.

Comment 15

[Max Kanat-Alexander]

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

>=== modified file 'Bugzilla/Classification.pm'
>=== modified file 'template/en/default/bug/edit.html.tmpl'
>+  /* Index all classifications so we can keep track of the classification
>+   * for the selected product (which could control field visibility).
>+   */

  This probably should go into its own template, no? (So that it can be used during enter_bug as well.)

>+  var all_classifications = new Array([% bug.choices.product.size %]);
>+  [% count = 0 %]
>+  [%- FOREACH product = bug.choices.product %]
>+      all_classifications[[% count %]] = '[% product.classification.name %]';

  That needs a "FILTER js" on it. Also, shouldn't you be making this list unique so that only specific classifications show up in it?

  And if it's going to have an index, shouldn't you just have it be indexed by the product name or id instead of "count"?

>+  /* Function to reset the classification value and fire an event change
>+   * on it, when the product is changed.  Called via the product field's
>+   * onchange event and necessary if classification controls the visibility
>+   * of other fields.
>+   */
>+  function resetClassification() {

  This should be in field.js.

>+      var classification = document.changeform.classification;
>+      var product_index = document.changeform.product.selectedIndex;
>+      for (var i = 0; i < classification.length; i++) {

  I think you may want to create a bz_selectValue function in js/util.js for this.

  You could also just populate the select with a single item.

>=== modified file 'template/en/default/bug/field.html.tmpl'
>--- template/en/default/bug/field.html.tmpl	2010-02-08 00:10:03 +0000
>+++ template/en/default/bug/field.html.tmpl	2010-02-19 20:57:34 +0000
>@@ -116,6 +116,10 @@
>               constants.FIELD_TYPE_MULTI_SELECT ] %]
>         <select id="[% field.name FILTER html %]" 
>                 name="[% field.name FILTER html %]" 
>+                [%# classification value is dependent on product %]
>+                [% IF field.name == 'product' %]
>+                  onChange="resetClassification();"
>+                [% END %]

  Ahh, instead of this, you could do a YAHOO.util.Event.addListener inside of field-events.js. That might be a better place to put it.

Comment 16

[Kent Rogers [:mrbball]]

Oops, I see I forgot to filter "product.classification.name" in
bug/edit.html.tmpl.  I will wait to see if there are any other changes
needed before submitting another patch.

BTW, I saw this error when running the filter test on the current trunk:

t/008filter..........NOK 141#   Failed test '(en/default)
template/en/default/bug/edit.html.tmpl - filterexceptions.pl has extra members:
#   bug.deadline
# --WARNING'
#   at t/008filter.t line 143.
t/008filter..........ok 229/271# Looks like you failed 1 test of 271.        
t/008filter..........dubious                                                 
        Test returned status 1 (wstat 256, 0x100)
DIED. FAILED test 141
        Failed 1/271 tests, 99.63% okay

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> BTW, I saw this error when running the filter test on the current trunk:
> [snip]

  Oh, thanks! I just fixed that.

Comment 18

[Kent Rogers [:mrbball]]

(In reply to comment #15)
> (From update of attachment 427829 [details] [diff] [review])
> >=== modified file 'Bugzilla/Classification.pm'
> >=== modified file 'template/en/default/bug/edit.html.tmpl'
> >+  /* Index all classifications so we can keep track of the classification
> >+   * for the selected product (which could control field visibility).
> >+   */
> 
>   This probably should go into its own template, no? (So that it can be used
> during enter_bug as well.)
> 
Is there a way to change the product (and thus the classification) on the
enter_bug form?

> >+  var all_classifications = new Array([% bug.choices.product.size %]);
> >+  [% count = 0 %]
> >+  [%- FOREACH product = bug.choices.product %]
> >+      all_classifications[[% count %]] = '[% product.classification.name %]';
> 
>   That needs a "FILTER js" on it. Also, shouldn't you be making this list
> unique so that only specific classifications show up in it?
> 
Yup, I noticed that right away.  Including all classifications (even if
non-unique) allows the array to be easily indexed by the selected product.

>   And if it's going to have an index, shouldn't you just have it be indexed by
> the product name or id instead of "count"?
> 
This is the only way I could figure out how to do it.  I struggled for days
to setup an array indexed by product name and then saw that this is how the
components, comp_desc, intitalowners, flags and initialqacontacts arrays
are setup in the enter_bug form.

> >+  /* Function to reset the classification value and fire an event change
> >+   * on it, when the product is changed.  Called via the product field's
> >+   * onchange event and necessary if classification controls the visibility
> >+   * of other fields.
> >+   */
> >+  function resetClassification() {
> 
>   This should be in field.js.
> 
Sure.  Are the functions there in a particular order?

> >+      var classification = document.changeform.classification;
> >+      var product_index = document.changeform.product.selectedIndex;
> >+      for (var i = 0; i < classification.length; i++) {
> 
>   I think you may want to create a bz_selectValue function in js/util.js for
> this.
> 
Something like:

function bz_selectValue(aSelect, aValue) {
    for (var i = 0; i < aSelect.length; i++) {
        if (aSelect[i].value = aValue)
            aSelect.selectedIndex = i;
    }
}

>   You could also just populate the select with a single item.
> 
By overriding the legal values?  What does that do for me?

> >=== modified file 'template/en/default/bug/field.html.tmpl'
> >--- template/en/default/bug/field.html.tmpl	2010-02-08 00:10:03 +0000
> >+++ template/en/default/bug/field.html.tmpl	2010-02-19 20:57:34 +0000
> >@@ -116,6 +116,10 @@
> >               constants.FIELD_TYPE_MULTI_SELECT ] %]
> >         <select id="[% field.name FILTER html %]" 
> >                 name="[% field.name FILTER html %]" 
> >+                [%# classification value is dependent on product %]
> >+                [% IF field.name == 'product' %]
> >+                  onChange="resetClassification();"
> >+                [% END %]
> 
>   Ahh, instead of this, you could do a YAHOO.util.Event.addListener inside of
> field-events.js. That might be a better place to put it.
>
This sounds better, but you lost me.  Where/how inside field-events.js?

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> Is there a way to change the product (and thus the classification) on the
> enter_bug form?

  Oh, right, there isn't! :-) Ha. :-)

> >   This should be in field.js.
> > 
> Sure.  Are the functions there in a particular order?

  I don't think so. They are sort of grouped into groups.

> function bz_selectValue(aSelect, aValue) {
>     for (var i = 0; i < aSelect.length; i++) {
>         if (aSelect[i].value = aValue)
>             aSelect.selectedIndex = i;
>     }
> }

  Yeah. There might also be something in YUI? I'm not sure, though.

> >   You could also just populate the select with a single item.
> > 
> By overriding the legal values?  What does that do for me?

  Nothing, really, I suppose, except that it makes it a little easier to write the onchange handler for product, maybe.

> This sounds better, but you lost me.  Where/how inside field-events.js?

  http://developer.yahoo.com/yui/docs/YAHOO.util.Event.html#method_addListener

Comment 20

[Kent Rogers [:mrbball]]

(In reply to comment #19)
> > Is there a way to change the product (and thus the classification) on the
> > enter_bug form?
> 
>   Oh, right, there isn't! :-) Ha. :-)
> 
Thus, no reason to put the code into its own template, right?  And in
that case, does it make sense to move resetClassification() to field.js?
The only place it is applicable is the edit template.

> > function bz_selectValue(aSelect, aValue) {
> >     for (var i = 0; i < aSelect.length; i++) {
> >         if (aSelect[i].value = aValue)
> >             aSelect.selectedIndex = i;
> >     }
> > }
> 
>   Yeah. There might also be something in YUI? I'm not sure, though.
> 
Couldn't find anything in YUI, so I'll add this to util.js.

> > This sounds better, but you lost me.  Where/how inside field-events.js?
> 
>   http://developer.yahoo.com/yui/docs/YAHOO.util.Event.html#method_addListener
>
Ok, but adding it to field-events.js seems like as much of a hack as
adding it to field.html.tmpl.  Can I simply add it in edit.html.tmpl
where the product field is defined?  Again, the edit template is the
only place where it is applicable:

    <tr>
       [% INCLUDE bug/field.html.tmpl
            bug = bug, field = bug_fields.product,
            override_legal_values = bug.choices.product
            desc_url = 'describecomponents.cgi', value = bug.product
            editable = bug.check_can_change_field('product', 0, 1) %]
       <script type="text/javascript">
         YAHOO.util.Event.addListener('product', 'change', resetClassification);
       </script>
    </tr>

Comment 21

[Max Kanat-Alexander]

(In reply to comment #20)
> Thus, no reason to put the code into its own template, right?

  Yeah, correct.


>  And in
> that case, does it make sense to move resetClassification() to field.js?
> The only place it is applicable is the edit template.

  Yeah, it does still make sense, because .js files are cached by the browser and so we like to keep static JS in there.

> Couldn't find anything in YUI, so I'll add this to util.js.

  Sounds good.

> Ok, but adding it to field-events.js seems like as much of a hack as
> adding it to field.html.tmpl. 

  It does, it's true.

> Can I simply add it in edit.html.tmpl
> where the product field is defined?  Again, the edit template is the
> only place where it is applicable:

  Yeah, that sounds good, actually, since we actually *don't* want it showing up in other places, right now.

Comment 22

[Kent Rogers [:mrbball]]

Attachment: v4

Hopefully getting close.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 428443 [details] [diff] [review]
v4

>=== modified file 'js/util.js'
> /**
>+ * Select the specified value in a <select> element.
>+ *
>+ * @param aSelect        The select element being modified.
>+ * @param aValue         The value being selected.
>+ */
>+function bz_setSelectedValue(aSelect, aValue) {
>+    for (var i = 0; i < aSelect.length; i++) {
>+        if (aSelect[i].value = aValue)

  I think you mean == there.

  I think that "classification" needs to be added to ABNORMAL_SELECTS in Bugzilla::Constants.

  For some reason, this isn't working on show_bug.cgi. If I inspect the HTML, it looks as though the "classification" field thinks that it is being controlled by another field, and that only its current value should appear. Why would that be?

Comment 24

[Max Kanat-Alexander]

  (You may have to test in Chrome, Safari, or IE to see the problem I'm experiencing.)

Comment 25

[Kent Rogers [:mrbball]]

(In reply to comment #24)
>   (You may have to test in Chrome, Safari, or IE to see the problem I'm
> experiencing.)

Actually, it shows up when I change '=' to '=='!

Comment 26

[Kent Rogers [:mrbball]]

Attachment: v5

(In reply to comment #23)
>   I think you mean == there.
> 
Absolutely!  Good catch.

>   I think that "classification" needs to be added to ABNORMAL_SELECTS in
> Bugzilla::Constants.
> 
I don't see any adverse effects if it isn't, but this seems reasonable.

>   For some reason, this isn't working on show_bug.cgi. If I inspect the HTML,
> it looks as though the "classification" field thinks that it is being
> controlled by another field, and that only its current value should appear.
> Why would that be?

I'm pretty sure the fix here is as simple as adding:

  use constant is_active => 1;

to Classification.pm (as is done in Components.pm).  The non-set
classification values were showing up as inactive, and thus being
hidden/disabled.  However, it's not possible for classifications to
be inactive.

Comment 27

[Max Kanat-Alexander]

(In reply to comment #26)
> >   I think that "classification" needs to be added to ABNORMAL_SELECTS in
> > Bugzilla::Constants.
> > 
> I don't see any adverse effects if it isn't, but this seems reasonable.

  Grep the code for "is_abnormal" to understand why.


  I'll check out the patch soon.

Comment 28

[Max Kanat-Alexander]

Comment on attachment 428704 [details] [diff] [review]
v5

Okay, this is working now as a visibility controller, but not as a value controller, on show_bug.cgi. Don't know why.

Comment 29

[Kent Rogers [:mrbball]]

Ah, I wasn't even thinking about Classification as a value controller.
I don't suppose there is a way to separate the two capabilities?  AFAIK,
this bug was only asking for visibility control, but I'm guessing the
two capabilities go hand in hand.

Comment 30

[Max Kanat-Alexander]

  Yeah, they go hand-in-hand; they both have to work if one of them works. They actually both *should* work fine together--they're working OK on enter_bug.cgi. It's just show_bug.cgi that's having trouble.

Comment 31

[Kent Rogers [:mrbball]]

(In reply to comment #28)
> (From update of attachment 428704 [details] [diff] [review])
> Okay, this is working now as a visibility controller, but not as a value
> controller, on show_bug.cgi. Don't know why.

I'm stumped.  Classification works great as a visibility controller (which
is really all I cared about), and also works fine as a value controller
except "on the fly", after a new product has been selected in the show_bug
form and before the change is submitted.  When a product in a different
Classification is selected, all controlled values disappear, including the
values that were valid for the previous Classification and the values that
are valid for the new Classification.  Re-selecting the original product
does not cause the original values to re-appear.

I dug around for a while but couldn't come up with anything.  It sure
looks like the correct showValueWhen() calls are defined in the html for
the field that is controlled by Classification.

I was not able to debug this any further, and am at the limit of my
"expertise".

Comment 32

[Max Kanat-Alexander]

  Okay, I'll add this to my TODO list, and investigate.

Comment 33

[Max Kanat-Alexander]

Comment on attachment 428704 [details] [diff] [review]
v5

Asking myself for review to keep this in my radar.

Comment 34

[Max Kanat-Alexander]

Attachment: v6

Okay, this fixes it. :-) I think the problem was the location of the YAHOO.util.Event.addListener, because it was inside of a <tr> with no <td>.

Anyhow, I did some general cleanup and now it works.

Comment 35

[Tiago Mello [:timello]]

Comment on attachment 455988 [details] [diff] [review]
v6

It works fine. But when you set the visibility field to 'Product' and the value 'A' and mark the values control to 'Classification'... when adding a value for the field, the interface allows the user to add a value that can never be displayed since the product A may not be part of the classification... that's confusing for user I think.

Maybe that can be addressed in another bug, but I think when the visibility is set to 'Product'... then the values control is either disabled for classification or when adding a value for the field show only the classification the product belongs to...

Comment 36

[Tiago Mello [:timello]]

(In reply to comment #35)
> (From update of attachment 455988 [details] [diff] [review])
> Maybe that can be addressed in another bug, but I think when the visibility is
> set to 'Product'... then the values control is either disabled for
> classification or when adding a value for the field show only the
> classification the product belongs to...

Actually... I don't think it makes sense let 'classification' controls the values when the visibility is 'product', at all.

Comment 37

[Max Kanat-Alexander]

  It probably doesn't, but there are lots of strange situations that you can get yourself into with visibility/value controls. For the most part, my view on it has been that if somebody wants to do something that's a bad idea, then they're an administrator, and they have that power. :-)

Comment 38

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified enter_bug.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Classification.pm
modified Bugzilla/Constants.pm
modified Bugzilla/Field.pm
modified Bugzilla/Product.pm
modified Bugzilla/Field/Choice.pm
modified js/field.js
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field-events.js.tmpl
Committed revision 7265.

Comment 39

[Max Kanat-Alexander]

Added to the release notes in bug 604256.