Bug 313129: Implement $milestone->create and $milestone->update based on…

Bugzilla/Constants.pm

@@ -142,7 +142,11 @@ use File::Basename;
 
     SAFE_PROTOCOLS
 
+    MIN_SMALLINT
+    MAX_SMALLINT
+
     MAX_LEN_QUERY_NAME
+    MAX_MILESTONE_SIZE
 );
 
 @Bugzilla::Constants::EXPORT_OK = qw(contenttypes);
@@ -397,9 +401,15 @@ use constant ROOT_USER => $^O =~ /MSWin32/i ? 'Administrator' : 'root';
 # True if we're on Win32.
 use constant ON_WINDOWS => ($^O =~ /MSWin32/i);
 
+use constant MIN_SMALLINT => -32768;
+use constant MAX_SMALLINT => 32767;
+
 # The longest that a saved search name can be.
 use constant MAX_LEN_QUERY_NAME => 64;
 
+# The longest milestone name allowed.
+use constant MAX_MILESTONE_SIZE => 20;
+
 sub bz_locations {
     # We know that Bugzilla/Constants.pm must be in %INC at this point.
     # So the only question is, what's the name of the directory

Bugzilla/Milestone.pm

@@ -14,6 +14,7 @@
 #
 # Contributor(s): Tiago R. Mello <timello@async.com.br>
 #                 Max Kanat-Alexander <mkanat@bugzilla.org>
+#                 Frédéric Buclin <LpSolit@gmail.com>
 
 use strict;
 
@@ -21,6 +22,7 @@ package Bugzilla::Milestone;
 
 use base qw(Bugzilla::Object);
 
+use Bugzilla::Constants;
 use Bugzilla::Util;
 use Bugzilla::Error;
 
@@ -31,6 +33,8 @@ use Bugzilla::Error;
 use constant DEFAULT_SORTKEY => 0;
 
 use constant DB_TABLE => 'milestones';
+use constant NAME_FIELD => 'value';
+use constant LIST_ORDER => 'sortkey, value';
 
 use constant DB_COLUMNS => qw(
     id
@@ -39,8 +43,26 @@ use constant DB_COLUMNS => qw(
     sortkey
 );
 
-use constant NAME_FIELD => 'value';
-use constant LIST_ORDER => 'sortkey, value';
+use constant REQUIRED_CREATE_FIELDS => qw(
+    name
+    product
+);
+
+use constant UPDATE_COLUMNS => qw(
+    value
+    sortkey
+);
+
+use constant VALIDATORS => {
+    product => \&_check_product,
+    sortkey => \&_check_sortkey,
+};
+
+use constant UPDATE_VALIDATORS => {
+    value => \&_check_value,
+};
+
+################################
 
 sub new {
     my $class = shift;
@@ -71,6 +93,118 @@ sub new {
     return $class->SUPER::new(@_);
 }
 
+sub run_create_validators {
+    my $class  = shift;
+    my $params = $class->SUPER::run_create_validators(@_);
+
+    my $product = delete $params->{product};
+    $params->{product_id} = $product->id;
+    $params->{value} = $class->_check_value($params->{name}, $product);
+    delete $params->{name};
+
+    return $params;
+}
+
+sub update {
+    my $self = shift;
+    my $changes = $self->SUPER::update(@_);
+
+    if (exists $changes->{value}) {
+        my $dbh = Bugzilla->dbh;
+        # The milestone value is stored in the bugs table instead of its ID.
+        $dbh->do('UPDATE bugs SET target_milestone = ?
+                  WHERE target_milestone = ? AND product_id = ?',
+                 undef, ($self->name, $changes->{value}->[0], $self->product_id));
+
+        # The default milestone also stores the value instead of the ID.
+        $dbh->do('UPDATE products SET defaultmilestone = ?
+                  WHERE id = ? AND defaultmilestone = ?',
+                 undef, ($self->name, $self->product_id, $changes->{value}->[0]));
+    }
+    return $changes;
+}
+
+sub remove_from_db {
+    my $self = shift;
+    my $dbh = Bugzilla->dbh;
+
+    # The default milestone cannot be deleted.
+    if ($self->name eq $self->product->default_milestone) {
+        ThrowUserError('milestone_is_default', { milestone => $self });
+    }
+
+    if ($self->bug_count) {
+        # We don't want to delete bugs when deleting a milestone.
+        # Bugs concerned are reassigned to the default milestone.
+        my $bug_ids =
+          $dbh->selectcol_arrayref('SELECT bug_id FROM bugs
+                                    WHERE product_id = ? AND target_milestone = ?',
+                                    undef, ($self->product->id, $self->name));
+
+        my $timestamp = $dbh->selectrow_array('SELECT NOW()');
+
+        $dbh->do('UPDATE bugs SET target_milestone = ?, delta_ts = ?
+                  WHERE bug_id IN (' . join(', ', @$bug_ids) . ')',
+                 undef, ($self->product->default_milestone, $timestamp));
+
+        require Bugzilla::Bug;
+        import Bugzilla::Bug qw(LogActivityEntry);
+        foreach my $bug_id (@$bug_ids) {
+            LogActivityEntry($bug_id, 'target_milestone',
+                             $self->name,
+                             $self->product->default_milestone,
+                             Bugzilla->user->id, $timestamp);
+        }
+    }
+
+    $dbh->do('DELETE FROM milestones WHERE id = ?', undef, $self->id);
+}
+
+################################
+# Validators
+################################
+
+sub _check_value {
+    my ($invocant, $name, $product) = @_;
+
+    trim($name) || ThrowUserError('milestone_blank_name');
+    if (length($name) > MAX_MILESTONE_SIZE) {
+        ThrowUserError('milestone_name_too_long', {name => $name});
+    }
+
+    $product = $invocant->product if (ref $invocant);
+    my $milestone = new Bugzilla::Milestone({product => $product, name => $name});
+    if ($milestone && (!ref $invocant || $milestone->id != $invocant->id)) {
+        ThrowUserError('milestone_already_exists', { name    => $milestone->name,
+                                                     product => $product->name });
+    }
+    return $name;
+}
+
+sub _check_sortkey {
+    my ($invocant, $sortkey) = @_;
+
+    # Keep a copy in case detaint_signed() clears the sortkey
+    my $stored_sortkey = $sortkey;
+
+    if (!detaint_signed($sortkey) || $sortkey < MIN_SMALLINT || $sortkey > MAX_SMALLINT) {
+        ThrowUserError('milestone_sortkey_invalid', {sortkey => $stored_sortkey});
+    }
+    return $sortkey;
+}
+
+sub _check_product {
+    my ($invocant, $product) = @_;
+    return Bugzilla->user->check_can_admin_product($product->name);
+}
+
+################################
+# Methods
+################################
+
+sub set_name { $_[0]->set('value', $_[1]); }
+sub set_sortkey { $_[0]->set('sortkey', $_[1]); }
+
 sub bug_count {
     my $self = shift;
     my $dbh = Bugzilla->dbh;
@@ -92,22 +226,12 @@ sub name       { return $_[0]->{'value'};      }
 sub product_id { return $_[0]->{'product_id'}; }
 sub sortkey    { return $_[0]->{'sortkey'};    }
 
-################################
-#####     Subroutines      #####
-################################
-
-sub check_sort_key {
-    my ($milestone_name, $sortkey) = @_;
-    # Keep a copy in case detaint_signed() clears the sortkey
-    my $stored_sortkey = $sortkey;
+sub product {
+    my $self = shift;
 
-    if (!detaint_signed($sortkey) || $sortkey < -32768
-        || $sortkey > 32767) {
-        ThrowUserError('milestone_sortkey_invalid',
-                       {'name' => $milestone_name,
-                        'sortkey' => $stored_sortkey});
-    }
-    return $sortkey;
+    require Bugzilla::Product;
+    $self->{'product'} ||= new Bugzilla::Product($self->product_id);
+    return $self->{'product'};
 }
 
 1;
@@ -122,13 +246,21 @@ Bugzilla::Milestone - Bugzilla product milestone class.
 
     use Bugzilla::Milestone;
 
-    my $milestone = new Bugzilla::Milestone(
-        { product => $product, name => 'milestone_value' });
+    my $milestone = new Bugzilla::Milestone({ name => $name, product => $product });
 
+    my $name       = $milestone->name;
     my $product_id = $milestone->product_id;
-    my $value = $milestone->value;
+    my $product    = $milestone->product;
+    my $sortkey    = $milestone->sortkey;
+
+    my $milestone = Bugzilla::Milestone->create(
+        { name => $name, product => $product, sortkey => $sortkey });
 
-    my $milestone = $hash_ref->{'milestone_value'};
+    $milestone->set_name($new_name);
+    $milestone->set_sortkey($new_sortkey);
+    $milestone->update();
+
+    $milestone->remove_from_db;
 
 =head1 DESCRIPTION
 
@@ -138,16 +270,48 @@ Milestone.pm represents a Product Milestone object.
 
 =over
 
-=item C<new($product_id, $value)>
+=item C<new({name => $name, product => $product})>
 
  Description: The constructor is used to load an existing milestone
-              by passing a product id and a milestone value.
+              by passing a product object and a milestone name.
 
- Params:      $product_id - Integer with a Bugzilla product id.
-              $value - String with a milestone value.
+ Params:      $product - a Bugzilla::Product object.
+              $name - the name of a milestone (string).
 
  Returns:     A Bugzilla::Milestone object.
 
+=item C<name()>
+
+ Description: Name (value) of the milestone.
+
+ Params:      none.
+
+ Returns:     The name of the milestone.
+
+=item C<product_id()>
+
+ Description: ID of the product the milestone belongs to.
+
+ Params:      none.
+
+ Returns:     The ID of a product.
+
+=item C<product()>
+
+ Description: The product object of the product the milestone belongs to.
+
+ Params:      none.
+
+ Returns:     A Bugzilla::Product object.
+
+=item C<sortkey()>
+
+ Description: Sortkey of the milestone.
+
+ Params:      none.
+
+ Returns:     The sortkey of the milestone.
+
 =item C<bug_count()>
 
  Description: Returns the total of bugs that belong to the milestone.
@@ -156,4 +320,55 @@ Milestone.pm represents a Product Milestone object.
 
  Returns:     Integer with the number of bugs.
 
+=item C<set_name($new_name)>
+
+ Description: Changes the name of the milestone.
+
+ Params:      $new_name - new name of the milestone (string). This name
+                          must be unique within the product.
+
+ Returns:     Nothing.
+
+=item C<set_sortkey($new_sortkey)>
+
+ Description: Changes the sortkey of the milestone.
+
+ Params:      $new_sortkey - new sortkey of the milestone (signed integer).
+
+ Returns:     Nothing.
+
+=item C<update()>
+
+ Description: Writes the new name and/or the new sortkey into the DB.
+
+ Params:      none.
+
+ Returns:     A hashref with changes made to the milestone object.
+
+=item C<remove_from_db()>
+
+ Description: Deletes the current milestone from the DB. The object itself
+              is not destroyed.
+
+ Params:      none.
+
+ Returns:     Nothing.
+
+=back
+
+=head1 CLASS METHODS
+
+=over
+
+=item C<create({name => $name, product => $product, sortkey => $sortkey})>
+
+ Description: Create a new milestone for the given product.
+
+ Params:      $name    - name of the new milestone (string). This name
+                         must be unique within the product.
+              $product - a Bugzilla::Product object.
+              $sortkey - the sortkey of the new milestone (signed integer)
+
+ Returns:     A Bugzilla::Milestone object.
+
 =back

Bugzilla/Object.pm

@@ -215,6 +215,7 @@ sub set {
     if (exists $validators{$field}) {
         my $validator = $validators{$field};
         $value = $self->$validator($value, $field);
+        trick_taint($value) if (defined $value && !ref($value));
 
         if ($self->can('_set_global_validator')) {
             $self->_set_global_validator($value, $field);

editmilestones.cgi

@@ -15,7 +15,6 @@
 #                Frédéric Buclin <LpSolit@gmail.com>
 #
 
-
 use strict;
 use lib ".";
 
@@ -24,7 +23,6 @@ use Bugzilla::Constants;
 use Bugzilla::Util;
 use Bugzilla::Error;
 use Bugzilla::Milestone;
-use Bugzilla::Bug;
 use Bugzilla::Token;
 
 my $cgi = Bugzilla->cgi;
@@ -37,7 +35,6 @@ my $vars = {};
 #
 
 my $user = Bugzilla->login(LOGIN_REQUIRED);
-my $whoid = $user->id;
 
 print $cgi->header();
 
@@ -86,16 +83,11 @@ unless ($action) {
 
     $vars->{'showbugcounts'} = $showbugcounts;
     $vars->{'product'} = $product;
-    $template->process("admin/milestones/list.html.tmpl",
-                       $vars)
+    $template->process("admin/milestones/list.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
-
     exit;
 }
 
-
-
-
 #
 # action='add' -> present form for parameters for new milestone
 #
@@ -105,62 +97,29 @@ unless ($action) {
 if ($action eq 'add') {
     $vars->{'token'} = issue_session_token('add_milestone');
     $vars->{'product'} = $product;
-    $template->process("admin/milestones/create.html.tmpl",
-                       $vars)
+    $template->process("admin/milestones/create.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
-
     exit;
 }
 
-
-
 #
 # action='new' -> add milestone entered in the 'action=add' screen
 #
 
 if ($action eq 'new') {
     check_token_data($token, 'add_milestone');
-    $milestone_name || ThrowUserError('milestone_blank_name');
-
-    if (length($milestone_name) > 20) {
-        ThrowUserError('milestone_name_too_long',
-                       {'name' => $milestone_name});
-    }
-
-    $sortkey = Bugzilla::Milestone::check_sort_key($milestone_name,
-                                                   $sortkey);
-
-    my $milestone = new Bugzilla::Milestone(
-        { product => $product, name => $milestone_name });
-
-    if ($milestone) {
-        ThrowUserError('milestone_already_exists',
-                       {'name' => $milestone->name,
-                        'product' => $product->name});
-    }
-
-    # Add the new milestone
-    trick_taint($milestone_name);
-    $dbh->do('INSERT INTO milestones ( value, product_id, sortkey )
-              VALUES ( ?, ?, ? )',
-             undef, $milestone_name, $product->id, $sortkey);
-
-    $milestone = new Bugzilla::Milestone(
-        { product => $product, name => $milestone_name });
+    my $milestone = Bugzilla::Milestone->create({ name    => $milestone_name,
+                                                  product => $product,
+                                                  sortkey => $sortkey });
     delete_token($token);
 
     $vars->{'milestone'} = $milestone;
     $vars->{'product'} = $product;
-    $template->process("admin/milestones/created.html.tmpl",
-                       $vars)
+    $template->process("admin/milestones/created.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
-
     exit;
 }
 
-
-
-
 #
 # action='del' -> ask if user really wants to delete
 #
@@ -176,7 +135,7 @@ if ($action eq 'del') {
 
     # The default milestone cannot be deleted.
     if ($product->default_milestone eq $milestone->name) {
-        ThrowUserError("milestone_is_default", $vars);
+        ThrowUserError("milestone_is_default", { milestone => $milestone });
     }
     $vars->{'token'} = issue_session_token('delete_milestone');
 
@@ -185,8 +144,6 @@ if ($action eq 'del') {
     exit;
 }
 
-
-
 #
 # action='delete' -> really delete the milestone
 #
@@ -195,47 +152,17 @@ if ($action eq 'delete') {
     check_token_data($token, 'delete_milestone');
     my $milestone = Bugzilla::Milestone->check({ product => $product,
                                                  name    => $milestone_name });
+    $milestone->remove_from_db;
+    delete_token($token);
+
     $vars->{'milestone'} = $milestone;
     $vars->{'product'} = $product;
 
-    # The default milestone cannot be deleted.
-    if ($milestone->name eq $product->default_milestone) {
-        ThrowUserError("milestone_is_default", $vars);
-    }
-
-    if ($milestone->bug_count) {
-        # We don't want to delete bugs when deleting a milestone.
-        # Bugs concerned are reassigned to the default milestone.
-        my $bug_ids =
-          $dbh->selectcol_arrayref("SELECT bug_id FROM bugs
-                                    WHERE product_id = ? AND target_milestone = ?",
-                                    undef, ($product->id, $milestone->name));
-        my $timestamp = $dbh->selectrow_array("SELECT NOW()");
-        foreach my $bug_id (@$bug_ids) {
-            $dbh->do("UPDATE bugs SET target_milestone = ?,
-                      delta_ts = ? WHERE bug_id = ?",
-                      undef, ($product->default_milestone, $timestamp,
-                              $bug_id));
-            # We have to update the 'bugs_activity' table too.
-            LogActivityEntry($bug_id, 'target_milestone',
-                             $milestone->name,
-                             $product->default_milestone,
-                             $whoid, $timestamp);
-        }
-    }
-
-    $dbh->do("DELETE FROM milestones WHERE product_id = ? AND value = ?",
-             undef, ($product->id, $milestone->name));
-
-    delete_token($token);
-
     $template->process("admin/milestones/deleted.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
     exit;
 }
 
-
-
 #
 # action='edit' -> present the edit milestone form
 #
@@ -251,15 +178,11 @@ if ($action eq 'edit') {
     $vars->{'product'} = $product;
     $vars->{'token'} = issue_session_token('edit_milestone');
 
-    $template->process("admin/milestones/edit.html.tmpl",
-                       $vars)
+    $template->process("admin/milestones/edit.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
-
     exit;
 }
 
-
-
 #
 # action='update' -> update the milestone
 #
@@ -267,93 +190,23 @@ if ($action eq 'edit') {
 if ($action eq 'update') {
     check_token_data($token, 'edit_milestone');
     my $milestone_old_name = trim($cgi->param('milestoneold') || '');
-    my $milestone_old = Bugzilla::Milestone->check(
-        { product => $product, name =>  $milestone_old_name });
-
-    if (length($milestone_name) > 20) {
-        ThrowUserError('milestone_name_too_long',
-                       {'name' => $milestone_name});
-    }
-
-    $dbh->bz_lock_tables('bugs WRITE',
-                         'milestones WRITE',
-                         'products WRITE');
-
-    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,
-                 $milestone_old->name);
-
-        $vars->{'updated_sortkey'} = 1;
-    }
-
-    if ($milestone_name ne $milestone_old->name) {
-        unless ($milestone_name) {
-            ThrowUserError('milestone_blank_name');
-        }
-        my $milestone = new Bugzilla::Milestone(
-            { product => $product, name => $milestone_name });
-        if ($milestone) {
-            ThrowUserError('milestone_already_exists',
-                           {'name' => $milestone->name,
-                            'product' => $product->name});
-        }
-
-        trick_taint($milestone_name);
-
-        $dbh->do('UPDATE bugs
-                  SET target_milestone = ?
-                  WHERE target_milestone = ?
-                  AND product_id = ?',
-                 undef,
-                 $milestone_name,
-                 $milestone_old->name,
-                 $product->id);
-
-        $dbh->do("UPDATE milestones
-                  SET value = ?
-                  WHERE product_id = ?
-                  AND value = ?",
-                 undef,
-                 $milestone_name,
-                 $product->id,
-                 $milestone_old->name);
-
-        $dbh->do("UPDATE products
-                  SET defaultmilestone = ?
-                  WHERE id = ?
-                  AND defaultmilestone = ?",
-                 undef,
-                 $milestone_name,
-                 $product->id,
-                 $milestone_old->name);
-
-        $vars->{'updated_name'} = 1;
-    }
+    my $milestone = Bugzilla::Milestone->check({ product => $product,
+                                                 name    => $milestone_old_name });
 
-    $dbh->bz_unlock_tables();
+    $milestone->set_name($milestone_name);
+    $milestone->set_sortkey($sortkey);
+    my $changes = $milestone->update();
 
-    my $milestone = Bugzilla::Milestone->check({ product => $product,
-                                                 name    => $milestone_name });
     delete_token($token);
 
     $vars->{'milestone'} = $milestone;
     $vars->{'product'} = $product;
-    $template->process("admin/milestones/updated.html.tmpl",
-                       $vars)
+    $vars->{'changes'} = $changes;
+    $template->process("admin/milestones/updated.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
-
     exit;
 }
 
-
 #
 # No valid action found
 #

template/en/default/admin/milestones/updated.html.tmpl

@@ -19,11 +19,10 @@
   #%]
 
 [%# INTERFACE:
+  # milestone: object; the milestone being edited.
   # product: object; Bugzilla::Product object representing the product to
   #               which the milestone belongs.
-  #
-  # 'updated_XXX' variables are booleans, and are defined if the
-  # 'XXX' field was updated during the edit just being handled.
+  # changes: hashref; contains changes made to the milestone.
   #%]
 
 [% title = BLOCK %]Updating Milestone '[% milestone.name FILTER html %]' of Product
@@ -32,15 +31,15 @@
   title = title
 %]
 
-[% IF updated_name %]
+[% IF changes.value.defined %]
   <p>Updated Milestone name to: '[% milestone.name FILTER html %]'.</p>
 [% END %]
 
-[% IF updated_sortkey %]
+[% IF changes.sortkey.defined %]
   <p>Updated Milestone sortkey to: '[% milestone.sortkey FILTER html %]'.</p>
 [% END %]
 
-[% UNLESS updated_sortkey || updated_name %]
+[% UNLESS changes.value.defined || changes.sortkey.defined %]
   <p>Nothing changed for milestone '[% milestone.name FILTER html %]'.</p>
 
 [% END %]

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

@@ -895,9 +895,9 @@
     [% title = "Default milestone not deletable" %]
     [% admindocslinks = {'products.html' => 'Administering products',
                          'milestones.html' => 'About Milestones'} %]
-    Sorry, but [% milestone.name FILTER html %] is the default milestone 
-    for product '[% product.name FILTER html %]', and so it can not be 
-    deleted.
+    Sorry, but [% milestone.name FILTER html %] is the default milestone
+    for the '[% milestone.product.name FILTER html %]' product, and so
+    it cannot be deleted.
 
   [% ELSIF error == "milestone_name_too_long" %]
     [% title = "Milestone Name Is Too Long" %]
@@ -913,9 +913,9 @@
 
   [% ELSIF error == "milestone_sortkey_invalid" %]
     [% title = "Invalid Milestone Sortkey" %]
-    The sortkey '[% sortkey FILTER html %]' for milestone '
-    [% name FILTER html %]' is not in the range -32768 ≤ sortkey
-    ≤ 32767.
+    The sortkey '[% sortkey FILTER html %]' is not in the range
+    [%+ constants.MIN_SMALLINT FILTER html %] ≤ sortkey ≤
+    [%+ constants.MAX_SMALLINT FILTER html %].
 
   [% ELSIF error == "misarranged_dates" %]
     [% title = "Misarranged Dates" %]