Bug 472217: Create a Bugzilla::Comment object and eliminate GetComments

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

7 files changed, 282 insertions, 172 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 14fe20c02..2a248905d 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -46,6 +46,7 @@ use Bugzilla::Product;
 use Bugzilla::Component;
 use Bugzilla::Group;
 use Bugzilla::Status;
+use Bugzilla::Comment;
 
 use List::Util qw(min);
 use Storable qw(dclone);
@@ -1835,12 +1836,12 @@ sub set_cclist_accessible { $_[0]->set('cclist_accessible', $_[1]); }
 sub set_comment_is_private {
     my ($self, $comment_id, $isprivate) = @_;
     return unless Bugzilla->user->is_insider;
-    my ($comment) = grep($comment_id eq $_->{id}, @{$self->longdescs});
+    my ($comment) = grep($comment_id == $_->id, @{ $self->comments });
     ThrowUserError('comment_invalid_isprivate', { id => $comment_id }) 
         if !$comment;
 
     $isprivate = $isprivate ? 1 : 0;
-    if ($isprivate != $comment->{isprivate}) {
+    if ($isprivate != $comment->is_private) {
         $self->{comment_isprivate} ||= {};
         $self->{comment_isprivate}->{$comment_id} = $isprivate;
     }
@@ -2665,12 +2666,38 @@ sub keyword_objects {
     return $self->{'keyword_objects'};
 }
 
-sub longdescs {
-    my ($self) = @_;
-    return $self->{'longdescs'} if exists $self->{'longdescs'};
+sub comments {
+    my ($self, $params) = @_;
     return [] if $self->{'error'};
-    $self->{'longdescs'} = GetComments($self->{bug_id});
-    return $self->{'longdescs'};
+    $params ||= {};
+
+    if (!defined $self->{'comments'}) {
+        $self->{'comments'} = Bugzilla::Comment->match({ bug_id => $self->id });
+        my $count = 0;
+        foreach my $comment (@{ $self->{'comments'} }) {
+            $comment->{count} = $count++;
+        }
+    }
+    my @comments = @{ $self->{'comments'} };
+
+    my $order = $params->{order} 
+        || Bugzilla->user->settings->{'comment_sort_order'}->{'value'};
+    if ($order ne 'oldest_to_newest') {
+        @comments = reverse @comments;
+        if ($order eq 'newest_to_oldest_desc_first') {
+            unshift(@comments, pop @comments);
+        }
+    }
+
+    if ($params->{after}) {
+        my $from = datetime_from($params->{after});
+        @comments = grep { datetime_from($_->creation_ts) > $from } @comments;
+    }
+    if ($params->{to}) {
+        my $to = datetime_from($params->{to});
+        @comments = grep { datetime_from($_->creation_ts) <= $to } @comments;
+    }
+    return \@comments;
 }
 
 sub milestoneurl {
@@ -2955,7 +2982,7 @@ sub update_comment {
       || ThrowCodeError('bad_arg', {argument => 'comment_id', function => 'update_comment'});
 
     # The comment ID must belong to this bug.
-    my @current_comment_obj = grep {$_->{'id'} == $comment_id} @{$self->longdescs};
+    my @current_comment_obj = grep {$_->id == $comment_id} @{$self->comments};
     scalar(@current_comment_obj)
       || ThrowCodeError('bad_arg', {argument => 'comment_id', function => 'update_comment'});
 
@@ -2972,7 +2999,7 @@ sub update_comment {
     $self->_sync_fulltext();
 
     # Update the comment object with this new text.
-    $current_comment_obj[0]->{'body'} = $new_comment;
+    $current_comment_obj[0]->{'thetext'} = $new_comment;
 }
 
 # Represents which fields from the bugs table are handled by process_bug.cgi.
@@ -3032,74 +3059,6 @@ sub ValidateTime {
     }
 }
 
-sub GetComments {
-    my ($id, $comment_sort_order, $start, $end, $raw) = @_;
-    my $dbh = Bugzilla->dbh;
-
-    $comment_sort_order = $comment_sort_order ||
-        Bugzilla->user->settings->{'comment_sort_order'}->{'value'};
-
-    my $sort_order = ($comment_sort_order eq "oldest_to_newest") ? 'asc' : 'desc';
-
-    my @comments;
-    my @args = ($id);
-
-    my $query = 'SELECT longdescs.comment_id AS id, profiles.userid, ' .
-                        $dbh->sql_date_format('longdescs.bug_when', '%Y.%m.%d %H:%i:%s') .
-                      ' AS time, longdescs.thetext AS body, longdescs.work_time,
-                        isprivate, already_wrapped, type, extra_data
-                   FROM longdescs
-             INNER JOIN profiles
-                     ON profiles.userid = longdescs.who
-                  WHERE longdescs.bug_id = ?';
-
-    if ($start) {
-        $query .= ' AND longdescs.bug_when > ?';
-        push(@args, $start);
-    }
-    if ($end) {
-        $query .= ' AND longdescs.bug_when <= ?';
-        push(@args, $end);
-    }
-
-    $query .= " ORDER BY longdescs.bug_when $sort_order";
-    my $sth = $dbh->prepare($query);
-    $sth->execute(@args);
-
-    # Cache the users we look up
-    my %users;
-
-    while (my $comment_ref = $sth->fetchrow_hashref()) {
-        my %comment = %$comment_ref;
-        $users{$comment{'userid'}} ||= new Bugzilla::User($comment{'userid'});
-        $comment{'author'} = $users{$comment{'userid'}};
-
-        # If raw data is requested, do not format 'special' comments.
-        $comment{'body'} = format_comment(\%comment) unless $raw;
-
-        push (@comments, \%comment);
-    }
-   
-    if ($comment_sort_order eq "newest_to_oldest_desc_first") {
-        unshift(@comments, pop @comments);
-    }
-
-    return \@comments;
-}
-
-# Format language specific comments.
-sub format_comment {
-    my $comment = shift;
-    my $template = Bugzilla->template_inner;
-    my $vars = {comment => $comment};
-    my $body;
-
-    $template->process("bug/format_comment.txt.tmpl", $vars, \$body)
-        || ThrowTemplateError($template->error());
-    $body =~ s/^X//;
-    return $body;
-}
-
 # Get the activity of a bug, starting from $starttime (if given).
 # This routine assumes Bugzilla::Bug->check has been previously called.
 sub GetBugActivity {
@@ -3644,7 +3603,7 @@ sub _validate_attribute {
     my @valid_attributes = (
         # Miscellaneous properties and methods.
         qw(error groups product_id component_id
-           longdescs milestoneurl attachments
+           comments milestoneurl attachments
            isopened isunconfirmed
            flag_types num_attachment_flag_types
            show_attachment_flags any_flags_requesteeble),
diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index d654a139b..2aff7daf4 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -119,6 +119,7 @@ sub Send {
     my $msg = "";
 
     my $dbh = Bugzilla->dbh;
+    my $bug = new Bugzilla::Bug($id);
 
     # XXX - These variables below are useless. We could use field object
     # methods directly. But we first have to implement a cache in
@@ -356,7 +357,7 @@ sub Send {
         }
     }
 
-    my $comments = get_comments_by_bug($id, $start, $end);
+    my $comments = $bug->comments({ after => $start, to => $end });
 
     ###########################################################################
     # Start of email filtering code
@@ -569,7 +570,7 @@ sub sendMail {
     }
 
     if (!$user->is_insider) {
-        @send_comments = grep { !$_->{isprivate} } @send_comments;
+        @send_comments = grep { !$_->is_private } @send_comments;
     }
 
     if ($difftext eq "" && !scalar(@send_comments) && !$isnew) {
@@ -650,38 +651,4 @@ sub sendMail {
     return 1;
 }
 
-# Get bug comments for the given period.
-sub get_comments_by_bug {
-    my ($id, $start, $end) = @_;
-    my $dbh = Bugzilla->dbh;
-
-    my $result = "";
-    my $count = 0;
-
-    # $start will be undef for new bugs, and defined for pre-existing bugs.
-    if ($start) {
-        # If $start is not NULL, obtain the count-index
-        # of this comment for the leading "Comment #xxx" line.
-        $count = $dbh->selectrow_array('SELECT COUNT(*) FROM longdescs
-                                        WHERE bug_id = ? AND bug_when <= ?',
-                                        undef, ($id, $start));
-    }
-
-    my $raw = 1; # Do not format comments which are not of type CMT_NORMAL.
-    my $comments = Bugzilla::Bug::GetComments($id, "oldest_to_newest", $start, $end, $raw);
-    my $attach_base = correct_urlbase() . 'attachment.cgi?id=';
-
-    foreach my $comment (@$comments) {
-        $comment->{count} = $count++;
-        # If an attachment was created, then add an URL. (Note: the 'g'lobal
-        # replace should work with comments with multiple attachments.)
-        if ($comment->{body} =~ /Created an attachment \(/) {
-            $comment->{body} =~ s/(Created an attachment \(id=([0-9]+)\))/$1\n --> \($attach_base$2\)/g;
-        }
-        $comment->{body} = $comment->{'already_wrapped'} ? $comment->{body} : wrap_comment($comment->{body});
-    }
-
-    return $comments;
-}
-
 1;
diff --git a/Bugzilla/Comment.pm b/Bugzilla/Comment.pm
new file mode 100644
index 000000000..7072dbbee
--- /dev/null
+++ b/Bugzilla/Comment.pm
@@ -0,0 +1,173 @@
+# -*- Mode: perl; indent-tabs-mode: nil -*-
+#
+# The contents of this file are subject to the Mozilla Public
+# License Version 1.1 (the "License"); you may not use this file
+# except in compliance with the License. You may obtain a copy of
+# the License at http://www.mozilla.org/MPL/
+#
+# Software distributed under the License is distributed on an "AS
+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+# implied. See the License for the specific language governing
+# rights and limitations under the License.
+#
+# The Original Code is the Bugzilla Bug Tracking System.
+#
+# The Initial Developer of the Original Code is James Robson.
+# Portions created by James Robson are Copyright (c) 2009 James Robson.
+# All rights reserved.
+#
+# Contributor(s): James Robson <arbingersys@gmail.com> 
+
+use strict;
+
+package Bugzilla::Comment;
+
+use base qw(Bugzilla::Object);
+
+use Bugzilla::Constants;
+use Bugzilla::Error;
+use Bugzilla::Util;
+
+###############################
+####    Initialization     ####
+###############################
+
+use constant DB_COLUMNS => qw(
+    comment_id
+    bug_id
+    who
+    bug_when
+    work_time
+    thetext
+    isprivate
+    already_wrapped
+    type
+    extra_data
+);
+
+use constant DB_TABLE => 'longdescs';
+use constant ID_FIELD => 'comment_id';
+use constant LIST_ORDER => 'bug_when';
+
+###############################
+####      Accessors      ######
+###############################
+
+sub already_wrapped { return $_[0]->{'already_wrapped'}; }
+sub body        { return $_[0]->{'thetext'}; }
+sub bug_id      { return $_[0]->{'bug_id'}; }
+sub creation_ts { return $_[0]->{'bug_when'}; }
+sub is_private  { return $_[0]->{'isprivate'}; }
+sub work_time   { return $_[0]->{'work_time'}; }
+
+sub author { 
+    my $self = shift;
+    $self->{'author'} ||= new Bugzilla::User($self->{'who'});
+    return $self->{'author'};
+}
+
+sub body_full {
+    my ($self, $params) = @_;
+    $params ||= {};
+    my $template = Bugzilla->template_inner;
+    my $body;
+    $template->process("bug/format_comment.txt.tmpl", 
+                       { comment => $self, %$params }, \$body)
+        || ThrowTemplateError($template->error());
+    $body =~ s/^X//;
+    if ($params->{wrap} and !$self->already_wrapped) {
+        $body = wrap_comment($body);
+    }
+    return $body;
+}
+
+1;
+
+__END__
+
+=head1 NAME
+
+Bugzilla::Comment - A Comment for a given bug 
+
+=head1 SYNOPSIS
+
+ use Bugzilla::Comment;
+
+ my $comment = Bugzilla::Comment->new($comment_id);
+ my $comments = Bugzilla::Comment->new_from_list($comment_ids);
+
+=head1 DESCRIPTION
+
+Bugzilla::Comment represents a comment attached to a bug.
+
+This implements all standard C<Bugzilla::Object> methods. See 
+L<Bugzilla::Object> for more details.
+
+=head2 Accessors
+
+=over
+
+=item C<bug_id>
+
+C<int> The ID of the bug to which the comment belongs.
+
+=item C<creation_ts>
+
+C<string> The comment creation timestamp.
+
+=item C<body>
+
+C<string> The body without any special additional text.
+
+=item C<work_time>
+
+C<string> Time spent as related to this comment.
+
+=item C<is_private>
+
+C<boolean> Comment is marked as private
+
+=item C<already_wrapped>
+
+If this comment is stored in the database word-wrapped, this will be C<1>.
+C<0> otherwise.
+
+=item C<author>
+
+L<Bugzilla::User> who created the comment.
+
+=item C<body_full>
+
+=over
+
+=item B<Description>
+
+C<string> Body of the comment, including any special text (such as
+"this bug was marked as a duplicate of...").
+
+=item B<Params>
+
+=over
+
+=item C<is_bugmail>
+
+C<boolean>. C<1> if this comment should be formatted specifically for
+bugmail.
+
+=item C<wrap>
+
+C<boolean>. C<1> if the comment should be returned word-wrapped.
+
+=back
+
+=item B<Returns>
+
+A string, the full text of the comment as it would be displayed to an end-user.
+
+=back
+
+
+
+=back
+
+=cut
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index d5e371f64..fa7247243 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -170,7 +170,7 @@ sub quoteUrls {
 
     # If the comment is already wrapped, we should ignore newlines when
     # looking for matching regexps. Else we should take them into account.
-    my $s = ($comment && $comment->{already_wrapped}) 
+    my $s = ($comment && $comment->already_wrapped) 
             ? qr/\s/ : qr/[[:blank:]]/;
 
     # However, note that adding the title (for buglinks) can affect things
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 3843062fb..0fc2db336 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -1390,7 +1390,7 @@ sub wants_bug_mail {
     my $self = shift;
     my ($bug_id, $relationship, $fieldDiffs, $comments, $dependencyText,
         $changer, $bug_is_new) = @_;
-    my $comments_concatenated = join("\n", map { $_->{body} } (@$comments));
+    my $comments_concatenated = join("\n", map { $_->body } @$comments);
 
     # Make a list of the events which have happened during this bug change,
     # from the point of view of this user.    
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 21588417c..d1a52a5fc 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -40,7 +40,7 @@ use base qw(Exporter);
                              diff_arrays
                              trim wrap_hard wrap_comment find_wrap_point
                              format_time format_time_decimal validate_date
-                             validate_time
+                             validate_time datetime_from
                              file_mod_time is_7bit_clean
                              bz_crypt generate_random_password
                              validate_email_syntax clean_text
@@ -396,7 +396,9 @@ sub format_time {
 
     # If $format is undefined, try to guess the correct date format.
     if (!defined($format)) {
-        if ($date =~ m/^(\d{4})[-\.](\d{2})[-\.](\d{2}) (\d{2}):(\d{2})(:(\d{2}))?$/) {
+        if (!ref $date
+            && $date =~ /^(\d{4})[-\.](\d{2})[-\.](\d{2}) (\d{2}):(\d{2})(:(\d{2}))?$/) 
+        {
             my $sec = $7;
             if (defined $sec) {
                 $format = "%Y-%m-%d %T %Z";
@@ -409,44 +411,49 @@ sub format_time {
         }
     }
 
-    # strptime($date) returns an empty array if $date has an invalid date format.
+    my $dt = ref $date ? $date : datetime_from($date, $timezone);
+    $date = defined $dt ? $dt->strftime($format) : '';
+    return trim($date);
+}
+
+sub datetime_from {
+    my ($date, $timezone) = @_;
+
+    # strptime($date) returns an empty array if $date has an invalid
+    # date format.
     my @time = strptime($date);
 
     unless (scalar @time) {
-        # If an unknown timezone is passed (such as MSK, for Moskow), strptime() is
-        # unable to parse the date. We try again, but we first remove the timezone.
+        # If an unknown timezone is passed (such as MSK, for Moskow),
+        # strptime() is unable to parse the date. We try again, but we first
+        # remove the timezone.
         $date =~ s/\s+\S+$//;
         @time = strptime($date);
     }
 
-    if (scalar @time) {
-        # Fix a bug in strptime() where seconds can be undefined in some cases.
-        $time[0] ||= 0;
-
-        # strptime() counts years from 1900, and months from 0 (January).
-        # We have to fix both values.
-        my $dt = DateTime->new({year   => 1900 + $time[5],
-                                month  => ++$time[4],
-                                day    => $time[3],
-                                hour   => $time[2],
-                                minute => $time[1],
-                                # DateTime doesn't like fractional seconds.
-                                second => int($time[0]),
-                                # If importing, use the specified timezone, otherwise 
-                                # use the timezone specified by the server.
-                                time_zone => Bugzilla->local_timezone->offset_as_string($time[6]) 
-                                          || Bugzilla->local_timezone});
-
-        # Now display the date using the given timezone,
-        # or the user's timezone if none is given.
-        $dt->set_time_zone($timezone || Bugzilla->user->timezone);
-        $date = $dt->strftime($format);
-    }
-    else {
-        # Don't let invalid (time) strings to be passed to templates!
-        $date = '';
-    }
-    return trim($date);
+    return undef if !@time;
+
+    # strptime() counts years from 1900, and months from 0 (January).
+    # We have to fix both values.
+    my $dt = DateTime->new({
+        year   => $time[5] + 1900,
+        month  => $time[4] + 1,
+        day    => $time[3],
+        hour   => $time[2],
+        minute => $time[1],
+        # DateTime doesn't like fractional seconds.
+        # Also, sometimes seconds are undef.
+        second => int($time[0] || 0),
+        # If a timezone was specified, use it. Otherwise, use the
+        # local timezone.
+        time_zone => Bugzilla->local_timezone->offset_as_string($time[6]) 
+                     || Bugzilla->local_timezone,
+    });
+
+    # Now display the date using the given timezone,
+    # or the user's timezone if none is given.
+    $dt->set_time_zone($timezone || Bugzilla->user->timezone);
+    return $dt;
 }
 
 sub format_time_decimal {
@@ -641,6 +648,7 @@ Bugzilla::Util - Generic utility functions for bugzilla
 
   # Functions for formatting time
   format_time($time);
+  datetime_from($time, $timezone);
 
   # Functions for dealing with files
   $time = file_mod_time($filename);
@@ -894,6 +902,15 @@ This routine is mainly called from templates to filter dates, see
 Returns a number with 2 digit precision, unless the last digit is a 0. Then it 
 returns only 1 digit precision.
 
+=item C<datetime_from($time, $timezone)>
+
+Returns a DateTime object given a date string. If the string is not in some
+valid date format that C<strptime> understands, we return C<undef>.
+
+You can optionally specify a timezone for the returned date. If not
+specified, defaults to the currently-logged-in user's timezone, or
+the Bugzilla server's local timezone if there isn't a logged-in user.
+
 =back
 
 
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index 6a3e93519..aea30cedb 100644
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -23,6 +23,7 @@ package Bugzilla::WebService::Bug;
 use strict;
 use base qw(Bugzilla::WebService);
 
+use Bugzilla::Comment;
 use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Field;
@@ -95,12 +96,12 @@ sub comments {
     foreach my $bug_id (@$bug_ids) {
         my $bug = Bugzilla::Bug->check($bug_id);
         # We want the API to always return comments in the same order.
-        my $comments = Bugzilla::Bug::GetComments(
-            $bug->id, 'oldest_to_newest', $params->{new_since});
+   
+        my $comments = $bug->comments({ order => 'oldest_to_newest',
+                                        after => $params->{new_since} });
         my @result;
         foreach my $comment (@$comments) {
-            next if $comment->{isprivate} && !$user->is_insider;
-            $comment->{bug_id} = $bug->id;
+            next if $comment->is_private && !$user->is_insider;
             push(@result, $self->_translate_comment($comment, $params));
         }
         $bugs{$bug->id}{'comments'} = \@result;
@@ -109,15 +110,10 @@ sub comments {
     my %comments;
     if (scalar @$comment_ids) {
         my @ids = map { trim($_) } @$comment_ids;
-        my @sql_ids = map { $dbh->quote($_) } @ids;
-        my $comment_data = $dbh->selectall_arrayref(
-            'SELECT comment_id AS id, bug_id, who, bug_when AS time,
-                    isprivate, thetext AS body, type, extra_data
-               FROM longdescs WHERE ' . $dbh->sql_in('comment_id', \@sql_ids),
-            {Slice=>{}});
+        my $comment_data = Bugzilla::Comment->new_from_list(\@ids);
 
         # See if we were passed any invalid comment ids.
-        my %got_ids = map { $_->{id} => 1 } @$comment_data;
+        my %got_ids = map { $_->id => 1 } @$comment_data;
         foreach my $comment_id (@ids) {
             if (!$got_ids{$comment_id}) {
                 ThrowUserError('comment_id_invalid', { id => $comment_id });
@@ -125,16 +121,14 @@ sub comments {
         }
  
         # Now make sure that we can see all the associated bugs.
-        my %got_bug_ids = map { $_->{bug_id} => 1 } @$comment_data;
+        my %got_bug_ids = map { $_->bug_id => 1 } @$comment_data;
         Bugzilla::Bug->check($_) foreach (keys %got_bug_ids);
 
         foreach my $comment (@$comment_data) {
-            if ($comment->{isprivate} && !$user->is_insider) {
-                ThrowUserError('comment_is_private', { id => $comment->{id} });
+            if ($comment->is_private && !$user->is_insider) {
+                ThrowUserError('comment_is_private', { id => $comment->id });
             }
-            $comment->{author} = new Bugzilla::User($comment->{who});
-            $comment->{body} = Bugzilla::Bug::format_comment($comment);
-            $comments{$comment->{id}} =
+            $comments{$comment->id} =
                 $self->_translate_comment($comment, $params);
         }
     }
@@ -146,12 +140,12 @@ sub comments {
 sub _translate_comment {
     my ($self, $comment, $filters) = @_;
     return filter $filters, {
-        id         => $self->type('int', $comment->{id}),
-        bug_id     => $self->type('int', $comment->{bug_id}),
-        author     => $self->type('string', $comment->{author}->login),
-        time       => $self->type('dateTime', $comment->{'time'}),
-        is_private => $self->type('boolean', $comment->{isprivate}),
-        text       => $self->type('string', $comment->{body}),
+        id         => $self->type('int', $comment->id),
+        bug_id     => $self->type('int', $comment->bug_id),
+        author     => $self->type('string', $comment->author->login),
+        time       => $self->type('dateTime', $comment->creation_ts),
+        is_private => $self->type('boolean', $comment->is_private),
+        text       => $self->type('string', $comment->body_full),
     };
 }