280412 - Templatize the 'list products' bit of editproducts

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[GavinS]

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

Comment 1

[GavinS]

Attachment: templatise the 'list products' part of editproducts v1

Comment 2

[Marc Schumann [:Wurblzap]]

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 3

[GavinS]

Attachment: templatise the list products bit of editproducts v2

Comment 4

[GavinS]

Comment on attachment 173986 [details] [diff] [review]
templatise the list products bit of editproducts v2

Got to add $terms.

Comment 5

[GavinS]

Attachment: templatise the list products bit v3

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 6

[Marc Schumann [:Wurblzap]]

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 7

[Max Kanat-Alexander]

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.

Comment 8

[Marc Schumann [:Wurblzap]]

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 9

[Max Kanat-Alexander]

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.

Comment 10

[Max Kanat-Alexander]

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

Comment 11

[GavinS]

Attachment: templatise the list products bit v4

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+