Bug 286294: cleanup editclassifications.cgi and migrate the existent code to use…

Bugzilla/Classification.pm

@@ -21,6 +21,7 @@ package Bugzilla::Classification;
 
 use Bugzilla;
 use Bugzilla::Util;
+use Bugzilla::Error;
 
 ###############################
 ####    Initialization     ####
@@ -87,7 +88,7 @@ sub product_count {
     if (!defined $self->{'product_count'}) {
         $self->{'product_count'} = $dbh->selectrow_array(q{
             SELECT COUNT(*) FROM products
-            WHERE classification_id = ?}, undef, $self->id);
+            WHERE classification_id = ?}, undef, $self->id) || 0;
     }
     return $self->{'product_count'};
 }
@@ -108,13 +109,31 @@ sub get_all_classifications () {
     my $dbh = Bugzilla->dbh;
 
     my $ids = $dbh->selectcol_arrayref(q{
-        SELECT id FROM classifications});
+        SELECT id FROM classifications ORDER BY name});
 
-    my $classifications;
+    my @classifications;
     foreach my $id (@$ids) {
-        $classifications->{$id} = new Bugzilla::Classification($id);
+        push @classifications, new Bugzilla::Classification($id);
     }
-    return $classifications;
+    return @classifications;
+}
+
+sub check_classification ($) {
+    my ($class_name) = @_;
+
+    unless ($class_name) {
+        ThrowUserError("classification_not_specified");
+    }
+
+    my $classification =
+        new Bugzilla::Classification({name => $class_name});
+
+    unless ($classification) {
+        ThrowUserError("classification_doesnt_exist",
+                       { name => $class_name });
+    }
+    
+    return $classification;
 }
 
 1;
@@ -140,11 +159,14 @@ Bugzilla::Classification - Bugzilla classification class.
     my $hash_ref = Bugzilla::Classification::get_all_classifications();
     my $classification = $hash_ref->{1};
 
+    my $classification =
+        Bugzilla::Classification::check_classification('AcmeClass');
+
 =head1 DESCRIPTION
 
 Classification.pm represents a Classification object.
 
-A Classification is a higher-level grouping of Bugzilla Products.
+A Classification is a higher-level grouping of Products.
 
 =head1 METHODS
 
@@ -181,12 +203,20 @@ A Classification is a higher-level grouping of Bugzilla Products.
 
 =item C<get_all_classifications()>
 
- Description: Returns all Bugzilla classifications.
+ Description: Returns all classifications.
 
  Params:      none.
 
- Returns:     A hash with classification id as key and
-              Bugzilla::Classification object as value.
+ Returns:     Bugzilla::Classification object list.
+
+=item C<check_classification($classification_name)>
+
+ Description: Checks if the classification name passed in is a
+              valid classification.
+
+ Params:      $classification_name - String with a classification name.
+
+ Returns:     Bugzilla::Classification object.
 
 =back
 

Bugzilla/Product.pm

@@ -196,13 +196,13 @@ sub get_products_by_classification ($) {
 
     my $ids = $dbh->selectcol_arrayref(q{
         SELECT id FROM products
-        WHERE classification_id = ?}, undef, $class_id);
+        WHERE classification_id = ? ORDER by name}, undef, $class_id);
 
-    my $products;
+    my @products;
     foreach my $id (@$ids) {
-        $products->{$id} = new Bugzilla::Product($id);
+        push @products, new Bugzilla::Product($id);
     }
-    return $products;
+    return @products;
 }
 
 sub get_all_products () {
@@ -265,8 +265,7 @@ Bugzilla::Product - Bugzilla product class.
     my $defaultmilestone = $product->default_milestone;
     my $classificationid = $product->classification_id;
 
-    my $hash_ref = Bugzilla::Product::get_products_by_classification(1);
-    my $product = $hash_ref->{1};
+    my @products = Bugzilla::Product::get_products_by_classification(1);
 
 =head1 DESCRIPTION
 
@@ -355,8 +354,7 @@ Product.pm represents a product object.
 
  Params:      $class_id - Integer with classification id.
 
- Returns:     A hash with product id as key and a Bugzilla::Product
-              object as value.
+ Returns:     Bugzilla::Product object list.
 
 =item C<get_all_products()>
 

editclassifications.cgi

@@ -28,6 +28,8 @@ use Bugzilla::Constants;
 use Bugzilla::Util;
 use Bugzilla::Error;
 use Bugzilla::Config qw($datadir);
+use Bugzilla::Classification;
+use Bugzilla::Product;
 
 require "globals.pl";
 
@@ -36,34 +38,6 @@ my $dbh = Bugzilla->dbh;
 my $template = Bugzilla->template;
 my $vars = {};
 
-# TestClassification:  just returns if the specified classification does exist
-# CheckClassification: same check, optionally  emit an error text
-
-sub TestClassification ($) {
-    my $cl = shift;
-    my $dbh = Bugzilla->dbh;
-
-    trick_taint($cl);
-    # does the classification exist?
-    my $sth = $dbh->prepare("SELECT name
-                             FROM classifications
-                             WHERE name=?");
-    $sth->execute($cl);
-    my @row = $sth->fetchrow_array();
-    return $row[0];
-}
-
-sub CheckClassification ($) {
-    my $cl = shift;
-
-    unless ($cl) {
-        ThrowUserError("classification_not_specified");
-    }
-    if (! TestClassification($cl)) {
-        ThrowUserError("classification_doesnt_exist", { name => $cl });
-    }
-}
-
 sub LoadTemplate ($) {
     my $action = shift;
 
@@ -94,43 +68,15 @@ ThrowUserError("auth_classification_not_enabled") unless Param("useclassificatio
 # often used variables
 #
 my $action     = trim($cgi->param('action')         || '');
-my $classification = trim($cgi->param('classification') || '');
-trick_taint($classification);
-$vars->{'classification'} = $classification;
+my $class_name = trim($cgi->param('classification') || '');
     
 #
 # action='' -> Show nice list of classifications
 #
 
 unless ($action) {
-    my @classifications;
-    # left join is tricky
-    #   - must select "classifications" fields if you want a REAL value
-    #   - must use "count(products.classification_id)" if you want a true
-    #     count.  If you use count(classifications.id), it will return 1 for NULL
-    #   - must use "group by classifications.id" instead of
-    #     products.classification_id. Otherwise it won't look for all
-    #     classification ids, just the ones used by the products.
-    my $sth = $dbh->prepare("SELECT classifications.id, classifications.name,
-                                    classifications.description,
-                                    COUNT(classification_id) AS total
-                             FROM classifications
-                             LEFT JOIN products
-                             ON classifications.id = products.classification_id
-                            " . $dbh->sql_group_by('classifications.id',
-                                         'classifications.name,
-                                          classifications.description') . "
-                             ORDER BY name");
-    $sth->execute();
-    while (my ($id,$classification,$description,$total) = $sth->fetchrow_array()) {
-        my $cl = {};
-        $cl->{'id'} = $id;
-        $cl->{'classification'} = $classification;
-        $cl->{'description'} = $description if (defined $description);
-        $cl->{'total'} = $total;
-
-        push(@classifications, $cl);
-    }
+    my @classifications =
+        Bugzilla::Classification::get_all_classifications();
 
     $vars->{'classifications'} = \@classifications;
     LoadTemplate("select");
@@ -151,19 +97,24 @@ if ($action eq 'add') {
 #
 
 if ($action eq 'new') {
-    unless ($classification) {
-        ThrowUserError("classification_not_specified");
-    }
-    if (TestClassification($classification)) {
-        ThrowUserError("classification_already_exists", { name => $classification });
+
+    $class_name || ThrowUserError("classification_not_specified");
+
+    my $classification =
+        new Bugzilla::Classification({name => $class_name});
+
+    if ($classification) {
+        ThrowUserError("classification_already_exists",
+                       { name => $classification->name });
     }
+    
     my $description = trim($cgi->param('description')  || '');
     trick_taint($description);
+    trick_taint($class_name);
 
     # Add the new classification.
-    my $sth = $dbh->prepare("INSERT INTO classifications (name,description)
-                            VALUES (?,?)");
-    $sth->execute($classification,$description);
+    $dbh->do("INSERT INTO classifications (name, description)
+              VALUES (?, ?)", undef, ($class_name, $description));
 
     # Make versioncache flush
     unlink "$datadir/versioncache";
@@ -178,25 +129,20 @@ if ($action eq 'new') {
 #
 
 if ($action eq 'del') {
-    CheckClassification($classification);
-    my $sth;
 
-    # display some data about the classification
-    $sth = $dbh->prepare("SELECT id, description
-                          FROM classifications
-                          WHERE name=?");
-    $sth->execute($classification);
-    my ($classification_id, $description) = $sth->fetchrow_array();
+    my $classification =
+        Bugzilla::Classification::check_classification($class_name);
 
-    ThrowUserError("classification_not_deletable") if ($classification_id eq "1");
+    if ($classification->id == 1) {
+        ThrowUserError("classification_not_deletable");
+    }
 
-    $sth = $dbh->prepare("SELECT name
-                          FROM products
-                          WHERE classification_id=$classification_id");
-    $sth->execute();
-    ThrowUserError("classification_has_products") if ($sth->fetchrow_array());
+    if ($classification->product_count()) {
+        ThrowUserError("classification_has_products");
+    }
 
-    $vars->{'description'} = $description if (defined $description);
+    $vars->{'description'} = $classification->description;
+    $vars->{'classification'} = $classification->name;
 
     LoadTemplate($action);
 }
@@ -206,32 +152,31 @@ if ($action eq 'del') {
 #
 
 if ($action eq 'delete') {
-    CheckClassification($classification);
 
-    my $sth;
-    my $classification_id = get_classification_id($classification);
+    my $classification =
+        Bugzilla::Classification::check_classification($class_name);
 
-    if ($classification_id == 1) {
-        ThrowUserError("cant_delete_default_classification", { name => $classification });
+    if ($classification->id == 1) {
+        ThrowUserError("classification_not_deletable");
     }
 
     # lock the tables before we start to change everything:
     $dbh->bz_lock_tables('classifications WRITE', 'products WRITE');
 
     # delete
-    $sth = $dbh->prepare("DELETE FROM classifications WHERE id=?");
-    $sth->execute($classification_id);
+    $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);
+    $dbh->do("UPDATE products SET classification_id = 1
+              WHERE classification_id = ?", undef, $classification->id);
 
     $dbh->bz_unlock_tables();
 
     unlink "$datadir/versioncache";
 
+    $vars->{'classification'} = $classification->name;
+
     LoadTemplate($action);
 }
 
@@ -242,34 +187,17 @@ if ($action eq 'delete') {
 #
 
 if ($action eq 'edit') {
-    CheckClassification($classification);
-
-    my @products = ();
-    my $has_products = 0;
-    my $sth;
-    
-
-    # get data of classification
-    $sth = $dbh->prepare("SELECT id,description
-                          FROM classifications
-                          WHERE name=?");
-    $sth->execute($classification);
-    my ($classification_id,$description) = $sth->fetchrow_array();
-    $vars->{'description'} = $description if (defined $description);
-
-    $sth = $dbh->prepare("SELECT name,description
-                          FROM products
-                          WHERE classification_id=?
-                          ORDER BY name");
-    $sth->execute($classification_id);
-    while ( my ($product, $prod_description) = $sth->fetchrow_array()) {
-        my $prod = {};
-        $has_products = 1;
-        $prod->{'name'} = $product;
-        $prod->{'description'} = $prod_description if (defined $prod_description);
-        push(@products, $prod);
-    }
-    $vars->{'products'} = \@products if ($has_products);
+
+    my $classification =
+        Bugzilla::Classification::check_classification($class_name);
+
+    my @products =
+        Bugzilla::Product::get_products_by_classification(
+            $classification->id);
+
+    $vars->{'description'} = $classification->description;
+    $vars->{'classification'} = $classification->name;
+    $vars->{'products'} = \@products;
 
     LoadTemplate($action);
 }
@@ -279,44 +207,39 @@ if ($action eq 'edit') {
 #
 
 if ($action eq 'update') {
-    my $classificationold   = trim($cgi->param('classificationold')   || '');
-    my $description         = trim($cgi->param('description')         || '');
-    my $descriptionold      = trim($cgi->param('descriptionold')      || '');
-    my $checkvotes = 0;
-    my $sth;
 
-    CheckClassification($classificationold);
+    $class_name || ThrowUserError("classification_not_specified");
 
-    my $classification_id = get_classification_id($classificationold);
-    trick_taint($description);
+    my $class_old_name = trim($cgi->param('classificationold') || '');
+    my $description    = trim($cgi->param('description')       || '');
 
-    # Note that we got the $classification_id using $classificationold
-    # above so it will remain static even after we rename the
-    # classification in the database.
+    my $class_old =
+        Bugzilla::Classification::check_classification($class_old_name);
 
     $dbh->bz_lock_tables('classifications WRITE');
 
-    if ($classification ne $classificationold) {
-        unless ($classification) {
-            ThrowUserError("classification_not_specified");
-        }
+    if ($class_name ne $class_old->name) {
 
-        if (TestClassification($classification)) {
-            ThrowUserError("classification_already_exists", { name => $classification });
+        my $class = new Bugzilla::Classification({name => $class_name});
+        if ($class) {
+            ThrowUserError("classification_already_exists",
+                           { name => $class->name });
         }
-        $sth = $dbh->prepare("UPDATE classifications
-                              SET name=? WHERE id=?");
-        $sth->execute($classification,$classification_id);
+        trick_taint($class_name);
+        $dbh->do("UPDATE classifications SET name = ? WHERE id = ?",
+                 undef, ($class_name, $class_old->id));
+        
         $vars->{'updated_classification'} = 1;
 
         unlink "$datadir/versioncache";
     }
 
-    if ($description ne $descriptionold) {
-        $sth = $dbh->prepare("UPDATE classifications
-                              SET description=?
-                              WHERE id=?");
-        $sth->execute($description,$classification_id);
+    if ($description ne $class_old->description) {
+        trick_taint($description);
+        $dbh->do("UPDATE classifications SET description = ?
+                  WHERE id = ?", undef,
+                 ($description, $class_old->id));
+
         $vars->{'updated_description'} = 1;
 
         unlink "$datadir/versioncache";
@@ -332,26 +255,20 @@ if ($action eq 'update') {
 #
 
 if ($action eq 'reclassify') {
-    CheckClassification($classification);
-    my $sth;
 
-    # display some data about the classification
-    $sth = $dbh->prepare("SELECT id, description
-                          FROM classifications
-                          WHERE name=?");
-    $sth->execute($classification);
-    my ($classification_id, $description) = $sth->fetchrow_array();
+    my $classification =
+        Bugzilla::Classification::check_classification($class_name);
    
-    $vars->{'description'} = $description if (defined $description);
+    $vars->{'description'} = $classification->description;
+
+    my $sth = $dbh->prepare("UPDATE products SET classification_id = ?
+                             WHERE name = ?");
 
-    $sth = $dbh->prepare("UPDATE products
-                          SET classification_id=?
-                          WHERE name=?");
     if (defined $cgi->param('add_products')) {
         if (defined $cgi->param('prodlist')) {
             foreach my $prod ($cgi->param("prodlist")) {
                 trick_taint($prod);
-                $sth->execute($classification_id,$prod);
+                $sth->execute($classification->id, $prod);
             }
         }
     } elsif (defined $cgi->param('remove_products')) {
@@ -361,44 +278,24 @@ if ($action eq 'reclassify') {
                 $sth->execute(1,$prod);
             }
         }
-    } 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);
-            }
-        }
     }
 
     my @selected_products = ();
-    my @class_products = ();
-
-    $sth = $dbh->prepare("SELECT classifications.id,
-                                 products.name,
-                                 classifications.name,
-                                 classifications.id > 1 as unknown
-                           FROM products
-                           INNER JOIN classifications
-                           ON classifications.id = products.classification_id
-                           ORDER BY unknown, products.name,
-                                    classifications.name");
-    $sth->execute();
-    while ( my ($clid, $name, $clname) = $sth->fetchrow_array() ) {
-        if ($clid == $classification_id) {
-            push(@selected_products,$name);
-        } else {
-            my $cl = {};
-            if ($clid == 1) {
-                $cl->{'name'} = "[$clname] $name";
+    my @unselected_products = ();
+
+    my @products = Bugzilla::Product::get_all_products();
+
+    foreach my $product (@products) {
+        if ($product->classification_id == $classification->id) {
+            push @selected_products, $product;
         } else {
-                $cl->{'name'} = "$name [$clname]";
-            }
-            $cl->{'value'} = $name;
-            push(@class_products,$cl);
+            push @unselected_products, $product;
         }
     }
+    
     $vars->{'selected_products'}   = \@selected_products;
-    $vars->{'class_products'} = \@class_products;
+    $vars->{'unselected_products'} = \@unselected_products;
+    $vars->{'classification'} = $classification->name;
 
     LoadTemplate($action);
 }
@@ -407,4 +304,4 @@ if ($action eq 'reclassify') {
 # No valid action found
 #
 
-ThrowCodeError("action_unrecognized", $vars);
+ThrowCodeError("action_unrecognized", {action => $action});

template/en/default/admin/classifications/edit.html.tmpl

@@ -59,7 +59,6 @@
   </table>
 
   <input type=hidden name="classificationold" value="[% classification FILTER html %]">
-  <input type=hidden name="descriptionold" value="[% description FILTER html %]">
   <input type=hidden name="action" value="update">
   <input type=submit value="Update">
 </form>

template/en/default/admin/classifications/reclassify.html.tmpl

@@ -51,9 +51,9 @@
       <td></td>
       <td valign="top">
       <select name="prodlist" id="prodlist" multiple="multiple" size="20">
-        [% FOREACH cl = class_products %]
-          <option value="[% cl.value FILTER html %]">
-             [% cl.name FILTER html %]
+        [% FOREACH product = unselected_products %]
+          <option value="[% product.name FILTER html %]">
+            [[% product.classification.name FILTER html %]]&nbsp;[% product.name FILTER html %]
           </option>
         [% END %]
       </select></td>
@@ -66,8 +66,8 @@
       <td valign="middle" rowspan=2>
         <select name="myprodlist" id="myprodlist" multiple="multiple" size="20">
           [% FOREACH product = selected_products %]
-            <option value="[% product FILTER html %]">
-              [% product FILTER html %]
+            <option value="[% product.name FILTER html %]">
+              [% product.name FILTER html %]
             </option>
           [% END %]
       </select></td>

template/en/default/admin/classifications/select.html.tmpl

@@ -23,8 +23,6 @@
   title = "Select classification"
 %]
 
-[% filt_classification = classification FILTER html %]
-
 <table border=1 cellpadding=4 cellspacing=0>
   <tr bgcolor="#6666ff">
     <th align="left">Edit Classification ...</th>
@@ -35,7 +33,7 @@
 
   [% FOREACH cl = classifications %]
     <tr>
-      <td valign="top"><a href="editclassifications.cgi?action=edit&amp;classification=[% cl.classification FILTER url_quote %]"><b>[% cl.classification FILTER html %]</b></a></td>
+      <td valign="top"><a href="editclassifications.cgi?action=edit&amp;classification=[% cl.name FILTER url_quote %]"><b>[% cl.name FILTER html %]</b></a></td>
       <td valign="top"> 
       [% IF cl.description %]
         [% cl.description %]
@@ -44,16 +42,16 @@
       [% END %]
       </td>
       [% IF (cl.id == 1) %]
-        <td valign="top">[% cl.total FILTER html %]</td>
+        <td valign="top">[% cl.product_count FILTER html %]</td>
       [% 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 %]
 
       [%# don't allow user to delete the default id. %]
       [% IF (cl.id == 1) %]
         <td valign="top">&nbsp;</td>
       [% ELSE %]
-        <td valign="top"><a href="editclassifications.cgi?action=del&amp;classification=[% cl.classification FILTER url_quote %]">delete</a></td>
+        <td valign="top"><a href="editclassifications.cgi?action=del&amp;classification=[% cl.name FILTER url_quote %]">delete</a></td>
       [% END %]
     </tr>
   [% END %]

template/en/default/global/user-error.html.tmpl

@@ -248,10 +248,6 @@
      must reassign those products to another classification before you
      can delete this one.
 
-  [% ELSIF error == "cant_delete_default_classification" %]
-     Sorry, but you can not delete the default classification,
-     '[% name FILTER html %]'.
-
   [% ELSIF error == "component_already_exists" %]
     [% title = "Component Already Exists" %]
     A component with the name '[% name FILTER html %]' already exists.