Closed Bug 221900 Opened 16 years ago Closed 16 years ago

duplicates.cgi query fails if more than one product selected

Categories

(Bugzilla :: Reporting/Charting, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: caduvall, Assigned: caduvall)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
I'm not sure if modifying the template interface is appropriate. This patch
works with what I've thrown at it.
Attachment #133110 - Flags: review?(gerv)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Patch by me awaiting r, taking.
Assignee: gerv → caduvall
Putting previous owner/review requestee on cc list.
Status: NEW → ASSIGNED
Attached patch v2 (obsolete) — Splinter Review
Clean up some unnecessary bits in the last patch.
Attachment #133110 - Attachment is obsolete: true
Attachment #133546 - Flags: review?(gerv)
Attachment #133110 - Flags: review?(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
Attachment #133546 - Flags: review?(gerv) → review-
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.
Attached patch v3, problems addressed (obsolete) — Splinter Review
Addresses concerns. Renamed array products to legal_products.
Attachment #133546 - Attachment is obsolete: true
Attachment #133711 - Flags: review?(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
Attached patch v4Splinter Review
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.
Attachment #133711 - Attachment is obsolete: true
Attachment #133711 - Flags: review?(gerv)
Attachment #133721 - Flags: review?(gerv)
Comment on attachment 133721 [details] [diff] [review]
v4

r=gerv.

Gerv
Attachment #133721 - Flags: review?(gerv) → review+
Flags: approval?
Flags: approval? → approval+
Summary: duplicates.cgi query fails more than one product selected → duplicates.cgi query fails if more than one product selected
Target Milestone: --- → Bugzilla 2.18
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Hardware: PC → All
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.