Bug 373281: Remove AppendComment entirely in favor of Bugzilla::Bug->add_comment - Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=mkanat

2 files changed, 42 insertions, 82 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 722914c5f..bc50ccff4 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -49,7 +49,6 @@ use Storable qw(dclone);
 
 use base qw(Bugzilla::Object Exporter);
 @Bugzilla::Bug::EXPORT = qw(
-    AppendComment ValidateComment
     bug_alias_to_id ValidateBugID
     RemoveVotes CheckIfVotedConfirmed
     LogActivityEntry
@@ -229,7 +228,7 @@ use constant UPDATE_COMMENT_COLUMNS => qw(
 # activity table.
 use constant MAX_LINE_LENGTH => 254;
 
-# Used in ValidateComment(). Gives the max length allowed for a comment.
+# 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(
@@ -992,7 +991,7 @@ sub _check_comment {
     $comment =~ s/\s*$//s;
     $comment =~ s/\r\n?/\n/g; # Get rid of \r.
 
-    ValidateComment($comment);
+    ThrowUserError('comment_too_long') if length($comment) > MAX_COMMENT_LENGTH;
 
     # Creation-only checks
     if (!ref $invocant) {
@@ -2668,43 +2667,6 @@ sub process_knob {
 # Subroutines
 #####################################################################
 
-sub AppendComment {
-    my ($bugid, $whoid, $comment, $isprivate, $timestamp, $work_time,
-        $type, $extra_data) = @_;
-    $work_time ||= 0;
-    $type ||= CMT_NORMAL;
-    my $dbh = Bugzilla->dbh;
-
-    ValidateTime($work_time, "work_time") if $work_time;
-    trick_taint($work_time);
-    detaint_natural($type)
-      || ThrowCodeError('bad_arg', {argument => 'type', function => 'AppendComment'});
-
-    # Use the date/time we were given if possible (allowing calling code
-    # to synchronize the comment's timestamp with those of other records).
-    $timestamp ||= $dbh->selectrow_array('SELECT NOW()');
-
-    $comment =~ s/\r\n/\n/g;     # Handle Windows-style line endings.
-    $comment =~ s/\r/\n/g;       # Handle Mac-style line endings.
-
-    if ($comment =~ /^\s*$/ && !$type) {  # Nothin' but whitespace
-        return;
-    }
-
-    # Comments are always safe, because we always display their raw contents,
-    # and we use them in a placeholder below.
-    trick_taint($comment); 
-    my $privacyval = $isprivate ? 1 : 0 ;
-    $dbh->do(q{INSERT INTO longdescs
-                      (bug_id, who, bug_when, thetext, isprivate, work_time,
-                       type, extra_data)
-               VALUES (?, ?, ?, ?, ?, ?, ?, ?)}, undef,
-             ($bugid, $whoid, $timestamp, $comment, $privacyval, $work_time,
-              $type, $extra_data));
-    $dbh->do("UPDATE bugs SET delta_ts = ? WHERE bug_id = ?",
-             undef, $timestamp, $bugid);
-}
-
 sub update_comment {
     my ($self, $comment_id, $new_comment) = @_;
 
@@ -2727,7 +2689,7 @@ sub update_comment {
     $new_comment =~ s/\r\n?/\n/g; # Handle Windows and Mac-style line endings.
     trick_taint($new_comment);
 
-    # We assume ValidateComment() has already been called earlier.
+    # We assume _check_comment() has already been called earlier.
     Bugzilla->dbh->do('UPDATE longdescs SET thetext = ? WHERE comment_id = ?',
                        undef, ($new_comment, $comment_id));
 
@@ -3043,14 +3005,6 @@ sub CountOpenDependencies {
     return @dependencies;
 }
 
-sub ValidateComment {
-    my ($comment) = @_;
-
-    if (defined($comment) && length($comment) > MAX_COMMENT_LENGTH) {
-        ThrowUserError("comment_too_long");
-    }
-}
-
 # If a bug is moved to a product which allows less votes per bug
 # compared to the previous product, extra votes need to be removed.
 sub RemoveVotes {
@@ -3158,6 +3112,9 @@ sub CheckIfVotedConfirmed {
     my ($id, $who) = (@_);
     my $dbh = Bugzilla->dbh;
 
+    # XXX - Use bug methods to update the bug status and everconfirmed.
+    my $bug = new Bugzilla::Bug($id);
+
     my ($votes, $status, $everconfirmed, $votestoconfirm, $timestamp) =
         $dbh->selectrow_array("SELECT votes, bug_status, everconfirmed, " .
                               "       votestoconfirm, NOW() " .
@@ -3168,6 +3125,9 @@ sub CheckIfVotedConfirmed {
 
     my $ret = 0;
     if ($votes >= $votestoconfirm && !$everconfirmed) {
+        $bug->add_comment('', { type => CMT_POPULAR_VOTES });
+        $bug->update();
+
         if ($status eq 'UNCONFIRMED') {
             my $fieldid = get_field_id("bug_status");
             $dbh->do("UPDATE bugs SET bug_status = 'NEW', everconfirmed = 1, " .
@@ -3189,8 +3149,6 @@ sub CheckIfVotedConfirmed {
                  "VALUES (?, ?, ?, ?, ?, ?)",
                  undef, ($id, $who, $timestamp, $fieldid, '0', '1'));
 
-        AppendComment($id, $who, "", 0, $timestamp, 0, CMT_POPULAR_VOTES);
-
         $ret = 1;
     }
     return $ret;
diff --git a/attachment.cgi b/attachment.cgi
index 529e0aa0d..65a8aa539 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -341,24 +341,25 @@ sub insert {
     my $dbh = Bugzilla->dbh;
     my $user = Bugzilla->user;
 
+    $dbh->bz_start_transaction;
+
     # Retrieve and validate parameters
     my $bugid = $cgi->param('bugid');
     ValidateBugID($bugid);
     validateCanChangeBug($bugid);
-    ValidateComment(scalar $cgi->param('comment'));
-    my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 
+    my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
 
     my $bug = new Bugzilla::Bug($bugid);
     my $attachment =
         Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR, $bug, $user,
                                                         $timestamp, $vars);
 
-  # Insert a comment about the new attachment into the database.
-  my $comment = "Created an attachment (id=" . $attachment->id . ")\n" .
-                $attachment->description . "\n";
-  $comment .= ("\n" . $cgi->param('comment')) if defined $cgi->param('comment');
+    # Insert a comment about the new attachment into the database.
+    my $comment = "Created an attachment (id=" . $attachment->id . ")\n" .
+                  $attachment->description . "\n";
+    $comment .= ("\n" . $cgi->param('comment')) if defined $cgi->param('comment');
 
-  AppendComment($bugid, $user->id, $comment, $attachment->isprivate, $timestamp);
+    $bug->add_comment($comment, { isprivate => $attachment->isprivate });
 
   # Assign the bug to the user, if they are allowed to take it
   my $owner = "";
@@ -372,18 +373,14 @@ sub insert {
       {
           $bug->set_status($bug_status->name);
           $bug->clear_resolution();
-          $bug->update($timestamp);
       }
       # Make sure the person we are taking the bug from gets mail.
       $owner = $bug->assigned_to->login;
-
-      # Ideally, the code below should be replaced by $bug->set_assignee().
-      $dbh->do('UPDATE bugs SET assigned_to = ?, delta_ts = ? WHERE bug_id = ?',
-               undef, ($user->id, $timestamp, $bugid));
-
-      LogActivityEntry($bugid, 'assigned_to', $owner, $user->login, $user->id, $timestamp);
-
+      $bug->set_assigned_to($user);
   }
+  $bug->update($timestamp);
+
+  $dbh->bz_commit_transaction;
 
   # Define the variables and functions that will be passed to the UI template.
   $vars->{'mailrecipients'} =  { 'changer' => $user->login,
@@ -452,7 +449,6 @@ sub update {
     my $dbh = Bugzilla->dbh;
 
     # Retrieve and validate parameters
-    ValidateComment(scalar $cgi->param('comment'));
     my $attachment = validateID();
     my $bug = new Bugzilla::Bug($attachment->bug_id);
     $attachment->validate_can_edit($bug->product_id);
@@ -495,6 +491,17 @@ sub update {
         $cgi->param('isprivate', $attachment->isprivate);
     }
 
+    # If the user submitted a comment while editing the attachment,
+    # add the comment to the bug. Do this after having validated isprivate!
+    if ($cgi->param('comment')) {
+        # Prepend a string to the comment to let users know that the comment came
+        # from the "edit attachment" screen.
+        my $comment = "(From update of attachment " . $attachment->id . ")\n" .
+                      $cgi->param('comment');
+
+        $bug->add_comment($comment, { isprivate => $cgi->param('isprivate') });
+    }
+
     # The order of these function calls is important, as Flag::validate
     # assumes User::match_field has ensured that the values in the
     # requestee fields are legitimate user email addresses.
@@ -578,19 +585,9 @@ sub update {
   # Commit the transaction now that we are finished updating the database.
   $dbh->bz_commit_transaction();
 
-  # If the user submitted a comment while editing the attachment,
-  # add the comment to the bug.
-  if ($cgi->param('comment'))
-  {
-    # Prepend a string to the comment to let users know that the comment came
-    # from the "edit attachment" screen.
-    my $comment = "(From update of attachment " . $attachment->id . ")\n" .
-                  $cgi->param('comment');
+  # Commit the comment, if any.
+  $bug->update();
 
-    # Append the comment to the list of comments in the database.
-    AppendComment($bug->id, $user->id, $comment, $updated_attachment->isprivate, $timestamp);
-  }
-  
   # Define the variables and functions that will be passed to the UI template.
   $vars->{'mailrecipients'} = { 'changer' => Bugzilla->user->login };
   $vars->{'attachment'} = $attachment;
@@ -639,6 +636,8 @@ sub delete_attachment {
             ThrowUserError('token_does_not_exist');
         }
 
+        my $bug = new Bugzilla::Bug($attachment->bug_id);
+
         # The token is valid. Delete the content of the attachment.
         my $msg;
         $vars->{'attachment'} = $attachment;
@@ -649,6 +648,9 @@ sub delete_attachment {
         $template->process("attachment/delete_reason.txt.tmpl", $vars, \$msg)
           || ThrowTemplateError($template->error());
 
+        # Paste the reason provided by the admin into a comment.
+        $bug->add_comment($msg);
+
         # If the attachment is stored locally, remove it.
         if (-e $attachment->_get_local_filename) {
             unlink $attachment->_get_local_filename;
@@ -658,11 +660,11 @@ sub delete_attachment {
         # Now delete the token.
         delete_token($token);
 
-        # Paste the reason provided by the admin into a comment.
-        AppendComment($attachment->bug_id, $user->id, $msg);
+        # Insert the comment.
+        $bug->update();
 
         # Required to display the bug the deleted attachment belongs to.
-        $vars->{'bugs'} = [new Bugzilla::Bug($attachment->bug_id)];
+        $vars->{'bugs'} = [$bug];
         $vars->{'header_done'} = 1;
 
         $template->process("attachment/updated.html.tmpl", $vars)