Bug 350307: Split out the create and update functionality of Bugzilla::Field::create_or_update

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

7 files changed, 237 insertions, 165 deletions

diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index f851e4533..cefd361ae 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -587,6 +587,8 @@ sub _bz_add_table_raw {
 sub bz_add_field_table {
     my ($self, $name) = @_;
     my $table_schema = $self->_bz_schema->FIELD_TABLE_SCHEMA;
+    # We do nothing if the table already exists.
+    return if $self->bz_table_info($name);
     my $indexes      = $table_schema->{INDEXES};
     # $indexes is an arrayref, not a hash. In order to fix the keys,
     # we have to fix every other item.
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index 602ab5e4d..782ee5581 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -43,8 +43,8 @@ Bugzilla::Field - a particular piece of information about bugs
   print Dumper(Bugzilla::Field->match({ obsolete => 1, custom => 1 }));
 
   # Create or update a custom field or field definition.
-  my $field = Bugzilla::Field::create_or_update(
-    {name => 'hilarity', desc => 'Hilarity', custom => 1});
+  my $field = Bugzilla::Field->create(
+    {name => 'cf_silly', description => 'Silly', custom => 1});
 
   # Instantiate a Field object for an existing field.
   my $field = new Bugzilla::Field({name => 'qacontact_accessible'});
@@ -99,6 +99,26 @@ use constant DB_COLUMNS => (
     'enter_bug',
 );
 
+use constant REQUIRED_CREATE_FIELDS => qw(name description);
+
+use constant VALIDATORS => {
+    custom      => \&_check_custom,
+    description => \&_check_description,
+    enter_bug   => \&_check_enter_bug,
+    mailhead    => \&_check_mailhead,
+    obsolete    => \&_check_obsolete,
+    sortkey     => \&_check_sortkey,
+    type        => \&_check_type,
+};
+
+use constant UPDATE_COLUMNS => qw(
+    description
+    mailhead
+    sortkey
+    obsolete
+    enter_bug
+);
+
 # How various field types translate into SQL data definitions.
 use constant SQL_DEFINITIONS => {
     # Using commas because these are constants and they shouldn't
@@ -168,6 +188,76 @@ use constant DEFAULT_FIELDS => (
     {name => "owner_idle_time",       desc => "Time Since Assignee Touched"},
 );
 
+##############
+# Validators #
+##############
+
+sub _check_custom { return $_[1] ? 1 : 0; }
+
+sub _check_description {
+    my ($invocant, $desc) = @_;
+    $desc = clean_text($desc);
+    $desc || ThrowUserError('field_missing_description');
+    return $desc;
+}
+
+sub _check_enter_bug { return $_[1] ? 1 : 0; }
+
+sub _check_mailhead { return $_[1] ? 1 : 0; }
+
+sub _check_name {
+    my ($invocant, $name, $is_custom) = @_;
+    $name = clean_text($name);
+    $name || ThrowUserError('field_missing_name');
+
+    # Don't want to allow a name that might mess up SQL.
+    my $name_regex = qr/^[\w\.]+$/;
+    # Custom fields have more restrictive name requirements than
+    # standard fields.
+    $name_regex = qr/^\w+$/ if $is_custom;
+    # Custom fields can't be named just "cf_", and there is no normal
+    # field named just "cf_".
+    ($name =~ $name_regex && $name ne "cf_")
+         || ThrowUserError('field_invalid_name', { name => $name });
+
+    # If it's custom, prepend cf_ to the custom field name to distinguish 
+    # it from standard fields.
+    if ($name !~ /^cf_/ && $is_custom) {
+        $name = 'cf_' . $name;
+    }
+
+    # Assure the name is unique. Names can't be changed, so we don't have
+    # to worry about what to do on updates.
+    my $field = new Bugzilla::Field({ name => $name });
+    ThrowUserError('field_already_exists', {'field' => $field }) if $field;
+
+    return $name;
+}
+
+sub _check_obsolete { return $_[1] ? 1 : 0; }
+
+sub _check_sortkey {
+    my ($invocant, $sortkey) = @_;
+    my $skey = $sortkey;
+    if (!defined $skey || $skey eq '') {
+        ($sortkey) = Bugzilla->dbh->selectrow_array(
+            'SELECT MAX(sortkey) + 100 FROM fielddefs') || 100;
+    }
+    detaint_natural($sortkey)
+        || ThrowUserError('field_invalid_sortkey', { sortkey => $skey });
+    return $sortkey;
+}
+
+sub _check_type {
+    my ($invocant, $type) = @_;
+    my $saved_type = $type;
+    # FIELD_TYPE_SINGLE_SELECT here should be updated every time a new,
+    # higher field type is added.
+    (detaint_natural($type) && $type <= FIELD_TYPE_SINGLE_SELECT)
+      || ThrowCodeError('invalid_customfield_type', { type => $saved_type });
+    return $type;
+}
+
 =pod
 
 =head2 Instance Properties
@@ -290,98 +380,105 @@ sub legal_values {
 
 =pod
 
-=head2 Class Methods
+=head2 Instance Mutators
+
+These set the particular field that they are named after.
+
+They take a single value--the new value for that field.
+
+They will throw an error if you try to set the values to something invalid.
 
 =over
 
-=item C<create_or_update({name => $name, desc => $desc, in_new_bugmail => 0, custom => 0})>
+=item C<set_description>
 
-Description: Creates a new field, or updates one that
-             already exists with the same name.
+=item C<set_enter_bug>
 
-Params:      This function takes named parameters in a hashref: 
-             C<name> - string - The name of the field.
-             C<desc> - string - The field label to display in the UI.
-             C<in_new_bugmail> - boolean - Whether this field appears at the
-                 top of the bugmail for a newly-filed bug.
+=item C<set_obsolete>
 
-             The following parameters are optional:
-             C<custom> - boolean - True if this is a Custom Field. The field
-                 will be added to the C<bugs> table if it does not exist.
-             C<sortkey> - integer - The sortkey of the field.
-             C<editable_on_enter_bug> - boolean - Whether this field is
-                 editable on the bug creation form.
-             C<is_obsolete> - boolean - Whether this field is obsolete.
+=item C<set_sortkey>
 
-Returns:     a C<Bugzilla::Field> object.
+=item C<set_in_new_bugmail>
 
 =back
 
 =cut
 
-sub create_or_update {
-    my ($params) = @_;
-    my $dbh = Bugzilla->dbh;
+sub set_description    { $_[0]->set('description', $_[1]); }
+sub set_enter_bug      { $_[0]->set('enter_bug',   $_[1]); }
+sub set_obsolete       { $_[0]->set('obsolete',    $_[1]); }
+sub set_sortkey        { $_[0]->set('sortkey',     $_[1]); }
+sub set_in_new_bugmail { $_[0]->set('mailhead',    $_[1]); }
 
-    my $name           = $params->{name};
-    my $custom         = $params->{custom} ? 1 : 0;
-    my $in_new_bugmail = $params->{in_new_bugmail} ? 1 : 0;
-    my $type           = $params->{type} || FIELD_TYPE_UNKNOWN;
+=pod
 
-    my $field = new Bugzilla::Field({name => $name});
-    if ($field) {
-        # Both fields are mandatory.
-        my @columns = ('description', 'mailhead');
-        my @values = ($params->{desc}, $in_new_bugmail);
+=head2 Class Methods
 
-        if (exists $params->{sortkey}) {
-            push(@columns, 'sortkey');
-            push(@values, $params->{sortkey} || 0);
-        }
-        if (exists $params->{editable_on_enter_bug}) {
-            push(@columns, 'enter_bug');
-            push(@values, $params->{editable_on_enter_bug} ? 1 : 0);
-        }
-        if (exists $params->{is_obsolete}) {
-            push(@columns, 'obsolete');
-            push(@values, $params->{is_obsolete} ? 1 : 0);
-        }
-        my $columns = join(', ', map {"$_ = ?"} @columns);
-        # Update the already-existing definition.
-        $dbh->do("UPDATE fielddefs SET $columns WHERE id = ?",
-                  undef, (@values, $field->id));
-    }
-    else {
-        my $sortkey     = $params->{sortkey} || 0;
-        my $enter_bug   = $params->{editable_on_enter_bug} ? 1 : 0;
-        my $is_obsolete = $params->{is_obsolete} ? 1 : 0;
+=over
 
-        $sortkey ||= $dbh->selectrow_array(
-            "SELECT MAX(sortkey) + 100 FROM fielddefs") || 100;
+=item C<create>
 
-        # Add the field to the list of fields at this Bugzilla installation.
-        $dbh->do("INSERT INTO fielddefs (name, description, sortkey, type,
-                                         custom, mailhead, obsolete, enter_bug)
-                       VALUES (?, ?, ?, ?, ?, ?, ?, ?)", undef,
-                 $name, $params->{desc}, $sortkey, $type, $custom, 
-                 $in_new_bugmail, $is_obsolete, $enter_bug);
-    }
+Just like L<Bugzilla::Object/create>. Takes the following parameters:
+
+=over
+
+=item C<name> B<Required> - The name of the field.
+
+=item C<description> B<Required> - The field label to display in the UI.
+
+=item C<mailhead> - boolean - Whether this field appears at the
+top of the bugmail for a newly-filed bug. Defaults to 0.
+
+=item C<custom> - boolean - True if this is a Custom Field. The field
+will be added to the C<bugs> table if it does not exist. Defaults to 0.
+
+=item C<sortkey> - integer - The sortkey of the field. Defaults to 0.
+
+=item C<enter_bug> - boolean - Whether this field is
+editable on the bug creation form. Defaults to 0.
+
+C<obsolete> - boolean - Whether this field is obsolete. Defaults to 0.
+
+=back
+
+=back
+
+=cut
+
+sub create {
+    my $class = shift;
+    my $field = $class->SUPER::create(@_);
 
-    if (!$dbh->bz_column_info('bugs', $name) && $custom) {
+    my $dbh = Bugzilla->dbh;
+    if ($field->custom) {
+        my $name = $field->name;
+        my $type = $field->type;
         # Create the database column that stores the data for this field.
         $dbh->bz_add_column('bugs', $name, SQL_DEFINITIONS->{$type});
+
+        if ($type == FIELD_TYPE_SINGLE_SELECT) {
+            # Create the table that holds the legal values for this field.
+            $dbh->bz_add_field_table($name);
+            # And insert a default value of "---" into it.
+            $dbh->do("INSERT INTO $name (value) VALUES ('---')");
+        }
     }
 
-    if ($custom && !$dbh->bz_table_info($name)
-        && $type eq FIELD_TYPE_SINGLE_SELECT)
-    {
-        # Create the table that holds the legal values for this field.
-        $dbh->bz_add_field_table($name);
-        # And insert a default value of "---" into it.
-        $dbh->do("INSERT INTO $name (value) VALUES ('---')");
+    return $field;
+}
+
+sub run_create_validators {
+    my $class = shift;
+    my $dbh = Bugzilla->dbh;
+    my $params = $class->SUPER::run_create_validators(@_);
+
+    $params->{name} = $class->_check_name($params->{name}, $params->{custom});
+    if (!exists $params->{sortkey}) {
+        $params->{sortkey} = $dbh->selectrow_array(
+            "SELECT MAX(sortkey) + 100 FROM fielddefs") || 100;
     }
 
-    return new Bugzilla::Field({name => $name});
+    return $params;
 }
 
 
@@ -508,8 +605,22 @@ sub populate_field_definitions {
     my $dbh = Bugzilla->dbh;
 
     # ADD and UPDATE field definitions
-    foreach my $definition (DEFAULT_FIELDS) {
-        create_or_update($definition);
+    foreach my $def (DEFAULT_FIELDS) {
+        my $field = new Bugzilla::Field({ name => $def->{name} });
+        if ($field) {
+            $field->set_description($def->{desc});
+            $field->set_in_new_bugmail($def->{in_new_bugmail});
+            $field->update();
+        }
+        else {
+            if (exists $def->{in_new_bugmail}) {
+                $def->{mailhead} = $def->{in_new_bugmail};
+                delete $def->{in_new_bugmail};
+            }
+            $def->{description} = $def->{desc};
+            delete $def->{desc};
+            Bugzilla::Field->create($def);
+        }
     }
 
     # DELETE fields which were added only accidentally, or which
@@ -583,8 +694,9 @@ sub populate_field_definitions {
 
     # This field has to be created separately, or the above upgrade code
     # might not run properly.
-    Bugzilla::Field::create_or_update(
-        {name => $new_field_name, desc => $field_description});
+    Bugzilla::Field->create({ name => $new_field_name, 
+                              description => $field_description })
+        unless new Bugzilla::Field({ name => $new_field_name });
 
 }
 
diff --git a/checksetup.pl b/checksetup.pl
index 2a67e8b6e..d2313c92b 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -199,7 +199,7 @@ check_graphviz(!$silent) if Bugzilla->params->{'webdotbase'};
 # Changes to the fielddefs --TABLE--
 ###########################################################################
 
-# Calling Bugzilla::Field::create_or_update depends on the
+# Using Bugzilla::Field's create() or update() depends on the
 # fielddefs table having a modern definition. So, we have to make
 # these particular schema changes before we make any other schema changes.
 Bugzilla::Install::DB::update_fielddefs_definition();
diff --git a/editfields.cgi b/editfields.cgi
index f7a059016..e57e1952c 100644
--- a/editfields.cgi
+++ b/editfields.cgi
@@ -55,49 +55,18 @@ elsif ($action eq 'add') {
 }
 elsif ($action eq 'new') {
     check_token_data($token, 'add_field');
-    my $name = clean_text($cgi->param('name') || '');
-    my $desc = clean_text($cgi->param('desc') || '');
-    my $type = trim($cgi->param('type') || FIELD_TYPE_FREETEXT);
-    my $sortkey = $cgi->param('sortkey') || 0;
-
-    # Validate these fields.
-    $name || ThrowUserError('customfield_missing_name');
-    # Don't want to allow a name that might mess up SQL.
-    $name =~ /^\w+$/ && $name ne "cf_"
-      || ThrowUserError('customfield_invalid_name', { name => $name });
-    # Prepend cf_ to the custom field name to distinguish it from standard fields.
-    if ($name !~ /^cf_/) {
-        $name = 'cf_' . $name;
-    }
-    my $field = new Bugzilla::Field({'name' => $name});
-    ThrowUserError('customfield_already_exists', {'field' => $field }) if $field;
-
-    $desc || ThrowUserError('customfield_missing_description', {'name' => $name});
-
-    # We hardcode valid values for $type. This doesn't matter.
-    my $typ = $type;
-    (detaint_natural($type) && $type < 3)
-      || ThrowCodeError('invalid_customfield_type', {'type' => $typ});
-
-    my $skey = $sortkey;
-    detaint_natural($sortkey)
-      || ThrowUserError('customfield_invalid_sortkey', {'name'    => $name,
-                                                        'sortkey' => $skey});
-
-    # All fields have been validated. We can create this new custom field.
-    trick_taint($name);
-    trick_taint($desc);
-
-    $vars->{'name'} = $name;
-    $vars->{'desc'} = $desc;
-    $vars->{'sortkey'} = $sortkey;
-    $vars->{'type'} = $type;
-    $vars->{'custom'} = 1;
-    $vars->{'in_new_bugmail'} = $cgi->param('new_bugmail') ? 1 : 0;
-    $vars->{'editable_on_enter_bug'} = $cgi->param('enter_bug') ? 1 : 0;
-    $vars->{'is_obsolete'} = $cgi->param('obsolete') ? 1 : 0;
-
-    Bugzilla::Field::create_or_update($vars);
+
+    $vars->{'field'} = Bugzilla::Field->create({
+        name        => scalar $cgi->param('name'),
+        description => scalar $cgi->param('desc'),
+        type        => scalar $cgi->param('type'),
+        sortkey     => scalar $cgi->param('sortkey'),
+        mailhead    => scalar $cgi->param('new_bugmail'),
+        enter_bug   => scalar $cgi->param('enter_bug'),
+        obsolete    => scalar $cgi->param('obsolete'),
+        custom      => 1,
+    });
+
     delete_token($token);
 
     $vars->{'message'} = 'custom_field_created';
@@ -106,7 +75,7 @@ elsif ($action eq 'new') {
         || ThrowTemplateError($template->error());
 }
 elsif ($action eq 'edit') {
-    my $name = $cgi->param('name') || ThrowUserError('customfield_missing_name');
+    my $name = $cgi->param('name') || ThrowUserError('field_missing_name');
     # Custom field names must start with "cf_".
     if ($name !~ /^cf_/) {
         $name = 'cf_' . $name;
@@ -123,11 +92,9 @@ elsif ($action eq 'edit') {
 elsif ($action eq 'update') {
     check_token_data($token, 'edit_field');
     my $name = $cgi->param('name');
-    my $desc = clean_text($cgi->param('desc') || '');
-    my $sortkey = $cgi->param('sortkey') || 0;
 
     # Validate fields.
-    $name || ThrowUserError('customfield_missing_name');
+    $name || ThrowUserError('field_missing_name');
     # Custom field names must start with "cf_".
     if ($name !~ /^cf_/) {
         $name = 'cf_' . $name;
@@ -135,25 +102,16 @@ elsif ($action eq 'update') {
     my $field = new Bugzilla::Field({'name' => $name});
     $field || ThrowUserError('customfield_nonexistent', {'name' => $name});
 
-    $desc || ThrowUserError('customfield_missing_description', {'name' => $name});
-    trick_taint($desc);
-
-    my $skey = $sortkey;
-    detaint_natural($sortkey)
-      || ThrowUserError('customfield_invalid_sortkey', {'name'    => $name,
-                                                        'sortkey' => $skey});
-
-    $vars->{'name'} = $field->name;
-    $vars->{'desc'} = $desc;
-    $vars->{'sortkey'} = $sortkey;
-    $vars->{'custom'} = 1;
-    $vars->{'in_new_bugmail'} = $cgi->param('new_bugmail') ? 1 : 0;
-    $vars->{'editable_on_enter_bug'} = $cgi->param('enter_bug') ? 1 : 0;
-    $vars->{'is_obsolete'} = $cgi->param('obsolete') ? 1 : 0;
+    $field->set_description($cgi->param('desc'));
+    $field->set_sortkey($cgi->param('sortkey'));
+    $field->set_in_new_bugmail($cgi->param('new_bugmail'));
+    $field->set_enter_bug($cgi->param('enter_bug'));
+    $field->set_obsolete($cgi->param('obsolete'));
+    $field->update();
 
-    Bugzilla::Field::create_or_update($vars);
     delete_token($token);
 
+    $vars->{'field'}   = $field;
     $vars->{'message'} = 'custom_field_updated';
 
     $template->process('admin/custom_fields/list.html.tmpl', $vars)
diff --git a/template/en/default/admin/custom_fields/create.html.tmpl b/template/en/default/admin/custom_fields/create.html.tmpl
index 995c4d0a9..a67e94680 100644
--- a/template/en/default/admin/custom_fields/create.html.tmpl
+++ b/template/en/default/admin/custom_fields/create.html.tmpl
@@ -93,7 +93,7 @@
     <tr>
       <th align="right"><label for="sortkey">Sortkey:</label></th>
       <td>
-        <input type="text" id="sortkey" name="sortkey" value="0" size="6" maxlength="6">
+        <input type="text" id="sortkey" name="sortkey" size="6" maxlength="6">
       </td>
 
       <th>&nbsp;</th>
diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl
index 3d05214dc..a3b29bee7 100644
--- a/template/en/default/global/messages.html.tmpl
+++ b/template/en/default/global/messages.html.tmpl
@@ -168,12 +168,12 @@
 
   [% ELSIF message_tag == "custom_field_created" %]
     [% title = "Custom Field Created" %]
-    The new custom field '[% name FILTER html %]' has been
+    The new custom field '[% field.name FILTER html %]' has been
     successfully created.
 
   [% ELSIF message_tag == "custom_field_updated" %]
     [% title = "Custom Field Updated" %]
-    Properties of the '[% name FILTER html %]' field have been
+    Properties of the '[% field.name FILTER html %]' field have been
     successfully updated.
 
   [% ELSIF message_tag == "emailold_change_cancelled" %]
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index d6b596ea5..ef53b2b8e 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -311,34 +311,10 @@
     Product [% product FILTER html %] does not have a component
     named [% name FILTER html %].
 
-  [% ELSIF error == "customfield_already_exists" %]
-    [% title = "Field Already Exists" %]
-    The field '[% field.name FILTER html %]' ([% field.description FILTER html %])
-    already exists. Please choose another name.
-
-  [% ELSIF error == "customfield_invalid_name" %]
-    [% title = "Invalid Custom Field Name" %]
-    '[% name FILTER html %]' is not a valid name for a custom field.
-    A name may contain only letters, numbers, and the underscore character. The
-    name should also be different from 'cf_'.
-
   [% ELSIF error == "customfield_nonexistent" %]
     [% title = "Unknown Custom Field" %]
     There is no custom field with the name '[% name FILTER html %]'.
 
-  [% ELSIF error == "customfield_invalid_sortkey" %]
-    [% title = "Invalid Sortkey for Field" %]
-    The sortkey [% sortkey FILTER html %] that you have provided for
-    the '[% name FILTER html %]' field is not a valid positive integer.
-
-  [% ELSIF error == "customfield_missing_description" %]
-    [% title = "Missing Description for Field" %]
-    You must enter a description for the '[% name FILTER html %]' field.
-
-  [% ELSIF error == "customfield_missing_name" %]
-    [% title = "Missing Name for Field" %]
-    You must enter a name for this field.
-
   [% ELSIF error == "dependency_loop_multi" %]
     [% title = "Dependency Loop Detected" %]
     The following [% terms.bug %](s) would appear on both the "depends on"
@@ -400,6 +376,30 @@
     does not exist or you aren't authorized to
     enter [% terms.abug %] into it.
 
+  [% ELSIF error == "field_already_exists" %]
+    [% title = "Field Already Exists" %]
+    The field '[% field.name FILTER html %]'     
+    ([% field.description FILTER html %]) already exists. Please
+    choose another name.
+
+  [% ELSIF error == "field_invalid_name" %]
+    [% title = "Invalid Field Name" %]
+    '[% name FILTER html %]' is not a valid name for a field.
+    A name may contain only letters, numbers, and the underscore character.
+
+  [% ELSIF error == "field_invalid_sortkey" %]
+    [% title = "Invalid Sortkey for Field" %]
+    The sortkey [% sortkey FILTER html %] that you have provided for
+    this field is not a valid positive integer.
+
+  [% ELSIF error == "field_missing_description" %]
+    [% title = "Missing Description for Field" %]
+    You must enter a description for this field.
+
+  [% ELSIF error == "field_missing_name" %]
+    [% title = "Missing Name for Field" %]
+    You must enter a name for this field.
+
   [% ELSIF error == "fieldname_invalid" %]
     [% title = "Specified Field Does Not Exist" %]
     The field '[% field FILTER html %]' does not exist or