Bug 300952: Change the editmilestones.cgi code to use Milestone.pm and Product.pm - Patch by Tiago R. Mello <timello@gmail.com> r=LpSolit a=justdave

4 files changed, 158 insertions, 280 deletions

diff --git a/Bugzilla/Milestone.pm b/Bugzilla/Milestone.pm
index dad8b6c11..6956d5313 100644
--- a/Bugzilla/Milestone.pm
+++ b/Bugzilla/Milestone.pm
@@ -74,11 +74,24 @@ sub _init {
     return $self;
 }
 
+sub bug_count {
+    my $self = shift;
+    my $dbh = Bugzilla->dbh;
+
+    if (!defined $self->{'bug_count'}) {
+        $self->{'bug_count'} = $dbh->selectrow_array(q{
+            SELECT COUNT(*) FROM bugs
+            WHERE product_id = ? AND target_milestone = ?},
+            undef, $self->product_id, $self->name) || 0;
+    }
+    return $self->{'bug_count'};
+}
+
 ################################
 #####      Accessors      ######
 ################################
 
-sub value      { return $_[0]->{'value'};      }
+sub name       { return $_[0]->{'value'};      }
 sub product_id { return $_[0]->{'product_id'}; }
 sub sortkey    { return $_[0]->{'sortkey'};    }
 
@@ -105,12 +118,42 @@ sub get_milestones_by_product ($) {
         SELECT value FROM milestones
         WHERE product_id = ?}, undef, $product_id);
 
-    my $milestones;
+    my @milestones;
     foreach my $value (@$values) {
-        $milestones->{$value} = new Bugzilla::Milestone($product_id,
-                                                        $value);
+        push @milestones, new Bugzilla::Milestone($product_id, $value);
     }
-    return $milestones;
+    return @milestones;
+}
+
+sub check_milestone ($$) {
+    my ($product, $milestone_name) = @_;
+
+    unless ($milestone_name) {
+        ThrowUserError('milestone_not_specified');
+    }
+
+    my $milestone = new Bugzilla::Milestone($product->id,
+                                            $milestone_name);
+    unless ($milestone) {
+        ThrowUserError('milestone_not_valid',
+                       {'product' => $product->name,
+                        'milestone' => $milestone_name});
+    }
+    return $milestone;
+}
+
+sub check_sort_key ($$) {
+    my ($milestone_name, $sortkey) = @_;
+    # Keep a copy in case detaint_signed() clears the sortkey
+    my $stored_sortkey = $sortkey;
+
+    if (!detaint_signed($sortkey) || $sortkey < -32768
+        || $sortkey > 32767) {
+        ThrowUserError('milestone_sortkey_invalid',
+                       {'name' => $milestone_name,
+                        'sortkey' => $stored_sortkey});
+    }
+    return $sortkey;
 }
 
 1;
@@ -151,6 +194,14 @@ Milestone.pm represents a Product Milestone object.
 
  Returns:     A Bugzilla::Milestone object.
 
+=item C<bug_count()>
+
+ Description: Returns the total of bugs that belong to the milestone.
+
+ Params:      none.
+
+ Returns:     Integer with the number of bugs.
+
 =back
 
 =head1 SUBROUTINES
@@ -159,13 +210,22 @@ Milestone.pm represents a Product Milestone object.
 
 =item C<get_milestones_by_product($product_id)>
 
- Description: Returns all Bugzilla product milestones that belong
+ Description: Returns all product milestones that belong
               to the supplied product.
 
- Params:      $product_id - Integer with a Bugzilla product id.
+ Params:      $product_id - Integer with a product id.
+
+ Returns:     Bugzilla::Milestone object list.
+
+=item C<check_milestone($product, $milestone_name)>
+
+ Description: Checks if a milestone name was passed in
+              and if it is a valid milestone.
+
+ Params:      $product - Bugzilla::Product object.
+              $milestone_name - String with a milestone name.
 
- Returns:     A hash with milestone value as key and a
-              Bugzilla::Milestone object as hash value.
+ Returns:     Bugzilla::Milestone object.
 
 =back
 
diff --git a/editmilestones.cgi b/editmilestones.cgi
index a2d4d4f5d..4fbcab046 100755
--- a/editmilestones.cgi
+++ b/editmilestones.cgi
@@ -25,112 +25,14 @@ require "globals.pl";
 use Bugzilla::Constants;
 use Bugzilla::Config qw(:DEFAULT $datadir);
 use Bugzilla::User;
+use Bugzilla::Product;
+use Bugzilla::Milestone;
 use Bugzilla::Bug;
 
 use vars qw($template $vars);
 
 my $cgi = Bugzilla->cgi;
-
-# TestProduct:  just returns if the specified product does exists
-# CheckProduct: same check, optionally  emit an error text
-# TestMilestone:  just returns if the specified product/version combination exists
-# CheckMilestone: same check, optionally emit an error text
-
-sub TestProduct ($)
-{
-    my $product = shift;
-
-    trick_taint($product);
-
-    # does the product exist?
-    my $dbh = Bugzilla->dbh;
-    my $sth = $dbh->prepare_cached("SELECT name
-                                    FROM products
-                                    WHERE name = ?");
-    $sth->execute($product);
-
-    my ($row) = $sth->fetchrow_array;
-
-    $sth->finish;
-
-    return $row;
-}
-
-sub CheckProduct ($)
-{
-    my $product = shift;
-
-    # do we have a product?
-    unless ($product) {
-        ThrowUserError('product_not_specified');    
-    }
-
-    # Does it exist in the DB?
-    unless (TestProduct $product) {
-        ThrowUserError('product_doesnt_exist',
-                       {'product' => $product});
-    }
-}
-
-sub TestMilestone ($$)
-{
-    my ($product, $milestone) = @_;
-
-    my $dbh = Bugzilla->dbh;
-
-    # does the product exist?
-    my $sth = $dbh->prepare_cached("
-             SELECT products.name, value
-             FROM milestones
-             INNER JOIN products
-                ON milestones.product_id = products.id
-             WHERE products.name = ?
-               AND value = ?");
-
-    trick_taint($product);
-    trick_taint($milestone);
-
-    $sth->execute($product, $milestone);
-
-    my ($db_milestone) = $sth->fetchrow_array();
-
-    $sth->finish();
-
-    return $db_milestone;
-}
-
-sub CheckMilestone ($$)
-{
-    my ($product, $milestone) = @_;
-
-    # do we have the milestone and product combination?
-    unless ($milestone) {
-        ThrowUserError('milestone_not_specified');
-    }
-
-    CheckProduct($product);
-
-    unless (TestMilestone $product, $milestone) {
-        ThrowUserError('milestone_not_valid',
-                       {'product' => $product,
-                        'milestone' => $milestone});
-    }
-}
-
-sub CheckSortkey ($$)
-{
-    my ($milestone, $sortkey) = @_;
-    # Keep a copy in case detaint_signed() clears the sortkey
-    my $stored_sortkey = $sortkey;
-
-    if (!detaint_signed($sortkey) || $sortkey < -32768 || $sortkey > 32767) {
-        ThrowUserError('milestone_sortkey_invalid',
-                       {'name' => $milestone,
-                        'sortkey' => $stored_sortkey});
-    }
-
-    return $sortkey;
-}
+my $dbh = Bugzilla->dbh;
 
 #
 # Preliminary checks:
@@ -139,7 +41,7 @@ sub CheckSortkey ($$)
 my $user = Bugzilla->login(LOGIN_REQUIRED);
 my $whoid = $user->id;
 
-print Bugzilla->cgi->header();
+print $cgi->header();
 
 UserInGroup("editcomponents")
   || ThrowUserError("auth_failure", {group  => "editcomponents",
@@ -149,38 +51,18 @@ UserInGroup("editcomponents")
 #
 # often used variables
 #
-my $product = trim($cgi->param('product')     || '');
-my $milestone = trim($cgi->param('milestone') || '');
-my $sortkey = trim($cgi->param('sortkey')     || '0');
-my $action  = trim($cgi->param('action')      || '');
+my $product_name   = trim($cgi->param('product')     || '');
+my $milestone_name = trim($cgi->param('milestone')   || '');
+my $sortkey        = trim($cgi->param('sortkey')     || 0);
+my $action         = trim($cgi->param('action')      || '');
 
 #
-# product = '' -> Show nice list of milestones
+# product = '' -> Show nice list of products
 #
 
-unless ($product) {
-
-    my @products = ();
-
-    my $dbh = Bugzilla->dbh;
-
-    my $sth = $dbh->prepare_cached('SELECT products.name, products.description
-                                    FROM products 
-                                    ORDER BY products.name');
-
-    my $data = $dbh->selectall_arrayref($sth);
-
-    foreach my $aref (@$data) {
-
-        my $prod = {};
-
-        my ($name, $description) = @$aref;
-
-        $prod->{'name'} = $name;
-        $prod->{'description'} = $description;
-
-        push(@products, $prod);
-    }
+unless ($product_name) {
+    
+    my @products = Bugzilla::Product::get_all_products();
 
     $vars->{'products'} = \@products;
     $template->process("admin/milestones/select-product.html.tmpl",
@@ -190,7 +72,7 @@ unless ($product) {
     exit;
 }
 
-
+my $product = Bugzilla::Product::check_product($product_name);
 
 #
 # action='' -> Show nice list of milestones
@@ -198,33 +80,10 @@ unless ($product) {
 
 unless ($action) {
 
-    CheckProduct($product);
-    my $product_id = get_product_id($product);
-    my @milestones = ();
-
-    my $dbh = Bugzilla->dbh;
-
-    my $sth = $dbh->prepare_cached('SELECT value, sortkey
-                                    FROM milestones
-                                    WHERE product_id = ?
-                                    ORDER BY sortkey, value');
-
-    my $data = $dbh->selectall_arrayref($sth,
-                                        undef,
-                                        $product_id);
-
-    foreach my $aref (@$data) {
-
-        my $milestone = {};
-        my ($name, $sortkey) = @$aref;
+    my @milestones =
+        Bugzilla::Milestone::get_milestones_by_product($product->id);
 
-        $milestone->{'name'} = $name;
-        $milestone->{'sortkey'} = $sortkey;
-
-        push(@milestones, $milestone);
-    }
-
-    $vars->{'product'} = $product;
+    $vars->{'product'} = $product->name;
     $vars->{'milestones'} = \@milestones;
     $template->process("admin/milestones/list.html.tmpl",
                        $vars)
@@ -244,10 +103,7 @@ unless ($action) {
 
 if ($action eq 'add') {
 
-    CheckProduct($product);
-    my $product_id = get_product_id($product);
-
-    $vars->{'product'} = $product;
+    $vars->{'product'} = $product->name;
     $template->process("admin/milestones/create.html.tmpl",
                        $vars)
       || ThrowTemplateError($template->error());
@@ -263,43 +119,36 @@ if ($action eq 'add') {
 
 if ($action eq 'new') {
 
-    CheckProduct($product);
-    my $product_id = get_product_id($product);
+    $milestone_name || ThrowUserError('milestone_blank_name');
 
-    # Cleanups and valididy checks
-    unless ($milestone) {
-        ThrowUserError('milestone_blank_name',
-                       {'name' => $milestone});
-    }
-
-    if (length($milestone) > 20) {
+    if (length($milestone_name) > 20) {
         ThrowUserError('milestone_name_too_long',
-                       {'name' => $milestone});
+                       {'name' => $milestone_name});
     }
 
-    $sortkey = CheckSortkey($milestone, $sortkey);
+    $sortkey = Bugzilla::Milestone::check_sort_key($milestone_name,
+                                                   $sortkey);
+
+    my $milestone = new Bugzilla::Milestone($product->id,
+                                            $milestone_name);
 
-    if (TestMilestone($product, $milestone)) {
+    if ($milestone) {
         ThrowUserError('milestone_already_exists',
-                       {'name' => $milestone,
-                        'product' => $product});
+                       {'name' => $milestone->name,
+                        'product' => $product->name});
     }
 
     # Add the new milestone
-    my $dbh = Bugzilla->dbh;
-    trick_taint($milestone);
+    trick_taint($milestone_name);
     $dbh->do('INSERT INTO milestones ( value, product_id, sortkey )
               VALUES ( ?, ?, ? )',
-             undef,
-             $milestone,
-             $product_id,
-             $sortkey);
+             undef, $milestone_name, $product->id, $sortkey);
 
     # Make versioncache flush
     unlink "$datadir/versioncache";
 
-    $vars->{'name'} = $milestone;
-    $vars->{'product'} = $product;
+    $vars->{'name'} = $milestone_name;
+    $vars->{'product'} = $product->name;
     $template->process("admin/milestones/created.html.tmpl",
                        $vars)
       || ThrowTemplateError($template->error());
@@ -317,28 +166,18 @@ if ($action eq 'new') {
 #
 
 if ($action eq 'del') {
-    CheckMilestone($product, $milestone);
-    my $product_id = get_product_id($product);
-    my $dbh = Bugzilla->dbh;
-
-    $vars->{'default_milestone'} =
-      $dbh->selectrow_array('SELECT defaultmilestone
-                             FROM products WHERE id = ?',
-                             undef, $product_id);
-
-    trick_taint($milestone);
-    $vars->{'name'} = $milestone;
-    $vars->{'product'} = $product;
+    my $milestone = Bugzilla::Milestone::check_milestone($product,
+                                                         $milestone_name);
+    
+    $vars->{'name'}    = $milestone->name;
+    $vars->{'product'} = $product->name;
 
     # The default milestone cannot be deleted.
-    if ($vars->{'default_milestone'} eq $milestone) {
+    if ($product->default_milestone eq $milestone->name) {
         ThrowUserError("milestone_is_default", $vars);
     }
 
-    $vars->{'bug_count'} =
-      $dbh->selectrow_array("SELECT COUNT(bug_id) FROM bugs
-                             WHERE product_id = ? AND target_milestone = ?",
-                             undef, ($product_id, $milestone)) || 0;
+    $vars->{'bug_count'} = $milestone->bug_count;
 
     $template->process("admin/milestones/confirm-delete.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
@@ -352,21 +191,15 @@ if ($action eq 'del') {
 #
 
 if ($action eq 'delete') {
-    CheckMilestone($product, $milestone);
-    my $product_id = get_product_id($product);
-    my $dbh = Bugzilla->dbh;
 
-    my $default_milestone =
-      $dbh->selectrow_array("SELECT defaultmilestone
-                             FROM products WHERE id = ?",
-                             undef, $product_id);
-
-    trick_taint($milestone);
-    $vars->{'name'} = $milestone;
-    $vars->{'product'} = $product;
+    my $milestone =
+        Bugzilla::Milestone::check_milestone($product,
+                                             $milestone_name);
+    $vars->{'name'} = $milestone->name;
+    $vars->{'product'} = $product->name;
 
     # The default milestone cannot be deleted.
-    if ($milestone eq $default_milestone) {
+    if ($milestone->name eq $product->default_milestone) {
         ThrowUserError("milestone_is_default", $vars);
     }
 
@@ -375,7 +208,7 @@ if ($action eq 'delete') {
     my $bug_ids =
       $dbh->selectcol_arrayref("SELECT bug_id FROM bugs
                                 WHERE product_id = ? AND target_milestone = ?",
-                                undef, ($product_id, $milestone));
+                                undef, ($product->id, $milestone->name));
 
     my $nb_bugs = scalar(@$bug_ids);
     if ($nb_bugs) {
@@ -383,17 +216,20 @@ if ($action eq 'delete') {
         foreach my $bug_id (@$bug_ids) {
             $dbh->do("UPDATE bugs SET target_milestone = ?,
                       delta_ts = ? WHERE bug_id = ?",
-                      undef, ($default_milestone, $timestamp, $bug_id));
+                      undef, ($product->default_milestone, $timestamp,
+                              $bug_id));
             # We have to update the 'bugs_activity' table too.
-            LogActivityEntry($bug_id, 'target_milestone', $milestone,
-                             $default_milestone, $whoid, $timestamp);
+            LogActivityEntry($bug_id, 'target_milestone',
+                             $milestone->name,
+                             $product->default_milestone,
+                             $whoid, $timestamp);
         }
     }
 
     $vars->{'bug_count'} = $nb_bugs;
 
     $dbh->do("DELETE FROM milestones WHERE product_id = ? AND value = ?",
-             undef, ($product_id, $milestone));
+             undef, ($product->id, $milestone->name));
 
     unlink "$datadir/versioncache";
 
@@ -412,25 +248,13 @@ if ($action eq 'delete') {
 
 if ($action eq 'edit') {
 
-    CheckMilestone($product, $milestone);
-    my $product_id = get_product_id($product);
-
-    my $dbh = Bugzilla->dbh;
-
-    my $sth = $dbh->prepare_cached('SELECT sortkey
-                                    FROM milestones
-                                    WHERE product_id = ?
-                                    AND value = ?');
-
-    trick_taint($milestone);
+    my $milestone =
+        Bugzilla::Milestone::check_milestone($product,
+                                             $milestone_name);
 
-    $vars->{'sortkey'} = $dbh->selectrow_array($sth,
-                                               undef,
-                                               $product_id,
-                                               $milestone) || 0;
-
-    $vars->{'name'} = $milestone;
-    $vars->{'product'} = $product;
+    $vars->{'sortkey'} = $milestone->sortkey;
+    $vars->{'name'}    = $milestone->name;
+    $vars->{'product'} = $product->name;
 
     $template->process("admin/milestones/edit.html.tmpl",
                        $vars)
@@ -447,80 +271,77 @@ if ($action eq 'edit') {
 
 if ($action eq 'update') {
 
-    my $milestoneold = trim($cgi->param('milestoneold') || '');
-    my $sortkeyold = trim($cgi->param('sortkeyold')     || '0');
-
-    CheckMilestone($product, $milestoneold);
-    my $product_id = get_product_id($product);
+    my $milestone_old_name = trim($cgi->param('milestoneold') || '');
+    my $milestone_old =
+        Bugzilla::Milestone::check_milestone($product,
+                                             $milestone_old_name);
 
-    if (length($milestone) > 20) {
+    if (length($milestone_name) > 20) {
         ThrowUserError('milestone_name_too_long',
-                       {'name' => $milestone});
+                       {'name' => $milestone_name});
     }
 
-    my $dbh = Bugzilla->dbh;
-
     $dbh->bz_lock_tables('bugs WRITE',
                          'milestones WRITE',
                          'products WRITE');
 
-    if ($sortkey ne $sortkeyold) {
-        $sortkey = CheckSortkey($milestone, $sortkey);
-
-        trick_taint($milestoneold);
+    if ($sortkey ne $milestone_old->sortkey) {
+        $sortkey = Bugzilla::Milestone::check_sort_key($milestone_name,
+                                                       $sortkey);
 
         $dbh->do('UPDATE milestones SET sortkey = ?
                   WHERE product_id = ?
                   AND value = ?',
                  undef,
                  $sortkey,
-                 $product_id,
-                 $milestoneold);
+                 $product->id,
+                 $milestone_old->name);
 
         unlink "$datadir/versioncache";
         $vars->{'updated_sortkey'} = 1;
         $vars->{'sortkey'} = $sortkey;
     }
 
-    if ($milestone ne $milestoneold) {
-        unless ($milestone) {
+    if ($milestone_name ne $milestone_old->name) {
+        unless ($milestone_name) {
             ThrowUserError('milestone_blank_name');
         }
-        if (TestMilestone($product, $milestone)) {
+        my $milestone = 
+            new Bugzilla::Milestone($product->id, $milestone_name);
+        if ($milestone) {
             ThrowUserError('milestone_already_exists',
-                           {'name' => $milestone,
-                            'product' => $product});
+                           {'name' => $milestone->name,
+                            'product' => $product->name});
         }
 
-        trick_taint($milestone);
-        trick_taint($milestoneold);
+        trick_taint($milestone_name);
 
         $dbh->do('UPDATE bugs
                   SET target_milestone = ?
                   WHERE target_milestone = ?
                   AND product_id = ?',
                  undef,
-                 $milestone,
-                 $milestoneold,
-                 $product_id);
+                 $milestone_name,
+                 $milestone_old->name,
+                 $product->id);
 
         $dbh->do("UPDATE milestones
                   SET value = ?
                   WHERE product_id = ?
                   AND value = ?",
                  undef,
-                 $milestone,
-                 $product_id,
-                 $milestoneold);
+                 $milestone_name,
+                 $product->id,
+                 $milestone_old->name);
 
         $dbh->do("UPDATE products
                   SET defaultmilestone = ?
                   WHERE id = ?
                   AND defaultmilestone = ?",
                  undef,
-                 $milestone,
-                 $product_id,
-                 $milestoneold);
+                 $milestone_name,
+                 $product->id,
+                 $milestone_old->name);
 
         unlink "$datadir/versioncache";
 
@@ -529,8 +350,8 @@ if ($action eq 'update') {
 
     $dbh->bz_unlock_tables();
 
-    $vars->{'name'} = $milestone;
-    $vars->{'product'} = $product;
+    $vars->{'name'} = $milestone_name;
+    $vars->{'product'} = $product->name;
     $template->process("admin/milestones/updated.html.tmpl",
                        $vars)
       || ThrowTemplateError($template->error());
diff --git a/template/en/default/admin/milestones/confirm-delete.html.tmpl b/template/en/default/admin/milestones/confirm-delete.html.tmpl
index eda5add36..61601d185 100644
--- a/template/en/default/admin/milestones/confirm-delete.html.tmpl
+++ b/template/en/default/admin/milestones/confirm-delete.html.tmpl
@@ -23,8 +23,6 @@
 [%# INTERFACE:
   # name: string; The name of the milestone
   #
-  # default_milestone: string; The default milestone for the product
-  #
   # bug_count: number; The number of bugs targetted at the milestone
   #
   # product: string; The name of the product
diff --git a/template/en/default/admin/milestones/edit.html.tmpl b/template/en/default/admin/milestones/edit.html.tmpl
index 6b5ec8fb0..417a69276 100644
--- a/template/en/default/admin/milestones/edit.html.tmpl
+++ b/template/en/default/admin/milestones/edit.html.tmpl
@@ -52,7 +52,6 @@
   </table>
 
   <input type="hidden" name="milestoneold" value="[% name FILTER html %]">
-  <input type="hidden" name="sortkeyold" value="[% sortkey FILTER html %]">
   <input type="hidden" name="action" value="update">
   <input type="hidden" name="product" value="[% product FILTER html %]">
   <input type="submit" value="Update">