452919 - English string "Created an attachment" in localized installations

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Cédric Corazza]

The strings "Created an attachment" and "details" are written in English in the comment field when creating and attachment in a bug in localized Bugzilla installations. See for instance : https://bugzilla.frenchmozilla.fr/show_bug.cgi?id=52#c0 .
Those strings should be localizable. Grepping the code returns me :

attachment.cgi:    my $comment = "Created an attachment (id=" . $attachment->id . ")\n" .
Bugzilla/BugMail.pm:    if ( $newcomments =~ /Created an attachment \(/ ) {
Bugzilla/BugMail.pm:        $newcomments =~ s/(Created an attachment \(id=([0-9]+)\))/$1\n --> \(${showattachurlbase}$2\)/g;
Bugzilla/User.pm:    if ($commentField =~ /Created an attachment \(/) {
Bugzilla/DB/Mysql.pm:                "Created an attachment (id=$attach_id)%");
importxml.pl:        $data =~ s/Created an attachment \(id=\d+\)/Created an attachment/g;
post_bug.cgi:        my $new_comment = "Created an attachment (id=" . $attachment->id . ")\n" .

Bugzilla/Template.pm:        $link_text =~ s/ \[details\]$//;
Bugzilla/Template.pm:               . qq| <a href="${linkval}&amp;action=edit" title="$title">[details]</a>|

Comment 1

[Max Kanat-Alexander]

I'm surprised, I would have thought we'd fixed this already.

Comment 2

[Max Kanat-Alexander]

I mean, filed this already, not fixed this already. :-)

Comment 3

[Max Kanat-Alexander]

The "details" part is very separate, code-wise, and will have to be fixed in another bug. For this bug, I'm going to fix the "Created an attachment" message.

Comment 4

[Max Kanat-Alexander]

Attachment: v1

This does several nice things:

1) It uses the Bugzilla::Comment object and traditional Bugzilla::Object methods to update the comment when filing a bug, eliminating update_comment from Bugzilla::Bug. This also allows much better validation of extra_data--validation which will be used more extensively in the future within Bugzilla::Bug during add_comment.

2) It changes the "Created an attachment" message to just use the standard "attachment 1234 [details]" highlight, and removes the special "Created an attachment" highlight.

3) It adds information about which attachment a comment was on to the bug XML and the WebService Bug.comments() interface.

4) It converts all old comments to the new system, in checksetup.pl.

This of course requires the Bugzilla::Comment patch in order to operate.

Comment 5

[Max Kanat-Alexander]

Attachment: v2

Update the code to work with the latest code from the blocker.

Comment 6

[Frédéric Buclin]

Comment on attachment 411306 [details] [diff] [review]
v2

>=== modified file 'Bugzilla/Comment.pm'

>+use constant VALIDATORS => {
>+    type => \&_check_type,
>+};
>+
>+use constant UPDATE_VALIDATORS => {
>+    extra_data => \&_check_extra_data,
>+};

Why is not extra_data added to VALIDATORS?


>+sub _check_extra_data {
>+    my ($invocant, $extra_data, $type) = @_;

Unless I miss something, you never pass $type to it.


>+    else {
>+        if (!defined $extra_data) {
>+            ThrowCodeError('comment_extra_data_required', { type => $type });
>+        }

Nit: I think you should merge them into elsif.


>+        if ($type == CMT_MOVED_TO) {

Nit: here to -> elsif


>+        else {
>+            detaint_natural($extra_data) 
>+              or ThrowCodeError('comment_extra_data_not_numeric',
>+                                { type => $type, extra_data => $extra_data });
>+        }

If $extra_data is not an integer, detaint_natural clears it and so the error message get an undefined value for extra_data.


>+sub _check_type {

>+    detaint_natural($type)
>+        or ThrowCodeError('comment_type_invalid', { type => $type });

Same problem here, $type becomes undefined if not an integer.



>=== modified file 'Bugzilla/Install/DB.pm'

> sub _populate_bugs_fulltext {

>+        my $limit = "";

Nit: I think $where would be clearer. I had LIMIT in mind when reading this variable name.


>+sub _move_attachment_creation_comments_into_comment_type {

>+    my $bug_ids = $dbh->selectcol_arrayref(
>+        'SELECT DISTINCT bug_id FROM longdescs WHERE comment_id IN('
>+        . join(',', @comment_ids) . ')');

You should use $dbh->sql_in() as it may contains several thousands of comments, and Oracle will fail.



>=== modified file 'Bugzilla/WebService/Bug.pm'

>+    my $attach_id = $comment->attachment ? $comment->attachment->id : undef;

It's overkill to create an attachment object to get the attachment ID only. Please write:

  my $attach_id = ($comment->type == CMT_ATTACHMENT_CREATED) ? $comment->extra_data : undef;

as extra_data has already been validated by the checker.



>=== modified file 'post_bug.cgi'

>+        $comment->set_type(CMT_ATTACHMENT_CREATED, $attachment->id);

This throws an error:

"You tried to set the extra_data field to '208' but comments of type extra_data do not accept an extra_data argument."

... of type extra_data?? Looks like the wrong argument was passed to the caller.



>=== modified file 'template/en/default/bug/show.xml.tmpl'

>+            [% IF c.attachment %]
>+              <attachid>[% c.attachment.id FILTER xml %]</attachid>
>+            [% END %]

Here too, this is overkill to create an attachment object to get its ID only. Please use the same trick as above.



I really like this cleanup; I wanted it for a long time! :)


Unrelated question: why is there so much upgrade code in DB/MySQL.pm itself, instead of being in Install/DB.pm? (I saw that when I found "Created an attachment" in DB/MySQL.pm)

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Why is not extra_data added to VALIDATORS?

  Because it requires an extra argument. There is no Bugzilla::Comment->create in use anywhere yet, but when there is, extra_data will have to go into run_create_validators, because it requires an extra argument.

> >+sub _check_extra_data {
> >+    my ($invocant, $extra_data, $type) = @_;
> 
> Unless I miss something, you never pass $type to it.

  Yeah, that will be done by Comment->create. I'm just preparing for it.

> >+    else {
> >+        if (!defined $extra_data) {
> >+            ThrowCodeError('comment_extra_data_required', { type => $type });
> >+        }
> 
> Nit: I think you should merge them into elsif.

  Mm, no, because all the types need the check for the *existence* of extra_data, but then each type also has its own checker below that.

> >+        if ($type == CMT_MOVED_TO) {
> 
> Nit: here to -> elsif

  No, that would be incorrect.

> It's overkill to create an attachment object to get the attachment ID only.
> Please write:
> 
>   my $attach_id = ($comment->type == CMT_ATTACHMENT_CREATED) ?
> $comment->extra_data : undef;
> 
> as extra_data has already been validated by the checker.

  Okay. Instead of this, I'm going to do $comment->is_about_attachment or something like that, because that will also handle the requirements of bug 526734 (the "updated an attachment" message).


  I've fixed everything else, and will be attaching a new patch.

> Unrelated question: why is there so much upgrade code in DB/MySQL.pm itself,
> instead of being in Install/DB.pm? (I saw that when I found "Created an
> attachment" in DB/MySQL.pm)

  Oh, because it's changing TIMESTAMP fields (which recorded the time the column was updated, and don't exist in other databases) to DATETIME fields, and the cleanup has to be done before the upgrade to 2.20.

Comment 8

[Max Kanat-Alexander]

Attachment: v3

Here's the updated patch.

Comment 9

[Frédéric Buclin]

Comment on attachment 414938 [details] [diff] [review]
v3

>=== modified file 'Bugzilla/Comment.pm'

>+sub is_about_attachment {
>+    my ($self) = @_;
>+    return 1 if $self->type == CMT_ATTACHMENT_CREATED;
>+    return 0;
>+}

Nit: return ($self->type == CMT_ATTACHMENT_CREATED) ? 1 : 0; is cleaner IMO.


>+sub attachment {
>+    my ($self) = @_;
>+    return undef if not $self->is_about_attachment;
>+    $self->{attachment} ||= new Bugzilla::Attachment($self->extra_data);
>+    return $self->{attachment};
>+}

Nit: I usually prefer having a single return in simple methods. From my point of view, this makes the code clearer, but maybe it's just me:

 my $self = shift;
 if ($self->is_about_attachment) {
     $self->{attachment} ||= new Bugzilla::Attachment($self->extra_data);
 }
 return $self->{attachment};



>=== modified file 'Bugzilla/Install/DB.pm'

> sub _populate_bugs_fulltext {

>+            print "Updating bugs_fulltext...\n";
>+            $where = "WHERE bugs.bug_id IN (" . join(',', @$bug_ids) . ")";

Use $dbh->sql_in() due to Oracle.


>+            $dbh->do("DELETE FROM bugs_fulltext WHERE bug_id IN (" 
>+                     . join(',', @$bug_ids) . ")");

Here too.


Looks good, and pass my tests. r=LpSolit

Comment 10

[Max Kanat-Alexander]

On checkin, I fixed the sql_in stuff, but I didn't make other other changes--I felt like it was actually more readable as it was, particularly considering what's coming up in the attachment-update patch.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.164; previous revision: 1.163
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.94; previous revision: 1.93
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.207; previous revision: 1.206
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.302; previous revision: 1.301
done
Checking in Bugzilla/Comment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Comment.pm,v  <--  Comment.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.122; previous revision: 1.121
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.121; previous revision: 1.120
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.197; previous revision: 1.196
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.76; previous revision: 1.75
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.47; previous revision: 1.46
done
Checking in template/en/default/bug/format_comment.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/format_comment.txt.tmpl,v  <--  format_comment.txt.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/bug/show.xml.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--  show.xml.tmpl
new revision: 1.37; previous revision: 1.36
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.126; previous revision: 1.125
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.289; previous revision: 1.288
done

Comment 11

[Michael Thomas (Mockodin)]

This will cause problems in the future with mssql, the massive WHERE IN causes mssql to throw:

[Microsoft][SQL Server Native Client 10.0][SQL Server]The query processor ran out of internal resources and could not produce a query plan. This is a rare event and only expected for extremely complex queries or queries that reference a very large number of tables or partitions. Please simplify the query. If you believe you have received this message in error, contact Customer Support Services for more information. (SQL-42000)

Comment 12

[Frédéric Buclin]

(In reply to comment #11)
> This will cause problems in the future with mssql, the massive WHERE IN causes
> mssql to throw:

Maybe you have to customize sql_in() for MS-SQL, as for Oracle.

Comment 13

[Michael Thomas (Mockodin)]

I will check. Though making the sql even more complex is not the right direction.

Comment 14

[Max Kanat-Alexander]

Michael: You will have to customize sql_in. You're pointing out a single location where we use in, when there are like FIFTY locations that we use it and where there could be thousands of items there. So you'll have to customize sql_in.