812054 - JavaScript error on the Advanced Search page when a classification has a product invisible to the user

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P1)

Description

[Simon Green]

If you have classifications enabled, and a product that isn't visible to a user (entry, mandatory/mandatory for one group the user can't see for example), a Javascript error occurs in the Advance Search page when you click on the classification that has restricted products.

TypeError: val is null
val.bz_value_id = optionId;

in field.js line 784

This is caused because this function is using $classification->products which does not filter products based on their visibility to the user. This change to is filter the products list.

Comment 1

[Simon Green]

Attachment: v1 patch

Diff is against trunk, but it should be applied to 4.4 as well. Earlier versions are not affected.

Comment 2

[Frédéric Buclin]

It would good to know which bug caused this problem.

Comment 3

[Simon Green]

(In reply to Frédéric Buclin from comment #2)
> It would good to know which bug caused this problem.

http://bzr.mozilla.org/bugzilla/4.4/revision/7907

Bug 636416: Use the standard value-controller javascript to control the
drop-down fields on the Advanced Search page.
r=glob, a=mkanat

Comment 4

[Frédéric Buclin]

Confirmed.

Comment 5

[:glob ✱]

Comment on attachment 681824 [details] [diff] [review]
v1 patch

r=glob

it's a shame 'product' isn't named 'selectable_products' in query.cgi.

Comment 6

[Frédéric Buclin]

Comment on attachment 681824 [details] [diff] [review]
v1 patch

>   [% FOREACH legal_value = legal_values %]
>     [% FOREACH sub_value = legal_value.$accessor %]
>+        [% FOREACH p = product %]
>+          [% IF p.id == sub_value.id %]

This code makes me unhappy. With a large number of products, this code will run as O(N^2), which is quite large. You should instead do something different for classifications (which is the unique field triggering your code):

[% IF field.name == "classification" %]
  [% FOREACH p = product %]
    [% prod_per_class.${p.classification_id}.push(p.id) %]
  [% END %]

  [% FOREACH c = prod_per_class.keys %]
    showValueWhen()
  [% END %]
[% ELSE %]
  ... existing code ...
[% END %]

This code runs as O(N), which is N times faster than your patch.

So despite your patch fixes the problem, this is r- for performance reasons.

Comment 7

[Simon Green]

Attachment: patch to fix this issue

Comment 8

[Frédéric Buclin]

Comment on attachment 685430 [details] [diff] [review]
patch to fix this issue

Has this patch been tested? It doesn't work at all. When selecting a classification in the Advanced Search page, the list of products isn't updated anymore.

Comment 9

[Simon Green]

(In reply to Frédéric Buclin from comment #8)
> Has this patch been tested?

Yes, on Firefox last week, and Chrome today. Since I use Linux, I don't have access to an IE machine.

> It doesn't work at all. When selecting a
> classification in the Advanced Search page, the list of products isn't
> updated anymore.

It works for me. Are you getting a Javascript error at all, or is it just not working? What browser and version are you using?

Comment 10

[Frédéric Buclin]

Firefox 17 on Linux. With your patch applied, no JS error is thrown, but the products list isn't refreshed anymore when selecting a classification.

Comment 11

[Simon Green]

Attachment: updated patch

Comment 12

[Marc Schumann [:Wurblzap]]

(In reply to Simon Green from comment #0)
> This is caused because this function is using $classification->products
> which does not filter products based on their visibility to the user. This
> change to is filter the products list.

Doesn't this mean that there is an unfiltered product list client-side? Why is this not a sec bug?

Comment 13

[Simon Green]

(In reply to Marc Schumann [:Wurblzap] from comment #12)
> Doesn't this mean that there is an unfiltered product list client-side? Why
> is this not a sec bug?

Two reasons:

1) It only appears in 4.4rc1 and trunk. Neither of these are production versions.
2) It is only exposing the product.id value, not the products name or other information about the product.

Regards,
Hugo.

Comment 14

[Frédéric Buclin]

Comment on attachment 687388 [details] [diff] [review]
updated patch

>+  [% IF accessor == 'products' %]

Because accessor == "products" is pretty cryptic, we should write field.name == "classification" instead. This can be fixed on checkin.

Now your patch looks good and is working fine. Thanks! :) r=LpSolit

Comment 15

[Frédéric Buclin]

I don't know your name, so I simply write "Hugo" as author in the commit message.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/search/field.html.tmpl
Committed revision 8501.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified template/en/default/search/field.html.tmpl
Committed revision 8477.