There are a number of small fixes to reporting that need to be made. Because I'm on a modem, lots of small patches is hard, so it'll have to be one medium-sized one. Gerv

Fixes all the bugs in the dependency list, with the exception of the keywords one - I've removed keywords from the list of available options in the meantime in that case. Gerv

Comment on attachment 106263 [details] [diff] [review] Patch v.1 Joel - could you review this bag of fixes for me, please? :-) Gerv

Comment on attachment 106263 [details] [diff] [review] Patch v.1 Boolean charts are still broken. If I try to use one (after having seen the charts and then hit "edit this query"), every attempt to add another chart results in the redisplay of the graphic rather than the redisplay of the form with another chart.

Boolean charts really do work now :-) Gerv

Comment on attachment 106471 [details] [diff] [review] Patch v.2 >Index: report.cgi > >+# Go straight back to query.cgi if we are adding a boolean chart. >+if ($::buffer =~ /[&?]cmd\-/) { if (defined $cgi->param('cmd')) { >+ my $params = $cgi->canonicalise_query("format", "ctype"); >+ print "Location: query.cgi?format=" . $cgi->param('query_format') . >+ ($params ? "&$params" : "") . "\n\n"; >+ # We need to keep track of the defined restrictions on each of the >+ # axes, because buglistbase, below, throws them away. Without this, we >+ # get buglistlinks wrong if there is a restriction on an axis field. >+ $vars->{'col_vals'} = join("&", $::buffer =~ /[&?]($col_field=[^&]+)/g); >+ $vars->{'row_vals'} = join("&", $::buffer =~ /[&?]($row_field=[^&]+)/g); >+ $vars->{'tbl_vals'} = join("&", $::buffer =~ /[&?]($tbl_field=[^&]+)/g); $cgi->delete($col_field); and so on. I think. This is really ugly - why dont you jsut use one version w/o col, row, and tbl_field, and then add the other restrictions later? Or something along those lines. Why are you using /g, though? Just in case? The reason you need to do this is correcness

> >+# Go straight back to query.cgi if we are adding a boolean chart. >+> if ($::buffer =~ /[&?]cmd\-/) { > > if (defined $cgi->param('cmd')) { No - read the regexp carefully :-) If there is a parameter _beginning_with_ "cmd-", then... Can the $cgi object answer that question for me? >+ my $params = $cgi->canonicalise_query("format", "ctype"); >+ print "Location: query.cgi?format=" . $cgi->param('query_format') . >+ ($params ? "&$params" : "") . "\n\n"; BTW, are you saying I should be using $cgi->delete() here? > >+ # We need to keep track of the defined restrictions on each of the > >+ # axes, because buglistbase, below, throws them away. Without this, we > >+ # get buglistlinks wrong if there is a restriction on an axis field. > >+ $vars->{'col_vals'} = join("&", $::buffer =~ /[&?]($col_field=[^&]+)/g); > >+ $vars->{'row_vals'} = join("&", $::buffer =~ /[&?]($row_field=[^&]+)/g); > >+ $vars->{'tbl_vals'} = join("&", $::buffer =~ /[&?]($tbl_field=[^&]+)/g); > > $cgi->delete($col_field); and so on. I think. This is really ugly - why dont > you jsut use one version w/o col, row, and tbl_field, and then add the other > restrictions later? Or something along those lines. > > Why are you using /g, though? Just in case? > > The reason you need to do this is correcness I think you slightly misunderstand this code. The issue here is that if you are plotting Product on one axis, and have restrictions on Product in the search part of the form, then you need to make sure that those restrictions hang around. So, "col_vals" and friends are strings detailing those restrictions. An example of col_vals might be "product=Browser&product=MailNews". So, I use /g because you can (obviously) have more than one param which restricts product. There is a urlbase value which has none of them (as you suggest), and we add the necessary ones - either specifying a single value (this is the total for all bugs in product "Browser") or having just the restriction (which in many cases is empty, if you aren't restricting this column in any way. This is a bit hard to understand; so, do a plot of Product vs Severity vs Priority, for only two products, two severities and two priorities. Then, ask for some totals by clicking links. Then, apply this patch and try it again. Gerv

> No - read the regexp carefully :-) If there is a parameter _beginning_with_ > "cmd-", then... Can the $cgi object answer that question for me? if (grep(/^cmd-/, $cgi->param())) should work > BTW, are you saying I should be using $cgi->delete() here? IF you can, but I think taht your cananicalisation may preclude that > I think you slightly misunderstand this code. The issue here is that if you are > plotting Product on one axis, and have restrictions on Product in the search > part of the form, then you need to make sure that those restrictions hang > around. So, "col_vals" and friends are strings detailing those restrictions. An > example of col_vals might be "product=Browser&product=MailNews". Oh. I see. Can I say: a) 'eww' b) How does this work with Boolean charts?

> Oh. I see. Can I say: > > a) 'eww' Better solutions welcome. I thought this one was rather neat, myself. > b) How does this work with Boolean charts? The short answer is probably "it doesn't". The long answer is that making it do so would probably be quite tricky, and certainly not part of a "quick fixes and cleanup" bug. The interaction between boolean charts and defined axes needs exploring. I'm happy to accept this for the moment as a restriction. Gerv

The "it doesn't" is what leads to the 'eww' ;) Neverheless, if it doesn't work now, then thats a separate bug. Fix up the $::cgi thing, file the new bug, and then I'll review this. (To fix the separate bug, wouldn't it be enough to take the given query without any added restrictions, and then add a new bolean chart entry for the current table/column/row, so that we get an AND for the current restriction? You have to do this via boolean fields to avoid conflict with other existing entries, which is my current concern)

> (To fix the separate bug, wouldn't it be enough to take the given query without > any added restrictions, and then add a new bolean chart entry for the current > table/column/row, so that we get an AND for the current restriction? No. :-) Internally, report.cgi does _one_ query and then sorts the results into all the little tiny boxes that you see on the screen. Doing one query for each box, which you would need to do with this approach, would have much worse performance. Gerv

With bbaetz' requested fix. New bug for boolean charts is bug 180586. Gerv

Right, but the totals are currently correct, its just the links which are wrong. So you'd do one query, but do what I gussested when linking them.

Comment on attachment 106569 [details] [diff] [review] Patch v.3 OK, this works r=bbaetz

Dave: can I get an a= for 2.17.1? Gerv

a= justdave

Fixed. Checking in template/en/default/search/search-report-select.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-report-select.html.tmpl,v <-- search-report-select.html.tmpl new revision: 1.2; previous revision: 1.1 done Checking in template/en/default/search/search.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search.html.tmpl,v <-- search.html.tmpl new revision: 1.11; previous revision: 1.10 done Checking in template/en/default/search/form.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v <-- form.html.tmpl new revision: 1.13; previous revision: 1.12 done Checking in report.cgi; /cvsroot/mozilla/webtools/bugzilla/report.cgi,v <-- report.cgi new revision: 1.9; previous revision: 1.8 done Checking in template/en/default/reports/report-table.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/report-table.html.tmpl,v <-- report-table.html.tmpl new revision: 1.5; previous revision: 1.4 done Gerv