Bug 452919: Allow the "created an attachment" message in comments to be localized

Bugzilla/Bug.pm

@@ -2678,6 +2678,7 @@ sub comments {
         my $count = 0;
         foreach my $comment (@{ $self->{'comments'} }) {
             $comment->{count} = $count++;
+            $comment->{bug} = $self;
         }
     }
     my @comments = @{ $self->{'comments'} };
@@ -2973,37 +2974,6 @@ sub bug_alias_to_id {
 # Subroutines
 #####################################################################
 
-sub update_comment {
-    my ($self, $comment_id, $new_comment) = @_;
-
-    # Some validation checks.
-    if ($self->{'error'}) {
-        ThrowCodeError("bug_error", { bug => $self });
-    }
-    detaint_natural($comment_id)
-      || 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->comments};
-    scalar(@current_comment_obj)
-      || ThrowCodeError('bad_arg', {argument => 'comment_id', function => 'update_comment'});
-
-    # If the new comment is undefined, then there is nothing to update.
-    # To delete a comment, an empty string should be passed.
-    return unless defined $new_comment;
-    $new_comment =~ s/\s*$//s;    # Remove trailing whitespaces.
-    $new_comment =~ s/\r\n?/\n/g; # Handle Windows and Mac-style line endings.
-    trick_taint($new_comment);
-
-    # We assume _check_comment() has already been called earlier.
-    Bugzilla->dbh->do('UPDATE longdescs SET thetext = ? WHERE comment_id = ?',
-                       undef, ($new_comment, $comment_id));
-    $self->_sync_fulltext();
-
-    # Update the comment object with this new text.
-    $current_comment_obj[0]->{'thetext'} = $new_comment;
-}
-
 # Represents which fields from the bugs table are handled by process_bug.cgi.
 sub editable_bug_fields {
     my @fields = Bugzilla->dbh->bz_table_columns('bugs');

Bugzilla/Comment.pm

@@ -24,6 +24,7 @@ package Bugzilla::Comment;
 
 use base qw(Bugzilla::Object);
 
+use Bugzilla::Attachment;
 use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Util;
@@ -45,20 +46,66 @@ use constant DB_COLUMNS => qw(
     extra_data
 );
 
+use constant UPDATE_COLUMNS => qw(
+    type
+    extra_data
+);
+
 use constant DB_TABLE => 'longdescs';
 use constant ID_FIELD => 'comment_id';
 use constant LIST_ORDER => 'bug_when';
 
+use constant VALIDATORS => {
+    type => \&_check_type,
+};
+
+use constant UPDATE_VALIDATORS => {
+    extra_data => \&_check_extra_data,
+};
+
+#########################
+# Database Manipulation #
+#########################
+
+sub update {
+    my $self = shift;
+    my $changes = $self->SUPER::update(@_);
+    $self->bug->_sync_fulltext();
+    return $changes;
+}
+
 ###############################
 ####      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 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 type        { return $_[0]->{'type'};      }
+sub extra_data  { return $_[0]->{'extra_data'} }
+
+sub bug {
+    my $self = shift;
+    require Bugzilla::Bug;
+    $self->{bug} ||= new Bugzilla::Bug($self->bug_id);
+    return $self->{bug};
+}
+
+sub is_about_attachment {
+    my ($self) = @_;
+    return 1 if $self->type == CMT_ATTACHMENT_CREATED;
+    return 0;
+}
+
+sub attachment {
+    my ($self) = @_;
+    return undef if not $self->is_about_attachment;
+    $self->{attachment} ||= new Bugzilla::Attachment($self->extra_data);
+    return $self->{attachment};
+}
 
 sub author { 
     my $self = shift;
@@ -81,6 +128,63 @@ sub body_full {
     return $body;
 }
 
+############
+# Mutators #
+############
+
+sub set_extra_data { $_[0]->set('extra_data', $_[1]); }
+
+sub set_type {
+    my ($self, $type, $extra_data) = @_;
+    $self->set('type', $type);
+    $self->set_extra_data($extra_data);
+}
+
+##############
+# Validators #
+##############
+
+sub _check_extra_data {
+    my ($invocant, $extra_data, $type) = @_;
+    $type = $invocant->type if ref $invocant;
+    if ($type == CMT_NORMAL or $type == CMT_POPULAR_VOTES) {
+        if (defined $extra_data) {
+            ThrowCodeError('comment_extra_data_not_allowed',
+                           { type => $type, extra_data => $extra_data });
+        }
+    }
+    else {
+        if (!defined $extra_data) {
+            ThrowCodeError('comment_extra_data_required', { type => $type });
+        }
+        if ($type == CMT_MOVED_TO) {
+            $extra_data = Bugzilla::User->check($extra_data)->login;
+        }
+        elsif ($type == CMT_ATTACHMENT_CREATED) {
+             my $attachment = Bugzilla::Attachment->check({ 
+                 id => $extra_data });
+             $extra_data = $attachment->id;
+        }
+        else {
+            my $original = $extra_data;
+            detaint_natural($extra_data) 
+              or ThrowCodeError('comment_extra_data_not_numeric',
+                                { type => $type, extra_data => $original });
+        }
+    }
+
+    return $extra_data;
+}
+
+sub _check_type {
+    my ($invocant, $type) = @_;
+    $type ||= CMT_NORMAL;
+    my $original = $type;
+    detaint_natural($type)
+        or ThrowCodeError('comment_type_invalid', { type => $original });
+    return $type;
+}
+
 1;
 
 __END__

Bugzilla/Constants.pm

@@ -91,6 +91,7 @@ use File::Basename;
     CMT_HAS_DUPE
     CMT_POPULAR_VOTES
     CMT_MOVED_TO
+    CMT_ATTACHMENT_CREATED
 
     THROW_ERROR
     
@@ -276,6 +277,7 @@ use constant CMT_DUPE_OF => 1;
 use constant CMT_HAS_DUPE => 2;
 use constant CMT_POPULAR_VOTES => 3;
 use constant CMT_MOVED_TO => 4;
+use constant CMT_ATTACHMENT_CREATED => 5;
 
 # Determine whether a validation routine should return 0 or throw
 # an error when the validation fails.

Bugzilla/Install/DB.pm

@@ -586,6 +586,8 @@ sub update_table_definitions {
     # 2009-11-01 LpSolit@gmail.com - Bug 525025
     _fix_invalid_custom_field_names();
 
+    _move_attachment_creation_comments_into_comment_type();
+
     ################################################################
     # New --TABLE-- changes should go *** A B O V E *** this point #
     ################################################################
@@ -3136,18 +3138,33 @@ sub _add_foreign_keys_to_multiselects {
     }
 }
 
+# This subroutine is used in multiple places (for times when we update
+# the text of comments), so it takes an argument, $bug_ids, which causes
+# it to update bugs_fulltext for those bug_ids instead of populating the
+# whole table.
 sub _populate_bugs_fulltext {
+    my $bug_ids = shift;
     my $dbh = Bugzilla->dbh;
     my $fulltext = $dbh->selectrow_array('SELECT 1 FROM bugs_fulltext '
                                          . $dbh->sql_limit(1));
-    # We only populate the table if it's empty...
-    if (!$fulltext) {
-        # ... and if there are bugs in the bugs table.
-        my $bug_ids = $dbh->selectcol_arrayref('SELECT bug_id FROM bugs');
+    # We only populate the table if it's empty or if we've been given a
+    # set of bug ids.
+    if ($bug_ids or !$fulltext) {
+        $bug_ids ||= $dbh->selectcol_arrayref('SELECT bug_id FROM bugs');
+        # If there are no bugs in the bugs table, there's nothing to populate.
         return if !@$bug_ids;
 
-        print "Populating bugs_fulltext...";
-        print " (this can take a long time.)\n";
+        my $where = "";
+        if ($fulltext) {
+            print "Updating bugs_fulltext...\n";
+            $where = "WHERE " . $dbh->sql_in('bugs.bug_id', $bug_ids);
+            $dbh->do("DELETE FROM bugs_fulltext WHERE " 
+                     . $dbh->sql_in('bug_id', $bug_ids));
+        }
+        else {
+            print "Populating bugs_fulltext...";
+            print " (this can take a long time.)\n";
+        }
         my $newline = $dbh->quote("\n");
         $dbh->do(
             q{INSERT INTO bugs_fulltext (bug_id, short_desc, comments, 
@@ -3155,12 +3172,13 @@ sub _populate_bugs_fulltext {
                    SELECT bugs.bug_id, bugs.short_desc, }
                  . $dbh->sql_group_concat('longdescs.thetext', $newline)
           . ', ' . $dbh->sql_group_concat('nopriv.thetext',    $newline) .
-                  q{ FROM bugs 
+                 qq{ FROM bugs 
                           LEFT JOIN longdescs
                                  ON bugs.bug_id = longdescs.bug_id
                           LEFT JOIN longdescs AS nopriv
                                  ON longdescs.comment_id = nopriv.comment_id
-                                    AND nopriv.isprivate = 0 }
+                                    AND nopriv.isprivate = 0 
+                     $where }
                  . $dbh->sql_group_by('bugs.bug_id', 'bugs.short_desc'));
     }
 }
@@ -3235,6 +3253,55 @@ sub _fix_invalid_custom_field_names {
     }
 }
 
+sub _move_attachment_creation_comments_into_comment_type {
+    my $dbh = Bugzilla->dbh;
+    # We check if there are any CMT_ATTACHMENT_CREATED comments already,
+    # first, because this is faster than a full LIKE search on the comments,
+    # and currently this will run every time we run checksetup.
+    my $test = $dbh->selectrow_array(
+        'SELECT 1 FROM longdescs WHERE type = ' . CMT_ATTACHMENT_CREATED
+        . ' ' . $dbh->sql_limit(1));
+    return if $test;
+    my %comments = @{ $dbh->selectcol_arrayref(
+        "SELECT comment_id, thetext FROM longdescs
+          WHERE thetext LIKE 'Created an attachment (id=%'", 
+        {Columns=>[1,2]}) };
+    my @comment_ids = keys %comments;
+    return if !scalar @comment_ids;
+    print "Setting the type field on attachment creation comments...\n";
+    my $sth = $dbh->prepare(
+        'UPDATE longdescs SET thetext = ?, type = ?, extra_data = ?
+          WHERE comment_id = ?');
+    my $count = 0;
+    my $total = scalar @comment_ids;
+    $dbh->bz_start_transaction();
+    foreach my $id (@comment_ids) {
+        $count++;
+        my $text = $comments{$id};
+        next if $text !~ /attachment \(id=(\d+)/;
+        my $attachment_id = $1;
+        # Now we have to remove the text up until we find a line that's
+        # just a single newline, because the old "Created an attachment"
+        # text included the attachment description underneath it, and in
+        # Bugzillas before 2.20, that could be wrapped into multiple lines,
+        # in the database.
+        my @lines = split("\n", $text);
+        while (1) {
+            my $line = shift @lines;
+            last if (!defined $line or trim($line) eq '');
+        }
+        $text = join("\n", @lines);
+        $sth->execute($text, CMT_ATTACHMENT_CREATED, $attachment_id, $id);
+        indicate_progress({ total => $total, current => $count, 
+                            every => 25 });
+    }
+    my $bug_ids = $dbh->selectcol_arrayref(
+        'SELECT DISTINCT bug_id FROM longdescs WHERE '
+        . $dbh->sql_in('comment_id', \@comment_ids));
+    _populate_bugs_fulltext($bug_ids);
+    $dbh->bz_commit_transaction();
+}
+
 1;
 
 __END__

Bugzilla/Template.pm

@@ -242,12 +242,7 @@ sub quoteUrls {
     $text =~ s~\b(mailto:|)?([\w\.\-\+\=]+\@[\w\-]+(?:\.[\w\-]+)+)\b
               ~<a href=\"mailto:$2\">$1$2</a>~igx;
 
-    # attachment links - handle both cases separately for simplicity
-    $text =~ s~((?:^Created\ an\ |\b)attachment\s*\(id=(\d+)\)(\s\[edit\])?)
-              ~($things[$count++] = get_attachment_link($2, $1)) &&
-               ("\0\0" . ($count-1) . "\0\0")
-              ~egmx;
-
+    # attachment links
     $text =~ s~\b(attachment$s*\#?$s*(\d+))
               ~($things[$count++] = get_attachment_link($2, $1)) &&
                ("\0\0" . ($count-1) . "\0\0")

Bugzilla/User.pm

@@ -1390,7 +1390,6 @@ 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);
 
     # Make a list of the events which have happened during this bug change,
     # from the point of view of this user.    
@@ -1439,7 +1438,7 @@ sub wants_bug_mail {
         }
     }
 
-    if ($comments_concatenated =~ /Created an attachment \(/) {
+    if (grep { $_->type == CMT_ATTACHMENT_CREATED } @$comments) {
         $events{+EVT_ATTACHMENT} = 1;
     }
     elsif (defined($$comments[0])) {

Bugzilla/WebService/Bug.pm

@@ -139,6 +139,8 @@ sub comments {
 # Helper for Bug.comments
 sub _translate_comment {
     my ($self, $comment, $filters) = @_;
+    my $attach_id = $comment->is_about_attachment ? $comment->extra_data
+                                                  : undef;
     return filter $filters, {
         id         => $self->type('int', $comment->id),
         bug_id     => $self->type('int', $comment->bug_id),
@@ -146,6 +148,7 @@ sub _translate_comment {
         time       => $self->type('dateTime', $comment->creation_ts),
         is_private => $self->type('boolean', $comment->is_private),
         text       => $self->type('string', $comment->body_full),
+        attachment_id => $self->type('int', $attach_id),
     };
 }
 
@@ -786,6 +789,11 @@ C<int> The globally unique ID for the comment.
 
 C<int> The ID of the bug that this comment is on.
 
+=item attachment_id
+
+C<int> If the comment was made on an attachment, this will be the
+ID of that attachment. Otherwise it will be null.
+
 =item text
 
 C<string> The actual text of the comment.
@@ -826,6 +834,16 @@ that id.
 
 =back
 
+=item B<History>
+
+=over
+
+=item Added in Bugzilla B<3.4>.
+
+=item C<attachment_id> was added to the return value in Bugzilla B<3.6>.
+
+=back
+
 =back
 
 

attachment.cgi

@@ -483,11 +483,10 @@ sub insert {
     $attachment->update($timestamp);
 
     # 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');
-
-    $bug->add_comment($comment, { isprivate => $attachment->isprivate });
+    my $comment = $cgi->param('comment');
+    $bug->add_comment($comment, { isprivate => $attachment->isprivate,
+                                  type => CMT_ATTACHMENT_CREATED,
+                                  extra_data => $attachment->id });
 
   # Assign the bug to the user, if they are allowed to take it
   my $owner = "";

importxml.pl

@@ -523,6 +523,8 @@ sub process_bug {
             $data = decode_base64($data);
         }
 
+        # For backwards-compatibility with Bugzillas before 3.6:
+        #
         # If we leave the attachment ID in the comment it will be made a link
         # to the wrong attachment. Since the new attachment ID is unknown yet
         # let's strip it out for now. We will make a comment with the right ID

post_bug.cgi

@@ -223,19 +223,9 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
                                       $bug, $attachment, $vars, SKIP_REQUESTEE_ON_ERROR);
         $attachment->set_flags($flags, $new_flags);
         $attachment->update($timestamp);
-
-        # Update the comment to include the new attachment ID.
-        # This string is hardcoded here because Template::quoteUrls()
-        # expects to find this exact string.
-        my $new_comment = "Created an attachment (id=" . $attachment->id . ")\n" .
-                          $attachment->description . "\n";
-        # We can use $bug->comments here because we are sure that the bug
-        # description is of type CMT_NORMAL. No need to include it if it's
-        # empty, though.
-        if ($bug->comments->[0]->body !~ /^\s+$/) {
-            $new_comment .= "\n" . $bug->comments->[0]->body;
-        }
-        $bug->update_comment($bug->comments->[0]->id, $new_comment);
+        my $comment = $bug->comments->[0];
+        $comment->set_type(CMT_ATTACHMENT_CREATED, $attachment->id);
+        $comment->update();
     }
     else {
         $vars->{'message'} = 'attachment_creation_failed';

template/en/default/bug/format_comment.txt.tmpl

@@ -32,11 +32,6 @@
 [% PROCESS 'global/variables.none.tmpl' %]
 
 [% SET comment_body = comment.body %]
-[% IF is_bugmail %]
-  [% comment_body = comment_body.replace( '(Created an attachment \(id=([0-9]+)\))',
-                                          '$1' _ "\n" _ ' --> (' _ urlbase 
-                                          _ 'attachment.cgi?id=$2)' ) %]
-[% END %]
 
 [% IF comment.type == constants.CMT_DUPE_OF %]
 X[% comment_body %]
@@ -54,6 +49,14 @@ If the move succeeded, [% comment.extra_data %] will receive a mail containing
 the number of the new [% terms.bug %] in the other database.
 If all went well, please paste in a link to the new [% terms.bug %].
 Otherwise, reopen this [% terms.bug %].
+[% ELSIF comment.type == constants.CMT_ATTACHMENT_CREATED %]
+Created attachment [% comment.extra_data %]
+[% IF is_bugmail %]
+  --> [% urlbase _ "attachment.cgi?id=" _ comment.extra_data %]
+[% END %]
+[%+ comment.attachment.description %]
+
+[%+ comment.body %]
 [% ELSE %]
 X[% comment_body %]
 [% END %]

template/en/default/bug/show.xml.tmpl

@@ -69,6 +69,9 @@
           [% NEXT IF c.is_private && !user.is_insider %]
           <long_desc isprivate="[% c.is_private FILTER xml %]">
             <commentid>[% c.id FILTER xml %]</commentid>
+            [% IF c.is_about_attachment %]
+              <attachid>[% c.extra_data FILTER xml %]</attachid>
+            [% END %]
             <who name="[% c.author.name FILTER xml %]">[% c.author.email FILTER email FILTER xml %]</who>
             <bug_when>[% c.creation_ts FILTER time("%Y-%m-%d %T %z") FILTER xml %]</bug_when>
             [% IF user.is_timetracker && (c.work_time - 0 != 0) %]

template/en/default/global/code-error.html.tmpl

@@ -120,6 +120,23 @@
     without specifying a default or something for $set_nulls_to, because
     there are NULL values currently in it.
 
+  [% ELSIF error == "comment_extra_data_not_allowed" %]
+    You tried to set the <code>extra_data</code> field to 
+    '[% extra_data FILTER html %]' but comments of type [% type FILTER html %]
+    do not accept an <code>extra_data</code> argument.
+
+  [% ELSIF error == "comment_extra_data_required" %]
+    Comments of type [% type FILTER html %] require an <code>extra_data</code>
+    argument to be set.
+
+  [% ELSIF error == "comment_extra_data_not_numeric" %]
+    You tried to set the <code>extra_data</code> field to
+    '[% extra_data FILTER html %]' but comments of type [% type FILTER html %]
+    require a numeric <code>extra_data</code> argument.
+
+  [% ELSIF error == "comment_type_invalid" %]
+    '[% type FILTER html %]' is not a valid comment type.
+
   [% ELSIF error == "db_rename_conflict" %]
     Name conflict: Cannot rename [% old FILTER html %] to
     [% new FILTER html %] because [% new FILTER html %] already exists.

template/en/default/global/user-error.html.tmpl

@@ -1721,7 +1721,9 @@
 [% PROCESS global/footer.html.tmpl %]
 
 [% BLOCK object_name %]
-  [% IF class == "Bugzilla::User" %]
+  [% IF class == "Bugzilla::Attachment" %]
+    attachment
+  [% ELSIF class == "Bugzilla::User" %]
     user
   [% ELSIF class == "Bugzilla::Component" %]
     component