255425 - Post templatisation editcomponents.cgi tidy up

Status: RESOLVED INVALID

(Bugzilla :: Administration, task)

Description

[GavinS]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-gb) AppleWebKit/125.4 (KHTML, like Gecko) Safari/125.9
Build Identifier: 

Some fixes and additions and changes:


1) Fix some bugs in original templatisation patch:
1.1) correctly report of too long component name
1.2) add interface comments for edit components footer template

2) Add an 'Add component' link to the common footer

3) Add the new user-selection dropdown

4) Add <label> tags

5) Add a new 'id' parameter to the user selection template (needed for
   the label support, I think)

6) replace SendSQL and cousins with DBI/DBH code (all but the
   {Check,Test}Product() ones, which are done in another patch. Some
   questions:

6.1) Is it OK to re-use statement handles?

6.2) What/Who decides if a statement should be cached?

6.3) Is the preferred way to get the $dbh once at the start of a file,
     or in each routine it is used?

Reproducible: Always
Steps to Reproduce:

Comment 1

[GavinS]

Attachment: editcomponents.cgi tidy v1

Comment 2

[GavinS]

Glob: I've added your user selection dropdown to the editcomponents page in attachment#155976 [details] [diff] [review], 
could you give that bit a quick check, and also check the new 'id' parameter I added to the template 
interface (so it can be used with <label> tags)

Comment 3

[Dave Miller [:justdave]]

(In reply to comment #0)
 
> 6.1) Is it OK to re-use statement handles?

Sure.  Assuming you mean the variable it's stored in.  The original handle gets
destroyed when you overwrite the variable.

> 6.2) What/Who decides if a statement should be cached?

You do.  If you think you might want to cache it, just don't overwrite that
handle (as mentioned above) so that you can run ->execute() on it again without
doing a new ->prepare().  This'll create headaches for the people using Sybase,
but since every other DB back end on the planet handles it, I'm not too concerned.

> 6.3) Is the preferred way to get the $dbh once at the start of a file,
>      or in each routine it is used?

If it's actually a sub {}, then get it from within each sub.  Otherwise you run
into problems with variable scoping when running under mod_perl.

Comment 4

[:glob ✱]

GavinS,
sorry for the delay, i've been hammered by a nasty flu.
hopefully i'll get time this week to do a full review of your patch.

Comment 5

[:glob ✱]

Comment on attachment 155976 [details] [diff] [review]
editcomponents.cgi tidy v1


just a few comments as i haven't have the chance to do a full review.
usermenu changes look ok.

the patch contains tabs, which need to be replaced with spaces.


>--- editcomponents.cgi	30 Jul 2004 22:16:36 -0000	1.43
>@@ -278,6 +300,7 @@
>                        {'name' => $component});
>         exit;
>     }
>+    trick_taint($component);

trick_taint should happen just before the execute, for clarity

>@@ -286,6 +309,7 @@
>                        {'name' => $component});
>         exit;
>     }
>+    trick_taint($description);

ditto

>--- t/008filter.t	30 Jul 2004 22:16:36 -0000	1.13
>+++ t/008filter.t	12 Aug 2004 21:06:33 -0000
>@@ -202,7 +202,7 @@
>     return 1 if $directive =~ /^(time2str|GetBugLink|url)\(/;
> 
>     # Safe Template Toolkit virtual methods
>-    return 1 if $directive =~ /\.((size)$|(push))/;
>+    return 1 if $directive =~ /\.(((length)|(size))$|(push))/;

i think this would be clearer as

  return 1 if $directive =~ /\.(length$|size$|push\()/;

Comment 6

[GavinS]

Attachment: editcomponents.cgi tidy v2

Comment 7

[GavinS]

Attachment: editcomponents.cgi tidy up v3

Updating to cvs tip

Comment 8

[GavinS]

Comment on attachment 159236 [details] [diff] [review]
editcomponents.cgi tidy up v3

Requesting review -- this is a patch which fixes a few different things all at
the same time, which I agree is not the best way of doing things. (It was done
before Jouni's blog entry on the subject.) A review would be nice, but	I'll
try and split it up into the distinct parts if I don't get a review in a couple
of weeks...

Comment 9

[Marc Schumann [:Wurblzap]]

Comment on attachment 159236 [details] [diff] [review]
editcomponents.cgi tidy up v3

Tested; works.
I just found very few things for such a big patch; all smallies, I'd say.

> sub TestComponent ($$)
> {
>+    my $sth = $dbh->prepare_cached(

Is 6.2 about prepare_cached? I think comment 3 understood "caching" in terms of
re-using a prepare()d handle with multiple executes.
I'd say there is no real reason to use prepare_cached in editcomponents.cgi. It
may be intersting with mod_perl in heavily used pages. (Bug 278018, comment 7
pointed that out to me.)

>+            "SELECT components.name
>+    $sth->execute($product, $component);
>+    return $sth->fetchrow_array();

Nit: how about moving these into one return $dbh->selectrow_array('SELECT ...',
{}, ($product, $component));?

>     if ($showbugcounts) {
>+        $sth = $dbh->prepare_cached("
>+                 SELECT name, description, initialowner,
>                         initialqacontact, COUNT(bug_id)
>                  FROM components LEFT JOIN bugs ON 
>                       components.id = bugs.component_id
>+                 WHERE components.product_id = ?
>                  GROUP BY name");
>     } else {
>+        $sth = $dbh->prepare_cached("
>+                 SELECT name, description, initialowner, initialqacontact
>                  FROM components 
>+                 WHERE product_id = ?
>                  GROUP BY name");
>+    }

Why are you setting up statement handles here instead of query strings? You use
query strings in the previous part of the patch, which I personally like
better.

>@@ -415,10 +442,13 @@
>+    my $sth = $dbh->prepare_cached("SELECT count(bug_id)
>+                                    FROM bugs
>+                                    WHERE component_id = ?");
>+
>+    $sth->execute($component_id);
>+
>+    $vars->{'bug_count'} = $sth->fetchrow_array() || 0;

Nit: maybe merge these into one $dbh->selectrow_array for readability.

>@@ -461,38 +493,57 @@
>+        my $sth = $dbh->prepare_cached("SELECT bug_id
>+                                        FROM bugs
>+                                        WHERE component_id = ?");
>+
>+        my $data = $dbh->selectall_arrayref($sth,
>+                                            undef,
>+                                            $component_id);

Nit: is there a reason you create a statement handle first instead of putting
the query string as first parameter into selectall_arrayref? If not, then maybe
you'd like consider a while($sth->fetchrow_array) loop instead of the
foreach(@$data) loop?

>+        foreach my $aref (@$data) {
>+
>+          my ($bugid) = @$aref;
>+
>+          $dbh->do("DELETE FROM attachments WHERE bug_id = ?",
>+                   undef,
>+                   $bugid);
>+          $dbh->do("DELETE FROM bugs_activity WHERE bug_id = ?",
>+                   undef,
>+                   $bugid);
>+          $dbh->do("DELETE FROM dependencies WHERE blocked = ?",
>+                   undef,
>+                   $bugid);

You do these in a loop -- you should use prepared statement handles here.

>@@ -518,25 +569,31 @@
>+    my $sth = $dbh->prepare_cached("
>+             SELECT products.name, components.name, components.initialowner,
>                     components.initialqacontact, components.description
>+             FROM products LEFT JOIN components ON
>                   products.id = components.product_id
>+             WHERE components.id = ?");
> 
>     my ($product, $component, $initialownerid, $initialqacontactid,
>+        $description) = $dbh->selectrow_array($sth,
>+                                              undef,
>+                                              $component_id);

Nit: again an imo redundant sth.

>+    my $sth2 = $dbh->prepare_cached("SELECT count(*)
>+                                     FROM bugs
>+                                     WHERE component_id = ?");
>+
>+    $sth->execute($component_id);
> 
>+    $vars->{'bug_count'} = $sth->fetchrow_array() || 0;

Nit: may be merged into $dbh->selectrow_array.

>@@ -576,21 +633,26 @@
>+        trick_taint($description);
>+        $dbh->do("UPDATE components
>+                  SET description = ?
>+                  WHERE id = ?",
>+                 undef,
>+                 $description,
>+                 $component_id);

Nit: maybe a comment would be nice before untainting, along the lines of "we
consider product admins' input safe here" or somesuch.

>@@ -636,27 +704,32 @@
>+        trick_taint($component);
>+        $dbh->do("UPDATE components
>+                  SET name = ?
>+                  WHERE id = ?",
>+                 undef,
>+                 $component,
>+                 $component_id);

Same here.

>Index: template/en/default/admin/components/edit.html.tmpl
>+      <td valign="top"><label for="description">Component Description:</label></td>
>+      <td><textarea id="description" rows="4" cols="64" wrap="virtual"
>         name="description">[% description FILTER html %]</textarea>

Nit: it's not in a line you changed, but indentation is a little off here.

>Index: template/en/default/admin/components/footer.html.tmpl
>+  # no_XXX_link: flag; if defined, then don't show the corresponding 
>+  #                    link. Supported parameters are:
>+  #
>+  # no_edit_component_link
>+  # no_edit_other_components_link
>+  # no_add_component_link

So you're an opt-out guy :)

> [% IF Param("usemenuforusers") %]
> <select name="[% name FILTER html %]"
>+  [% IF id %]
>+    id="[% id FILTER html %]"
>+  [% END %]

I think it makes sense to put the id into the [% ELSE %] part as well.

Comment 10

[GavinS]

Marc: thanks for the review, I'll try and address your points soon. I've already
split out the user dropdown changes to bug#272658, and I've addressed your
relevant review comment in a new patch on that bug.

Comment 11

[GavinS]

Attachment: Edit components tidy v4

Updated to remove all the prepares and prepare_cacheds

(FYI Bits of the SendSQL removal in this patch may be soon superceded by
bug#257240 and bug#293524, but there's still some good stuff in here,
hopefully)

Comment 12

[Frédéric Buclin]

I think bug 301743 should go in first as it converts all the code from
editcomponents.cgi to use Bugzilla::Product and Bugzilla::Component. Remaining
stuff could then be fixed here.

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 188970 [details] [diff] [review]
Edit components tidy v4

Comment 12 makes sense to me. Removing r? for now.

Comment 14

[Frédéric Buclin]

GavinS, is this bug still valid 3 years after you filed it?

Comment 15

[GavinS]

(In reply to comment #14)
> GavinS, is this bug still valid 3 years after you filed it?
> 

No, all the bits have been done in other fixes, or are no longer relevant