Bug 471871: Bugzilla::Version has duplicated code compared to Bugzilla::Object (make Bugzilla::Version really a subclass of Bugzilla::Object) - Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=mkanat

6 files changed, 105 insertions, 118 deletions

diff --git a/Bugzilla/Install.pm b/Bugzilla/Install.pm
index 0a1f0e955..8365f4182 100644
--- a/Bugzilla/Install.pm
+++ b/Bugzilla/Install.pm
@@ -240,7 +240,8 @@ sub create_default_product {
         my $product = new Bugzilla::Product({name => $default_prod->{name}});
 
         # The default version.
-        Bugzilla::Version::create(Bugzilla::Version::DEFAULT_VERSION, $product);
+        Bugzilla::Version->create({name => Bugzilla::Version::DEFAULT_VERSION,
+                                   product => $product});
 
         # And we automatically insert the default milestone.
         $dbh->do(q{INSERT INTO milestones (product_id, value, sortkey)
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 03ebe2639..488624c43 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -112,7 +112,7 @@ sub create {
     my $product = $class->insert_create_data($params);
 
     # Add the new version and milestone into the DB as valid values.
-    Bugzilla::Version::create($version, $product);
+    Bugzilla::Version->create({name => $version, product => $product});
     Bugzilla::Milestone->create({name => $params->{defaultmilestone}, product => $product});
 
     # Create groups and series for the new product, if requested.
diff --git a/Bugzilla/Version.pm b/Bugzilla/Version.pm
index a2ef6b01e..1c96003f1 100644
--- a/Bugzilla/Version.pm
+++ b/Bugzilla/Version.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;
 
@@ -32,6 +33,10 @@ use Bugzilla::Error;
 use constant DEFAULT_VERSION => 'unspecified';
 
 use constant DB_TABLE => 'versions';
+use constant NAME_FIELD => 'value';
+# This is "id" because it has to be filled in and id is probably the fastest.
+# We do a custom sort in new_from_list below.
+use constant LIST_ORDER => 'id';
 
 use constant DB_COLUMNS => qw(
     id
@@ -39,10 +44,26 @@ use constant DB_COLUMNS => qw(
     product_id
 );
 
-use constant NAME_FIELD => 'value';
-# This is "id" because it has to be filled in and id is probably the fastest.
-# We do a custom sort in new_from_list below.
-use constant LIST_ORDER => 'id';
+use constant REQUIRED_CREATE_FIELDS => qw(
+    name
+    product
+);
+
+use constant UPDATE_COLUMNS => qw(
+    value
+);
+
+use constant VALIDATORS => {
+    product => \&_check_product,
+};
+
+use constant UPDATE_VALIDATORS => {
+    value => \&_check_value,
+};
+
+################################
+# Methods
+################################
 
 sub new {
     my $class = shift;
@@ -79,6 +100,18 @@ sub new_from_list {
     return [sort { vers_cmp(lc($a->name), lc($b->name)) } @$list];
 }
 
+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 bug_count {
     my $self = shift;
     my $dbh = Bugzilla->dbh;
@@ -92,87 +125,71 @@ sub bug_count {
     return $self->{'bug_count'};
 }
 
-sub remove_from_db {
+sub update {
     my $self = shift;
-    my $dbh = Bugzilla->dbh;
+    my ($changes, $old_self) = $self->SUPER::update(@_);
 
-    # The version cannot be removed if there are bugs
-    # associated with it.
-    if ($self->bug_count) {
-        ThrowUserError("version_has_bugs", { nb => $self->bug_count });
+    if (exists $changes->{value}) {
+        my $dbh = Bugzilla->dbh;
+        $dbh->do('UPDATE bugs SET version = ?
+                  WHERE version = ? AND product_id = ?',
+                  undef, ($self->name, $old_self->name, $self->product_id));
     }
-
-    $dbh->do(q{DELETE FROM versions WHERE product_id = ? AND value = ?},
-              undef, ($self->product_id, $self->name));
+    return $changes;
 }
 
-sub update {
+sub remove_from_db {
     my $self = shift;
-    my ($name, $product) = @_;
     my $dbh = Bugzilla->dbh;
 
-    $name || ThrowUserError('version_not_specified');
-
-    # Remove unprintable characters
-    $name = clean_text($name);
-
-    return 0 if ($name eq $self->name);
-    my $version = new Bugzilla::Version({ product => $product, name => $name });
-
-    if ($version) {
-        ThrowUserError('version_already_exists',
-                       {'name' => $version->name,
-                        'product' => $product->name});
+    # The version cannot be removed if there are bugs
+    # associated with it.
+    if ($self->bug_count) {
+        ThrowUserError("version_has_bugs", { nb => $self->bug_count });
     }
-
-    trick_taint($name);
-    $dbh->do("UPDATE bugs SET version = ?
-              WHERE version = ? AND product_id = ?", undef,
-              ($name, $self->name, $self->product_id));
-
-    $dbh->do("UPDATE versions SET value = ?
-              WHERE product_id = ? AND value = ?", undef,
-              ($name, $self->product_id, $self->name));
-
-    $self->{'value'} = $name;
-
-    return 1;
+    $self->SUPER::remove_from_db();
 }
 
 ###############################
 #####     Accessors        ####
 ###############################
 
-sub name       { return $_[0]->{'value'};      }
 sub product_id { return $_[0]->{'product_id'}; }
 
-###############################
-#####     Subroutines       ###
-###############################
+sub product {
+    my $self = shift;
 
-sub create {
-    my ($name, $product) = @_;
-    my $dbh = Bugzilla->dbh;
+    require Bugzilla::Product;
+    $self->{'product'} ||= new Bugzilla::Product($self->product_id);
+    return $self->{'product'};
+}
 
-    # Cleanups and validity checks
-    $name || ThrowUserError('version_blank_name');
+################################
+# Validators
+################################
 
+sub set_name { $_[0]->set('value', $_[1]); }
+
+sub _check_value {
+    my ($invocant, $name, $product) = @_;
+
+    $name = trim($name);
+    $name || ThrowUserError('version_blank_name');
     # Remove unprintable characters
     $name = clean_text($name);
 
+    $product = $invocant->product if (ref $invocant);
     my $version = new Bugzilla::Version({ product => $product, name => $name });
-    if ($version) {
-        ThrowUserError('version_already_exists',
-                       {'name' => $version->name,
-                        'product' => $product->name});
+    if ($version && (!ref $invocant || $version->id != $invocant->id)) {
+        ThrowUserError('version_already_exists', { name    => $version->name,
+                                                   product => $product->name });
     }
+    return $name;
+}
 
-    # Add the new version
-    trick_taint($name);
-    $dbh->do(q{INSERT INTO versions (value, product_id)
-               VALUES (?, ?)}, undef, ($name, $product->id));
-
-    return new Bugzilla::Version($dbh->bz_last_key('versions', 'id'));
+sub _check_product {
+    my ($invocant, $product) = @_;
+    return Bugzilla->user->check_can_admin_product($product->name);
 }
 
 1;
@@ -187,37 +204,33 @@ Bugzilla::Version - Bugzilla product version class.
 
     use Bugzilla::Version;
 
-    my $version = new Bugzilla::Version(1, 'version_value');
+    my $version = new Bugzilla::Version({ name => $name, product => $product });
 
+    my $value = $version->name;
     my $product_id = $version->product_id;
-    my $value = $version->value;
+    my $product = $version->product;
 
-    $version->remove_from_db;
-
-    my $updated = $version->update($version_name, $product);
+    my $version = Bugzilla::Version->create(
+        { name => $name, product => $product });
 
-    my $version = $hash_ref->{'version_value'};
+    $version->set_name($new_name);
+    $version->update();
 
-    my $version = Bugzilla::Version::create($version_name, $product);
+    $version->remove_from_db;
 
 =head1 DESCRIPTION
 
-Version.pm represents a Product Version object.
+Version.pm represents a Product Version 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::Version> are listed
+below.
 
 =head1 METHODS
 
 =over
 
-=item C<new($product_id, $value)>
-
- Description: The constructor is used to load an existing version
-              by passing a product id and a version value.
-
- Params:      $product_id - Integer with a product id.
-              $value - String with a version value.
-
- Returns:     A Bugzilla::Version object.
-
 =item C<bug_count()>
 
  Description: Returns the total of bugs that belong to the version.
@@ -226,38 +239,6 @@ Version.pm represents a Product Version object.
 
  Returns:     Integer with the number of bugs.
 
-=item C<remove_from_db()>
-
- Description: Removes the version from the database.
-
- Params:      none.
-
- Retruns:     none.
-
-=item C<update($name, $product)>
-
- Description: Update the value of the version.
-
- Params:      $name - String with the new version value.
-              $product - Bugzilla::Product object the version belongs to.
-
- Returns:     An integer - 1 if the version has been updated, else 0.
-
-=back
-
-=head1 SUBROUTINES
-
-=over
-
-=item C<create($version_name, $product)>
-
- Description: Create a new version for the given product.
-
- Params:      $version_name - String with a version value.
-              $product - A Bugzilla::Product object.
-
- Returns:     A Bugzilla::Version object.
-
 =back
 
 =cut
diff --git a/editversions.cgi b/editversions.cgi
index 85f4f8ca4..7e6b9247d 100755
--- a/editversions.cgi
+++ b/editversions.cgi
@@ -119,7 +119,8 @@ if ($action eq 'add') {
 
 if ($action eq 'new') {
     check_token_data($token, 'add_version');
-    my $version = Bugzilla::Version::create($version_name, $product);
+    my $version = Bugzilla::Version->create(
+        {name => $version_name, product => $product});
     delete_token($token);
 
     $vars->{'message'} = 'version_created';
@@ -202,7 +203,8 @@ if ($action eq 'update') {
 
     $dbh->bz_start_transaction();
 
-    $vars->{'updated'} = $version->update($version_name, $product);
+    $version->set_name($version_name);
+    my $changes = $version->update();
 
     $dbh->bz_commit_transaction();
     delete_token($token);
@@ -210,6 +212,7 @@ if ($action eq 'update') {
     $vars->{'message'} = 'version_updated';
     $vars->{'version'} = $version;
     $vars->{'product'} = $product;
+    $vars->{'changes'} = $changes;
     $template->process("admin/versions/list.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
 
diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl
index c8e4dd225..d2915780c 100644
--- a/template/en/default/global/messages.html.tmpl
+++ b/template/en/default/global/messages.html.tmpl
@@ -796,7 +796,13 @@
 
   [% ELSIF message_tag == "version_updated" %]
     [% title = "Version Updated" %]
-    Version renamed as <em>[% version.name FILTER html %]</em>.
+    [% IF changes.size %]
+      [% IF changes.value.defined %]
+        Version renamed to <em>[% version.name FILTER html %]</em>.
+      [% END %]
+    [% ELSE %]
+      No changes made to version <em>[% version.name FILTER html %]</em>.
+    [% END %]
 
   [% ELSIF message_tag == "workflow_updated" %]
     The workflow has been updated.
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 879e62ade..d1c3bd643 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -1579,10 +1579,6 @@
     version! You must reassign those [% terms.bugs %] to another version
     before you can delete this one.
 
-  [% ELSIF error == "version_not_specified" %]
-    [% title = "No Version Specified" %]
-    No version specified when trying to edit versions.
-
   [% ELSIF error == "users_deletion_disabled" %]
     [% title = "Deletion not activated" %]
     [% admindocslinks = {'useradmin.html' => 'User administration'} %]