Bug 590334: Change Bug.pm to use the comment object (Bugzilla::Comment)

when creating or updating bug comment
r=mkanat, a=mkanat

3 files changed, 235 insertions, 158 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 327e55ccc..70f3f607d 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -26,6 +26,7 @@
 #                 Frédéric Buclin <LpSolit@gmail.com>
 #                 Lance Larsh <lance.larsh@oracle.com>
 #                 Elliotte Martin <elliotte_martin@yahoo.com>
+#                 Christian Legnitto <clegnitto@mozilla.com>
 
 package Bugzilla::Bug;
 
@@ -125,11 +126,11 @@ sub VALIDATORS {
         bug_status     => \&_check_bug_status,
         cc             => \&_check_cc,
         comment        => \&_check_comment,
-        commentprivacy => \&_check_commentprivacy,
         component      => \&_check_component,
+        creation_ts    => \&_check_creation_ts,
         deadline       => \&_check_deadline,
         dup_id         => \&_check_dup_id,
-        estimated_time => \&_check_estimated_time,
+        estimated_time => \&Bugzilla::Object::check_time,
         everconfirmed  => \&Bugzilla::Object::check_boolean,
         groups         => \&_check_groups,
         keywords       => \&_check_keywords,
@@ -137,7 +138,7 @@ sub VALIDATORS {
         priority       => \&_check_priority,
         product        => \&_check_product,
         qa_contact     => \&_check_qa_contact,
-        remaining_time => \&_check_remaining_time,
+        remaining_time => \&Bugzilla::Object::check_time,
         rep_platform   => \&_check_select_field,
         resolution     => \&_check_resolution,
         short_desc     => \&_check_short_desc,
@@ -185,6 +186,7 @@ sub VALIDATOR_DEPENDENCIES {
         assigned_to      => ['component'],
         bug_status       => ['product', 'comment', 'target_milestone'],
         cc               => ['component'],
+        comment          => ['creation_ts'],
         component        => ['product'],
         dup_id           => ['bug_status', 'resolution'],
         groups           => ['product'],
@@ -246,16 +248,6 @@ sub DATE_COLUMNS {
     return map { $_->name } @fields;
 }
 
-# This is used by add_comment to know what we validate before putting in
-# the DB.
-use constant UPDATE_COMMENT_COLUMNS => qw(
-    thetext
-    work_time
-    type
-    extra_data
-    isprivate
-);
-
 # Used in LogActivityEntry(). Gives the max length of lines in the
 # activity table.
 use constant MAX_LINE_LENGTH => 254;
@@ -292,6 +284,9 @@ use constant REQUIRED_FIELD_MAP => {
     component_id => 'component',
 };
 
+# Creation timestamp is here because it needs to be validated
+# but it can be NULL in the database (see comments in create above)
+#
 # Target Milestone is here because it has a default that the validator
 # creates (product.defaultmilestone) that is different from the database
 # default.
@@ -305,7 +300,7 @@ use constant REQUIRED_FIELD_MAP => {
 #
 # Groups are in a separate table, but must always be validated so that
 # mandatory groups get set on bugs.
-use constant EXTRA_REQUIRED_FIELDS => qw(target_milestone cc qa_contact groups);
+use constant EXTRA_REQUIRED_FIELDS => qw(creation_ts target_milestone cc qa_contact groups);
 
 #####################################################################
 
@@ -615,14 +610,12 @@ sub create {
 
     # These are not a fields in the bugs table, so we don't pass them to
     # insert_create_data.
-    my $cc_ids     = delete $params->{cc};
-    my $groups     = delete $params->{groups};
-    my $depends_on = delete $params->{dependson};
-    my $blocked    = delete $params->{blocked};
-    my $keywords   = delete $params->{keywords};
-    my ($comment, $privacy) = ($params->{comment}, $params->{commentprivacy});
-    delete $params->{comment};
-    delete $params->{commentprivacy};
+    my $cc_ids           = delete $params->{cc};
+    my $groups           = delete $params->{groups};
+    my $depends_on       = delete $params->{dependson};
+    my $blocked          = delete $params->{blocked};
+    my $keywords         = delete $params->{keywords};
+    my $creation_comment = delete $params->{comment};
 
     # We don't want the bug to appear in the system until it's correctly
     # protected by groups.
@@ -686,20 +679,14 @@ sub create {
         }
     }
 
-    # And insert the comment. We always insert a comment on bug creation,
+    # Comment #0 handling...
+
+    # We now have a bug id so we can fill this out
+    $creation_comment->{'bug_id'} = $bug->id;
+
+    # Insert the comment. We always insert a comment on bug creation,
     # but sometimes it's blank.
-    my @columns = qw(bug_id who bug_when thetext);
-    my @values  = ($bug->bug_id, $bug->{reporter_id}, $timestamp, $comment);
-    # We don't include the "isprivate" column unless it was specified. 
-    # This allows it to fall back to its database default.
-    if (defined $privacy) {
-        push(@columns, 'isprivate');
-        push(@values, $privacy);
-    }
-    my $qmarks = "?," x @columns;
-    chop($qmarks);
-    $dbh->do('INSERT INTO longdescs (' . join(',', @columns)  . ")
-                   VALUES ($qmarks)", undef, @values);
+    Bugzilla::Comment->insert_create_data($creation_comment);
 
     Bugzilla::Hook::process('bug_end_of_create', { bug => $bug,
                                                    timestamp => $timestamp,
@@ -726,8 +713,6 @@ sub run_create_validators {
 
     # Callers cannot set reporter, creation_ts, or delta_ts.
     $params->{reporter} = $class->_check_reporter();
-    $params->{creation_ts} = 
-        Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
     $params->{delta_ts} = $params->{creation_ts};
 
     if ($params->{estimated_time}) {
@@ -903,26 +888,21 @@ sub update {
 
     # Comments
     foreach my $comment (@{$self->{added_comments} || []}) {
-        my $columns = join(',', keys %$comment);
-        my @values  = values %$comment;
-        my $qmarks  = join(',', ('?') x @values);
-        $dbh->do("INSERT INTO longdescs (bug_id, who, bug_when, $columns)
-                       VALUES (?,?,?,$qmarks)", undef,
-                 $self->bug_id, Bugzilla->user->id, $delta_ts, @values);
+        $comment = Bugzilla::Comment->insert_create_data($comment);
         if ($comment->{work_time}) {
             LogActivityEntry($self->id, "work_time", "", $comment->{work_time},
                 Bugzilla->user->id, $delta_ts);
         }
     }
-   
+
     # Comment Privacy 
-    foreach my $comment_id (keys %{$self->{comment_isprivate} || {}}) {
-        $dbh->do("UPDATE longdescs SET isprivate = ? WHERE comment_id = ?",
-                 undef, $self->{comment_isprivate}->{$comment_id}, $comment_id);
+    foreach my $comment (@{$self->{comment_isprivate} || []}) {
+        $comment->update();
+        
         my ($from, $to) 
-            = $self->{comment_isprivate}->{$comment_id} ? (0, 1) : (1, 0);
+            = $comment->is_private ? (0, 1) : (1, 0);
         LogActivityEntry($self->id, "longdescs.isprivate", $from, $to, 
-                         Bugzilla->user->id, $delta_ts, $comment_id);
+                         Bugzilla->user->id, $delta_ts, $comment->id);
     }
 
     # Insert the values into the multiselect value tables
@@ -1417,33 +1397,43 @@ sub _check_cc {
 }
 
 sub _check_comment {
-    my ($invocant, $comment) = @_;
+    my ($invocant, $comment_txt, undef, $params) = @_;
 
-    $comment = '' unless defined $comment;
+    # Comment can be empty. We should force it to be empty if the text is undef
+    if (!defined $comment_txt) {
+        $comment_txt = '';
+    }
 
-    # Remove any trailing whitespace. Leading whitespace could be
-    # a valid part of the comment.
-    $comment =~ s/\s*$//s;
-    $comment =~ s/\r\n?/\n/g; # Get rid of \r.
+    # Load up some data
+    my $isprivate = $params->{commentprivacy};
+    my $timestamp = $params->{creation_ts};
 
-    ThrowUserError('comment_too_long') if length($comment) > MAX_COMMENT_LENGTH;
-    return $comment;
-}
+    # Create the new comment so we can check it
+    my $comment = {
+        thetext  => $comment_txt,
+        bug_when => $timestamp,
+    };
 
-sub _check_commentprivacy {
-    my ($invocant, $comment_privacy) = @_;
-    if ($comment_privacy && !Bugzilla->user->is_insider) {
-        ThrowUserError('user_not_insider');
+    # We don't include the "isprivate" column unless it was specified. 
+    # This allows it to fall back to its database default.
+    if (defined $isprivate) {
+        $comment->{isprivate} = $isprivate;
     }
-    return $comment_privacy ? 1 : 0;
-}
 
-sub _check_comment_type {
-    my ($invocant, $type) = @_;
-    detaint_natural($type)
-      || ThrowCodeError('bad_arg', { argument => 'type', 
-                                     function => caller });
-    return $type;
+    # Don't need this anymore as it is now in the comment hash
+    delete $params->{commentprivacy};
+
+    # Validate comment. We have to do this special as a comment normally
+    # requires a bug to be already created. For a new bug, the first comment
+    # obviously can't get the bug if the bug is created after this
+    # (see bug 590334)
+    Bugzilla::Comment->check_required_create_fields($comment);
+    $comment = Bugzilla::Comment->run_create_validators($comment,
+                                                        { skip => ['bug_id'] }
+    );
+
+    return $comment; 
+
 }
 
 sub _check_component {
@@ -1456,6 +1446,10 @@ sub _check_component {
     return $obj;
 }
 
+sub _check_creation_ts {
+    return Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
+}
+
 sub _check_deadline {
     my ($invocant, $date) = @_;
     
@@ -1605,10 +1599,6 @@ sub _check_dup_id {
     return $dupe_of;
 }
 
-sub _check_estimated_time {
-    return $_[0]->_check_time($_[1], 'estimated_time');
-}
-
 sub _check_groups {
     my ($invocant, $group_names, undef, $params) = @_;
     my $product = blessed($invocant) ? $invocant->product_obj 
@@ -1728,10 +1718,6 @@ sub _check_qa_contact {
     return $id || undef;
 }
 
-sub _check_remaining_time {
-    return $_[0]->_check_time($_[1], 'remaining_time');
-}
-
 sub _check_reporter {
     my $invocant = shift;
     my $reporter;
@@ -1886,20 +1872,6 @@ sub _check_target_milestone {
     return $object->name;
 }
 
-sub _check_time {
-    my ($invocant, $time, $field) = @_;
-
-    my $current = 0;
-    if (ref $invocant && $field ne 'work_time') {
-        $current = $invocant->$field;
-    }
-    return $current unless Bugzilla->user->is_timetracker;
-    
-    $time = trim($time) || 0;
-    ValidateTime($time, $field);
-    return $time;
-}
-
 sub _check_version {
     my ($invocant, $version, undef, $params) = @_;
     $version = trim($version);
@@ -1910,10 +1882,6 @@ sub _check_version {
     return $object->name;
 }
 
-sub _check_work_time {
-    return $_[0]->_check_time($_[1], 'work_time');
-}
-
 # Custom Field Validators
 
 sub _check_field_is_mandatory {
@@ -2265,8 +2233,9 @@ sub set_comment_is_private {
 
     $isprivate = $isprivate ? 1 : 0;
     if ($isprivate != $comment->is_private) {
-        $self->{comment_isprivate} ||= {};
-        $self->{comment_isprivate}->{$comment_id} = $isprivate;
+        $self->{comment_isprivate} ||= [];
+        $comment->set_is_private($isprivate);
+        push @{$self->{comment_isprivate}}, $comment;
     }
 }
 sub set_component  {
@@ -2636,23 +2605,22 @@ sub remove_cc {
 sub add_comment {
     my ($self, $comment, $params) = @_;
 
-    $comment = $self->_check_comment($comment);
-
     $params ||= {};
-    $params->{work_time} = $self->_check_work_time($params->{work_time});
-    if (exists $params->{type}) {
-        $params->{type} = $self->_check_comment_type($params->{type});
-    }
-    if (exists $params->{isprivate}) {
-        $params->{isprivate} = 
-            $self->_check_commentprivacy($params->{isprivate});
-    }
-    # XXX We really should check extra_data, too.
 
+    # This makes it so we won't create new comments when there is nothing
+    # to add 
     if ($comment eq '' && !($params->{type} || abs($params->{work_time}))) {
         return;
     }
 
+    # Fill out info that doesn't change and callers may not pass in
+    $params->{'bug_id'}  = $self;
+    $params->{'thetext'} = $comment;
+
+    # Validate all the entered data
+    Bugzilla::Comment->check_required_create_fields($params);
+    $params = Bugzilla::Comment->run_create_validators($params);
+
     # If the user has explicitly set remaining_time, this will be overridden
     # later in set_all. But if they haven't, this keeps remaining_time
     # up-to-date.
@@ -2660,23 +2628,9 @@ sub add_comment {
         $self->set_remaining_time(max($self->remaining_time - $params->{work_time}, 0));
     }
 
-    # So we really want to comment. Make sure we are allowed to do so.
-    my $privs;
-    $self->check_can_change_field('longdesc', 0, 1, \$privs)
-        || ThrowUserError('illegal_change', { field => 'longdesc', privs => $privs });
-
     $self->{added_comments} ||= [];
-    my $add_comment = dclone($params);
-    $add_comment->{thetext} = $comment;
 
-    # We only want to trick_taint fields that we know about--we don't
-    # want to accidentally let somebody set some field that's not OK
-    # to set!
-    foreach my $field (UPDATE_COMMENT_COLUMNS) {
-        trick_taint($add_comment->{$field}) if defined $add_comment->{$field};
-    }
-
-    push(@{$self->{added_comments}}, $add_comment);
+    push(@{$self->{added_comments}}, $params);
 }
 
 # There was a lot of duplicate code when I wrote this as three separate
@@ -3638,29 +3592,6 @@ sub EmitDependList {
     return $list_ref;
 }
 
-sub ValidateTime {
-    my ($time, $field) = @_;
-
-    # regexp verifies one or more digits, optionally followed by a period and
-    # zero or more digits, OR we have a period followed by one or more digits
-    # (allow negatives, though, so people can back out errors in time reporting)
-    if ($time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) {
-        ThrowUserError("number_not_numeric",
-                       {field => "$field", num => "$time"});
-    }
-
-    # Only the "work_time" field is allowed to contain a negative value.
-    if ( ($time < 0) && ($field ne "work_time") ) {
-        ThrowUserError("number_too_small",
-                       {field => "$field", num => "$time", min_num => "0"});
-    }
-
-    if ($time > 99999.99) {
-        ThrowUserError("number_too_large",
-                       {field => "$field", num => "$time", max_num => "99999.99"});
-    }
-}
-
 # Get the activity of a bug, starting from $starttime (if given).
 # This routine assumes Bugzilla::Bug->check has been previously called.
 sub GetBugActivity {
diff --git a/Bugzilla/Comment.pm b/Bugzilla/Comment.pm
index b4f94e213..e8bdb4e89 100644
--- a/Bugzilla/Comment.pm
+++ b/Bugzilla/Comment.pm
@@ -17,6 +17,7 @@
 # All rights reserved.
 #
 # Contributor(s): James Robson <arbingersys@gmail.com> 
+#                 Christian Legnitto <clegnitto@mozilla.com> 
 
 use strict;
 
@@ -50,6 +51,7 @@ use constant DB_COLUMNS => qw(
 );
 
 use constant UPDATE_COLUMNS => qw(
+    isprivate
     type
     extra_data
 );
@@ -62,12 +64,21 @@ use constant ID_FIELD => 'comment_id';
 use constant LIST_ORDER => 'bug_when, comment_id';
 
 use constant VALIDATORS => {
-    extra_data => \&_check_extra_data,
-    type => \&_check_type,
+    bug_id      => \&_check_bug_id,
+    who         => \&_check_who,
+    bug_when    => \&_check_bug_when,
+    work_time   => \&_check_work_time,
+    thetext     => \&_check_thetext,
+    isprivate   => \&_check_isprivate,
+    extra_data  => \&_check_extra_data,
+    type        => \&_check_type,
 };
 
 use constant VALIDATOR_DEPENDENCIES => {
     extra_data => ['type'],
+    bug_id     => ['who'],
+    work_time  => ['who'],
+    isprivate  => ['who'],
 };
 
 #########################
@@ -157,12 +168,9 @@ sub body_full {
 # Mutators #
 ############
 
-sub set_extra_data { $_[0]->set('extra_data', $_[1]); }
-
-sub set_type {
-    my ($self, $type) = @_;
-    $self->set('type', $type);
-}
+sub set_is_private  { $_[0]->set('isprivate',  $_[1]); }
+sub set_type        { $_[0]->set('type',       $_[1]); }
+sub set_extra_data  { $_[0]->set('extra_data', $_[1]); }
 
 ##############
 # Validators #
@@ -209,6 +217,87 @@ sub _check_type {
     return $type;
 }
 
+sub _check_bug_id {
+    my ($invocant, $bug_id) = @_;
+
+    ThrowCodeError('param_required', {function => 'Bugzilla::Comment->create',
+                                      param => 'bug_id'}) unless $bug_id;
+
+    my $bug;
+    if (blessed $bug_id) {
+        # We got a bug object passed in, use it
+        $bug = $bug_id;
+        $bug->check_is_visible;
+    }
+    else {
+        # We got a bug id passed in, check it and get the bug object
+        $bug = Bugzilla::Bug->check({ id => $bug_id });
+    }
+
+    # Make sure the user can edit the product
+    Bugzilla->user->can_edit_product($bug->{product_id});
+
+    # Make sure the user can comment
+    my $privs;
+    $bug->check_can_change_field('longdesc', 0, 1, \$privs)
+        || ThrowUserError('illegal_change', 
+                          { field => 'longdesc', privs => $privs });
+    return $bug->id;
+}
+
+sub _check_who {
+    my ($invocant, $who) = @_;
+    Bugzilla->login(LOGIN_REQUIRED);
+    return Bugzilla->user->id;
+}
+
+sub _check_bug_when {
+    my ($invocant, $when) = @_;
+
+    # Make sure the timestamp is defined, default to a timestamp from the db
+    if (!defined $when) {
+        $when = Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
+    }
+
+    # Make sure the timestamp parses
+    if (!datetime_from($when)) {
+        ThrowCodeError('invalid_timestamp', { timestamp => $when });
+    }
+
+    return $when;
+}
+
+sub _check_work_time {
+    my ($invocant, $value_in, $field, $params) = @_;
+
+    # Call down to Bugzilla::Object, letting it know negative
+    # values are ok
+    return $invocant->check_time( $value_in, $field, $params, 1);
+}
+
+sub _check_thetext {
+    my ($invocant, $thetext) = @_;
+
+    ThrowCodeError('param_required',{function => 'Bugzilla::Comment->create',
+                                     param => 'thetext'}) unless defined $thetext;
+
+    # Remove any trailing whitespace. Leading whitespace could be
+    # a valid part of the comment.
+    $thetext =~ s/\s*$//s;
+    $thetext =~ s/\r\n?/\n/g; # Get rid of \r.
+
+    ThrowUserError('comment_too_long') if length($thetext) > MAX_COMMENT_LENGTH;
+    return $thetext;
+}
+
+sub _check_isprivate {
+    my ($invocant, $isprivate) = @_;
+    if ($isprivate && !Bugzilla->user->is_insider) {
+        ThrowUserError('user_not_insider');
+    }
+    return $isprivate ? 1 : 0;
+}
+
 sub count {
     my ($self) = @_;
 
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 5b8576512..281143889 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -30,6 +30,7 @@ use Bugzilla::Error;
 
 use Date::Parse;
 use List::MoreUtils qw(part);
+use Scalar::Util qw(blessed);
 
 use constant NAME_FIELD => 'name';
 use constant ID_FIELD   => 'id';
@@ -462,13 +463,21 @@ sub check_required_create_fields {
 }
 
 sub run_create_validators {
-    my ($class, $params) = @_;
+    my ($class, $params, $options) = @_;
 
     my $validators = $class->_get_validators;
     my %field_values = %$params;
 
+    # Make a hash skiplist for easier searching later
+    my %skip_list = map { $_ => 1 } @{ $options->{skip} || [] };
+
+    # Get the sorted field names
     my @sorted_names = $class->_sort_by_dep(keys %field_values);
-    foreach my $field (@sorted_names) {
+
+    # Remove the skipped names
+    my @unskipped = grep { !$skip_list{$_} } @sorted_names;
+
+    foreach my $field (@unskipped) {
         my $value;
         if (exists $validators->{$field}) {
             my $validator = $validators->{$field};
@@ -527,10 +536,54 @@ sub get_all {
 
 sub check_boolean { return $_[1] ? 1 : 0 }
 
+sub check_time {
+    my ($invocant, $value, $field, $params, $allow_negative) = @_;
+
+    # If we don't have a current value default to zero
+    my $current = blessed($invocant) ? $invocant->{$field}
+                                     : 0;
+    $current ||= 0;
+
+    # Don't let the user set the value if they aren't a timetracker
+    return $current unless Bugzilla->user->is_timetracker;
+
+    # Get the new value or zero if it isn't defined
+    $value = trim($value) || 0;
+
+    # Make sure the new value is well formed
+    _validate_time($value, $field, $allow_negative);
+
+    return $value;
+}
+
+
 ###################
 # General Helpers #
 ###################
 
+sub _validate_time {
+    my ($time, $field, $allow_negative) = @_;
+
+    # regexp verifies one or more digits, optionally followed by a period and
+    # zero or more digits, OR we have a period followed by one or more digits
+    # (allow negatives, though, so people can back out errors in time reporting)
+    if ($time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) {
+        ThrowUserError("number_not_numeric",
+                       {field => $field, num => "$time"});
+    }
+
+    # Callers can optionally allow negative times
+    if ( ($time < 0) && !$allow_negative ) {
+        ThrowUserError("number_too_small",
+                       {field => $field, num => "$time", min_num => "0"});
+    }
+
+    if ($time > 99999.99) {
+        ThrowUserError("number_too_large",
+                       {field => $field, num => "$time", max_num => "99999.99"});
+    }
+}
+
 # Sorts fields according to VALIDATOR_DEPENDENCIES. This is not a
 # traditional topological sort, because a "dependency" does not
 # *have* to be in the list--it just has to be earlier than its dependent
@@ -1036,7 +1089,11 @@ Description: Runs the validation of input parameters for L</create>.
              of their input parameters. This method is B<only> called
              by L</create>.
 
-Params:      The same as L</create>.
+Params:      C<$params> - hashref - A value to put in each database
+             field for this object.
+             C<$options> - hashref - Processing options. Currently
+             the only option supported is B<skip>, which can be
+             used to specify a list of fields to not validate.
 
 Returns:     A hash, in a similar format as C<$params>, except that
              these are the values to be inserted into the database,