806809 - Custom field values with "Enabled for bugs" set to "No" break the values list if the field's values visibility depends on another field values

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P2)

Description

[Alexander Tereschenko]

Attachment: Proposed fix v1 (git diff format)

If an instance has two custom fields and one is managing the visibility of the other + that controlled field has disabled values in the values list ("Enabled for bugs" is "No"), in the interface the values list for the controlled field stops at the value right before the disabled one.

If the disabled value is the first in the list, no values shown at all.
I've been able to reproduce this on both 4.0.8 and 4.2.3.

Exact steps to reproduce:
1) Create custom field cf_controller, add values "Controller1" and "Controller2" into the list;
2) Create custom field cf_controlled, setting the "Field that controls the values that appear in this field" to "cf_controller";
3) Add three values into the cf_controlled values list, "Controlled1", "Controlled2" and "Controlled3". Set the "Enabled for bugs" for "Controlled2" to "No" (uncheck the checkbox). Set the "Only appears when..." to "Controller1" for all three values.
4) After that try submitting the bug and looking at it, or looking at the existing bug, changing the values of the cf_controller field and observing changes in the values of the cf_controlled field. You'll notice that instead of having values "Controlled1" and "Controlled3" in the list, cf_controlled will have only "Controlled1".
5) Try setting the "Enabled for bugs" to "No" for the "Controlled1" value. After that the list will always be empty (that is it will contain only the default "---") item.

Proposed fix:
There are several places where this can be fixed, I'm proposing one of them that looked best to me (see attachment).

I've traced this behaviour down to the following combination if events:
1) In field.js.tmpl, line 49 adds calls to showValueWhen populating the value id arrays with all legal values for the field (that would include those having "Enabled for bugs" set to "No");
2) At the same time field.html.tmpl on line 123 skips adding the lines for legal values that have "Enabled for bugs" set to "No"
3) During the run time, in field.js, when showValueWhen is called upon the controller field change, it in turn calls handleValControllerChange that does the actual job of enabling/disabling values;
4) handleValControllerChange gets the list of value ids generated by field.js.tmpl and goes through it calling "item = getPossiblyHiddenOption" and then executing "if" statement that calls methods of the returned object stored in "item";
5) As long as actual HTML line for the disabled value doesn't exist, getPossiblyHiddenOption returns null and when "if" tries to call the method the whole "for" loop iterating over values list crashes (or something like that, I'm an expert in JS and YUI, stepping into with debugger leads to some YUI internals) resulting in other (possibly enabled) values not even being considered.

I've just added the check for object existence into that "if" statement (see patch attached), but it can also be fixed in field.js.tmpl (so that it doesn't add the inactive value id into the arguments list for showValueWhen), or in the field.html.tmpl (so that it would not skip inactive values - but this is the least accurate fix, no sense to put garbage into HTML).

Comment 1

[Alexander Tereschenko]

correction for two typos:
1) "and one is managing the visibility of the other" should read as "and one is managing the visibility of THE VALUES of the other";

2) "I'm an expert in JS and YUI" should read as "I'm NOT an expert in JS and YUI"

Comment 2

[Alexander Tereschenko]

Ok, after reading more on the bug handling in BMO I think I should have set the "review" flag, not "approval", but I don't see the one in the list. Switching off the flag anyway.

Comment 3

[Koosha KM]

'review' is a flag for attachments. Click on "Details" link of your attachment, then you will see the 'review' flag. :-)

Comment 4

[Alexander Tereschenko]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #3)
> 'review' is a flag for attachments. Click on "Details" link of your
> attachment, then you will see the 'review' flag. :-)
Ah, indeed, how could I forget that? :) Thank you Koosha!

Comment 5

[Sunil Joshi [:sjoshi]]

Alex: you need to set the person name also i.e. from whom you are asking for review.

When you have set the review flag-> ? there is a text box coming besides it, you need to fill the reviewer name,
Please type in lpsolit and you will get an option of Frédéric Buclin , you can ask him for review as of now. :)

Comment 6

[Alexander Tereschenko]

I had suspicions about this field, but left it empty being unsure.

Thanks for the advice Sunil and thank you for your patience dear all :)

Comment 7

[Frédéric Buclin]

Following your steps in comment 0, I cannot reproduce your issue. Only the value marked as not available for bugs is skipped. I tested with Bugzilla 4.5.

Comment 8

[Alexander Tereschenko]

Thanks for looking into this.

I haven't tried this on 4.4 or 4.5 and quick look reveals that those have very different controller code (though observations 1 and 2 regarding field.js.tmpl & field.html.tmpl still hold true), maybe that's the reason for you not seeing it. I don't have any of those versions installed at the moment to trace it in detail, I'll try to set something up and check it out.

In the meanwhile, what do you think on bringing this fix to 4.2.x and 4.0.x? I understand 4.0 may fall off due to 4.4. being released, but 4.2.x is going to be around for a while, so I guess it would make sense to do that.

Comment 9

[Frédéric Buclin]

(In reply to Alex Tereschenko from comment #8)
> In the meanwhile, what do you think on bringing this fix to 4.2.x and 4.0.x?
> I understand 4.0 may fall off due to 4.4. being released, but 4.2.x is going
> to be around for a while, so I guess it would make sense to do that.

Ah, I didn't check with 4.2. I assumed the code was pretty much the same. I will test with 4.2 this week and if I can reproduce your issue, I'm fine to take your fix for 4.2.

(Note that with the release of 4.4, we will drop support for 3.6, not 4.0. But 4.0 is already restricted to security fixes only.)

Comment 10

[Frédéric Buclin]

On 4.2.4, I get the same error as you. And actually, I also get an error with 4.5:

Error : TypeError: val is null

This is in getPossiblyHiddenOption().

Comment 11

[Alexander Tereschenko]

Thanks for checking with 4.2.

In the meanwhile I've been able to check it with 4.4rc1+ (used bzr checkout from 4.4 branch, not the tarball). The issue is reproducible there as well (in IE, Firefox and Chrome - just in case).

The exact failure path is slightly different, but the root cause is the same - mismatch between field-events.js.tmpl and field.html.tmpl + further attempt to use the "null" returned by getELementById inside getPossiblyHiddenOption. That breaks the field.js execution and the controlled field values list stops at that.

Trunk is a little harder to test hands-on as it requires Perl 5.10.1 and it turned out I don't have a machine with this one right away. But the code in question is identical in 4.4rc1 and trunk, so it should be the same (and your comment 10 kind of confirms that).

In view of the fact the code base in field.js has changed between 4.2 and 4.4, I think it would be better to fix it in field-events.js.tmpl, by just not adding the non-active values to showValueWhen calls. Attached is a proof of concept patch against 4.4rc1+ (but should apply to trunk too), which fixes the problem for me on 4.4 and 4.2.

I'm not sure why these non-active values are added in one place (field-events.js.tmpl), but not in the other (field.html.tmpl), maybe there's some idea behind that, so I'm really interested in the feedback.

Comment 12

[Alexander Tereschenko]

Attachment: Proposed patch v2

New patch against 4.4rc1 (but should cleanly apply to trunk too), more generic solution. Will also work for 4.2 and 4.0 (just the line numbers will be slightly different).

Comment 13

[Alexander Tereschenko]

Frédéric, could you please take a look at this one? The patch itself is really trivial, but the concept is what I believe needs verification.

Comment 14

[Alexander Tereschenko]

Corrected the BZ version in view of the new findings, looks like I forgot to do that right away.

Comment 15

[Frédéric Buclin]

Comment on attachment 685165 [details] [diff] [review]
Proposed patch v2

>     [% FOREACH val = legal_value.controlled_values.$controlled_field %]
>+      [% NEXT IF !val.is_active %]

This looks good, but I have one concern: if this inactive value was already used for a bug before the value was marked as inactive, it will be listed in the drop-down field, but will have no showValueWhen() associated with it. But I suppose that's fine as it should always be possible to leave this field alone. Thanks for the patch! r=LpSolit

Comment 16

[Frédéric Buclin]

I hope we are not going to regress something...

Comment 17

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/bug/field-events.js.tmpl
Committed revision 8514.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified template/en/default/bug/field-events.js.tmpl
Committed revision 8480.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified template/en/default/bug/field-events.js.tmpl
Committed revision 8177.