286294 - cleanup editclassifications.cgi and migrate the existent code to use Classification.pm

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

The code of editclassifications.cgi could be cleaned up a bit and should use
already existing functions from globals.pl (or anywhere these functions could
go), such as get_classification_id() and GetSelectableClassifications(). The
duplication of functions is bad.

Comment 1

[Tiago Mello [:timello]]

Attachment: v1: cleaning up editclassifications.cgi

Comment 2

[Frédéric Buclin]

Comment on attachment 189446 [details] [diff] [review]
v1: cleaning up editclassifications.cgi

>Index: editclassifications.cgi

>+my $action =              trim($cgi->param('action')         || '');
>+my $classification_name = trim($cgi->param('classification') || '');
>+my $classification = undef;

my $classification = undef; is useless here. Remove it.


> unless ($action) {

>+} elsif ($action ne 'add' && $action ne 'new') {

> }

unless... elsif... Hrm! Anyway, I don't like having the classification object
defined here. I prefer you to rewrite it in all required ($action) blocks
later.


> if ($action eq 'new') {
>-    unless ($classification) {
>+
>+    unless ($classification_name) {
>         ThrowUserError("classification_not_specified");
>     }
>-    if (TestClassification($classification)) {
>-        ThrowUserError("classification_already_exists", { name => $classification });
>+    if ($classification) {
>+        ThrowUserError("classification_already_exists",
>+                       { name => $classification->name });
>     }

Most remarks I made about editversions.cgi also apply for this bug. Create a
classification object; if this object is not undefined, then this
classification name is already in use.


>Index: Bugzilla/Product.pm

Product.pm has already been updated.


This only a partial review, but this patch already needs an updated version.

Comment 3

[Tiago Mello [:timello]]

Attachment: v2: fixing some bugs.

Comment 4

[Frédéric Buclin]

Comment on attachment 189766 [details] [diff] [review]
v2: fixing some bugs.

>Index: editclassifications.cgi

> unless ($action) {
>+    my @classifications =
>+        Bugzilla::Classification::get_all_classifications();
> 
>     $vars->{'classifications'} = \@classifications;
>     LoadTemplate("select");

get_all_classifications() doesn't include the number of products per
classification, but this information is required in
admin/classifications/select.html.tmpl. You will have to include
$self->bug_count() in get_all_classifications(). I will come back to this point
later.


> if ($action eq 'new') {
>     # Add the new classification.
>-    my $sth = $dbh->prepare("INSERT INTO classifications (name,description)
>-                            VALUES (?,?)");
>-    $sth->execute($classification,$description);
>+    my $sth = $dbh->prepare("INSERT INTO classifications
>+                              (name,description)
>+                             VALUES (?,?)");
>+
>+    $sth->execute($class_name, $description);

Even better: $dbh->do("INSERT INTO classifications (name, description) VALUES
(?, ?)"
		      undef, ($class_name, $description));

This is cleaner, and we do the same thing with less code (also easier to
review).


> if ($action eq 'del') {
>+    my $classification =
>+        Bugzilla::Classification::check_classification($class_name);
>+
>+    my $sth;

$sth is not used anymore. Remove it!


>+    ThrowUserError("classification_not_deletable")
>+        if ($classification->id eq "1");

The classification ID is numeric, so use the numeric comparator '=='. And the
code is short enough to remain on a single line. Or use:

if (...) {
    ThrowUserError(...);
}


> if ($action eq 'delete') {

>     my $sth;

We won't need $sth, see below.


>+    if ($classification->id == 1) {
>+        ThrowUserError("cant_delete_default_classification",
>+                       { name => $classification->name });
>     }

I know this is not your code, but you are doing some cleanup, so... please
remove "cant_delete_default_classification" completely, i.e. also from
global/user-error.html.tmpl and replace it by "classification_not_deletable",
which doesn't take any parameter and which says the same as the error message
above. Moreover, "classification_not_deletable" is already in use in the
($action eq "del") section above.


>     # delete
>     $sth = $dbh->prepare("DELETE FROM classifications WHERE id=?");
>-    $sth->execute($classification_id);
>+    $sth->execute($classification->id);

Even better: $dbh->do("DELETE FROM classifications WHERE id = ?", undef,
$classification->id);


>     # update products just in case
>     $sth = $dbh->prepare("UPDATE products 
>                           SET classification_id=1
>                           WHERE classification_id=?");
>-    $sth->execute($classification_id);
>+    $sth->execute($classification->id);

Even better: $dbh->do("UPDATE products SET classification_id = 1 WHERE
classification_id = ?",
		      undef, $classification->id);

As before, we don't need $sth and we do the same job with less code.


> if ($action eq 'edit') {

>+    $vars->{'description'} ||= $classification->description;

$vars->{'description'} is not defined yet, never. So remove "||".


>+    $vars->{'classification'} = $classification->name;
>+    $vars->{'products'} = \@products if (scalar(@products));

Nit: Please check, but I think the test is not needed (i.e. remove if (...)).


> if ($action eq 'update') {
>+    my $classificationold = trim($cgi->param('classificationold')   || '');

Please rename this variable as $class_old_name, for consistency.


>+    my $descriptionold    = trim($cgi->param('descriptionold')      || '');

You never use $descriptionold, because you consider the one obtained from the
object (which is good). So remove this variable as well as its corresponding
hidden field in admin/classifications/edit.html.tmpl. This way, we avoid that a
developer tries to hack this hidden field but doesn't understand why this
modification is not reported in this .cgi file.


>+    my $checkvotes = 0;
>+    my $sth;

$checkvotes and $sth are never used. Remove them!


>         $sth = $dbh->prepare("UPDATE classifications
>                               SET name=? WHERE id=?");
>-        $sth->execute($classification,$classification_id);
>+        trick_taint($class_name);
>+        $sth->execute($class_name, $classification_old->id);

Again, use $dbh->do("UPDATE ...", undef, (...)).


>+    if ($description ne $classification_old->description) {
>         $sth = $dbh->prepare("UPDATE classifications
>                               SET description=?
>                               WHERE id=?");
>-        $sth->execute($description,$classification_id);
>+        $sth->execute($description, $classification_old->id);

You forgot to detaint $description first.


> if ($action eq 'reclassify') {

>+    $vars->{'description'} ||= $classification->description;

$vars->{'description'} is not defined yet. So remove "||".


>         if (defined $cgi->param('prodlist')) {
>             foreach my $prod ($cgi->param("prodlist")) {
>                 trick_taint($prod);
>-                $sth->execute($classification_id,$prod);
>+                $sth->execute($classification->id,$prod);

Nit: add a whitespace after the comma.


>    } elsif (defined $cgi->param('migrate_products')) {
>         if (defined $cgi->param('clprodlist')) {
>             foreach my $prod ($cgi->param("clprodlist")) {
>                 trick_taint($prod);
>-                $sth->execute($classification_id,$prod);
>+                $sth->execute($classification->id,$prod);

$cgi->param('migrate_products') and $cgi->param('clprodlist') do not exist and
I have no idea where does this come from. This is old code, remove this block
completely.



>Index: Bugzilla/Classification.pm

> use Bugzilla;
> use Bugzilla::Util;
>+use Bugzilla::Error;
>+use Bugzilla::Product;

Product.pm already uses Classification.pm. We should avoid loops. Moreover,
Bugzilla::Product is not required (at least for the moment), because... (see my
next comment)


>+sub products {
>+    my $self = shift;
>+
>+    if (!defined $self->{'products'}) {
>+        $self->{'products'} = [
>+            Bugzilla::Product::get_products_by_classification($self->id)
>+        ];
>+    }
>+    return $self->{'products'};
>+}

$classification->products is never used in editclassifications.cgi. So we don't
need this routine for now and consequently we don't require Bugzilla::Product,
see my previous comment.


>@@ -108,13 +121,31 @@
> sub get_all_classifications () {

>     foreach my $id (@$ids) {
>-        $classifications->{$id} = new Bugzilla::Classification($id);
>+        push @classifications, new Bugzilla::Classification($id);
>+    }
>+    return @classifications;

As mentioned already, get_all_classifications() has to include the number of
products per classification.



>Index: template/en/default/admin/classifications/select.html.tmpl

While you are editing this file, could you please remove the line:
[% filt_classification = classification FILTER html %]

filt_classification is never used and should be removed.


>@@ -46,14 +46,14 @@
>       [% IF (cl.id == 1) %]
>         <td valign="top">[% cl.total FILTER html %]</td>

1) cl.total should be cl.product_count, and 2) cl.product_count has not been
defined, as said twice already.


>       [% ELSE %]
>-        <td valign="top"><a href="editclassifications.cgi?action=reclassify&amp;classification=[% cl.classification FILTER url_quote %]">reclassify ([% cl.total FILTER html %])</a></td>
>+        <td valign="top"><a href="editclassifications.cgi?action=reclassify&amp;classification=[% cl.name FILTER url_quote %]">reclassify ([% cl.product_count FILTER html %])</a></td>
>       [% END %]

Same here, cl.product_count is undefined.

Comment 5

[Max Kanat-Alexander]

Instead of including bug_count in get_all_classifications, editproducts.cgi
should just have a way of adding the bug_count to the classification hash. This
will also allow us the option in the future of having an editclassifications
that only optionally displays the bug counts (which can be extremely slow on
large installations).

Comment 6

[Max Kanat-Alexander]

Oh wait--never mind that, even. You don't even need to call bug_count inside of
editclassifications -- doing [% classification.bug_count %] in the template will
do it.

Comment 7

[Tiago Mello [:timello]]

Attachment: v3: some cleanups.

I don't know when phpfi.com expires the links... anyway here it is, just for
help: http://phpfi.com/71393

Comment 8

[Tiago Mello [:timello]]

Attachment: v4: fixing edit a classification product list.

Comment 9

[Frédéric Buclin]

Comment on attachment 190880 [details] [diff] [review]
v4: fixing edit a classification product list.

work fine. r=LpSolit

Comment 10

[Frédéric Buclin]

Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/Classification.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v  <-- 
Classification.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/classifications/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/admin/classifications/reclassify.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/reclassify.html.tmpl,v
 <--  reclassify.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/classifications/select.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/select.html.tmpl,v
 <--select.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.118; previous revision: 1.117
done