Kiko said that if I came up with a redesign for the `Query Page' of Bugzilla, he'd have a go at implementing it. Well, here it is. This redesign would fix bug bug 16775, bug 28763, bug 66090, bug 14887 (partially), bug 28585, and bug 93908. Its main goal, however, is to make Bugzilla both more learnable and more usable in general. It clears up a lot of unnecessarily obscure language (like `query' and `OpSys'). And it dramatically reduces the amount of scrolling required to search for bugs, by placing the most useful search fields (and a `Search' button) first and by arranging the rest of the fields in a more compact fashion.

Matthew, my comments: a) I don't really like having two search buttons. If the placement of the second one is changed a bit, then it might improve; right now it seems (to me at least) a bit lost down there. b) I think we'll have to use simple tables to separate e-mail and numbering from advanced options; it's sad but NS4 just doesn't support <fieldset> and I think a lot of QA people still use NS4 to do triage. I'm not too sure on this one. c) Doesn't having search right next to the summary textbox make it seem like a separate search altogether and not part of the main form? d) I think Gerv's suggestion for changing the "What is this stuff?" link to something like "What is this advanced section down here?" is a good idea. e) I agree 100% with Gerv when he says "add an OR or AND label" to the E-mail _AND_ Advanced boxes - see the "Only bugs changed ..." and "Only bugs where any ..." section, which is a bit confusing too. x) I'm not _too_ sure about separating summary from comment/URL/status inputs but I guess this follows usage patterns? We need a bit more feedback from QA folks and the bugzilla team. It's starting to look good. :)

(a) The reason for having two Search buttons is firstly to minimize scrolling, and secondly to ensure reliable behavior of the Enter key (which is perhaps the single biggest benefit from this redesign). (b) My earlier complaint that 4.x didn't support FIELDSET turned out to be a false alarm: I'd used <fieldset><fieldset> instead of <fieldset></fieldset> by mistake. 4.x doesn't display the border, but does display the content inside as it should, and (IMO) the presentation of those fields in 4.x is still clearer than it is in the current query.cgi. (c) If that encourages novice users to carry out searches without feeling that they have to specify every field, then that's good. (d) and (e) I didn't touch that boolean stuff, because I don't understand it, and I get the feeling that if I did I'd end up redesigning it completely. I'd rather do that in a later less urgent bug. (x) Yes.

I think the new interface is much better and should be pressed into service asap. I keep avoiding the big query interface (and using the one-line textbox at http://bugzilla.mozilla.org/ instead) because of its awkardness. One question: Why is the default Status field just New, Assigned, and Reopened? Like everything else, I would expect Status to only be relevant if I decide I *want* it to be relevant. Sure, I can (in theory) define my own default template, but the default default should make sense too.

> Why is the default Status field just New, Assigned, and Reopened? There's nothing new here. Most of the time, you're not interested in bugs which have already been fixed (especially if they were fixed months or years ago); and `Unconfirmed' state was introduced specifically so that developers using the default search wouldn't have to sift through untriaged bugs.

"Status contains:" is a confusing label. Directly underneath it there is a multiselect called "Status:" and I suspect that this will confuse people. If we're going to trim that can we trim the status part and not the whiteboard part? Opersting system and Hardware have the choice ALL at the top. Is there anythig that can be done to clue people that this does not mean "any"? Email and Numbering seems confusing to me. It isn't immediately obviois that there are two seperate forms there. Is there any kind of vertical HR like thing that can be used to make this more obvious? Email and Advanced Options still look haphazard in Nav4.x

additional suggestions: If I assume that the two primary groups that for which this form is intended are reporters/triagers (people querying for an individual bug) and developers/qa/managers (people looking querying for groups of bugs) and if I also assume that the the most used fields should be most prominent (simplified to left most and top most) then I think that the Version select should be shifted to the far right of its row to communicate that it is less useful than the others. (I'm considering using the version field for the app suite components so this may become more used but still not more than the other fields in that row.) Can we come up with a better label for the Advanced Options that communicates that they all have to do with changes that happened to the bug and not necessarily the current state of a bug? "Advanced" seems like a bad label. I happen to think that a query against the status whiteboard is more advanced than bug creation date.

Asa: how about "All platforms" and "All OS" instead of plain all? Or perhaps "Affects all", "Omnipresent", "Multiple OS/HW"? We could, I guess, use a legend to say something like: "All means the bug affects _all_ of the supported platforms or OS" Just brainstorming.

In Bugzilla today, this field is not used to mean All. It's used to mean "multiple" - because if a problem appears on Windows and Linux only, people still select All. So I think the name should reflect that if possible. I'm not sure, however, if Bugzilla supports making the display name different to the underlying name. Gerv

So, lads - I just implemented this in the new templatised version. But I need to know where the radio buttons go, as you've missed them off :-) Gerv

Thankyou, Gerv! I guess I wasn't logged in when I took a copy of the current version, so I forgot about the radio buttons. Please put them where they are in the current layout (just above the row which contains the `Search' button), changing `the' to `my' and removing the period after `footer'. Other suggested changes, from Asa's comments: 1. Change `Status contains:' to `Whiteboard contains:' 2. Change `Advanced options' to `Bug changes' 3. Change to this value: [ ] (optional) to to this value: (optional) [ ]

Comment on attachment 54265 [details] template/default/query/query.atml v.1 r= justdave (on the template only, not the query.cgi file)

Comment on attachment 54264 [details] New query.cgi v.1 Tried this out on Syndicomm's bugzilla, and it works nice. Marking needs-work, though, because it fails to load the default query if you load the page via the Log In link in the footer. Mentioned this to Gerv in IRC and he said he'd found and fixed it, but the revised file isn't here yet.

> INCLUDE_PATH => "template/default" , This should be "template/custom:template/default". There's also a couple places where on the brief scan I did it appeared the style guide wasn't followed :) What I noticed (or thought I saw :) was some instances where a 4 space indent wasn't used and where the there was a brace that started a subroutine block on it's own line. It should be: sub whatever { # Do your thing }

Fixed the path problem; there was one function with dodgy braces, but that's been fixed. I kept the my $template = Template->new( { INCLUDE_PATH => "template/custom:template/default" , ... style for the two Template-related hash definitions, a) because there's precedent in all the other files, and b) because my $template = Template->new({ INCLUDE_PATH => "template/custom:template/default" , RELATIVE => 1, PRE_CHOMP => 1, }); makes it much less clear what's going on. Gerv

jake/dave: this is blocking me in moving charting into the DB, which is blocking MattyT's customisable resolutions. Any chance of completing your reviews? Gerv

I am still reviewing this. Here's what I have to say so far: It is not particularly obvious why you built up boolean hashes on the product specific fields before making a list of them. I recognise this code was present before, but it would be nice if you commented it is because there can be duplicate names across products. Please use lsearch instead of Contains. It is already in the codebase, and is more flexible. It would be OK to introduce a "Contains" function that calls "lsearch" but it should be in globals.pl. I am concerned because I want all templates to get the same general vars soonish, see bug #100094. The ppl who developed the query JS assumedly are no longer contributors to the CGI, so their names should probably be removed from it, and moved to the template (see bug #99518). I'm interested in this so we have an accurate record of whose copyright is where, in case we ever need to relicense code or some such. Now would be the right time to move this CGI over to taint mode. There is a place where you indented eight chars in the CGI: if (Param('usetargetmilestone')) { foreach my $p (@::product_list) { my $mstones = \%::target_milestone; Also places where you indented 2 characters. Now would be the time to fix these all up to 4 characters. You have at least one <a href=blah> in the template without quotes around the HREF. This is illegal XHTML as far as I know. Please rename "<a>Boolean Charts</a>" to "Advanced Querying using <a>Boolean Charts</a>". This is more accurate and obvious to newbies. In various places in the template you use the Perlism [% IF namedqueries %]. This doesn't seem to work in TT. When I have no namedqueries I still get the namedquery paraphenalia. Try [% IF 0 < namedqueries.size %]. I will have further comments but I'm still looking at them.

2.16 block list

I'm still working on it... aside from the concerns Matty had (which I agree with) I was mostly concerned with the validity of the HTML output, which I'm still working on getting a default setup that I can run validation on. I have this installed on the production bugzilla at Syndicomm and no one's reported problems, and people generally like it :) Everything seems to work so far.

More comments: What's the ternary list stuff about? Comments indicate it's about "email stuff", but what does that mean, and why is it new? I need to understand that section. How come you use <span>s to indicate headings rather than <h1>s? If there's some style you're trying to avoid we could suppress it I would imagine. We should be trying to move in the direction of structural HTML. How come "Target Milestone:" became "Target:"? There are places that are [% x %] that don't seem to be escaped, eg bug_status. method="GET" should probably be method="get". This is legal for HTML as per spec, and possibly required for XHTML from my limited understanding. In any case it's more consistent. Why is a whole heap of stuff SQL quoted and then passed to the template? The query part has been separated into three parts. I assume the rationale is that the top part is all that might be needed for most users. I also think we shouldn't place the actions in between query parts (ie the simple sections and the advanced-query/boolean-chart section). Given these two, maybe the actions should be moved to the top? We need to formulate an indenting policy. Gerv seems to be aiming for proper indentation in the source code, whereas I was more interested in proper indentation in the template. We need to decide which one we want. Adding Myk. The naming seems to be a little inconsistent. For most selections, the normally named variable is a list. However for the product specific fields, they are hashes, and you need to put _list on the end to get the normal variable. How about the list being normally named and the field->product mapping being called something like version_to_product_hash?

Comment on attachment 54265 [details] template/default/query/query.atml v.1 I put the first review here, now that I have my html-checking setup working, I'll take it away... > extra = "onLoad=\"selectProduct(document.forms['queryform']);\"" This shows up in the source like this: <body ... ALINK="#FF0000"onLoad="selectProduct(document.forms['queryform']);"> note the lack of space between the ALINK and onLoad parameters. This is probably the fault of the global/header template though. ><dl> > <dt>Only bugs changed in the last </dt> > <dd><input name=changedin size=3 value="[% default.changedin.0 FILTER html %]"> days</dd> > <br> > <dt>Only bugs where any of the fields</dt> The <br> above gets flagged by the validator... "no tags allowed here." It looks just fine if you do it like this: <dl> <dt>Only bugs changed in the last </dt> <dd><input name=changedin size=3 value="[% default.changedin.0 FILTER html %]"> days</dd> </dl> <br> <dl> <dt>Only bugs where any of the fields</dt> > <dt>were changed between</dt> > <dd> > <input name="chfieldfrom" size="10" value="[% default.chfieldfrom.0 FILTER html %]" /> > and <input name="chfieldto" size="10" value="[% default.chfieldto.0 FILTER html %]" /> > </dd> > <dt></dt> > <dd>(YYYY-MM-DD)</dd> I get a big huge space here between the text field and the (YYYY-MM-DD) indicator, which I assume should be directly under it. I solved this by doing it like this: <dt>were changed between</dt> <dd> <input name="chfieldfrom" size="10" value="[% default.chfieldfrom.0 FILTER html %]" /> and <input name="chfieldto" size="10" value="[% default.chfieldto.0 FILTER html %]" /> <br>(YYYY-MM-DD)</dd>

Blocks: bug 104211?

Yes - fixing this will fix the Bugzilla part of 104211, because the first button will be a submit button. Gerv

> Now would be the right time to move this CGI over to taint mode. It runs in taint mode now. This required a change to CGI.pl. However, should we really be running them in production in taint mode? There's bound to be a performance hit, right? Can't we just test them in taint? > We need to formulate an indenting policy. Yes. We have a known indenting policy for code, which I am attempting to follow, but we need one for templates. Taken to the newsgroup. > Please use lsearch instead of Contains. I didn't know about lsearch. Annoyingly, it doesn't return 0 on failure so you have to check for -1 explicitly. But I switched anyway. > In various places in the template you use the Perlism [% IF namedqueries %]. > This doesn't seem to work in TT. It works for scalars but not for lists. You are correct - namedqueries.size > 0 is good for lists. I believe they should fix this, but there you go. It's not fixed now. > What's the ternary list stuff about? Email searching. I didn't want to change the names of the fields in the form for backwards compatibility reasons, yet I needed to find a sane way of passing the defaults so I didn't have to repeat the code in the template. The defaults are all lists, in case they are multiply defined - so I initialise all the lists to ("", "", "") and use positions 1 and 2 (corresponding to email block 1 and email block 2) to store the default data. If I didn't do this, and just list-appended, things would break if block 2 had a default and block 1 did not. I've added more comment. > How come you use <span>s to indicate headings rather than <h1>s? All I want is for them to be normal font but bold. This is the most semantic way of achieving this. I could also use <strong>, I suppose. <h1>s are far too big. > How come "Target Milestone:" became "Target:"? > The query part has been separated into three parts.... Ask mpt about the layout questions. I'm just the code monkey :-) > method="GET" should probably be method="get". This is legal for HTML as per > spec, and possibly required for XHTML from my limited understanding. XHTML certainly doesn't specify that attribute _values_ have to be lower case. But it's more consistent, as you say, so I changed it. New patch coming up with these and all other issues addressed. Gerv

> title = "Bugzilla Query Page" > h1 = "Query" These should both be `Search for bugs' ... > h2 = "This page lets you search the database for recorded bugs." ... whereupon a translation of `query' into non-geek English would be no longer required, so this line becomes a waste of vertical scrolling and should be removed. > <p>Give me a <a href="queryhelp.cgi">clue</a> about how to use this form.</p> In theory, the redesign should also make this merely a waste of vertical scrolling. Please remove it, or put it at the bottom of the page. > <td align="right"><span class="heading">Summary:</span></td> The mockup uses <th align="right">, which is more structural and has the desired effect in more browsers. Why change it? > <tr> > <th align="left" valign="bottom">Program:</th> > <th align="left" valign="bottom"><a > href="describecomponents.cgi">Component</a>:</th> > <th align="left" valign="bottom">Version:</th> <tr valign="bottom"> would save repetition here.

I'll make mpt's changes; don't let them stop anyone reviewing this again :-) Gerv

mpt, pls comment on whether you think moving the actions to the top would be a good idea. OK, some of those problems seem to be fixed, but many aren't and haven't been replied to. In particular, SQL quoting, inconsistent naming, boolean hash comments and bad indenting. Other stuff: You use lsearch, but Contains is still defined and passed to the template. You added a license header to the template, but should the JS contributors be removed from the CGI? Our precedent is taint mode on. Most people seem to think there won't be a noticeable perf reduction. Taint mode is designed for things like CGIs. You still have "if [% IF keywords %]". I think this is a hash, you should use .keys.size instead if so. "orders" should probably be filtered HTML, even though it doesn't matter at this stage. You have "[% END%]" in one place.

> In particular, SQL quoting, This is the ideal function for creating JS array definitions. I've added a comment to that effect. If you wish to rename SqlQuote to SingleQuote throughout Bugzilla, feel free. :-) > inconsistent naming, Where? I've changed keywords to a boolean scalar, if that helps. > boolean hash comments What about: # We build up boolean hashes in the "-set" hashes for each of these things # before making a list because there may be duplicates names across products. ? > and bad indenting. One instance. > You added a license header to the template, but should the JS contributors be > removed from the CGI? Yes. :-) The JS is now in the template. Gerv

> This is the ideal function for creating JS array definitions. I've added a > comment to that effect. If you wish to rename SqlQuote to SingleQuote > throughout Bugzilla, feel free. :-) SqlQuote doesn't just put single quotes around the string. It also escapes the proper characters in the right way. Does JS and SQL do this exactly the same way? If so, we should create a sub called JSQuote that calls SqlQuote. It should probably be passed as a filter to the template though. Other templates wouldn't want to use Javascript quoting necessarily. > What about: Sounds fine. >> and bad indenting. > One instance. Don't know when it was fixed, but it seems to be. > -require "CGI.pl"; > +require "./CGI.pl"; We generally use 'use lib "."'. You did something similar on CGI.pl. I think consistency is useful here. > inconsistent naming, I missed the fix for this, I must have been looking at the wrong patch! Once these are dealt with, I'll give the patch another going over. I expect I'll find a couple more issues then.

> Once these are dealt with, Oh, come on. The patch is hardly un-reviewable. Pretend I removed all the calls to SQLQuote and passed in a jsquote filter and used that. And made the tiny lib change. Gerv

> Oh, come on. The patch is hardly un-reviewable. Pretend I removed all the > calls to SQLQuote and passed in a jsquote filter and used that. And made the > tiny lib change. This is your code. You can read it better than us. I was having trouble reviewing it. Little niggling issues _do_ get in the way of other things. And I probably won't have time for a full review here for another day anyway. Plus you're going to need a second reviewer anyway, why not bring one in now?

> why not bring one in now? That would be justdave. <cough> Gerv

I was also concerned about the removal of the backwards compatibility hack. I think to remove any backwards compatibility cookie hacks for non-browser stuff, we would need to deprecate upgrading from version X to Y, and remove the code from checksetup.pl. This seems to be a consensus of a small number of us on IRC. We haven't done this. Plus, this is cookie stuff. It doesn't get updated by server upgrades, so we can't expect them to be updated. Hence we should wait a while after we deprecate the version upgrade to remove this hack.

Named queries were moved into the DB almost two years ago now, just after version 2.8 was released. Removing this hack means that people upgrading from versions of Bugzilla <= 2.8 will lose their stored queries. It doesn't mean we have to deprecate the upgrade - it should all still work just fine. We'll just need to release note it. We can put it back in if you like - but I think there's a legitimate debate here how much backwards compatibility cruft Bugzilla should be carrying around that's not in checksetup.pl. Gerv

um, i'm running a 2.8 install, and i've been asked to pick a time to upgrade it. I don't know all the users who use our install, nor do all the users know me. I certainly don't want to meet all of them when they complain that i broke their systems. -- I promise to read this bug carefully before i upgade.

<shrug> OK, I'll put it back. Gerv

Here's the latest; I've rewritten part to use a SELECT block, like enter_bug.atml. I've also added back the backwards-compatibility hack. Gerv

Comment on attachment 57395 [details] [diff] [review] Patch v.6 - all in one >+ [% INCLUDE select %] This here seems to make the tests think there's a template named "select" it should be looking for and thus the tests fail. Does this do what you're thinking it does and the tests are broken, or are we using the wrong command to import the object you built earlier?

Having checked the docs, you can use either INCLUDE or PROCESS. The difference lies in the fact that in INCLUDE, variables are passed by value (i.e. copied) and in PROCESS, they are passed by reference (i.e. changes come back to the calling block.) "The PROCESS directive is slightly faster than INCLUDE because it avoids the need to localise (i.e. copy) the variable stash before processing the template." So, for this sort of thing, we should probably be using PROCESS. But the tests should recognise both. It's merely a case of going through and changing it - I'll do that in the next version, after review - unless those were your only review comments. ;-) Gerv

those were the only comments so far. I installed the patch, then ran the tests on it. Got an error that it couldn't find a template named "select". If that statement is legal, than the tests need to be more careful about what they pick out as template names. I haven't tried running it yet. Haven't had a chance to. Jake, Myk, zach, anyone else have some time to look at this? We REALLY need this reviewed and in by the end of this week to have time for everything else on the list before December, among other things.

more CCs for people who might help

Comment on attachment 57395 [details] [diff] [review] Patch v.6 - all in one >$::CheckOptionValues = 0; # It's OK if we have some bogus things in the > # pop-up lists here, from a remembered query > # that is no longer quite valid. We don't > # want to crap out in the query page. It took me a few minutes to figure out the purpose of this switch. A comment like "Prevents &make_options in CGI.pl from throwing an error if we give it an invalid list of selections (from a remembered query containing values that no longer exist). We don't want to crap out in the query page even if strict value checks are turned on." would make more sense. > # Function to jsquote a value - SqlQuote() does this for us nicely, > # escaping ' and \, and adding single quotes around the value. > 'jsquote' => \&SqlQuote, Do this instead (from patches for bug 72837), as it handles more cases: + # Returns the text with backslashes, single/double quotes, + # and newlines/carriage returns escaped for use in JS strings. + js => sub { my ($str) = @_; $str =~ s/([\\\'\"])/\\$1/g; $str =~ s/\n/\\n/g; $str =~ s/\r/\\r/g; return $str; } , >my $userid = 0; >if (defined $::COOKIE{"Bugzilla_login"}) { > $userid = DBNameToIdAndCheck($::COOKIE{"Bugzilla_login"}); >} Doesn't quietly_check_login already take care of this by defining $::userid? #' ># Backwards compatability hack -- if there are any of the old QUERY_* compatability -> compatibility # If the name ends in a number (which it does for the fields which # are part of the email searching), we use the array # positions to show the defaults for that number field. > if ($name =~ m/(.*)(\d)/ && defined($default{$1})) { This line matches names that do not end in numbers in the same manner as this command line prints "matched": perl -e '$name = "foo4bar"; if ($name =~ m/(.*)(\d)/) { print "matched\n"; }' > # Otherwise, if no default yet, so we replace the blank value Hunh? ># "foo_list" is a list of all the possible (or legal) products, components, ># versions or target milestones. This comment needs updating for the new "foo_to_product_hash" naming scheme. >shift @::legal_resolution; > # Another hack - this array contains "" for some reason. Mention bug 106589 here. >$vars->{'products'} = \@products; >$vars->{'components'} = \@components; >$vars->{'version'} = \@versions; >$vars->{'resolution'} = \@::legal_resolution; Be consistent with pluralization. >my @chfield = ("[Bug creation]", @::log_columns); >$vars->{'chfield'} = \@chfield; Make your life easier, do this: $vars->{'chfield'} = ["[Bug creation]", @::log_columns]; >my @types = (["noop", "---"], ... > ["changedby", "changed by"], > ); Match the indentation of the parentheses to each other. CGI.pl: >use lib qw(.); As we discussed, use the same syntax for this line as you use in query.cgi: use lib "."; query.atml: > [% IF have_keywords %] My test installation has keywords, but the field does not show up in the query form. I strongly recommend using sensible variable names and hashes instead of arrays for non-linear values. It would make the code much more accessible, especially for the UI designers and novice programmers (for whom making it easier to hack on Bugzilla UI is part of the reason for the templatization effort). i.e. the code: > [% FOREACH x = [ ["anywords", "contains any of the keywords"], > ["allwords", "contains all of the keywords"], > ["nowords", "contains none of the keywords"] ] %] > <option value="[% x.0 %]" > [% " selected" IF default.keywords_type.0 == x.0 %]>[% x.1 %]</option> > [% END %] ...is more readable, if more verbose, as: [% FOREACH type = [ { value => "anywords", description => "contains any of the keywords" } , { value => "allwords", description => "contains all of the keywords" } , { value => "nowords", description => "contains none of the keywords" } ] %] <option value="[% type.value %]" [% " selected" IF default.keywords_type.0 == type.value %]>[% type.description %]</option> [% END %] This is especially true for the more complicated parts of the template.

How many more times can I spank this patch? :-) Gerv

I believe the record is 13 and is held by kiko on bug #96534.

Comment on attachment 57702 [details] [diff] [review] Patch v.7 query.atml: >var usetms = [% IF Param('usetargetmilestone') %]0[% ELSE %]1[% END %]; Isn't this logic reversed, and shouldn't this be using JavaScript's boolean true and false values? >[%# For each of components, versions and target milestone... %] >[% FOREACH x = [[componentsbyproduct, "cpts"], > [versionsbyproduct, "vers"], > [milestonesbyproduct, "tms"]] %] > var [% x.1 %] = new Array(); > [%# ...create an array. For each x, set it equal to a list of values %] > [% FOREACH p = product %] > [% x.1 %]['[% p %]'] = > [[%- FOREACH y = x.0.$p -%][%- y FILTER js -%], [%- END -%]]; > > [% END %] > >[% END %] The new JavaScript escaping routine you are using does not place quotes around the values you escape, so you need to add them around "[%- y FILTER js -%]" or use the following routine, which is a more efficient way to do this because it only requires a single loop through the products (instead of three): +var cpts = new Object(); +var vers = new Object(); +var tms = new Object(); + +[% FOREACH p = allproducts %] + cpts['[% p FILTER js %]'] = [ [%- FOREACH item = componentsbyproduct.$p -%]'[% item FILTER js %]', [%- END -%] ] + vers['[% p FILTER js %]'] = [ [%- FOREACH item = versionsbyproduct.$p -%]'[% item FILTER js %]', [%- END -%] ] + tms['[% p FILTER js %]'] = [ [%- FOREACH item = milestonesbyproduct.$p -%]'[% item FILTER js %]', [%- END -%] ] +[% END %] > [% sel = { name => 'version', size => 5 } %] > [% PROCESS select %] You can combine these like so: [% PROCESS select sel = { name => 'version', size => 5 } %] > <td><input name="[% field.name %]" size="40" value=" > [% default.${field.name}.0 FILTER html %]"></td> Break up this line in a way that does not add a newline to the value of the field. > [% FOREACH x = chfield %] > [% default.chfield.0 %] > <option value="[% x FILTER html %]" > [% " selected" IF lsearch(default.chfield, x) != -1 %]>[% x FILTER html %]</option> > [% END %] > [% FOREACH x = namedqueries %] > <option value="[% x FILTER html %]">[% x FILTER html %]</option> > [% END %] > [% FOREACH x = orders %] > <option value="[% x FILTER html %]" > [% " selected" IF default.order.0 == x %]>[% x %]</option> > [% END %] x -> something that makes more sense.

The template test has been updated to not choke on the [% BLOCK %] element (I pulled the code that was looking for included fragments as if they don't exist or have errors, the template that includes them will fail the test).

The JS error is, in fact, in the original version :-) No idea how long it's been there. The newline isn't actually put in, because I have PRE_CHOMP turned on. But I added a - sign to make it clear. All issues addressed. Gerv

New version. Gerv

Comment on attachment 57722 [details] [diff] [review] Patch v.9 looks good. r=myk. second review definitely needed.

Comment on attachment 57722 [details] [diff] [review] Patch v.9 I hate doing this, honest!

Problems blocking positive review: Field "Only bugs where any of the fields" does not work properly when you load a default value into it, either through storing a default query or editing an existing query. This comment: # "foobyproduct" is a list ... # ... # "foo" or "foos" ... # ... Has a couple of problems. What is says is a list is a hash and vice versa. It also still talks about SQL quoting. I don't see any reply to Myk's comment on consistent pluralisation. In particular, this seems to be the reason why you don't use PROCESS select on components. Problems not blocking positive review, but which you might want to fix given the patch has to be redone: I think the 2 milestone parameters should be moved after said comment and an additional if block introduced. You have used " ," in two places (INCLUDE_PATH and Param) as opposed to just the "," on nearby lines. The comment about field zero should say something more explicit like "Array position 0 is unused for email fields because the form parameters historically started at 1.". In places (FetchSQLData()) appears, which can be just FetchSQLData(). The indentation at one part of the template is out of whack and goes back to column 0. Is the theory here that indentation was getting too deep? There are places where you passed in data hardcoded into the CGI, such as order, search type, etc. I generally prefer to be conservative here and html filter all these instances even if they don't (currently) need it. Advanced querying conditions don't get saved when you save your default query. This doesn't appear to be a regression though, and you could possibly put a case (that I don't agree with) that they shouldn't. They load OK with edit query. This: # If there's already a default, we push on the new value. else { should I believe be this: # If there's already a default, we push on the new value. elsif (defined($default{$name})) { although it probably doesn't matter - it will just pass extras to the template.

> Field "Only bugs where any of the fields" does not work properly when you load > a default value into it, either through storing a default query or editing an > existing query. Works for me. > I don't see any reply to Myk's comment on consistent pluralisation. Sorry; we talked about this, and I fixed it as far as I could. I couldn't fix it totally because "component" is a reserved word in the toolkit. > The indentation at one part of the template is out of whack and goes back to > column 0. Is the theory here that indentation was getting too deep? Yes. I use a double-blank-line separator to make this more clear. OK. New version is here. r=matty? :-) Gerv

The bug appears to be because of this line: [% default.chfield.0 %] It appears it shouldn't be in the FOREACH on the previous line. Regarding pluralisation, I think it would be nice if we used the plural form everywhere when passing to the template. Then the BLOCK could take the plural and singular forms as two parameters or just the plural and strip the s off. I think the extra parameter would be nicer given we get consistent, nice capitalisation and components can use the BLOCK. We won't really be able to do this later, but I won't hold up review for it if you don't agree.

> It appears it shouldn't be in the FOREACH on the previous line. You are correct - that was debugging code. Thanks :-) > Regarding pluralisation, I think it would be nice if we used the plural form > everywhere when passing to the template. I prefer to see the situation as using the URL-named parameter and standard form name for everything, but with a special exception for component[s], because of the toolkit problem. If the plural bothers you, we could go with "component_". New patch attached. This one doesn't remember components etc. in remembered queries - but I seem to remember there's another bug about that, right - it's a JS problem. It certainly doesn't work on Landfill. If I've got that right, Myk and Matty, could you stamp this one, please? :-) Gerv

Product specific fields disappearing is bug #97966.

Checked in. Wahey! Time now to hack it to pieces again... Gerv

Woohoo! Filed bug 110708, bug 110709, bug 110710, bug 110711, and bug 110712 on some of the places where the implementation doesn't match the spec. :-)

Oh, for goodness sake - one bug with all the stuff in would have been fine :-) It's blatantly obvious that I'll fix it all in one go. ;-) Gerv

Gee, and I thought I was being lazy filing bug 110710 as a single bug instead of two separate ones ...

Why does the Status Whiteboard field still say "Status Contains:" and not "Whiteboard Contains:"?

Kerz: that would be what we in the business call a "bug". ;-) File it on me. Gerv

It appears that this redesigned form (which is so incredibly better that I can hardly wait for this to be on bugzilla.mozilla.org) uses the label "Program" instead of "Product". Unless this is going to be changed globally in bugzilla, it should probably stay Product. Product makes more sense to me anyway. Also, the ability to save queries seems to have gone away. I found them quite useful especially when added to my footer, although somewhat difficult to set up. Is this feature being eliminated? Or is there a new way to do it that I'm missing? Let me know if I should log new bugs about these. Thanks for the great work!

Tim: see bug 99864

At Netscape, we've actually pulled the changes to query.cgi into our instance of bugzilla and have been using that version. I've come across one "bug" about the query field. Pls let me know if you want me to file a separate bug. For example, I have a query where I set the keyword field to equal smoketest. Here is an example of the query (note: on my query, bugzilla.mozilla.org is substituted by our database name) http://bugzilla.mozilla.org/query.cgi?bug_severity=blocker&email1=&emailtype1=substring&emailassigned_to1=1&email2=&emailtype2=substring&emailreporter2=1&bugidtype=include&bug_id=&changedin=&votes=&chfield=%5BBug+creation%5D&chfieldfrom=12%2F3%2F2001&chfieldto=12%2F9%2F2001&chfieldvalue=&product=Browser&product=Browser+Localizations&product=CCK&product=MailNews&product=NSPR&product=NSS&product=PSM&short_desc=&short_desc_type=substring&long_desc=&long_desc_type=substring&bug_file_loc=&bug_file_loc_type=substring&status_whiteboard=&status_whiteboard_type=substring&keywords=smoketest&keywords_type=anywords&field0-0-0=noop&type0-0-0=noop&value0-0-0=&cmdtype=doit&namedcmd=bugs+for+triage&newqueryname=&order=Bug+Number Using the newer query.cgi screen, I run this query. Then, I choose to "edit this query" The problem is that the keyword field is in the advance fields and no longer just a plain text field and when I edit the query, the keyword field doesn't get populated automatically. Note: Luckily, I knew the the results I got back was larger than normal and looked into it further. This problem may cause some false hits/totals for other people who run pre-canned queries after doing a minor modification to the query. Thanks.

yes, please file a separate bug. tag it as a regression if this used to work.

http://bugzilla.mozilla.org/show_bug.cgi?id=116071 filed. Thank you.

*** Bug 66090 has been marked as a duplicate of this bug. ***