This is part of the templatization. It also does DBI conversion of the bit being templatized

Comment on attachment 172872 [details] [diff] [review] templatise the 'list products' part of editproducts v1 Mostly nits... >Index: editproducts.cgi >+ trick_taint($classification); Please comment when untainting. >+ my $sth = $dbh->prepare_cached($query); I think prepare_cached doesn't make a lot of sense here -- we don't use the query any more before dbh goes out of scope. >+ $sth->execute(@execute_params); >+ >+ my $data = $dbh->selectall_arrayref($sth); >+ >+ foreach my $aref (@$data) { [...] >+ >+ push(@products, $prod); > } >+ $vars->{'products'} = \@products; I think this whole block might be replacable by a $vars->{'products'} = $dbh->selectall_arrayref($query, {'Slice' => {}}, @execute_params) or similar, increasing readability a *lot* imo. [The disallownew field would need some tweaking either in the SELECT or in the template.] >+++ template/en/default/admin/products/list.html.tmpl Tue Dec 28 19:05:41 2004 >+ # The Initial Developer of the Original Code is Netscape Communications >+ # Corporation. Portions created by Netscape are >+ # Copyright (C) 1998 Netscape Communications Corporation. All >+ # Rights Reserved. This part should afaik not go into new files. >+ # - status: boolean; Can new bugs be created for the product. Nit: please finish the question with a question mark. >+ # - votes_per_user: number; How many votes a user is allowed in the product Nit: "The number of votes..." >+ # - votes_to_confirm: number; How many votes are needed to auto-confirm a >+ # bug in this product Similar nit here. Nittynitnit: please be consistent on whether or not to put a '.' at the end of the parameter description. >+ # classification: string; If classififcations are enabled, then this is >+ # the classification "... the currently selected classification", right? >+[% USE Bugzilla %] >+[% cgi = Bugzilla.cgi %] Do we need these? >+ [% classification_url_part = BLOCK %]&classification= >+ [%- classification FILTER url_quote %][% END %] Please align [% END %] to [% BLOCK %]... Assuming it still works then; maybe [%- END %] is needed. >+ [% classification_title = BLOCK %] >+ in classification '[% classification FILTER html %]'[% END %] Same here. >+[% title = BLOCK %] >+ Select product [% classification_title FILTER none %][% END %] >+ >+[% PROCESS global/header.html.tmpl >+ title = title >+%] Why do you use a separate BLOCK? It's being used in one place only, so title = "Select product $classification_title" right inside PROCESS header.html.tmpl should work as well... Moreover, FILTER none is reserved for error pages (see bug 256567, comment 6, where I got told so), so if you need stuff unfiltered, put an entry into filterexceptions.pl. There are several other places in your patch which need correction, too. >+[% edit_contentlink = BLOCK %] >+ editproducts.cgi?action=edit&product=%%name%% >+ [%- classification_url_part FILTER none %][% END %] >+[% delete_contentlink = BLOCK %] >+ editproducts.cgi?action=del&product=%%name%% >+ [%- classification_url_part FILTER none %][% END %] Same for these two. >+[% columns.push({ >+ heading => "Action" >+ content => "Delete" >+ contentlink => delete_contentlink >+ }) %] Nit: new line for %] >+++ template/en/default/admin/products/footer.html.tmpl Tue Dec 28 19:05:34 2004 >+ # The Initial Developer of the Original Code is Netscape Communications >+ # Corporation. Portions created by Netscape are >+ # Copyright (C) 1998 Netscape Communications Corporation. All >+ # Rights Reserved. Same as above. >+[% IF classification %] >+ [% classification_url_part = BLOCK %]&classification= >+ [%- classification FILTER url_quote %][% END %] >+ [% classification_text = BLOCK %] >+ of classification '[% classification FILTER html %]'[% END %] Similar nit to the one above -- [% END %] indentation should imo be corrected.

Comment on attachment 173986 [details] [diff] [review] templatise the list products bit of editproducts v2 Got to add $terms.

This should fix the review comments (and 1 or 2 other minor problems with the original). It also uses $terms properly, and adds a link from the count of bugs to a buglist

Comment on attachment 174084 [details] [diff] [review] templatise the list products bit v3 Excellent. The FILTER none occurrences in table.html.tmpl didn't come in by your patch and can be addressed in another bug.

Comment on attachment 174084 [details] [diff] [review] templatise the list products bit v3 COALESCE is the ANSI-standard version of IFNULL. It works in MySQL 3, as far as I know.

My bug 282460 addressing this got shot down (and rightfully so) -- Gavin, please replace IFNULL by COALESCE and put r+ on the new patch yourself.

Comment on attachment 174084 [details] [diff] [review] templatise the list products bit v3 OK, I will check in this patch, and modify it before checkin. But I'd still like the author to post a new patch, for people to use. And I'm going to leave my r- here, so people don't try to use this patch on their local installations.

Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.68; previous revision: 1.67 done Checking in template/en/default/filterexceptions.pl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v <-- filterexceptions.pl new revision: 1.34; previous revision: 1.33 done Checking in template/en/default/admin/table.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/table.html.tmpl,v <-- table.html.tmpl new revision: 1.3; previous revision: 1.2 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/footer.html.tmpl,v done Checking in template/en/default/admin/products/footer.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/footer.html.tmpl,v <-- footer.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/list.html.tmpl,v done Checking in template/en/default/admin/products/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/list.html.tmpl,v <-- list.html.tmpl initial revision: 1.1 done

As requested, here is my version of the patch, using COALESCE instead if IFNULL This is exactly as Max checked in, and I'm attaching it for completeness, carrying over Marcs r+