Bug 339381: Make Bugzilla::Classification use Bugzilla::Object - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit

5 files changed, 75 insertions, 159 deletions

diff --git a/Bugzilla/Classification.pm b/Bugzilla/Classification.pm
index 37ae3a0cc..2eb253aa7 100644
--- a/Bugzilla/Classification.pm
+++ b/Bugzilla/Classification.pm
@@ -13,7 +13,7 @@
 # The Original Code is the Bugzilla Bug Tracking System.
 #
 # Contributor(s): Tiago R. Mello <timello@async.com.br>
-#
+#                 Frédéric Buclin <LpSolit@gmail.com>
 
 use strict;
 
@@ -23,68 +23,76 @@ use Bugzilla::Util;
 use Bugzilla::Error;
 use Bugzilla::Product;
 
+use base qw(Bugzilla::Object);
+
 ###############################
 ####    Initialization     ####
 ###############################
 
+use constant DB_TABLE => 'classifications';
+
 use constant DB_COLUMNS => qw(
-    classifications.id
-    classifications.name
-    classifications.description
-    classifications.sortkey
+    id
+    name
+    description
+    sortkey
+);
+
+use constant REQUIRED_CREATE_FIELDS => qw(
+    name
+);
+
+use constant UPDATE_COLUMNS => qw(
+    name
+    description
+    sortkey
 );
 
-our $columns = join(", ", DB_COLUMNS);
+use constant VALIDATORS => {
+    name        => \&_check_name,
+    description => \&_check_description,
+    sortkey     => \&_check_sortkey,
+};
 
 ###############################
-####       Methods         ####
+####      Validators       ####
 ###############################
 
-sub new {
-    my $invocant = shift;
-    my $class = ref($invocant) || $invocant;
-    my $self = {};
-    bless($self, $class);
-    return $self->_init(@_);
-}
+sub _check_name {
+    my ($invocant, $name) = @_;
 
-sub _init {
-    my $self = shift;
-    my ($param) = @_;
-    my $dbh = Bugzilla->dbh;
+    $name = trim($name);
+    $name || ThrowUserError('classification_not_specified');
 
-    my $id = $param unless (ref $param eq 'HASH');
-    my $classification;
+    my $classification = new Bugzilla::Classification({name => $name});
+    if ($classification && (!ref $invocant || $classification->id != $invocant->id)) {
+        ThrowUserError("classification_already_exists", { name => $classification->name });
+    }
+    return $name;
+}
 
-    if (defined $id) {
-        detaint_natural($id)
-          || ThrowCodeError('param_must_be_numeric',
-                            {function => 'Bugzilla::Classification::_init'});
+sub _check_description {
+    my ($invocant, $description) = @_;
 
-        $classification = $dbh->selectrow_hashref(qq{
-            SELECT $columns FROM classifications
-            WHERE id = ?}, undef, $id);
+    $description  = trim($description || '');
+    return $description;
+}
 
-    } elsif (defined $param->{'name'}) {
+sub _check_sortkey {
+    my ($invocant, $sortkey) = @_;
 
-        trick_taint($param->{'name'});
-        $classification = $dbh->selectrow_hashref(qq{
-            SELECT $columns FROM classifications
-            WHERE name = ?}, undef, $param->{'name'});
-    } else {
-        ThrowCodeError('bad_arg',
-            {argument => 'param',
-             function => 'Bugzilla::Classification::_init'});
-    }
+    $sortkey ||= 0;
+    my $stored_sortkey = $sortkey;
+    detaint_natural($sortkey)
+      || ThrowUserError('classification_invalid_sortkey', { 'sortkey' => $stored_sortkey });
 
-    return undef unless (defined $classification);
-
-    foreach my $field (keys %$classification) {
-        $self->{$field} = $classification->{$field};
-    }
-    return $self;
+    return $sortkey;
 }
 
+###############################
+####       Methods         ####
+###############################
+
 sub product_count {
     my $self = shift;
     my $dbh = Bugzilla->dbh;
@@ -116,46 +124,9 @@ sub products {
 ####      Accessors        ####
 ###############################
 
-sub id          { return $_[0]->{'id'};          }
-sub name        { return $_[0]->{'name'};        }
 sub description { return $_[0]->{'description'}; }
 sub sortkey     { return $_[0]->{'sortkey'};     }
 
-###############################
-####      Subroutines      ####
-###############################
-
-sub get_all_classifications {
-    my $dbh = Bugzilla->dbh;
-
-    my $ids = $dbh->selectcol_arrayref(q{
-        SELECT id FROM classifications ORDER BY sortkey, name});
-
-    my @classifications;
-    foreach my $id (@$ids) {
-        push @classifications, new Bugzilla::Classification($id);
-    }
-    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;
 
 __END__
@@ -174,18 +145,18 @@ Bugzilla::Classification - Bugzilla classification class.
     my $id = $classification->id;
     my $name = $classification->name;
     my $description = $classification->description;
+    my $sortkey = $classification->sortkey;
     my $product_count = $classification->product_count;
     my $products = $classification->products;
 
-    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.
+Classification.pm represents a classification object. It is an
+implementation of L<Bugzilla::Object>, and thus provides all methods
+that L<Bugzilla::Object> provides.
+
+The methods that are specific to C<Bugzilla::Classification> are listed
+below.
 
 A Classification is a higher-level grouping of Products.
 
@@ -193,20 +164,6 @@ A Classification is a higher-level grouping of Products.
 
 =over
 
-=item C<new($param)>
-
- Description: The constructor is used to load an existing
-              classification by passing a classification
-              id or classification name using a hash.
-
- Params:      $param - If you pass an integer, the integer is the
-                      classification_id from the database that we
-                      want to read in. If you pass in a hash with
-                      'name' key, then the value of the name key
-                      is the name of a classification from the DB.
-
- Returns:     A Bugzilla::Classification object.
-
 =item C<product_count()>
 
  Description: Returns the total number of products that belong to
@@ -226,27 +183,4 @@ A Classification is a higher-level grouping of Products.
 
 =back
 
-=head1 SUBROUTINES
-
-=over
-
-=item C<get_all_classifications()>
-
- Description: Returns all classifications.
-
- Params:      none.
-
- 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
-
 =cut
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 7a560a3e6..396aaa346 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -409,8 +409,7 @@ sub _check_classification {
 
     my $classification_id = 1;
     if (Bugzilla->params->{'useclassification'}) {
-        my $classification =
-            Bugzilla::Classification::check_classification($classification_name);
+        my $classification = Bugzilla::Classification->check($classification_name);
         $classification_id = $classification->id;
     }
     return $classification_id;
diff --git a/editclassifications.cgi b/editclassifications.cgi
index 1eef8ae90..f2a91a68e 100755
--- a/editclassifications.cgi
+++ b/editclassifications.cgi
@@ -40,7 +40,7 @@ sub LoadTemplate {
     my $cgi = Bugzilla->cgi;
     my $template = Bugzilla->template;
 
-    $vars->{'classifications'} = [Bugzilla::Classification::get_all_classifications()]
+    $vars->{'classifications'} = [Bugzilla::Classification->get_all]
       if ($action eq 'select');
     # There is currently only one section about classifications,
     # so all pages point to it. Let's define it here.
@@ -115,8 +115,7 @@ if ($action eq 'new') {
     my $sortkey = trim($cgi->param('sortkey') || 0);
     my $stored_sortkey = $sortkey;
     detaint_natural($sortkey)
-      || ThrowUserError('classification_invalid_sortkey', {'name' => $class_name,
-                                                           'sortkey' => $stored_sortkey});
+      || ThrowUserError('classification_invalid_sortkey', {'sortkey' => $stored_sortkey});
 
     trick_taint($description);
     trick_taint($class_name);
@@ -129,7 +128,7 @@ if ($action eq 'new') {
 
     $vars->{'message'} = 'classification_created';
     $vars->{'classification'} = new Bugzilla::Classification({name => $class_name});
-    $vars->{'classifications'} = [Bugzilla::Classification::get_all_classifications];
+    $vars->{'classifications'} = [Bugzilla::Classification->get_all];
     $vars->{'token'} = issue_session_token('reclassify_classifications');
     LoadTemplate('reclassify');
 }
@@ -142,8 +141,7 @@ if ($action eq 'new') {
 
 if ($action eq 'del') {
 
-    my $classification =
-        Bugzilla::Classification::check_classification($class_name);
+    my $classification = Bugzilla::Classification->check($class_name);
 
     if ($classification->id == 1) {
         ThrowUserError("classification_not_deletable");
@@ -166,8 +164,7 @@ if ($action eq 'del') {
 if ($action eq 'delete') {
     check_token_data($token, 'delete_classification');
 
-    my $classification =
-        Bugzilla::Classification::check_classification($class_name);
+    my $classification = Bugzilla::Classification->check($class_name);
 
     if ($classification->id == 1) {
         ThrowUserError("classification_not_deletable");
@@ -200,8 +197,7 @@ if ($action eq 'delete') {
 
 if ($action eq 'edit') {
 
-    my $classification =
-        Bugzilla::Classification::check_classification($class_name);
+    my $classification = Bugzilla::Classification->check($class_name);
 
     $vars->{'classification'} = $classification;
     $vars->{'token'} = issue_session_token('edit_classification');
@@ -220,16 +216,14 @@ if ($action eq 'update') {
 
     my $class_old_name = trim($cgi->param('classificationold') || '');
 
-    my $class_old =
-        Bugzilla::Classification::check_classification($class_old_name);
+    my $class_old = Bugzilla::Classification->check($class_old_name);
 
     my $description = trim($cgi->param('description') || '');
 
     my $sortkey = trim($cgi->param('sortkey') || 0);
     my $stored_sortkey = $sortkey;
     detaint_natural($sortkey)
-      || ThrowUserError('classification_invalid_sortkey', {'name' => $class_old->name,
-                                                           'sortkey' => $stored_sortkey});
+      || ThrowUserError('classification_invalid_sortkey', {'sortkey' => $stored_sortkey});
 
     $dbh->bz_start_transaction();
 
@@ -277,9 +271,7 @@ if ($action eq 'update') {
 #
 
 if ($action eq 'reclassify') {
-
-    my $classification =
-        Bugzilla::Classification::check_classification($class_name);
+    my $classification = Bugzilla::Classification->check($class_name);
    
     my $sth = $dbh->prepare("UPDATE products SET classification_id = ?
                              WHERE name = ?");
@@ -304,9 +296,7 @@ if ($action eq 'reclassify') {
         delete_token($token);
     }
 
-    my @classifications = 
-        Bugzilla::Classification::get_all_classifications;
-    $vars->{'classifications'} = \@classifications;
+    $vars->{'classifications'} = [Bugzilla::Classification->get_all];
     $vars->{'classification'} = $classification;
     $vars->{'token'} = issue_session_token('reclassify_classifications');
 
diff --git a/editproducts.cgi b/editproducts.cgi
index 01b497628..8dd42d741 100755
--- a/editproducts.cgi
+++ b/editproducts.cgi
@@ -81,7 +81,7 @@ if (Bugzilla->params->{'useclassification'}
     && !$product_name)
 {
     $vars->{'classifications'} = $user->in_group('editcomponents') ?
-      [Bugzilla::Classification::get_all_classifications] : $user->get_selectable_classifications;
+      [Bugzilla::Classification->get_all] : $user->get_selectable_classifications;
 
     $template->process("admin/products/list-classifications.html.tmpl", $vars)
         || ThrowTemplateError($template->error());
@@ -99,9 +99,7 @@ if (!$action && !$product_name) {
     my $products;
 
     if (Bugzilla->params->{'useclassification'}) {
-        $classification =
-            Bugzilla::Classification::check_classification($classification_name);
-
+        $classification = Bugzilla::Classification->check($classification_name);
         $products = $user->get_selectable_products($classification->id);
         $vars->{'classification'} = $classification;
     } else {
@@ -142,8 +140,7 @@ if ($action eq 'add') {
                                          object => "products"});
 
     if (Bugzilla->params->{'useclassification'}) {
-        my $classification = 
-            Bugzilla::Classification::check_classification($classification_name);
+        my $classification = Bugzilla::Classification->check($classification_name);
         $vars->{'classification'} = $classification;
     }
     $vars->{'token'} = issue_session_token('add_product');
@@ -235,7 +232,7 @@ if ($action eq 'delete') {
 
     if (Bugzilla->params->{'useclassification'}) {
         $vars->{'classifications'} = $user->in_group('editcomponents') ?
-          [Bugzilla::Classification::get_all_classifications] : $user->get_selectable_classifications;
+          [Bugzilla::Classification->get_all] : $user->get_selectable_classifications;
 
         $template->process("admin/products/list-classifications.html.tmpl", $vars)
           || ThrowTemplateError($template->error());
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 8969b9d30..b072d2bbd 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -271,14 +271,10 @@
     [% title = "Classification Already Exists" %]
     A classification with the name '[% name FILTER html %]' already exists.
 
-  [% ELSIF error == "classification_doesnt_exist" %]
-    [% title = "Classification Does Not Exist" %]
-    The classification '[% name FILTER html %]' does not exist.
-
   [% ELSIF error == "classification_invalid_sortkey" %]
     [% title = "Invalid Sortkey for Classification" %]
-    The sortkey <em>[% sortkey FILTER html %]</em> for the '[% name FILTER html %]'
-    classification is invalid. It must be a positive integer.
+    The sortkey <em>[% sortkey FILTER html %]</em> is invalid.
+    It must be a positive integer.
 
   [% ELSIF error == "classification_not_deletable" %]
     [% title = "Default Classification Can Not Be Deleted" %]