539894 - Use <optgroup/> to group products into classifications in the product drop-down on show_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Staś Małolepszy :stas]

The product drop-down on show_bug.cgi does not take advantage of the classifications, putting all products in one big list. It could use some <optgroup/> love.

Comment 2

[rojanu]

Attachment: Product Select w/optgroup

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 526991 [details] [diff] [review]
Product Select w/optgroup

Review of attachment 526991 [details] [diff] [review]:
-----------------------------------------------------------------

We will need this support added to help with 556236 which will be happening soon. This patch looks good so far but has some bit rot with the latest trunk that will need to be addressed. Also some indention issues which I have pointed out in the review comments. If you do not have time to work on this, I can take over the bug and finish it up if you like.

Thanks
dkl

::: template/en/default/admin/flag-type/edit.html.tmpl
@@ +112,5 @@
> +          	 		id => "product"
> +            		name => "product"
> +               		add => "__Any__"
> +               		onchange="selectProduct(this, this.form.component, null, null, '__Any__');"
> +              %]<br>

Nit: Please line each value up with the beginning of include. Also make sure that all => line up as well to look cleaner.

Example:

[% INCLUDE global/productselect.html.tmpl
   id       => "product"
   name     => "product"
   add      => "__Any__"
   onchange => "selectProduct(this, this.form.component, null, null, '__Any__');"
%]

::: template/en/default/global/productselect.html.tmpl
@@ +38,5 @@
> +  [% IF accesskey %] accesskey="[% accesskey FILTER html %]" [% END %]
> +  [% IF multiple %] multiple="multiple" size="[% multiple FILTER html %]" [% END %]
> +  [% IF title %] title="[% title FILTER html %]" [% END %]
> +>
> +	[% IF add %]

Make sure your indention is correct in all places. Normally in templates you use 2 spaces in and in Perl code you do 4.

::: template/en/default/request/queue.html.tmpl
@@ +85,5 @@
>        <td>
> +	    [% INCLUDE global/productselect.html.tmpl
> +    		id => "product"
> +     		name => "product"
> +     		size => 60

The size => 60 is not needed.

Comment 4

[rojanu]

I don't have time to look at this straight away, you can take over

Comment 5

[David Lawrence [:dkl]]

Attachment: Product select w/optgroup (v2)

Thanks. Attaching a revised patch.

dkl

Comment 6

[:glob ✱]

Comment on attachment 545768 [details] [diff] [review]
Product select w/optgroup (v2)

Review of attachment 545768 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/admin/flag-type/edit.html.tmpl
@@ +112,5 @@
> +                 id       => "product"
> +                 name     => "product"
> +                 add      => "__Any__"
> +                 onchange => "selectProduct(this,   this.form.component,   null,   null,   '__Any__');"
> +              %]<br>

the list of products editflagtypes.cgi provides to the template is different from user.get_selectable_products.  you'll probably have to update productselect to allow an optional list of products.

::: template/en/default/admin/flag-type/list.html.tmpl
@@ +65,5 @@
> +           id       => "product"
> +           name     => "product"
> +           add      => "__Any__"
> +           onchange => "selectProduct(this, this.form.component, null, null, '__Any__');"
> +        %]

this is another editflagtypes.cgi template, and will also need fixing.

::: template/en/default/bug/edit.html.tmpl
@@ +329,5 @@
> +           id    => "product"
> +           name  => "product"
> +           value => bug.product
> +         %]
> +       </td>

this changes us from using bug.choices.product as the product list to user.get_selectable_products, which results in a different list of products (eg. products with no components are no longer hidden).

also need to ensure the 'editable' via check_can_change_field functionality is retained, with this patch the product select is always shown as editable, even to logged out users.

::: template/en/default/reports/old-charts.html.tmpl
@@ +39,5 @@
>            <td align="center">
> +            [% INCLUDE global/productselect.html.tmpl
> +               id   => "product"
> +               name => "product"
> +            %]

you also need to update the INTERFACE section of this template, and remove the perl which which builds the 'products' var from reports.cgi.

::: template/en/default/request/queue.html.tmpl
@@ +87,5 @@
> +        [% INCLUDE global/productselect.html.tmpl
> +           id       => "product"
> +           name     => "product"
> +           add      => "Any"
> +           onchange => "selectProduct(this,  this.form.component,  null,  null,  'Any');"

nit: remove extra spaces after commas.

Comment 7

[Gordon P. Hemsley [:GPHemsley]]

What's the status of this?

Comment 8

[Andre Klapper]

Gordon: Is something specifically unclear about the previous comments? 
In comment 6 the last patch was reviewed with "review-" so status is that the patch needs rework.

Comment 9

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Andre Klapper from comment #8)
> Gordon: Is something specifically unclear about the previous comments? 

Well, the fact that they are two years old suggests that progress has stalled. I don't think it was an unreasonable question to ask.

Comment 10

[rojanu]

Attachment: Product select w/optgroup (v3)

Comment 11

[Frédéric Buclin]

Comment on attachment 8340804 [details] [diff] [review]
Product select w/optgroup (v3)

>=== added file 'template/en/default/global/productselect.html.tmpl'

Please name the file "product-select.html.tmpl". It's more readable (I know we have userselect.html.tmpl).



>+[%# The contents of this file are subject to the Mozilla Public
>+  # License Version 1.1 (the "License"); you may not use this file

Bugzilla now uses the MPL 2.0 license.


>+[% IF ! isselect %] [% isselect = 1 %] [% END %]

This code cannot work. It always falls back to a true value. You must check if the value is defined, not false.


>+      [% h = {} %]
>+      [% IF products %]
>+        [% FOREACH p = products %]
>+          [%    key = p.id -%]
>+          [% h.$key = p.id -%]
>+        [% END %]
>+      [% END %]

What means "h" and what's its job? I don't understand what you try to do here.


(was just a quick look at your patch. Still ask glob for review once you updated your patch.)

Comment 12

[rojanu]

Attachment: Product select w/optgroup (v4)

Comment 13

[Frédéric Buclin]

Comment on attachment 8340809 [details] [diff] [review]
Product select w/optgroup (v4)

Fix the license, please.

Comment 14

[rojanu]

Attachment: Product select w/optgroup (v5)

Comment 15

[:glob ✱]

Comment on attachment 8340811 [details] [diff] [review]
Product select w/optgroup (v5)

Review of attachment 8340811 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for updating this patch, it looks like an excellent improvement.
there's a few issues that i'd like to see addressed:

::: template/en/default/bug/edit.html.tmpl
@@ +256,4 @@
>      [%#  PRODUCT  #%]
>      [%#############%]
>      <tr>
> +      [% PROCESS "bug/field-label.html.tmpl" 

nit: remove trailing space

::: template/en/default/global/product-select.html.tmpl
@@ +42,5 @@
> +    [% END %]
> +    [% IF dontchange %]
> +      <option value="[% dontchange FILTER html %]">[% dontchange FILTER html %]</option>
> +    [% END %]
> +    

nit: remove trailing whitespace

@@ +47,5 @@
> +    [% IF Param('useclassification') %]
> +      [% product_ids = {} %]
> +      [% IF products %]
> +        [% FOREACH p = products %]
> +          [%    key = p.id -%]

nit: remove whitespace before key

@@ +48,5 @@
> +      [% product_ids = {} %]
> +      [% IF products %]
> +        [% FOREACH p = products %]
> +          [%    key = p.id -%]
> +          [% producs_ids.$key = p.id -%]

producs_ids --> product_ids

@@ +54,5 @@
> +      [% END %]
> +      [% FOREACH c = user.get_selectable_classifications %]
> +        <optgroup label="[% c.name FILTER html %]">
> +          [% FOREACH p = user.get_selectable_products(c.id) %]
> +            [% NEXT IF (products && ! producs_ids.exists(p.id)) %]

producs_ids --> product_ids

@@ +56,5 @@
> +        <optgroup label="[% c.name FILTER html %]">
> +          [% FOREACH p = user.get_selectable_products(c.id) %]
> +            [% NEXT IF (products && ! producs_ids.exists(p.id)) %]
> +            <option value="[% p.$valueattribute FILTER html %]"
> +              [% " selected" IF (cgi.param('product') == p.name) || (value == p.name) %]>

cgi.param('product') should be cgi.param(name)

@@ +66,5 @@
> +    [% ELSE %]
> +      [% products = user.get_selectable_products UNLESS products %]
> +      [% FOREACH p = products %]
> +        <option value="[% p.$valueattribute FILTER html %]"
> +          [% " selected" IF (cgi.param('product') == p.name) || (value == p.name) %]>

cgi.param(name) here too

::: template/en/default/reports/old-charts.html.tmpl
@@ +29,5 @@
>            <td align="center">
> +            [% INCLUDE "global/product-select.html.tmpl"
> +               id   => "product_id"
> +               name => "product_id"
> +               valueattribute => "id"

i suspect we need to pass add => '-All-' to match the existing functionality.

Comment 16

[rojanu]

Attachment: Product select w/optgroup (v6)

OK, made the suggested changes to the previous patch

Comment 17

[:glob ✱]

Comment on attachment 8364572 [details] [diff] [review]
Product select w/optgroup (v6)

Review of attachment 8364572 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: template/en/default/global/product-select.html.tmpl
@@ +24,5 @@
> +  # valueattribute: optional; the product attribute to be used for <option value="">,
> +  #           defaults to product name
> +  #%]
> +
> +[% DEFAULT isselect = 1 %]

when viewing a bug without being logged in the select is always used.
this is because DEFAULT applies to variables that are currently undefined or have no "true" value, so it can't be used for boolean values.

we can change this to the following on commit:
[% IF !isselect.defined %]
  [% isselect = 1 %]
[% END %]

Comment 18

[Dave Miller [:justdave]]

I like useful pretty

Comment 19

[:glob ✱]

thanks rojanu!

To ssh://[e-mail]/bugzilla/bugzilla.git
   b602705..b4aecfe  master -> master

Comment 20

[Frédéric Buclin]

Comment on attachment 8364572 [details] [diff] [review]
Product select w/optgroup (v6)

>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+      [% PROCESS "bug/field-label.html.tmpl"
>+         field = bug_fields.product
>+         desc_url = 'describecomponents.cgi'
>+        %]

It must be INCLUDE, not PROCESS, else all subsequent calls to field-label.html.tmpl use the same desc_url. This also has the side-effect to copy the product name into the URL field.

To ssh://[e-mail]/bugzilla/bugzilla.git
   8796328..2033e6b  master -> master