Bug 349741: Make Bugzilla::Bug able to do basic bug creation, and have post_bug.cgi use it

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

3 files changed, 238 insertions, 151 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 8c61a657b..642a71d3f 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -96,6 +96,34 @@ sub DB_COLUMNS {
     Bugzilla->custom_field_names;
 }
 
+use constant REQUIRED_CREATE_FIELDS => qw(
+    bug_severity
+    component
+    creation_ts
+    op_sys
+    priority
+    product
+    rep_platform
+    short_desc
+    version
+);
+
+# There are also other, more complex validators that are called
+# from run_create_validators.
+use constant VALIDATORS => {
+    alias          => \&_check_alias,
+    bug_file_loc   => \&_check_bug_file_loc,
+    bug_severity   => \&_check_bug_severity,
+    deadline       => \&_check_deadline,
+    estimated_time => \&_check_estimated_time,
+    op_sys         => \&_check_op_sys,
+    priority       => \&_check_priority,
+    remaining_time => \&_check_remaining_time,
+    rep_platform   => \&_check_rep_platform,
+    short_desc     => \&_check_short_desc,
+    status_whiteboard => \&_check_status_whiteboard,
+};
+
 # Used in LogActivityEntry(). Gives the max length of lines in the
 # activity table.
 use constant MAX_LINE_LENGTH => 254;
@@ -157,6 +185,79 @@ sub new {
     return $self;
 }
 
+# Docs for create() (there's no POD in this file yet, but we very
+# much need this documented right now):
+#
+# The same as Bugzilla::Object->create. Parameters are only required
+# if they say so below.
+#
+# Params:
+#
+# C<product>     - B<Required> The name of the product this bug is being
+#                  filed against.
+# C<component>   - B<Required> The name of the component this bug is being
+#                  filed against.
+#
+# C<bug_severity> - B<Required> The severity for the bug, a string.
+# C<creation_ts>  - B<Required> A SQL timestamp for when the bug was created.
+# C<short_desc>   - B<Required> A summary for the bug.
+# C<op_sys>       - B<Required> The OS the bug was found against.
+# C<priority>     - B<Required> The initial priority for the bug.
+# C<rep_platform> - B<Required> The platform the bug was found against.
+# C<version>      - B<Required> The version of the product the bug was found in.
+#
+# C<alias>        - An alias for this bug. Will be ignored if C<usebugaliases>
+#                   is off.
+# C<target_milestone> - When this bug is expected to be fixed.
+# C<status_whiteboard> - A string.
+# C<bug_status>   - The initial status of the bug, a string.
+# C<bug_file_loc> - The URL field.
+#
+# C<assigned_to> - The full login name of the user who the bug is
+#                  initially assigned to.
+# C<qa_contact>  - The full login name of the QA Contact for this bug. 
+#                  Will be ignored if C<useqacontact> is off.
+#
+# C<estimated_time> - For time-tracking. Will be ignored if 
+#                     C<timetrackinggroup> is not set, or if the current
+#                     user is not a member of the timetrackinggroup.
+# C<deadline>       - For time-tracking. Will be ignored for the same
+#                     reasons as C<estimated_time>.
+
+sub run_create_validators {
+    my $class  = shift;
+    my $params = shift;
+
+    my $product = _check_product($params->{product});
+    $params->{product_id} = $product->id;
+    delete $params->{product};
+
+    ($params->{bug_status}, $params->{everconfirmed})
+        = _check_bug_status($product, $params->{bug_status});
+
+    $params->{target_milestone} = _check_target_milestone($product,
+        $params->{target_milestone});
+
+    $params->{version} = _check_version($product, $params->{version});
+
+    my $component = _check_component($product, $params->{component});
+    $params->{component_id} = $component->id;
+    delete $params->{component};
+
+    $params->{assigned_to} = 
+        _check_assigned_to($component, $params->{assigned_to});
+    $params->{qa_contact} =
+        _check_qa_contact($component, $params->{qa_contact});
+    # Callers cannot set Reporter, currently.
+    $params->{reporter} = Bugzilla->user->id;
+
+    $params->{delta_ts} = $params->{creation_ts};
+    $params->{remaining_time} = $params->{estimated_time};
+
+    unshift @_, $params;
+    return $class->SUPER::run_create_validators(@_);
+}
+
 # This is the correct way to delete bugs from the DB.
 # No bug should be deleted from anywhere else except from here.
 #
@@ -233,12 +334,13 @@ sub remove_from_db {
 sub _check_alias {
    my ($alias) = @_;
    $alias = trim($alias);
-   ValidateBugAlias($alias) if (defined $alias && $alias ne '');
+   return undef if (!Bugzilla->params->{'usebugaliases'} || !$alias);
+   ValidateBugAlias($alias);
    return $alias;
 }
 
 sub _check_assigned_to {
-    my ($name, $component) = @_;
+    my ($component, $name) = @_;
     my $user = Bugzilla->user;
 
     $name = trim($name);
@@ -254,9 +356,6 @@ sub _check_assigned_to {
 
 sub _check_bug_file_loc {
     my ($url) = @_;
-    if (!defined $url) {
-        ThrowCodeError('undefined_field', { field => 'bug_file_loc' });
-    }
     # If bug_file_loc is "http://", the default, use an empty value instead.
     $url = '' if $url eq 'http://';
     return $url;
@@ -270,7 +369,7 @@ sub _check_bug_severity {
 }
 
 sub _check_bug_status {
-    my ($status, $product) = @_;
+    my ($product, $status) = @_;
     my $user = Bugzilla->user;
 
     my @valid_statuses = VALID_ENTRY_STATUS;
@@ -293,7 +392,7 @@ sub _check_bug_status {
     shift @valid_statuses if !$product->votes_to_confirm;
 
     check_field('bug_status', $status, \@valid_statuses);
-    return $status;
+    return ($status, $status eq 'UNCONFIRMED' ? 0 : 1);
 }
 
 sub _check_cc {
@@ -331,7 +430,7 @@ sub _check_comment {
 }
 
 sub _check_component {
-    my ($name, $product) = @_;
+    my ($product, $name) = @_;
     $name = trim($name);
     $name || ThrowUserError("require_component");
     my $obj = Bugzilla::Component::check_component($product, $name);
@@ -341,6 +440,18 @@ sub _check_component {
     return $obj;
 }
 
+sub _check_deadline {
+    my ($date) = @_;
+    $date = trim($date);
+    my $tt_group = Bugzilla->params->{"timetrackinggroup"};
+    return undef unless $date && $tt_group 
+                        && Bugzilla->user->in_group($tt_group);
+    validate_date($date)
+        || ThrowUserError('illegal_date', { date   => $date,
+                                            format => 'YYYY-MM-DD' });
+    return $date;
+}
+
 # Takes two comma/space-separated strings and returns arrayrefs
 # of valid bug IDs.
 sub _check_dependencies {
@@ -370,6 +481,10 @@ sub _check_dependencies {
     return ($deps{'dependson'}, $deps{'blocked'});
 }
 
+sub _check_estimated_time {
+    return _check_time($_[0], 'estimated_time');
+}
+
 sub _check_keywords {
     my ($keyword_string) = @_;
     $keyword_string = trim($keyword_string);
@@ -417,6 +532,10 @@ sub _check_priority {
     return $priority;
 }
 
+sub _check_remaining_time {
+    return _check_time($_[0], 'remaining_time');
+}
+
 sub _check_rep_platform {
     my ($platform) = @_;
     $platform = trim($platform);
@@ -435,6 +554,8 @@ sub _check_short_desc {
     return $short_desc;
 }
 
+sub _check_status_whiteboard { return defined $_[0] ? $_[0] : ''; }
+
 # Unlike other checkers, this one doesn't return anything.
 sub _check_strict_isolation {
     my ($product, $cc_ids, $assignee_id, $qa_contact_id) = @_;
@@ -466,10 +587,30 @@ sub _check_strict_isolation {
     }
 }
 
+sub _check_target_milestone {
+    my ($product, $target) = @_;
+    $target = trim($target);
+    $target = $product->default_milestone if !defined $target;
+    check_field('target_milestone', $target,
+            [map($_->name, @{$product->milestones})]);
+    return $target;
+}
+
+sub _check_time {
+    my ($time, $field) = @_;
+    my $tt_group = Bugzilla->params->{"timetrackinggroup"};
+    return 0 unless $tt_group && Bugzilla->user->in_group($tt_group);
+    $time = trim($time) || 0;
+    ValidateTime($time, $field);
+    return $time;
+}
+
 sub _check_qa_contact {
-    my ($name, $component) = @_;
+    my ($component, $name) = @_;
     my $user = Bugzilla->user;
 
+    return undef unless Bugzilla->params->{'useqacontact'};
+
     $name = trim($name);
 
     my $id;
@@ -483,6 +624,13 @@ sub _check_qa_contact {
     return $id;
 }
 
+sub _check_version {
+    my ($product, $version) = @_;
+    $version = trim($version);
+    check_field('version', $version, [map($_->name, @{$product->versions})]);
+    return $version;
+}
+
 
 #####################################################################
 # Class Accessors
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 219658a92..833cdcd2f 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -123,16 +123,29 @@ 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) = $class->run_create_validators($params);
+
+    my $qmarks = '?,' x @$values;
+    chop($qmarks);
+    my $table = $class->DB_TABLE;
+    $dbh->do("INSERT INTO $table (" . join(', ', @$field_names) 
+             . ") VALUES ($qmarks)", undef, @$values);
+    my $id = $dbh->bz_last_key($table, $class->ID_FIELD);
+
+    return $class->new($id);
+}
+
+sub run_create_validators {
+    my ($class, $params) = @_;
+
+    my $validators = $class->VALIDATORS;
+
     my (@field_names, @values);
     # We do the sort just to make sure that validation always
     # happens in a consistent order.
@@ -144,18 +157,14 @@ sub create {
         else {
             $value = $params->{$field};
         }
-        trick_taint($value);
+        # We want people to be able to explicitly set fields to NULL,
+        # and that means they can be set to undef.
+        trick_taint($value) if defined $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, $class->ID_FIELD);
-
-    return $class->new($id);
+    return (\@field_names, \@values);
 }
 
 sub get_all {
@@ -307,6 +316,20 @@ Notes:       In order for this function to work in your subclass,
              type in the database. Your subclass also must
              define L</REQUIRED_CREATE_FIELDS> and L</VALIDATORS>.
 
+=item C<run_create_validators($params)>
+
+Description: Runs the validation of input parameters for L</create>.
+             This subroutine exists so that it can be overridden
+             by subclasses who need to do special validations
+             of their input parameters. This method is B<only> called
+             by L</create>.
+
+Params:      The same as L</create>.
+
+Returns:     Two arrayrefs. The first is an array of database field names.
+             The second is an untainted array of values that should go 
+             into those fields (in the same order).
+
 =item C<get_all>
 
  Description: Returns all objects in this table from the database.
diff --git a/post_bug.cgi b/post_bug.cgi
index 95621c3ed..2257d543f 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -139,12 +139,6 @@ if (defined $cgi->param('maketemplate')) {
 
 umask 0;
 
-# Some sanity checking
-my $component = 
-    Bugzilla::Bug::_check_component(scalar $cgi->param('component'), $product);
-$cgi->param('short_desc', 
-            Bugzilla::Bug::_check_short_desc($cgi->param('short_desc')));
-
 # This has to go somewhere after 'maketemplate' 
 # or it breaks bookmarks with no comments. 
 $comment = Bugzilla::Bug::_check_comment($cgi->param('comment'));
@@ -152,81 +146,19 @@ $comment = Bugzilla::Bug::_check_comment($cgi->param('comment'));
 # OK except for the fact that it causes e-mail to be suppressed.
 $comment = $comment ? $comment : " ";
 
-$cgi->param('bug_file_loc', 
-            Bugzilla::Bug::_check_bug_file_loc($cgi->param('bug_file_loc')));
-$cgi->param('assigned_to', 
-            Bugzilla::Bug::_check_assigned_to(scalar $cgi->param('assigned_to'),
-                                              $component));
-
-
-my @enter_bug_field_names = map {$_->name} Bugzilla->get_fields({ custom => 1,
-    obsolete => 0, enter_bug => 1});
-
-my @bug_fields = ("version", "rep_platform",
-                  "bug_severity", "priority", "op_sys", "assigned_to",
-                  "bug_status", "everconfirmed", "bug_file_loc", "short_desc",
-                  "target_milestone", "status_whiteboard",
-                  @enter_bug_field_names);
-
-if (Bugzilla->params->{"usebugaliases"}) {
-    my $alias = Bugzilla::Bug::_check_alias($cgi->param('alias'));
-    if ($alias) {
-        $cgi->param('alias', $alias);
-        push(@bug_fields, "alias");
-    }
-}
-
-if (Bugzilla->params->{"useqacontact"}) {
-    my $qa_contact = Bugzilla::Bug::_check_qa_contact(
-        scalar $cgi->param('qa_contact'), $component);
-    if ($qa_contact) {
-        $cgi->param('qa_contact', $qa_contact);
-        push(@bug_fields, "qa_contact");
-    }
-}
-
-$cgi->param('bug_status', Bugzilla::Bug::_check_bug_status(
-                scalar $cgi->param('bug_status'), $product));
-
-if (!defined $cgi->param('target_milestone')) {
-    $cgi->param(-name => 'target_milestone', -value => $product->default_milestone);
-}
-
-# Some more sanity checking
-$cgi->param(-name => 'priority', -value => Bugzilla::Bug::_check_priority(
-    $cgi->param('priority')));
-$cgi->param(-name  => 'rep_platform', 
-    -value => Bugzilla::Bug::_check_rep_platform($cgi->param('rep_platform')));
-$cgi->param(-name => 'bug_severity', 
-    -value => Bugzilla::Bug::_check_bug_severity($cgi->param('bug_severity')));
-$cgi->param(-name => 'op_sys', -value => Bugzilla::Bug::_check_op_sys(
-    $cgi->param('op_sys')));
- 
-check_field('version',      scalar $cgi->param('version'),
-            [map($_->name, @{$product->versions})]);
-check_field('target_milestone', scalar $cgi->param('target_milestone'),
-            [map($_->name, @{$product->milestones})]);
-
-my $everconfirmed = ($cgi->param('bug_status') eq 'UNCONFIRMED') ? 0 : 1;
-$cgi->param(-name => 'everconfirmed', -value => $everconfirmed);
-
-my @used_fields;
-foreach my $field (@bug_fields) {
-    if (defined $cgi->param($field)) {
-        push (@used_fields, $field);
-    }
-}
-
-$cgi->param(-name => 'product_id', -value => $product->id);
-push(@used_fields, "product_id");
-$cgi->param(-name => 'component_id', -value => $component->id);
-push(@used_fields, "component_id");
-
 my $cc_ids = Bugzilla::Bug::_check_cc([$cgi->param('cc')]);
 my @keyword_ids = @{Bugzilla::Bug::_check_keywords($cgi->param('keywords'))};
 
-Bugzilla::Bug::_check_strict_isolation($product, $cc_ids, 
-    $cgi->param('assigned_to'), $cgi->param('qa_contact'));
+# XXX These checks are only here until strict_isolation can move fully
+# into Bugzilla::Bug.
+my $component = Bugzilla::Bug::_check_component($product, 
+    $cgi->param('component'));
+my $assigned_to_id = Bugzilla::Bug::_check_assigned_to($component,
+    $cgi->param('assigned_to'));
+my $qa_contact_id = Bugzilla::Bug::_check_qa_contact($component,
+    $cgi->param('qa_contact'));
+Bugzilla::Bug::_check_strict_isolation($product, $cc_ids, $assigned_to_id, 
+    $qa_contact_id);
 
 my ($depends_on_ids, $blocks_ids) = Bugzilla::Bug::_check_dependencies(
     scalar $cgi->param('dependson'), scalar $cgi->param('blocked'));
@@ -234,54 +166,6 @@ my ($depends_on_ids, $blocks_ids) = Bugzilla::Bug::_check_dependencies(
 # get current time
 my $timestamp = $dbh->selectrow_array(q{SELECT NOW()});
 
-# Build up SQL string to add bug.
-# creation_ts will only be set when all other fields are defined.
-
-my @fields_values;
-
-foreach my $field (@used_fields) {
-    my $value = $cgi->param($field);
-    trick_taint($value);
-    push (@fields_values, $value);
-}
-
-my $sql_used_fields = join(", ", @used_fields);
-my $sql_placeholders = "?, " x scalar(@used_fields);
-
-my $query = qq{INSERT INTO bugs ($sql_used_fields, reporter, delta_ts,
-                                 estimated_time, remaining_time, deadline)
-               VALUES ($sql_placeholders ?, ?, ?, ?, ?)};
-
-push (@fields_values, $user->id);
-push (@fields_values, $timestamp);
-
-my $est_time = 0;
-my $deadline;
-
-# Time Tracking
-if (UserInGroup(Bugzilla->params->{"timetrackinggroup"}) &&
-    defined $cgi->param('estimated_time')) {
-
-    $est_time = $cgi->param('estimated_time');
-    Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
-    trick_taint($est_time);
-
-}
-
-push (@fields_values, $est_time, $est_time);
-
-if ( UserInGroup(Bugzilla->params->{"timetrackinggroup"})
-     && $cgi->param('deadline') ) 
-{
-    validate_date($cgi->param('deadline'))
-      || ThrowUserError('illegal_date', {date => $cgi->param('deadline'),
-                                         format => 'YYYY-MM-DD'});
-    $deadline = $cgi->param('deadline');
-    trick_taint($deadline);
-}
-
-push (@fields_values, $deadline);
-
 # Groups
 my @groupstoadd = ();
 my $sth_othercontrol = $dbh->prepare(q{SELECT othercontrol
@@ -339,17 +223,50 @@ foreach my $group (@$groups) {
     }
 }
 
+my @bug_fields = map {$_->name} Bugzilla->get_fields(
+    { custom => 1, obsolete => 0, enter_bug => 1});
+push(@bug_fields, qw(
+    product
+    component
+
+    assigned_to
+    qa_contact
+
+    alias
+    bug_file_loc
+    bug_severity
+    bug_status
+    short_desc
+    op_sys
+    priority
+    rep_platform
+    version
+    target_milestone
+    status_whiteboard
+
+    estimated_time
+    deadline
+));
+my %bug_params;
+foreach my $field (@bug_fields) {
+    $bug_params{$field} = $cgi->param($field);
+}
+$bug_params{'creation_ts'} = $timestamp;
+
 # Add the bug report to the DB.
 $dbh->bz_lock_tables('bugs WRITE', 'bug_group_map WRITE', 'longdescs WRITE',
                      'cc WRITE', 'keywords WRITE', 'dependencies WRITE',
                      'bugs_activity WRITE', 'groups READ',
                      'user_group_map READ', 'group_group_map READ',
-                     'keyworddefs READ', 'fielddefs READ');
+                     'keyworddefs READ', 'fielddefs READ',
+                     'products READ', 'versions READ', 'milestones READ',
+                     'components READ', 'profiles READ', 'bug_severity READ',
+                     'op_sys READ', 'priority READ', 'rep_platform READ');
 
-$dbh->do($query, undef, @fields_values);
+my $bug = Bugzilla::Bug->create(\%bug_params);
 
 # Get the bug ID back.
-my $id = $dbh->bz_last_key('bugs', 'bug_id');
+my $id = $bug->bug_id;
 
 # Add the group restrictions
 my $sth_addgroup = $dbh->prepare(q{
@@ -420,7 +337,6 @@ $dbh->do("UPDATE bugs SET creation_ts = ? WHERE bug_id = ?",
 
 $dbh->bz_unlock_tables();
 
-my $bug = new Bugzilla::Bug($id);
 # We don't have to check if the user can see the bug, because a user filing
 # a bug can always see it. You can't change reporter_accessible until
 # after the bug is filed.