Bug 313126: Clean up editclassification.cgi and make it use…

Bugzilla/Classification.pm

@@ -30,6 +30,7 @@ use base qw(Bugzilla::Object);
 ###############################
 
 use constant DB_TABLE => 'classifications';
+use constant LIST_ORDER => 'sortkey, name';
 
 use constant DB_COLUMNS => qw(
     id
@@ -55,6 +56,26 @@ use constant VALIDATORS => {
 };
 
 ###############################
+####     Constructors     #####
+###############################
+sub remove_from_db {
+    my $self = shift;
+    my $dbh = Bugzilla->dbh;
+
+    ThrowUserError("classification_not_deletable") if ($self->id == 1);
+
+    $dbh->bz_start_transaction();
+    # Reclassify products to the default classification, if needed.
+    $dbh->do("UPDATE products SET classification_id = 1
+              WHERE classification_id = ?", undef, $self->id);
+
+    $dbh->do("DELETE FROM classifications WHERE id = ?", undef, $self->id);
+
+    $dbh->bz_commit_transaction();
+
+}
+
+###############################
 ####      Validators       ####
 ###############################
 
@@ -93,6 +114,10 @@ sub _check_sortkey {
 ####       Methods         ####
 ###############################
 
+sub set_name        { $_[0]->set('name', $_[1]); }
+sub set_description { $_[0]->set('description', $_[1]); }
+sub set_sortkey     { $_[0]->set('sortkey', $_[1]); }
+
 sub product_count {
     my $self = shift;
     my $dbh = Bugzilla->dbh;

Bugzilla/User.pm

@@ -682,20 +682,12 @@ sub get_selectable_products {
 sub get_selectable_classifications {
     my ($self) = @_;
 
-    if (defined $self->{selectable_classifications}) {
-        return $self->{selectable_classifications};
-    }
-
-    my $products = $self->get_selectable_products;
+    if (!defined $self->{selectable_classifications}) {
+        my $products = $self->get_selectable_products;
+        my %class_ids = map { $_->classification_id => 1 } @$products;
 
-    my $class;
-    foreach my $product (@$products) {
-        $class->{$product->classification_id} ||= 
-            new Bugzilla::Classification($product->classification_id);
+        $self->{selectable_classifications} = Bugzilla::Classification->new_from_list([keys %class_ids]);
     }
-    my @sorted_class = sort {$a->sortkey <=> $b->sortkey 
-                             || lc($a->name) cmp lc($b->name)} (values %$class);
-    $self->{selectable_classifications} = \@sorted_class;
     return $self->{selectable_classifications};
 }
 

editclassifications.cgi

@@ -100,34 +100,15 @@ if ($action eq 'add') {
 if ($action eq 'new') {
     check_token_data($token, 'add_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')  || '');
-
-    my $sortkey = trim($cgi->param('sortkey') || 0);
-    my $stored_sortkey = $sortkey;
-    detaint_natural($sortkey)
-      || ThrowUserError('classification_invalid_sortkey', {'sortkey' => $stored_sortkey});
-
-    trick_taint($description);
-    trick_taint($class_name);
-
-    # Add the new classification.
-    $dbh->do("INSERT INTO classifications (name, description, sortkey)
-              VALUES (?, ?, ?)", undef, ($class_name, $description, $sortkey));
+      Bugzilla::Classification->create({name        => $class_name,
+                                        description => scalar $cgi->param('description'),
+                                        sortkey     => scalar $cgi->param('sortkey')});
 
     delete_token($token);
 
     $vars->{'message'} = 'classification_created';
-    $vars->{'classification'} = new Bugzilla::Classification({name => $class_name});
+    $vars->{'classification'} = $classification;
     $vars->{'classifications'} = [Bugzilla::Classification->get_all];
     $vars->{'token'} = issue_session_token('reclassify_classifications');
     LoadTemplate('reclassify');
@@ -165,27 +146,11 @@ if ($action eq 'delete') {
     check_token_data($token, 'delete_classification');
 
     my $classification = Bugzilla::Classification->check($class_name);
-
-    if ($classification->id == 1) {
-        ThrowUserError("classification_not_deletable");
-    }
-
-    # lock the tables before we start to change everything:
-    $dbh->bz_start_transaction();
-
-    # update products just in case
-    $dbh->do("UPDATE products SET classification_id = 1
-              WHERE classification_id = ?", undef, $classification->id);
-
-    # delete
-    $dbh->do("DELETE FROM classifications WHERE id = ?", undef,
-             $classification->id);
-
-    $dbh->bz_commit_transaction();
+    $classification->remove_from_db;
+    delete_token($token);
 
     $vars->{'message'} = 'classification_deleted';
-    $vars->{'classification'} = $class_name;
-    delete_token($token);
+    $vars->{'classification'} = $classification;
     LoadTemplate('select');
 }
 
@@ -196,7 +161,6 @@ if ($action eq 'delete') {
 #
 
 if ($action eq 'edit') {
-
     my $classification = Bugzilla::Classification->check($class_name);
 
     $vars->{'classification'} = $classification;
@@ -212,57 +176,19 @@ if ($action eq 'edit') {
 if ($action eq 'update') {
     check_token_data($token, 'edit_classification');
 
-    $class_name || ThrowUserError("classification_not_specified");
-
     my $class_old_name = trim($cgi->param('classificationold') || '');
+    my $classification = Bugzilla::Classification->check($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', {'sortkey' => $stored_sortkey});
-
-    $dbh->bz_start_transaction();
-
-    if ($class_name ne $class_old->name) {
-
-        my $class = new Bugzilla::Classification({name => $class_name});
-        if ($class) {
-            ThrowUserError("classification_already_exists",
-                           { name => $class->name });
-        }
-        trick_taint($class_name);
-        $dbh->do("UPDATE classifications SET name = ? WHERE id = ?",
-                 undef, ($class_name, $class_old->id));
-
-        $vars->{'updated_classification'} = 1;
-    }
+    $classification->set_name($class_name);
+    $classification->set_description(scalar $cgi->param('description'));
+    $classification->set_sortkey(scalar $cgi->param('sortkey'));
 
-    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;
-    }
-
-    if ($sortkey ne $class_old->sortkey) {
-        $dbh->do("UPDATE classifications SET sortkey = ?
-                  WHERE id = ?", undef,
-                 ($sortkey, $class_old->id));
-
-        $vars->{'updated_sortkey'} = 1;
-    }
-
-    $dbh->bz_commit_transaction();
+    my $changes = $classification->update;
+    delete_token($token);
 
     $vars->{'message'} = 'classification_updated';
-    $vars->{'classification'} = $class_name;
-    delete_token($token);
+    $vars->{'classification'} = $classification;
+    $vars->{'changes'} = $changes;
     LoadTemplate('select');
 }
 

template/en/default/global/messages.html.tmpl

@@ -186,26 +186,26 @@
 
   [% ELSIF message_tag == "classification_deleted" %]
     [% title = "Classification Deleted" %]
-    The <em>[% classification FILTER html %]</em> classification has been deleted.
+    The <em>[% classification.name FILTER html %]</em> classification has been deleted.
 
   [% ELSIF message_tag == "classification_updated" %]
-    [% IF updated_classification || updated_description || updated_sortkey %]
-      [% title = "Classification Updated" %]
-      Changes to the <em>[% classification FILTER html %]</em> classification
+    [% title = "Classification Updated" %]
+    [% IF changes.keys.size %]
+      Changes to the <em>[% classification.name FILTER html %]</em> classification
       have been saved:
       <ul>
-        [% IF updated_classification %]
-          <li>Classification name updated</li>
+        [% IF changes.name.defined %]
+          <li>Name updated to '[% classification.name FILTER html %]'</li>
         [% END %]
-        [% IF updated_description %]
-          <li>Description updated</li>
+        [% IF changes.description.defined %]
+          <li>Description updated to '[% classification.description FILTER html %]'</li>
         [% END %]
-        [% IF updated_sortkey %]
-          <li>Sortkey updated</li>
+        [% IF changes.sortkey.defined %]
+          <li>Sortkey updated to '[% classification.sortkey FILTER html %]'</li>
         [% END %]
       </ul>
     [% ELSE %]
-      No changes made to <em>[% classification FILTER html %]</em>.
+      No changes made to <em>[% classification.name FILTER html %]</em>.
     [% END %]
 
   [% ELSIF message_tag == "component_created" %]