English string "Created an attachment" in localized installations

RESOLVED FIXED in Bugzilla 3.6

Status

()

--
minor
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: cedric.corazza, Assigned: mkanat)

Tracking

({l12y})

Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

(Whiteboard: [es-ita])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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>|
(Assignee)

Comment 1

10 years ago
I'm surprised, I would have thought we'd fixed this already.
Severity: normal → minor
(Assignee)

Comment 2

10 years ago
I mean, filed this already, not fixed this already. :-)
(Assignee)

Updated

9 years ago
Blocks: 381912
(Assignee)

Updated

9 years ago
Depends on: 472217
(Assignee)

Updated

9 years ago
Assignee: attach-and-request → mkanat
Status: NEW → ASSIGNED
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6
(Assignee)

Updated

9 years ago
Summary: English strings "Created an attachment" and "details" in localized installations → English string "Created an attachment" in localized installations
Whiteboard: [es-ita]
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Updated

9 years ago
Blocks: 228846
(Assignee)

Updated

9 years ago
Blocks: 317127
(Assignee)

Comment 4

9 years ago
Created attachment 410464 [details] [diff] [review]
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.
Attachment #410464 - Flags: review?(dkl)
Attachment #410464 - Flags: review?(LpSolit)
(Assignee)

Updated

9 years ago
Blocks: 526734
(Assignee)

Comment 5

9 years ago
Created attachment 411306 [details] [diff] [review]
v2

Update the code to work with the latest code from the blocker.
Attachment #410464 - Attachment is obsolete: true
Attachment #411306 - Flags: review?(dkl)
Attachment #411306 - Flags: review?(LpSolit)
Attachment #410464 - Flags: review?(dkl)
Attachment #410464 - Flags: review?(LpSolit)

Comment 6

9 years ago
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)
Attachment #411306 - Flags: review?(dkl)
Attachment #411306 - Flags: review?(LpSolit)
Attachment #411306 - Flags: review-

Updated

9 years ago
Whiteboard: [es-ita] → [es-ita] [needs new patch asap]
(Assignee)

Comment 7

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
Created attachment 414938 [details] [diff] [review]
v3

Here's the updated patch.
Attachment #411306 - Attachment is obsolete: true
Attachment #414938 - Flags: review?(LpSolit)
(Assignee)

Updated

9 years ago
Whiteboard: [es-ita] [needs new patch asap] → [es-ita]

Comment 9

9 years ago
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
Attachment #414938 - Flags: review?(LpSolit) → review+

Updated

9 years ago
Flags: approval+
(Assignee)

Comment 10

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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

9 years ago
(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.
I will check. Though making the sql even more complex is not the right direction.
(Assignee)

Comment 14

9 years ago
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.

Updated

9 years ago
Duplicate of this bug: 564160
You need to log in before you can comment on or make changes to this bug.