From bc19204d3dffa448b364bfa4b5691a24f39f6765 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Fri, 4 Dec 2009 14:28:47 +0000 Subject: Bug 452919: Allow the "created an attachment" message in comments to be localized Patch by Max Kanat-Alexander r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 32 +------ Bugzilla/Comment.pm | 110 +++++++++++++++++++++++- Bugzilla/Constants.pm | 2 + Bugzilla/Install/DB.pm | 83 ++++++++++++++++-- Bugzilla/Template.pm | 7 +- Bugzilla/User.pm | 3 +- Bugzilla/WebService/Bug.pm | 18 ++++ attachment.cgi | 9 +- importxml.pl | 2 + post_bug.cgi | 16 +--- template/en/default/bug/format_comment.txt.tmpl | 13 +-- template/en/default/bug/show.xml.tmpl | 3 + template/en/default/global/code-error.html.tmpl | 17 ++++ template/en/default/global/user-error.html.tmpl | 4 +- 14 files changed, 245 insertions(+), 74 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index d8134e3cb..d55549cd6 100644 --- a/Bugzilla/Bug.pm +++ b/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'); diff --git a/Bugzilla/Comment.pm b/Bugzilla/Comment.pm index 7072dbbee..f09985426 100644 --- a/Bugzilla/Comment.pm +++ b/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__ diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 0bce3ed2f..0be6d1efa 100644 --- a/Bugzilla/Constants.pm +++ b/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. diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index d7eb14c24..0ae909ecd 100644 --- a/Bugzilla/Install/DB.pm +++ b/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__ diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index f714f04d1..028914164 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -242,12 +242,7 @@ sub quoteUrls { $text =~ s~\b(mailto:|)?([\w\.\-\+\=]+\@[\w\-]+(?:\.[\w\-]+)+)\b ~$1$2~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") diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 0fc2db336..6a318c93f 100644 --- a/Bugzilla/User.pm +++ b/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])) { diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index 006fa0fee..5c38ebc49 100644 --- a/Bugzilla/WebService/Bug.pm +++ b/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 The globally unique ID for the comment. C The ID of the bug that this comment is on. +=item attachment_id + +C If the comment was made on an attachment, this will be the +ID of that attachment. Otherwise it will be null. + =item text C The actual text of the comment. @@ -826,6 +834,16 @@ that id. =back +=item B + +=over + +=item Added in Bugzilla B<3.4>. + +=item C was added to the return value in Bugzilla B<3.6>. + +=back + =back diff --git a/attachment.cgi b/attachment.cgi index a10d9f970..a89a46b99 100755 --- a/attachment.cgi +++ b/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 = ""; diff --git a/importxml.pl b/importxml.pl index c08f5c8b2..05f95d646 100755 --- a/importxml.pl +++ b/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 diff --git a/post_bug.cgi b/post_bug.cgi index c9de83bb7..ef0f40d4d 100755 --- a/post_bug.cgi +++ b/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'; diff --git a/template/en/default/bug/format_comment.txt.tmpl b/template/en/default/bug/format_comment.txt.tmpl index 8e97d4d08..49ab95ad1 100644 --- a/template/en/default/bug/format_comment.txt.tmpl +++ b/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 %] diff --git a/template/en/default/bug/show.xml.tmpl b/template/en/default/bug/show.xml.tmpl index 31e56d299..2349602dc 100644 --- a/template/en/default/bug/show.xml.tmpl +++ b/template/en/default/bug/show.xml.tmpl @@ -69,6 +69,9 @@ [% NEXT IF c.is_private && !user.is_insider %] [% c.id FILTER xml %] + [% IF c.is_about_attachment %] + [% c.extra_data FILTER xml %] + [% END %] [% c.author.email FILTER email FILTER xml %] [% c.creation_ts FILTER time("%Y-%m-%d %T %z") FILTER xml %] [% IF user.is_timetracker && (c.work_time - 0 != 0) %] diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl index 09a1c4cf6..e851a00d9 100644 --- a/template/en/default/global/code-error.html.tmpl +++ b/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 extra_data field to + '[% extra_data FILTER html %]' but comments of type [% type FILTER html %] + do not accept an extra_data argument. + + [% ELSIF error == "comment_extra_data_required" %] + Comments of type [% type FILTER html %] require an extra_data + argument to be set. + + [% ELSIF error == "comment_extra_data_not_numeric" %] + You tried to set the extra_data field to + '[% extra_data FILTER html %]' but comments of type [% type FILTER html %] + require a numeric extra_data 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. diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 144e2e7ea..c3e84c0ab 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/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 -- cgit v1.2.3-24-g4f1b