Bug 347061: Create Bugzilla::Object->create and make Bugzilla::Keyword use it

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=bkor, a=myk

6 files changed, 120 insertions, 58 deletions

diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index e997e4187..50785c5b7 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -346,7 +346,7 @@ use constant ABSTRACT_SCHEMA => {
 
     keyworddefs => {
         FIELDS => [
-            id          => {TYPE => 'INT2', NOTNULL => 1,
+            id          => {TYPE => 'SMALLSERIAL', NOTNULL => 1,
                             PRIMARYKEY => 1},
             name        => {TYPE => 'varchar(64)', NOTNULL => 1},
             description => {TYPE => 'MEDIUMTEXT'},
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index dfe7f957a..a4ab54260 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -468,6 +468,9 @@ sub update_table_definitions {
     $dbh->bz_alter_column('flagtypes', 'id',
           {TYPE => 'SMALLSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
 
+    $dbh->bz_alter_column('keyworddefs', 'id',
+        {TYPE => 'SMALLSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
+
     ################################################################
     # New --TABLE-- changes should go *** A B O V E *** this point #
     ################################################################
diff --git a/Bugzilla/Keyword.pm b/Bugzilla/Keyword.pm
index 2e0f19f4c..fead77821 100644
--- a/Bugzilla/Keyword.pm
+++ b/Bugzilla/Keyword.pm
@@ -20,6 +20,9 @@ package Bugzilla::Keyword;
 
 use base qw(Bugzilla::Object);
 
+use Bugzilla::Error;
+use Bugzilla::Util;
+
 ###############################
 ####    Initialization     ####
 ###############################
@@ -32,6 +35,13 @@ use constant DB_COLUMNS => qw(
 
 use constant DB_TABLE => 'keyworddefs';
 
+use constant REQUIRED_CREATE_FIELDS => qw(name description);
+
+use constant VALIDATORS => {
+    name        => \&_check_name,
+    description => \&_check_description,
+};
+
 ###############################
 ####      Accessors      ######
 ###############################
@@ -81,6 +91,30 @@ sub get_all_with_bug_count {
     return $keywords;
 }
 
+###############################
+###       Validators        ###
+###############################
+
+sub _check_name {
+    my ($name) = @_;
+    $name = trim($name);
+    $name eq "" && ThrowUserError("keyword_blank_name");
+    if ($name =~ /[\s,]/) {
+        ThrowUserError("keyword_invalid_name");
+    }
+    my $keyword = new Bugzilla::Keyword({ name => $name });
+    ThrowUserError("keyword_already_exists", { name => $name }) if $keyword;
+
+    return $name;
+}
+
+sub _check_description {
+    my ($desc) = @_;
+    $desc = trim($desc);
+    $desc eq '' && ThrowUserError("keyword_blank_description");
+    return $desc;
+}
+
 1;
 
 __END__
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 56789f584..c250f80fd 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -108,6 +108,45 @@ sub name              { return $_[0]->{'name'};        }
 ####      Subroutines    ######
 ###############################
 
+sub create {
+    my ($class, $params) = @_;
+    my $dbh = Bugzilla->dbh;
+
+    my $required   = $class->REQUIRED_CREATE_FIELDS;
+    my $validators = $class->VALIDATORS;
+    my $table      = $class->DB_TABLE;
+
+    foreach my $field ($class->REQUIRED_CREATE_FIELDS) {
+        ThrowCodeError('param_required', 
+            { function => "${class}->create", param => $field })
+            if !exists $params->{$field};
+    }
+
+    my (@field_names, @values);
+    # We do the sort just to make sure that validation always
+    # happens in a consistent order.
+    foreach my $field (sort keys %$params) {
+        my $value;
+        if (exists $validators->{$field}) {
+            $value = &{$validators->{$field}}($params->{$field});
+        }
+        else {
+            $value = $params->{$field};
+        }
+        trick_taint($value);
+        push(@field_names, $field);
+        push(@values, $value);
+    }
+
+    my $qmarks = '?,' x @values;
+    chop($qmarks);
+    $dbh->do("INSERT INTO $table (" . join(', ', @field_names) 
+             . ") VALUES ($qmarks)", undef, @values);
+    my $id = $dbh->bz_last_key($table, 'id');
+
+    return $class->new($id);
+}
+
 sub get_all {
     my $class = shift;
     my $dbh = Bugzilla->dbh;
@@ -173,6 +212,19 @@ The order that C<new_from_list> and C<get_all> should return objects
 in. This should be the name of a database column. Defaults to
 C<name>.
 
+=item C<REQUIRED_CREATE_FIELDS>
+
+The list of fields that B<must> be specified when the user calls
+C<create()>. This should be an array.
+
+=item C<VALIDATORS>
+
+A hashref that points to a function that will validate each param to
+C<create()>. Each function in this hashref will be passed a single
+argument, the value passed to C<create()> for that field. These
+functions should call L<Bugzilla::Error/ThrowUserError> if they fail.
+They must return the validated value.
+
 =back
 
 =head1 METHODS
@@ -210,6 +262,25 @@ C<name>.
 
 =over
 
+=item C<create($params)>
+
+Description: Creates a new item in the database.
+             Throws a User Error if any of the passed-in params
+             are invalid.
+
+Params:      C<$params> - hashref - A value to put in each database
+               field for this object. Certain values must be set (the 
+               ones specified in L</REQUIRED_CREATE_FIELDS>), and
+               the function will throw a Code Error if you don't set
+               them.
+
+Returns:     The Object just created in the database.
+
+Notes:       In order for this function to work in your subclass,
+             your subclass's C<id> field must be of C<SERIAL>
+             type in the database. Your subclass also must
+             define L</REQUIRED_CREATE_FIELDS> and L</VALIDATORS>.
+
 =item C<get_all>
 
  Description: Returns all objects in this table from the database.
diff --git a/editkeywords.cgi b/editkeywords.cgi
index 7b906c30b..7b94dbbe3 100755
--- a/editkeywords.cgi
+++ b/editkeywords.cgi
@@ -34,25 +34,6 @@ my $dbh = Bugzilla->dbh;
 my $template = Bugzilla->template;
 my $vars = {};
 
-sub Validate {
-    my ($name, $description) = @_;
-    if ($name eq "") {
-        ThrowUserError("keyword_blank_name");
-    }
-    if ($name =~ /[\s,]/) {
-        ThrowUserError("keyword_invalid_name");
-    }    
-    if ($description eq "") {
-        ThrowUserError("keyword_blank_description");
-    }
-    # It is safe to detaint these values as they are only
-    # used in placeholders.
-    trick_taint($name);
-    $_[0] = $name;
-    trick_taint($description);
-    $_[1] = $description;
-}
-
 sub ValidateKeyID {
     my $id = shift;
 
@@ -102,49 +83,16 @@ if ($action eq 'add') {
 #
 # action='new' -> add keyword entered in the 'action=add' screen
 #
-
 if ($action eq 'new') {
-    # Cleanups and validity checks
-
-    my $name = trim($cgi->param('name') || '');
-    my $description  = trim($cgi->param('description')  || '');
-
-    Validate($name, $description);
-
-    my $id = $dbh->selectrow_array('SELECT id FROM keyworddefs
-                                    WHERE name = ?', undef, $name);
-
-    if ($id) {
-        $vars->{'name'} = $name;
-        ThrowUserError("keyword_already_exists", $vars);
-    }
-
-
-    # Pick an unused number.  Be sure to recycle numbers that may have been
-    # deleted in the past.  This code is potentially slow, but it happens
-    # rarely enough, and there really aren't ever going to be that many
-    # keywords anyway.
-
-    my $existing_ids =
-        $dbh->selectcol_arrayref('SELECT id FROM keyworddefs ORDER BY id');
+    my $name = $cgi->param('name') || '';
+    my $desc = $cgi->param('description')  || '';
 
-    my $newid = 1;
-
-    foreach my $oldid (@$existing_ids) {
-        if ($oldid > $newid) {
-            last;
-        }
-        $newid = $oldid + 1;
-    }
-
-    # Add the new keyword.
-    $dbh->do('INSERT INTO keyworddefs
-              (id, name, description) VALUES (?, ?, ?)',
-              undef, ($newid, $name, $description));
+    my $keyword = Bugzilla::Keyword->create(
+        { name => $name, description => $desc });
 
     print $cgi->header();
 
-    $vars->{'name'} = $name;
+    $vars->{'name'} = $keyword->name;
     $template->process("admin/keywords/created.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
 
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index f64cf6411..9d039a07c 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -294,6 +294,12 @@
     Invalid parameter passed to [% function FILTER html %].
     It must be numeric.
 
+  [% ELSIF error == "param_required" %]
+    [% title = "Missing Parameter" %]
+    The function <code>[% function FILTER html %]</code> requires
+    a <code>[% param FILTER html %]</code> argument, and that
+    argument was not set.
+
   [% ELSIF error == "unknown_comparison_type" %]
     Specified comparison type is not supported.