221900 - duplicates.cgi query fails if more than one product selected

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[chaduv]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20030925
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20030925

If you attempt to use duplicates.cgi to query for more than one product, it
throws an Invalid Product Name exception, and the product names appear
concatenated together, e.g.  The product name 'BugzillaCalendar' is invalid or
does not exist. 

Reproducible: Always

Steps to Reproduce:
1. Go to the main http://bugzilla.mozilla.org page
2. Click the most recently duplicated bugs link
3. Scroll down to the bottom of the dup page, choose any pair (or more)
products, push the "Change" button.

Actual Results:  
An error page appeared saying there was an invalid product name.

Expected Results:  
The duplicates list should be filtered by the selected products.

Comment 1

[chaduv]

Attachment: Filter multiple products properly

I'm not sure if modifying the template interface is appropriate. This patch
works with what I've thrown at it.

Comment 2

[chaduv]

Patch by me awaiting r, taking.

Comment 3

[chaduv]

Putting previous owner/review requestee on cc list.

Comment 4

[chaduv]

Attachment: v2

Clean up some unnecessary bits in the last patch.

Comment 5

[Gervase Markham [:gerv]]

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

This is pretty good; a couple of things to clean up, though.

>@@ -78,7 +78,7 @@
>                 [% "&maxrows=$maxrows" IF maxrows %]
>                 [% "&changedsince=$changedsince" IF changedsince %]
>                 [% "&openonly=1" IF openonly %]
>-                [% IF product %]&product=[% product FILTER html %][% END %]
>+                [% FOREACH p = product %]&product=[% p FILTER html %][% END %]

What happens here if none of the clauses above the "product" one fires? Will
you get:
foo.cgi?&product=bar
?

>+  # product: list of strings. Restrict to these products only.

This is getting a bit confusing, because we now have a "products" array with
all the products, and a "product" array with a subset of them in. Perhaps we
should be calling this parameter "query_products" or something?

	  <option name="[% p FILTER html %]"
>-            [% " selected" IF product == p %]>[% p FILTER html %]</option>
>+            [% FOREACH p2 = product %]
>+              [% IF p2 == p %] selected[% END %]
>+            [% END %]>[% p FILTER html %]</option>

We have a function "lsearch" which does this for you. Grep for usage examples.

Gerv

Comment 6

[chaduv]

1) A few lines above that is: a href="duplicates.cgi?sortby=[% column.name %] 
which ensures you can't get a ?&product...

2) Hm. I didn't think about this, as I was just changing string "product" to 
array "product". Array "products" might be able to go away if 
GetSelectableProducts was used in the right places, I think. I'll look into it.

3) Ok. Will attach a patch with this fixed when I get home.

Comment 7

[chaduv]

Attachment: v3, problems addressed

Addresses concerns. Renamed array products to legal_products.

Comment 8

[Gervase Markham [:gerv]]

Comment on attachment 133711 [details] [diff] [review]
v3, problems addressed

>@@ -258,8 +258,8 @@
> $vars->{'openonly'} = $openonly;
> $vars->{'reverse'} = $reverse;
> $vars->{'format'} = $::FORM{'format'};
>-$vars->{'product'} = $product;
>-$vars->{'products'} = \@::legal_product;
>+$vars->{'product'} = \@product;
>+$vars->{'legal_products'} = \@::legal_product;

The problem with renaming the array of all the products (as opposed to the
array of used ones) is that we have a convention in most templates that the
array of all products is called "products". Hence my suggestion of renaming the
other one to e.g. query_products, or some better name.

Gerv

Comment 9

[chaduv]

Attachment: v4

Gotcha. It had crossed my mind to wonder why "legal_products" only occured once
in template files, when it was the name of the global variable storing 'em.

I couldn't think up a better name than query products, so here it is.

Comment 10

[Gervase Markham [:gerv]]

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

r=gerv.

Gerv

Comment 11

[Vlad Dascalu]

Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in template/en/default/reports/duplicates-table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates-table.html.tmpl,v
 <--  duplicates-table.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/reports/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates.html.tmpl,v
 <--  duplicates.html.tmpl
new revision: 1.13; previous revision: 1.12
done