297186 - Send emails in the recipient's locale, not the current user's

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Emmanuel Seyman]

I'm working on Bugzilla i18n, especially trying to make statuses and resolutions
appear in the same language as the rest of Bugzilla. Bug #215148 was a bug part
of this and I'm now trying to make all parts of Bugzilla use localized statuses
and resolutions. One difficulty I'm having is localizing the emails that
Bugzilla sends out. Right now, the contents of the mails are contained in Params.

Mkanat has filed bugs to templatise the four types of emails that Bugzilla emits
but I realized that simply using templates will send out mails in the language
in which the person responsible for the mail-sending is using Bugzilla. See bug
#275637 has all the details.

I'ld like to have BugMail be able to select a language that a user wants to
receive mail and use the templates from that language to send mail to that user.

Comment 1

[Marc Schumann [:Wurblzap]]

One way to do this would be to store the last language a user requested in the
user profiles table. You could use this entry for e-mails.

Comment 2

[Emmanuel Seyman]

That's what I wanted to do, as per #275637c4 but mkanat prefers letting specify a 
language in their email settings. I spoke to Myk about this on IRC a few days
ago and he preferred having a global language setting, not just for email but
everything.

Comment 3

[u197037]

Agree with mkanat. Also, EmailPrefs could pre-set this parameter to current preferred language from browser, if unset.
Just my 2c.

Comment 4

[Frédéric Buclin]

We should have a user pref where you can specify your preferred language, based on languages available. Then BugMail.pm would look at this user pref to select the correct template.

We should have it for Bugzilla 3.0.

Comment 5

[Frédéric Buclin]

Requesting blocking3.0 for the following reason:

Now that all email templates have been templatized, an installation having several languages available would keep sending emails in the language used by the sender, which is not what we want (I guess mkanat or wurblzap would be unhappy getting emails in french everytime I change something).

In order to make these templatizations useful, we have to fix this bug for 3.0, else people will complain that 3.0 email system is completely buggy.

Comment 6

[Max Kanat-Alexander]

Yes, this is definitely a blocker, for the reasons LpSolit explained.

We'll also have to re-arch Bugzilla::Template to be able to specify a specific locale for the Template object, which is something we need in other places already, anyway.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> We'll also have to re-arch Bugzilla::Template to be able to specify a specific
> locale for the Template object

Yeah, I tried a simple fix a few days ago, but the problem is that Bugzilla->template calls Template->create() only once (when $_template is undefined), so passing the locale as an argument won't work, except when called for the first time where Bugzilla->template($locale) could be passed to ->create(). TT is the bottleneck when processing pages, so I'm not a fan of creating a Template object for each locale available. But maybe don't we have other choices?

We should make sure if there is a way to force the order in which folders are read, and make it read the desired language first.

Comment 8

[Max Kanat-Alexander]

We'll have to create a new template object for every person we're sending an email to, anyway, if we're going to be doing this. Yes, we can cache one template_inner per locale.

Comment 9

[Frédéric Buclin]

Attachment: non-working patch (work in progress)

For some reason, this patch makes template pre-compilation to fail:

file error - account/auth/login-small.html.tmpl: not found

The error is thrown by:

  $template->context->template($file);

in Bugzilla::Template::precompile_templates(). No idea what the problem is.

Comment 10

[Frédéric Buclin]

Attachment: patch, v1

This one is working fine. :)

Comment 11

[Frédéric Buclin]

Comment on attachment 245800 [details] [diff] [review]
patch, v1

This patch is awaiting review for too long. Max, have you some free time to look at it?

Comment 12

[Frédéric Buclin]

Attachment: patch, v2

It now display "special" comments in the right language.

Comment 13

[Frédéric Buclin]

Attachment: patch, v2.1

This patch updates the 'lang' user pref from editparams.cgi if the 'languages' param has changed. This way, you don't need to run checksetup.pl again.

Also, this means that checksetup.pl no longer has to update the list of valid values for 'lang' itself.

Comment 14

[Olav Vitters]

Comment on attachment 248164 [details] [diff] [review]
patch, v2.1

>Index: Bugzilla.pm

> sub template_inner {
>-    my $class = shift;
>-    $class->request_cache->{template_inner} ||= Bugzilla::Template->create();
>-    return $class->request_cache->{template_inner};
>+    my ($class, $lang) = @_;
>+    $lang = defined($lang) ? $lang : (request_cache()->{language} || "");

Nit: I still think not caching the language is cleaner and better follows how Bugzilla->template works. Even if you have to pass $lang via a few functions along to get_text.

>@@ -497,7 +500,8 @@ The current C<Template> object, to be us
> =item C<template_inner>
> 
> If you ever need a L<Bugzilla::Template> object while you're already
>-processing a template, use this.
>+processing a template, use this. Also use it if you want to specify
>+the language to use.

Fix on checkin: Please document that when using template_inner, it'll remember the language you specified and you have to add a template_inner("") to reset it (clearly specify this.. warning or something).

>Index: Bugzilla/BugMail.pm
>@@ -416,15 +419,24 @@ sub Send {

>+        # What's the language chosen by this user for email?
>+        my $lang = $user->settings->{'lang'}->{'value'};

Nit: Would be nice to avoid this call if the user cannot see the bug (as per below). I know $lang is used again in sendMail, so I'll leave this as a nit (although sendMail part should not be called -hopefully- if user cannot see the bug).

>+        if ($user->can_see_bug($id)) {

Comment 15

[Max Kanat-Alexander]

> Fix on checkin: Please document that when using template_inner, it'll remember
> the language you specified and you have to add a template_inner("") to reset it

  I actually would not r+ that behavior. template_inner is used in other places than this, and that behavior is really not good.

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
>   I actually would not r+ that behavior. template_inner is used in other places
> than this, and that behavior is really not good.

I checked all other places already, and that's fine. The caller has to reset the language using "" itself. In the same way we use Bugzilla->error_mode(ERROR_MODE_DIE) in CGI scripts (e.g. post_bug.cgi) to catch errors without dying, and we later reset it to its initial mode.

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> I checked all other places already, and that's fine.

  Yes, but it won't be intuitive to customizers or new developers. They will encounter strange bugs and not know why. You are making their future lives more difficult.

> In the same way we use
> Bugzilla->error_mode(ERROR_MODE_DIE) in CGI scripts (e.g. post_bug.cgi) to
> catch errors without dying, and we later reset it to its initial mode.

  That's intuitive because they set it right there. It's also intuitive because it's a *mode*, where here we're simply asking for an object.

  I would still r- the current behvior of your patch, instead requiring the caller to always specify $lang if they want a specific language of template. Otherwise they should get the default language.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Delaying approval until the issue that was recently raised gets resolved.

Comment 19

[Frédéric Buclin]

I explained to mkanat on IRC that this was the best way to get templates in the correct language, due to get_text(). Else we would have to pass $lang recursively through 3 or 4 routines BugMail -> Bug -> get_text() -> template_inner().

As bkor mentioned, I will update POD to make clear that the correct way to go is:

Bugzilla->template_inner("fr"); # Set french as the language to use in templates
... get your templates in french ...
Bugzilla->template_inner(""); # Reset the language to the current one.

Unless I misunderstood mkanat, his objection was most about the name of the routine than its implementation. mkanat, if your remaining objections are not as critical as before we talked on IRC, could you request approval yourself? As I told you yesterday, I still don't see how confusing my approach could be, nor how this could make the life of developers harder.

Comment 20

[Max Kanat-Alexander]

Well, since this is a blocker, I can request approval again.

All I was saying is that I'd like a separate accessor, called Bugzilla->template_language, that changes the forced language of both ->template and ->template_inner, because I think that would be easier to read and make more sense to people reading the code.

Comment 21

[Dave Miller [:justdave]]

reminder on commit messages (because the summary of this bug makes a bad one) -- make sure your commit message tells what problem is being fixed, not necessarily the summary of the bug.  The summary of this bug tells the problem, and sounds like we're doing it the opposite way when taken in the context of a commit message.  A good commit message for this might be "send emails in the recipient's locale, not the current user's".

Comment 22

[Frédéric Buclin]

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.55; previous revision: 1.54
done
Checking in editparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v  <--  editparams.cgi
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.167; previous revision: 1.166
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.98; previous revision: 1.97
done
Checking in Bugzilla/Install.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install.pm,v  <--  Install.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.66; previous revision: 1.65
done
Checking in Bugzilla/User/Setting.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting.pm,v  <--  Setting.pm
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/global/setting-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/setting-descs.none.tmpl,v  <--  setting-descs.none.tmpl
new revision: 1.10; previous revision: 1.9
done