Bug 455632: Add Bugzilla::Field::Choice->create and have editvalues.cgi use it

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

8 files changed, 231 insertions, 159 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 1d12ee2cf..d620f222c 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -54,7 +54,6 @@ use base qw(Bugzilla::Object Exporter);
     RemoveVotes CheckIfVotedConfirmed
     LogActivityEntry
     editable_bug_fields
-    SPECIAL_STATUS_WORKFLOW_ACTIONS
 );
 
 #####################################################################
@@ -234,13 +233,6 @@ use constant MAX_LINE_LENGTH => 254;
 # Used in _check_comment(). Gives the max length allowed for a comment.
 use constant MAX_COMMENT_LENGTH => 65535;
 
-use constant SPECIAL_STATUS_WORKFLOW_ACTIONS => qw(
-    none
-    duplicate
-    change_resolution
-    clearresolution
-);
-
 #####################################################################
 
 sub new {
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index abe1fe248..601ea52b2 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -151,6 +151,7 @@ use File::Basename;
     MAX_PRODUCT_SIZE
     MAX_MILESTONE_SIZE
     MAX_COMPONENT_SIZE
+    MAX_FIELD_VALUE_SIZE
     MAX_FREETEXT_LENGTH
 );
 
@@ -427,6 +428,9 @@ use constant MAX_MILESTONE_SIZE => 20;
 # The longest component name allowed.
 use constant MAX_COMPONENT_SIZE => 64;
 
+# The maximum length for values of <select> fields.
+use constant MAX_FIELD_VALUE_SIZE => 64;
+
 # Maximum length allowed for free text fields.
 use constant MAX_FREETEXT_LENGTH => 255;
 
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index bb516b8f4..5d8e5adc0 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -75,7 +75,6 @@ use base qw(Exporter Bugzilla::Object);
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
-use Bugzilla::Field::Choice;
 use Bugzilla::Util;
 
 ###############################
@@ -376,7 +375,8 @@ sub legal_values {
     my $self = shift;
 
     if (!defined $self->{'legal_values'}) {
-        my @values = Bugzilla::Field::Choice->get_all({ field => $self });
+        require Bugzilla::Field::Choice;
+        my @values = Bugzilla::Field::Choice->type($self)->get_all();
         $self->{'legal_values'} = \@values;
     }
     return $self->{'legal_values'};
diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm
index 6ef911941..7705258eb 100644
--- a/Bugzilla/Field/Choice.pm
+++ b/Bugzilla/Field/Choice.pm
@@ -26,6 +26,8 @@ use base qw(Bugzilla::Object);
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
+use Bugzilla::Field;
+use Bugzilla::Util qw(trim detaint_natural);
 
 use Scalar::Util qw(blessed);
 
@@ -42,104 +44,141 @@ use constant DB_COLUMNS => qw(
 use constant NAME_FIELD => 'value';
 use constant LIST_ORDER => 'sortkey, value';
 
-##########################################
-# Constructors and Database Manipulation #
-##########################################
+use constant REQUIRED_CREATE_FIELDS => qw(value);
 
-# When calling class methods, we aren't based on just one table,
-# so we need some slightly hacky way to do DB_TABLE. We do it by overriding
-# class methods and making them specify $_new_table. This is possible
-# because we're always called from inside Bugzilla::Field, so it always
-# has a $self to pass us which contains info about which field we're
-# attached to.
-#
-# This isn't thread safe, but Bugzilla isn't a threaded application.
-our $_new_table;
-our $_current_field;
-sub DB_TABLE {
-    my $invocant = shift;
-    if (blessed $invocant) {
-        return $invocant->field->name;
+use constant VALIDATORS => {
+    value   => \&_check_value,
+    sortkey => \&_check_sortkey,
+};
+
+use constant CLASS_MAP => {
+    bug_status => 'Bugzilla::Status',
+};
+
+#################
+# Class Factory #
+#################
+
+# Bugzilla::Field::Choice is actually an abstract base class. Every field
+# type has its own dynamically-generated class for its values. This allows
+# certain fields to have special types, like how bug_status's values
+# are Bugzilla::Status objects.
+
+sub type {
+    my ($class, $field) = @_;
+    my $field_obj = blessed $field ? $field : Bugzilla::Field->check($field);
+    my $field_name = $field_obj->name;
+
+    if ($class->CLASS_MAP->{$field_name}) {
+        return $class->CLASS_MAP->{$field_name};
     }
-    return $_new_table;
+
+    # For generic classes, we use a lowercase class name, so as
+    # not to interfere with any real subclasses we might make some day.
+    my $package = "Bugzilla::Field::Choice::$field_name";
+
+    # We check defined so that the package and the stored field are only
+    # created once globally (at least per request). We prefix it with
+    # field_ (instead of suffixing it) so that we don't somehow conflict
+    # with the names of custom fields.
+    if (!defined Bugzilla->request_cache->{"field_$package"}) {
+        eval <<EOC;
+            package $package;
+            use base qw(Bugzilla::Field::Choice);
+            use constant DB_TABLE => '$field_name';
+EOC
+        Bugzilla->request_cache->{"field_$package"} = $field_obj;
+    }
+
+    return $package;
 }
 
+################
+# Constructors #
+################
+
+# We just make new() enforce this, which should give developers 
+# the understanding that you can't use Bugzilla::Field::Choice
+# without calling type().
 sub new {
     my $class = shift;
-    my ($params) = @_;
-    _check_field_arg($params);
-    my $self = $class->SUPER::new($params);
-    _fix_return($self);
-    return $self;
+    if ($class eq 'Bugzilla::Field::Choice') {
+        ThrowCodeError('field_choice_must_use_type');
+    }
+    $class->SUPER::new(@_);
 }
 
-sub new_from_list {
-    my $class = shift;
-    my ($ids, $params) = @_;
-    _check_field_arg($params);
-    my $list = $class->SUPER::new_from_list(@_);
-    _fix_return($list);
-    return $list;
-}
+#########################
+# Database Manipulation #
+#########################
 
-sub match {
+# Our subclasses can take more arguments than we normally accept.
+# So, we override create() to remove arguments that aren't valid
+# columns. (Normally Bugzilla::Object dies if you pass arguments
+# that aren't valid columns.)
+sub create {
     my $class = shift;
     my ($params) = @_;
-    _check_field_arg($params);
-    my $results = $class->SUPER::match(@_);
-    _fix_return($results);
-    return $results;
+    foreach my $key (keys %$params) {
+        if (!grep {$_ eq $key} $class->DB_COLUMNS) {
+            delete $params->{$key};
+        }
+    }
+    return $class->SUPER::create(@_);
 }
 
-sub get_all {
-    my $class = shift;
-    _check_field_arg(@_);
-    my @list = $class->SUPER::get_all(@_);
-    _fix_return(\@list);
-    return @list;
-}
+#############
+# Accessors #
+#############
 
-sub _check_field_arg {
-    my ($params) = @_;
-    my ($class, undef, undef, $func) = caller(1);
-    if (!defined $params->{field}) {
-        ThrowCodeError('param_required',
-                       { function => "${class}::$func",
-                         param    => 'field' });
-    }
-    elsif (!blessed $params->{field}) {
-        ThrowCodeError('bad_arg', { function => "${class}::$func",
-                                    argument => 'field' });
-    }
-    $_new_table = $params->{field}->name;
-    $_current_field = $params->{field};
-    delete $params->{field};
+sub sortkey { return $_[0]->{'sortkey'}; }
+sub field {
+    my $invocant = shift;
+    my $class = ref $invocant || $invocant;
+    my $cache = Bugzilla->request_cache;
+    # This is just to make life easier for subclasses. Our auto-generated
+    # subclasses from type() already have this set.
+    $cache->{"field_$class"} ||=  
+        new Bugzilla::Field({ name => $class->DB_TABLE });
+    return $cache->{"field_$class"};
 }
 
-sub _fix_return {
-    my $retval = shift;
-    if (ref $retval eq 'ARRAY') {
-        foreach my $obj (@$retval) {
-            $obj->{field} = $_current_field;
-        }
-    }
-    elsif (defined $retval) {
-        $retval->{field} = $_current_field;
+##############
+# Validators #
+##############
+
+sub _check_value {
+    my ($invocant, $value) = @_;
+
+    my $field = $invocant->field;
+
+    $value = trim($value);
+    ThrowUserError('fieldvalue_undefined') if !defined $value || $value eq "";
+    ThrowUserError('fieldvalue_name_too_long', { value => $value })
+        if length($value) > MAX_FIELD_VALUE_SIZE;
+
+    my $exists = $invocant->type($field)->new({ name => $value });
+    if ($exists) {
+        ThrowUserError('fieldvalue_already_exists', 
+                       { field => $field, value => $value });
     }
 
-    # We do this just to avoid any possible bugs where $_new_table or
-    # $_current_field are set from a previous call. It also might save
-    # a little memory under mod_perl by releasing $_current_field explicitly.
-    undef $_new_table;
-    undef $_current_field;
+    return $value;
 }
 
-#############
-# Accessors #
-#############
+sub _check_sortkey {
+    my ($invocant, $value) = @_;
+    $value = trim($value);
+    return 0 if !$value;
+    # Store for the error message in case detaint_natural clears it.
+    my $orig_value = $value;
+    detaint_natural($value)
+        || ThrowUserError('fieldvalue_sortkey_invalid',
+                          { sortkey => $orig_value,
+                            field   => $invocant->field });
+    return $value;
+}
 
-sub sortkey { return $_[0]->{'sortkey'}; }
-sub field   { return $_[0]->{'field'};   }
 
 1;
 
@@ -153,27 +192,47 @@ Bugzilla::Field::Choice - A legal value for a <select>-type field.
 
  my $field = new Bugzilla::Field({name => 'bug_status'});
 
- my $choice = new Bugzilla::Field::Choice({ field => $field, id => 1 });
- my $choice = new Bugzilla::Field::Choice({ field => $field, name => 'NEW' });
+ my $choice = new Bugzilla::Field::Choice->type($field)->new(1);
 
- my $choices = Bugzilla::Field::Choice->new_from_list([1,2,3], 
-                                                      { field => $field});
- my $choices = Bugzilla::Field::Choice->get_all({ field => $field });
- my $choices = Bugzilla::Field::Choice->match({ sortkey => 10, 
-                                                field => $field });
+ my $choices = Bugzilla::Field::Choice->type($field)->new_from_list([1,2,3]);
+ my $choices = Bugzilla::Field::Choice->type($field)->get_all();
+ my $choices = Bugzilla::Field::Choice->type($field->match({ sortkey => 10 }); 
 
 =head1 DESCRIPTION
 
 This is an implementation of L<Bugzilla::Object>, but with a twist.
-All the class methods require that you specify an additional C<field>
-argument, which is a L<Bugzilla::Field> object that represents the
-field whose value this is.
+You can't call any class methods (such as C<new>, C<create>, etc.) 
+directly on C<Bugzilla::Field::Choice> itself. Instead, you have to
+call C<Bugzilla::Field::Choice-E<gt>type($field)> to get the class
+you're going to instantiate, and then you call the methods on that.
+
+We do that because each field has its own database table for its values, so
+each value type needs its own class.
 
-You can look at the L</SYNOPSIS> to see where this extra C<field>
-argument goes in each function.
+See the L</SYNOPSIS> for examples of how this works.
 
 =head1 METHODS
 
+=head2 Class Factory
+
+In object-oriented design, a "class factory" is a method that picks
+and returns the right class for you, based on an argument that you pass.
+
+=over
+
+=item C<type>
+
+Takes a single argument, which is either the name of a field from the
+C<fielddefs> table, or a L<Bugzilla::Field> object representing a field.
+
+Returns an appropriate subclass of C<Bugzilla::Field::Choice> that you
+can now call class methods on (like C<new>, C<create>, C<match>, etc.)
+
+B<NOTE>: YOU CANNOT CALL CLASS METHODS ON C<Bugzilla::Field::Choice>. You
+must call C<type> to get a class you can call methods on.
+
+=back
+
 =head2 Accessors
 
 These are in addition to the standard L<Bugzilla::Object> accessors.
diff --git a/Bugzilla/Status.pm b/Bugzilla/Status.pm
index f8b77331c..5f37974e7 100644
--- a/Bugzilla/Status.pm
+++ b/Bugzilla/Status.pm
@@ -22,13 +22,28 @@ use strict;
 
 package Bugzilla::Status;
 
-use base qw(Bugzilla::Object Exporter);
-@Bugzilla::Status::EXPORT = qw(BUG_STATE_OPEN is_open_state closed_bug_statuses);
+use Bugzilla::Error;
+
+use base qw(Bugzilla::Field::Choice Exporter);
+@Bugzilla::Status::EXPORT = qw(
+    BUG_STATE_OPEN
+    SPECIAL_STATUS_WORKFLOW_ACTIONS
+
+    is_open_state 
+    closed_bug_statuses
+);
 
 ################################
 #####   Initialization     #####
 ################################
 
+use constant SPECIAL_STATUS_WORKFLOW_ACTIONS => qw(
+    none
+    duplicate
+    change_resolution
+    clearresolution
+);
+
 use constant DB_TABLE => 'bug_status';
 
 use constant DB_COLUMNS => qw(
@@ -39,8 +54,24 @@ use constant DB_COLUMNS => qw(
     is_open
 );
 
-use constant NAME_FIELD => 'value';
-use constant LIST_ORDER => 'sortkey, value';
+sub VALIDATORS {
+    my $invocant = shift;
+    my $validators = $invocant->SUPER::VALIDATORS;
+    $validators->{is_open} = \&Bugzilla::Object::check_boolean;
+    $validators->{value} = \&_check_value;
+    return $validators;
+}
+
+#########################
+# Database Manipulation #
+#########################
+
+sub create {
+    my $class = shift;
+    my $self = $class->SUPER::create(@_);
+    add_missing_bug_status_transitions();
+    return $self;
+}
 
 ###############################
 #####     Accessors        ####
@@ -51,6 +82,22 @@ sub sortkey   { return $_[0]->{'sortkey'};  }
 sub is_active { return $_[0]->{'isactive'}; }
 sub is_open   { return $_[0]->{'is_open'};  }
 
+##############
+# Validators #
+##############
+
+sub _check_value {
+    my $invocant = shift;
+    my $value = $invocant->SUPER::_check_value(@_);
+
+    if (grep { lc($value) eq lc($_) } SPECIAL_STATUS_WORKFLOW_ACTIONS) {
+        ThrowUserError('fieldvalue_reserved_word',
+                       { field => $invocant->field, value => $value });
+    }
+    return $value;
+}
+
+
 ###############################
 #####       Methods        ####
 ###############################
diff --git a/editvalues.cgi b/editvalues.cgi
index ffb7ce576..4525774e1 100755
--- a/editvalues.cgi
+++ b/editvalues.cgi
@@ -29,7 +29,7 @@ use Bugzilla::Config qw(:admin);
 use Bugzilla::Token;
 use Bugzilla::Field;
 use Bugzilla::Bug;
-use Bugzilla::Status;
+use Bugzilla::Status qw(SPECIAL_STATUS_WORKFLOW_ACTIONS);
 
 # List of different tables that contain the changeable field values
 # (the old "enums.") Keep them in alphabetical order by their 
@@ -172,12 +172,8 @@ trick_taint($field);
 sub display_field_values {
     my $template = Bugzilla->template;
     my $field = $vars->{'field'}->name;
-    my $fieldvalues =
-      Bugzilla->dbh->selectall_arrayref("SELECT value AS name, sortkey"
-                                      . "  FROM $field ORDER BY sortkey, value",
-                                        {Slice =>{}});
 
-    $vars->{'values'} = $fieldvalues;
+    $vars->{'values'} = $vars->{'field'}->legal_values;
     $vars->{'default'} = Bugzilla->params->{$defaults{$field}} if defined $defaults{$field};
     $vars->{'static'} = $static{$field} if exists $static{$field};
 
@@ -212,53 +208,14 @@ if ($action eq 'add') {
 if ($action eq 'new') {
     check_token_data($token, 'add_field_value');
 
-    # Cleanups and validity checks
-    $value || ThrowUserError('fieldvalue_undefined');
-
-    if (length($value) > 60) {
-        ThrowUserError('fieldvalue_name_too_long',
-                       {'value' => $value});
-    }
-    # Need to store in case detaint_natural() clears the sortkey
-    my $stored_sortkey = $sortkey;
-    if (!detaint_natural($sortkey)) {
-        ThrowUserError('fieldvalue_sortkey_invalid',
-                       {'name' => $field,
-                        'sortkey' => $stored_sortkey});
-    }
-    if (ValueExists($field, $value)) {
-        ThrowUserError('fieldvalue_already_exists',
-                       {'field' => $field_obj,
-                        'value' => $value});
-    }
-    if ($field eq 'bug_status'
-        && (grep { lc($value) eq $_ } SPECIAL_STATUS_WORKFLOW_ACTIONS))
-    {
-        $vars->{'value'} = $value;
-        ThrowUserError('fieldvalue_reserved_word', $vars);
-    }
-
-    # Value is only used in a SELECT placeholder and through the HTML filter.
-    trick_taint($value);
-
-    # Add the new field value.
-    $dbh->do("INSERT INTO $field (value, sortkey) VALUES (?, ?)",
-             undef, ($value, $sortkey));
-
-    if ($field eq 'bug_status') {
-        unless ($cgi->param('is_open')) {
-            # The bug status is a closed state, but they are open by default.
-            $dbh->do('UPDATE bug_status SET is_open = 0 WHERE value = ?', undef, $value);
-        }
-        # Allow the transition from this new bug status to the one used
-        # by the 'duplicate_or_move_bug_status' parameter.
-        Bugzilla::Status::add_missing_bug_status_transitions();
-    }
+    my $created_value = Bugzilla::Field::Choice->type($field_obj)->create(
+        { value => $value, sortkey => $sortkey, 
+          is_open => scalar $cgi->param('is_open') });
 
     delete_token($token);
 
     $vars->{'message'} = 'field_value_created';
-    $vars->{'value'} = $value;
+    $vars->{'value'} = $created_value->name;
     display_field_values();
 }
 
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index b93b92efd..f85375849 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -145,6 +145,12 @@
     in the database for '[% username FILTER html %]', but your
     account source says that '[% extern_user FILTER html %]' has that ID.
 
+  [% ELSIF error == "field_choice_must_use_type" %]
+    When you call a class method on <code>Bugzilla::Field::Choice</code>,
+    you must call <code>Bugzilla::Field::Choice-&gt;type('some_field')</code>
+    to generate the right class (you can't call class methods directly
+    on Bugzilla::Field::Choice).
+
   [% ELSIF error == "field_type_mismatch" %]
     Cannot seem to handle <code>[% field FILTER html %]</code>
     and <code>[% type FILTER html %]</code> together.
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index c61a1f42a..bcb6b6630 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -432,7 +432,7 @@
   [% ELSIF error == "fieldvalue_already_exists" %]
     [% title = "Field Value Already Exists" %]
     The value '[% value FILTER html %]' already exists for the
-    '[%- field.description FILTER html %]' field.
+    [%+ field.description FILTER html %] field.
 
   [% ELSIF error == "fieldvalue_doesnt_exist" %]
     [% title = "Specified Field Value Does Not Exist" %]
@@ -450,8 +450,9 @@
 
   [% ELSIF error == "fieldvalue_name_too_long" %]
     [% title = "Field Value Is Too Long" %]
-    The value of a field is limited to 60 characters.
-    '[% value FILTER html %]' is too long ([% value.length %] characters).
+    The value of a field is limited to [% constants.FIELD_VALUE_MAX_SIZE %]
+    characters. '[% value FILTER html %]' is too long ([% value.length %]
+    characters).
 
   [% ELSIF error == "fieldvalue_not_editable" %]
     [% title = "Field Value Not Editable" %]
@@ -475,8 +476,9 @@
 
   [% ELSIF error == "fieldvalue_sortkey_invalid" %]
     [% title = "Invalid Field Value Sortkey" %]
-    The sortkey '[% sortkey FILTER html %]' for the '[% name FILTER html %]'
-    field is not a valid (positive) number.
+    The sortkey '[% sortkey FILTER html %]' for the 
+    [%+ field.description FILTER html %] field is not a valid 
+    (positive) number.
 
   [% ELSIF error == "fieldvalue_still_has_bugs" %]
     [% title = "You Cannot Delete This Field Value" %]
@@ -1178,13 +1180,13 @@
     is less than the minimum allowable value of '[% min_num FILTER html %]'.
 
   [% ELSIF error == "object_name_not_specified" %]
-    [% type = BLOCK %][% PROCESS object_name %][% END %]
+    [% type = BLOCK %][% INCLUDE object_name class = class %][% END %]
     [% title = BLOCK %][% type FILTER ucfirst FILTER html %] Not 
     Specified[% END %]
     You must select/enter a [% type FILTER html %].
 
   [% ELSIF error == "object_does_not_exist" %]
-    [% type = BLOCK %][% PROCESS object_name %][% END %]
+    [% type = BLOCK %][% INCLUDE object_name class = class %][% END %]
     [% title = BLOCK %]Invalid [% type FILTER ucfirst FILTER html %][% END %]
     There is no [% type FILTER html %] named '[% name FILTER html %]'
     [% IF product.defined %]
@@ -1672,5 +1674,10 @@
     milestone
   [% ELSIF class == "Bugzilla::Status" %]
     status
+  [% ELSIF class == "Bugzilla::Field" %]
+    field
+  [% ELSIF ( matches = class.match('^Bugzilla::Field::Choice::(.+)') ) %]
+    [% SET field_name = matches.0 %]
+    [% field_descs.$field_name %]
   [% END %]
 [% END %]