From c1d01f36129ccfa0f76f4681058b10a348a6e9a2 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Fri, 6 Aug 2010 12:41:54 +0200 Subject: Bug 466968: Remove hardcoded strings from BugMail.pm, and refactor it so that bugmails are 100% localizable r/a=mkanat --- Bugzilla/BugMail.pm | 295 +++++++++++++++++---------------------------------- Bugzilla/Template.pm | 43 ++++++++ Bugzilla/User.pm | 63 +++++------ Bugzilla/Util.pm | 20 +--- 4 files changed, 169 insertions(+), 252 deletions(-) (limited to 'Bugzilla') diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm index 5688c1e7f..84bb62e3c 100644 --- a/Bugzilla/BugMail.pm +++ b/Bugzilla/BugMail.pm @@ -28,6 +28,7 @@ # Gervase Markham # Byron Jones # Reed Loden +# Frédéric Buclin use strict; @@ -38,58 +39,18 @@ use Bugzilla::User; use Bugzilla::Constants; use Bugzilla::Util; use Bugzilla::Bug; -use Bugzilla::Classification; -use Bugzilla::Product; -use Bugzilla::Component; -use Bugzilla::Status; +use Bugzilla::Comment; use Bugzilla::Mailer; use Bugzilla::Hook; use Date::Parse; use Date::Format; - -use constant FORMAT_TRIPLE => "%19s|%-28s|%-28s"; -use constant FORMAT_3_SIZE => [19,28,28]; -use constant FORMAT_DOUBLE => "%19s %-55s"; -use constant FORMAT_2_SIZE => [19,55]; +use Scalar::Util qw(blessed); +use List::MoreUtils qw(uniq); use constant BIT_DIRECT => 1; use constant BIT_WATCHING => 2; -# We use this instead of format because format doesn't deal well with -# multi-byte languages. -sub multiline_sprintf { - my ($format, $args, $sizes) = @_; - my @parts; - my @my_sizes = @$sizes; # Copy this so we don't modify the input array. - foreach my $string (@$args) { - my $size = shift @my_sizes; - my @pieces = split("\n", wrap_hard($string, $size)); - push(@parts, \@pieces); - } - - my $formatted; - while (1) { - # Get the first item of each part. - my @line = map { shift @$_ } @parts; - # If they're all undef, we're done. - last if !grep { defined $_ } @line; - # Make any single undef item into '' - @line = map { defined $_ ? $_ : '' } @line; - # And append a formatted line - $formatted .= sprintf($format, @line); - # Remove trailing spaces, or they become lots of =20's in - # quoted-printable emails. - $formatted =~ s/\s+$//; - $formatted .= "\n"; - } - return $formatted; -} - -sub three_columns { - return multiline_sprintf(FORMAT_TRIPLE, \@_, FORMAT_3_SIZE); -} - sub relationships { my $ref = RELATIONSHIPS; # Clone it so that we don't modify the constant; @@ -143,22 +104,26 @@ sub Send { push(@ccs, Bugzilla::User->check($cc)); } } - - my ($diffparts, $changedfields, $diffs) = - $params->{dep_only} ? ([], [], []) : _get_diffs($bug, $end); - - if ($params->{dep_only} && $start) { - my $blocker_id = $params->{blocker}->id; - my $deptext = - "\nBug " . $bug->id . " depends on bug $blocker_id, which changed state.\n\n" . - "Bug $blocker_id Summary: " . $params->{blocker}->short_desc . "\n" . - correct_urlbase() . "show_bug.cgi?id=$blocker_id\n\n"; - $deptext .= three_columns("What ", "Old Value", "New Value"); - $deptext .= ('-' x 76) . "\n"; - $deptext .= three_columns('Status', @{$params->{changes}->{bug_status}}); - $deptext .= three_columns('Resolution', @{$params->{changes}->{resolution}}); - - push(@$diffparts, {text => $deptext}); + + my @diffs; + if (!$start) { + @diffs = _get_new_bugmail_fields($bug); + } + + if ($params->{dep_only}) { + push(@diffs, { field_name => 'bug_status', + old => $params->{changes}->{bug_status}->[0], + new => $params->{changes}->{bug_status}->[1], + login_name => $changer->login, + blocker => $params->{blocker} }, + { field_name => 'resolution', + old => $params->{changes}->{resolution}->[0], + new => $params->{changes}->{resolution}->[1], + login_name => $changer->login, + blocker => $params->{blocker} }); + } + else { + push(@diffs, _get_diffs($bug, $end)); } my $comments = $bug->comments({ after => $start, to => $end }); @@ -194,24 +159,23 @@ sub Send { # The last relevant set of people are those who are being removed from # their roles in this change. We get their names out of the diffs. - foreach my $ref (@$diffs) { - my ($who, $whoname, $what, $when, $old, $new) = (@$ref); - if ($old) { + foreach my $change (@diffs) { + if ($change->{old}) { # You can't stop being the reporter, so we don't check that # relationship here. # Ignore people whose user account has been deleted or renamed. - if ($what eq "CC") { - foreach my $cc_user (split(/[\s,]+/, $old)) { + if ($change->{field_name} eq 'cc') { + foreach my $cc_user (split(/[\s,]+/, $change->{old})) { my $uid = login_to_id($cc_user); $recipients{$uid}->{+REL_CC} = BIT_DIRECT if $uid; } } - elsif ($what eq "QAContact") { - my $uid = login_to_id($old); + elsif ($change->{field_name} eq 'qa_contact') { + my $uid = login_to_id($change->{old}); $recipients{$uid}->{+REL_QA} = BIT_DIRECT if $uid; } - elsif ($what eq "AssignedTo") { - my $uid = login_to_id($old); + elsif ($change->{field_name} eq 'assigned_to') { + my $uid = login_to_id($change->{old}); $recipients{$uid}->{+REL_ASSIGNEE} = BIT_DIRECT if $uid; } } @@ -251,9 +215,6 @@ sub Send { my @sent; my @excluded; - # Only used for headers in bugmail for new bugs - my @fields = $start ? () : Bugzilla->get_fields({obsolete => 0, mailhead => 1}); - foreach my $user_id (keys %recipients) { my %rels_which_want; my $sent_mail = 0; @@ -265,13 +226,12 @@ sub Send { # Go through each role the user has and see if they want mail in # that role. foreach my $relationship (keys %{$recipients{$user_id}}) { - if ($user->wants_bug_mail($id, + if ($user->wants_bug_mail($bug, $relationship, - $diffs, + $start ? \@diffs : [], $comments, $params->{dep_only}, - $changer, - !$start)) + $changer)) { $rels_which_want{$relationship} = $recipients{$user_id}->{$relationship}; @@ -291,27 +251,23 @@ sub Send { $dep_ok = $user->can_see_bug($params->{blocker}->id) ? 1 : 0; } - # Make sure the user isn't in the nomail list, and the insider and - # dep checks passed. + # Make sure the user isn't in the nomail list, and the dep check passed. if ($user->email_enabled && $dep_ok) { # OK, OK, if we must. Email the user. $sent_mail = sendMail( { to => $user, - fields => \@fields, bug => $bug, comments => $comments, - is_new => !$start, delta_ts => $params->{dep_only} ? $end : undef, changer => $changer, watchers => exists $watching{$user_id} ? $watching{$user_id} : undef, - diff_parts => $diffparts, + diffs => \@diffs, rels_which_want => \%rels_which_want, - changed_fields => $changedfields, }); } } - + if ($sent_mail) { push(@sent, $user->login); } @@ -336,96 +292,38 @@ sub sendMail { my $params = shift; my $user = $params->{to}; - my @fields = @{ $params->{fields} }; my $bug = $params->{bug}; my @send_comments = @{ $params->{comments} }; - my $isnew = $params->{is_new}; my $delta_ts = $params->{delta_ts}; my $changer = $params->{changer}; my $watchingRef = $params->{watchers}; - my @diffparts = @{ $params->{diff_parts} }; + my @diffs = @{ $params->{diffs} }; my $relRef = $params->{rels_which_want}; - my @changed_fields = @{ $params->{changed_fields} }; - # Build difftext (the actions) by verifying the user should see them - my $difftext = ""; - my $diffheader = ""; - my $add_diff; + # Only display changes the user is allowed see. + my @display_diffs; - foreach my $diff (@diffparts) { - $add_diff = 0; + foreach my $diff (@diffs) { + my $add_diff = 0; - if (exists($diff->{'fieldname'}) && - ($diff->{'fieldname'} eq 'estimated_time' || - $diff->{'fieldname'} eq 'remaining_time' || - $diff->{'fieldname'} eq 'work_time' || - $diff->{'fieldname'} eq 'deadline')) - { + if (grep { $_ eq $diff->{field_name} } TIMETRACKING_FIELDS) { $add_diff = 1 if $user->is_timetracker; - } elsif ($diff->{'isprivate'} - && !$user->is_insider) - { - $add_diff = 0; - } else { - $add_diff = 1; } - - if ($add_diff) { - if (exists($diff->{'header'}) && - ($diffheader ne $diff->{'header'})) { - $diffheader = $diff->{'header'}; - $difftext .= $diffheader; - } - $difftext .= $diff->{'text'}; + elsif (!$diff->{isprivate} || $user->is_insider) { + $add_diff = 1; } + push(@display_diffs, $diff) if $add_diff; } if (!$user->is_insider) { @send_comments = grep { !$_->is_private } @send_comments; } - if ($difftext eq "" && !scalar(@send_comments) && !$isnew) { + if (!scalar(@display_diffs) && !scalar(@send_comments)) { # Whoops, no differences! return 0; } - my $diffs = $difftext; - # Remove extra newlines. - $diffs =~ s/^\n+//s; $diffs =~ s/\n+$//s; - if ($isnew) { - my $head = ""; - foreach my $field (@fields) { - my $name = $field->name; - my $value = $bug->$name; - - if (ref $value eq 'ARRAY') { - $value = join(', ', @$value); - } - elsif (ref $value && $value->isa('Bugzilla::User')) { - $value = $value->login; - } - elsif (ref $value && $value->isa('Bugzilla::Object')) { - $value = $value->name; - } - elsif ($name eq 'estimated_time') { - $value = ($value == 0) ? 0 : format_time_decimal($value); - } - elsif ($name eq 'deadline') { - $value = time2str("%Y-%m-%d", str2time($value)) if $value; - } - - # If there isn't anything to show, don't include this header. - next unless $value; - # Only send estimated_time if it is enabled and the user is in the group. - if (($name ne 'estimated_time' && $name ne 'deadline') || $user->is_timetracker) { - my $desc = $field->description; - $head .= multiline_sprintf(FORMAT_DOUBLE, ["$desc:", $value], - FORMAT_2_SIZE); - } - } - $diffs = $head . ($difftext ? "\n\n" : "") . $diffs; - } - my (@reasons, @reasons_watch); while (my ($relationship, $bits) = each %{$relRef}) { push(@reasons, $relationship) if ($bits & BIT_DIRECT); @@ -440,19 +338,18 @@ sub sendMail { push @watchingrel, map { user_id_to_login($_) } @$watchingRef; my $vars = { - isnew => $isnew, delta_ts => $delta_ts, to_user => $user, bug => $bug, - changedfields => \@changed_fields, reasons => \@reasons, reasons_watch => \@reasons_watch, reasonsheader => join(" ", @headerrel), reasonswatchheader => join(" ", @watchingrel), changer => $changer, - diffs => $diffs, + diffs => \@display_diffs, + changedfields => [uniq map { $_->{field_name} } @display_diffs], new_comments => \@send_comments, - threadingmarker => build_thread_marker($bug->id, $user->id, $isnew), + threadingmarker => build_thread_marker($bug->id, $user->id, !$bug->lastdiffed), }; my $msg; @@ -478,9 +375,9 @@ sub _get_diffs { } my $diffs = $dbh->selectall_arrayref( - "SELECT profiles.login_name, profiles.realname, fielddefs.description, - bugs_activity.bug_when, bugs_activity.removed, - bugs_activity.added, bugs_activity.attach_id, fielddefs.name, + "SELECT profiles.login_name, profiles.realname, fielddefs.name AS field_name, + bugs_activity.bug_when, bugs_activity.removed AS old, + bugs_activity.added AS new, bugs_activity.attach_id, bugs_activity.comment_id FROM bugs_activity INNER JOIN fielddefs @@ -489,50 +386,58 @@ sub _get_diffs { ON profiles.userid = bugs_activity.who WHERE bugs_activity.bug_id = ? $when_restriction - ORDER BY bugs_activity.bug_when", undef, @args); - - my $difftext = ""; - my $diffheader = ""; - my @diffparts; - my $lastwho = ""; - my $fullwho; - my @changedfields; - foreach my $ref (@$diffs) { - my ($who, $whoname, $what, $when, $old, $new, $attachid, $fieldname, $comment_id) = @$ref; - my $diffpart = {}; - if ($who ne $lastwho) { - $lastwho = $who; - $fullwho = $whoname ? "$whoname <$who>" : $who; - $diffheader = "\n$fullwho changed:\n\n"; - $diffheader .= three_columns("What ", "Removed", "Added"); - $diffheader .= ('-' x 76) . "\n"; + ORDER BY bugs_activity.bug_when", {Slice=>{}}, @args); + + foreach my $diff (@$diffs) { + if ($diff->{attach_id}) { + $diff->{isprivate} = $dbh->selectrow_array( + 'SELECT isprivate FROM attachments WHERE attach_id = ?', + undef, $diff->{attach_id}); + } + if ($diff->{field_name} eq 'longdescs.isprivate') { + my $comment = Bugzilla::Comment->new($diff->{comment_id}); + $diff->{num} = $comment->count; + $diff->{isprivate} = $diff->{new}; + } + } + + return @$diffs; +} + +sub _get_new_bugmail_fields { + my $bug = shift; + my @fields = Bugzilla->get_fields({obsolete => 0, mailhead => 1}); + my @diffs; + + foreach my $field (@fields) { + my $name = $field->name; + my $value = $bug->$name; + + if (ref $value eq 'ARRAY') { + $value = join(', ', @$value); } - $what =~ s/^(Attachment )?/Attachment #$attachid / if $attachid; - if( $fieldname eq 'estimated_time' || - $fieldname eq 'remaining_time' ) { - $old = format_time_decimal($old); - $new = format_time_decimal($new); + elsif (blessed($value) && $value->isa('Bugzilla::User')) { + $value = $value->login; } - if ($attachid) { - ($diffpart->{'isprivate'}) = $dbh->selectrow_array( - 'SELECT isprivate FROM attachments WHERE attach_id = ?', - undef, ($attachid)); + elsif (blessed($value) && $value->isa('Bugzilla::Object')) { + $value = $value->name; + } + elsif ($name eq 'estimated_time') { + # "0.00" (which is what we get from the DB) is true, + # so we explicitly do a numerical comparison with 0. + $value = 0 if $value == 0; } - if ($fieldname eq 'longdescs.isprivate') { - my $comment = Bugzilla::Comment->new($comment_id); - my $comment_num = $comment->count; - $what =~ s/^(Comment )?/Comment #$comment_num /; - $diffpart->{'isprivate'} = $new; + elsif ($name eq 'deadline') { + $value = time2str("%Y-%m-%d", str2time($value)) if $value; } - $difftext = three_columns($what, $old, $new); - $diffpart->{'header'} = $diffheader; - $diffpart->{'fieldname'} = $fieldname; - $diffpart->{'text'} = $difftext; - push(@diffparts, $diffpart); - push(@changedfields, $what); + + # If there isn't anything to show, don't include this header. + next unless $value; + + push(@diffs, {field_name => $name, new => $value}); } - return (\@diffparts, \@changedfields, $diffs); + return @diffs; } 1; diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index 923336d45..c964dd022 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -61,6 +61,11 @@ use Scalar::Util qw(blessed); use base qw(Template); +use constant FORMAT_TRIPLE => '%19s|%-28s|%-28s'; +use constant FORMAT_3_SIZE => [19,28,28]; +use constant FORMAT_DOUBLE => '%19s %-55s'; +use constant FORMAT_2_SIZE => [19,55]; + # Convert the constants in the Bugzilla::Constants module into a hash we can # pass to the template object for reflection into its "constants" namespace # (which is like its "variables" namespace, but for constants). To do so, we @@ -352,6 +357,36 @@ sub get_bug_link { return qq{$pre$link_text$post}; } +# We use this instead of format because format doesn't deal well with +# multi-byte languages. +sub multiline_sprintf { + my ($format, $args, $sizes) = @_; + my @parts; + my @my_sizes = @$sizes; # Copy this so we don't modify the input array. + foreach my $string (@$args) { + my $size = shift @my_sizes; + my @pieces = split("\n", wrap_hard($string, $size)); + push(@parts, \@pieces); + } + + my $formatted; + while (1) { + # Get the first item of each part. + my @line = map { shift @$_ } @parts; + # If they're all undef, we're done. + last if !grep { defined $_ } @line; + # Make any single undef item into '' + @line = map { defined $_ ? $_ : '' } @line; + # And append a formatted line + $formatted .= sprintf($format, @line); + # Remove trailing spaces, or they become lots of =20's in + # quoted-printable emails. + $formatted =~ s/\s+$//; + $formatted .= "\n"; + } + return $formatted; +} + ##################### # Header Generation # ##################### @@ -833,6 +868,14 @@ sub create { # Function to create date strings 'time2str' => \&Date::Format::time2str, + # Fixed size column formatting for bugmail. + 'format_columns' => sub { + my $cols = shift; + my $format = ($cols == 3) ? FORMAT_TRIPLE : FORMAT_DOUBLE; + my $col_size = ($cols == 3) ? FORMAT_3_SIZE : FORMAT_2_SIZE; + return multiline_sprintf($format, \@_, $col_size); + }, + # Generic linear search function 'lsearch' => sub { my ($array, $item) = @_; diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 2bbe63101..595964bf9 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -1492,35 +1492,33 @@ sub match_field { } -# Changes in some fields automatically trigger events. The 'field names' are -# from the fielddefs table. We really should be using proper field names -# throughout. +# Changes in some fields automatically trigger events. The field names are +# from the fielddefs table. our %names_to_events = ( - 'Resolution' => EVT_OPENED_CLOSED, - 'Keywords' => EVT_KEYWORD, - 'CC' => EVT_CC, - 'Severity' => EVT_PROJ_MANAGEMENT, - 'Priority' => EVT_PROJ_MANAGEMENT, - 'Status' => EVT_PROJ_MANAGEMENT, - 'Target Milestone' => EVT_PROJ_MANAGEMENT, - 'Attachment description' => EVT_ATTACHMENT_DATA, - 'Attachment mime type' => EVT_ATTACHMENT_DATA, - 'Attachment is patch' => EVT_ATTACHMENT_DATA, - 'Depends on' => EVT_DEPEND_BLOCK, - 'Blocks' => EVT_DEPEND_BLOCK); + 'resolution' => EVT_OPENED_CLOSED, + 'keywords' => EVT_KEYWORD, + 'cc' => EVT_CC, + 'bug_severity' => EVT_PROJ_MANAGEMENT, + 'priority' => EVT_PROJ_MANAGEMENT, + 'bug_status' => EVT_PROJ_MANAGEMENT, + 'target_milestone' => EVT_PROJ_MANAGEMENT, + 'attachments.description' => EVT_ATTACHMENT_DATA, + 'attachments.mimetype' => EVT_ATTACHMENT_DATA, + 'attachments.ispatch' => EVT_ATTACHMENT_DATA, + 'dependson' => EVT_DEPEND_BLOCK, + 'blocked' => EVT_DEPEND_BLOCK); # Returns true if the user wants mail for a given bug change. # Note: the "+" signs before the constants suppress bareword quoting. sub wants_bug_mail { my $self = shift; - my ($bug_id, $relationship, $fieldDiffs, $comments, $dep_mail, - $changer, $bug_is_new) = @_; + my ($bug, $relationship, $fieldDiffs, $comments, $dep_mail, $changer) = @_; # Make a list of the events which have happened during this bug change, # from the point of view of this user. my %events; - foreach my $ref (@$fieldDiffs) { - my ($who, $whoname, $fieldName, $when, $old, $new) = @$ref; + foreach my $change (@$fieldDiffs) { + my $fieldName = $change->{field_name}; # A change to any of the above fields sets the corresponding event if (defined($names_to_events{$fieldName})) { $events{$names_to_events{$fieldName}} = 1; @@ -1532,16 +1530,16 @@ sub wants_bug_mail { # If the user is in a particular role and the value of that role # changed, we need the ADDED_REMOVED event. - if (($fieldName eq "AssignedTo" && $relationship == REL_ASSIGNEE) || - ($fieldName eq "QAContact" && $relationship == REL_QA)) + if (($fieldName eq "assigned_to" && $relationship == REL_ASSIGNEE) || + ($fieldName eq "qa_contact" && $relationship == REL_QA)) { $events{+EVT_ADDED_REMOVED} = 1; } - if ($fieldName eq "CC") { + if ($fieldName eq "cc") { my $login = $self->login; - my $inold = ($old =~ /^(.*,\s*)?\Q$login\E(,.*)?$/); - my $innew = ($new =~ /^(.*,\s*)?\Q$login\E(,.*)?$/); + my $inold = ($change->{old} =~ /^(.*,\s*)?\Q$login\E(,.*)?$/); + my $innew = ($change->{new} =~ /^(.*,\s*)?\Q$login\E(,.*)?$/); if ($inold != $innew) { $events{+EVT_ADDED_REMOVED} = 1; @@ -1549,7 +1547,7 @@ sub wants_bug_mail { } } - if ($bug_is_new) { + if (!$bug->lastdiffed) { # Notify about new bugs. $events{+EVT_BUG_CREATED} = 1; @@ -1589,19 +1587,8 @@ sub wants_bug_mail { $wants_mail &= $self->wants_mail([EVT_CHANGED_BY_ME], $relationship); } - if ($wants_mail) { - my $dbh = Bugzilla->dbh; - # We don't create a Bug object from the bug_id here because we only - # need one piece of information, and doing so (as of 2004-11-23) slows - # down bugmail sending by a factor of 2. If Bug creation was more - # lazy, this might not be so bad. - my $bug_status = $dbh->selectrow_array('SELECT bug_status - FROM bugs WHERE bug_id = ?', - undef, $bug_id); - - if ($bug_status eq "UNCONFIRMED") { - $wants_mail &= $self->wants_mail([EVT_UNCONFIRMED], $relationship); - } + if ($wants_mail && $bug->bug_status eq 'UNCONFIRMED') { + $wants_mail &= $self->wants_mail([EVT_UNCONFIRMED], $relationship); } return $wants_mail; diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index f5ab51d2b..6f5686a3b 100644 --- a/Bugzilla/Util.pm +++ b/Bugzilla/Util.pm @@ -39,8 +39,7 @@ use base qw(Exporter); do_ssl_redirect_if_required use_attachbase diff_arrays on_main_db trim wrap_hard wrap_comment find_wrap_point - format_time format_time_decimal validate_date - validate_time datetime_from + format_time validate_date validate_time datetime_from file_mod_time is_7bit_clean bz_crypt generate_random_password validate_email_syntax clean_text @@ -466,18 +465,6 @@ sub datetime_from { return $dt; } -sub format_time_decimal { - my ($time) = (@_); - - my $newtime = sprintf("%.2f", $time); - - if ($newtime =~ /0\Z/) { - $newtime = sprintf("%.1f", $time); - } - - return $newtime; -} - sub file_mod_time { my ($filename) = (@_); my ($dev,$ino,$mode,$nlink,$uid,$gid,$rdev,$size, @@ -935,11 +922,6 @@ is used, as defined in his preferences. This routine is mainly called from templates to filter dates, see "FILTER time" in L. -=item C - -Returns a number with 2 digit precision, unless the last digit is a 0. Then it -returns only 1 digit precision. - =item C Returns a DateTime object given a date string. If the string is not in some -- cgit v1.2.3-24-g4f1b