Customer emails aren't sent in a consistent language

[sheridanvk]

Describe the bug I use the website in English and get order confirmation emails in English, as expected. However the emails from the restaurant (order accepted, delayed, etc) come through in what I assume is the local language of the restaurant (German, in my case - I'm using the service in Berlin).

Expected behavior I would expect all emails to come through to me in English. I assume this happens because there's no language saved on the Customer account? It would be nice to have that default to the language of the interface that I'm using when I sign up, and even better if I can see/change that on my profile settings page (i.e. the page like https://kolyma2.coopcycle.org/profile/?_locale=en). Then all emails that get sent to me should ideally be in that language.

Happy to help implement a fix for this issue if you agree with this approach to fixing this bug (I'm a developer!)

Screenshots

[alexsegura]

Hi. Thank you for opening this issue.

All emails are sent through the EmailManager class.

It uses Symfony Translator component, and the locale to use is resolved via the request, it should use the locale that was saved in session, either via the path (i.e when the URL starts with /de), or via the languages dropdown (the _locale query string parameter)

The emails are indeed translated, as you can see in this file, so yes, there's something wrong.

---

There's something wrong, because emails are sent in different "contexts", where the locale is not the same. The "order created" email (the first one on your screenshot) is sent in the customer context, so it works (the emails are sent synchronously, which is bad, they should be moved to a background job).

The 2nd and 3rd emails are sent in the restaurant context (i.e, the restaurant owner, either via the web or the app), so this is the problem! If the restaurant owner has the locale set to en, it will send an email in English 😖 I still can't explain why 2nd and 3rd emails are in different languages, however...

Also, if the order is accepted through the app, the request is executed via the API, and if I remember well, API requests ignore the locale, everything will use the default locale of the instance (in this case, German).

---

Indeed, the first thing to do, is what you are suggesting → allow to attach a "preferred language" to each user. See Setting the Locale Based on the User’s Preferences for some guidelines on how to do it.

If you can do this, it would be super nice 🙂

[alexsegura]

After that, I think the 2nd thing to do, would be to send all emails via background jobs. There's everything to do it already (the Message and the Handler for Symfony Messenger), it's just not implemented. It would save a few seconds of execution time for all requests sending emails.

It should be easy to achieve it, without modifying too much code, straight inside the EmailManager class, by injecting the MessageBusInterface. Then, inside EmailManager::send() method, instead of sending directly, use the message bus.

Still not sure if the locale to use for the email should be attached to the message, or resolved inside the handler based on the email (like, SELECT locale FROM user WHERE email = X, or use fallback)

[sheridanvk]

This is super helpful context and pointers, thanks so much! Happy to work on this on the weekend coming up - I can think through both ways of passing through the language and see if one seems nicer than the other (and let me know in the meantime if you end up thinking one’s preferable over the other :) )

On Mon, 6 Jul 2020 at 10:41, Alexandre Segura notifications@github.com wrote:

After that, I think the 2nd thing to do, would be to send all emails via background jobs. There's everything to do it already (the Message https://github.com/coopcycle/coopcycle-web/blob/0fbb661c0b273713868d527e8083a3f1e5eaced3/src/Message/Email.php and the Handler https://github.com/coopcycle/coopcycle-web/blob/0fbb661c0b273713868d527e8083a3f1e5eaced3/src/MessageHandler/EmailHandler.php for Symfony Messenger https://symfony.com/doc/4.4/components/messenger.html), it's just not implemented. It would save a few seconds of execution time for all requests sending emails.

It should be easy to achieve it, without modifying too much code, straight inside the EmailManager class, by injecting the MessageBusInterface. Then, inside EmailManager::send() method, instead of sending directly, use the message bus.

Still not sure if the locale to use for the email should be attached to the message, or resolved inside the handler based on the email (like, SELECT locale FROM user WHERE email = X, or use fallback)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/coopcycle/coopcycle-web/issues/1547#issuecomment-654098892, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACLFWLFAHTO7INMRUMGSMHLR2GE3NANCNFSM4OQ4DY6A .

[sheridanvk]

Hi! Definitely making some progress over here, couple of questions though:

1. I've added the locale field to the ApiUser model, but it's not clear to me whether I need to create the migration files manually to update the DB, or if you have a way for these to be generated automatically based on model changes (e.g. I don't see a cli-config.php for the Doctrine command-line tool). How do you typically go about this?
2. I think once the locale is in available in the DB, I like the idea of just grabbing the locale from the customer attached to the order in the email handler, rather than passing languages in separately. It would likely also require passing in the order to the EmailManager::createCovid19Message() so that context is available in the function; let me know if that works for you!

[alexsegura]

1. To generate the migration file, you have a Symfony command. Run docker-compose exec php bin/console to see the list of available commands. Actually, you can just run make migrations-diff, which will do this for you.

2. Why not, I don't see any problem. Just one thing to keep in mind: at some point, we would like to have "guest checkout", i.e the possibility to order, without having to register (= without having to create a password). This will be based on the Customer/ShopUser mechanism of Sylius.

👉 TL;DR there is a Customer class, which holds all the info to execute the service (name, addresses, phone number...), and the ApiUser class will contain the username/password (more or less)

This is already there: the Customer class was introduced in 718c0275839301fce815b68c33621d6c85533b1f For now, orders are still associated to the ApiUser class, the Customer is created behind the scenes. One day, it will be the opposite, they will be associated to the Customer class.

I suppose things will look like this:

$customer = $order->getCustomer();
if ($customer->hasUser()) {
    $locale = $customer->getUser()->getLocale();
} else {
    // Use fallback locale?
}

It doesn't look "right" to associate the locale to the Customer class, as this is something that seems to belong to registered users only. Or maybe not? I'm not sure. Any suggestions? Sorry for the confusion, no need to anticipate too much about this, one step at a time.

[sheridanvk]

nice, thank you again! no confusion at all, happy to think this through together :) just a couple of questions to get me unstuck before we think about the rest:

1. When I run make migrations-diff, I get some additional things in the migration that I hadn't modified: in up():

   $this->addSql('COMMENT ON COLUMN zone.polygon IS \'(DC2Type:geojson)\'');

in down():

       $this->addSql('CREATE SCHEMA public');
        $this->addSql('CREATE SCHEMA topology');
        $this->addSql('CREATE SCHEMA tiger');
        $this->addSql('CREATE SCHEMA tiger_data');
        $this->addSql('COMMENT ON COLUMN zone.polygon IS \'(DC2Type:geojson)(DC2Type:geojson)\'');

I'll just delete these lines out for now, but let me know if there's some obvious reason why these are getting pulled in.

2. Then a slightly basic question (sorry, I'm new to PHP - have been mostly a JS dev up to this point!) - what debugger setup do you use, and does it require any additional config steps? I'm working on setting up xdebug, but perhaps you have a different way to do this (especially as I noticed you've taken xdebug out of the repo in the past)?

[alexsegura]

For the migrations, it's "normal" 😐 Out of laziness, I always remove manually those lines.

For the extra line in up(), it's due to something in jsor/doctrine-postgis, I opened an issue a long time ago (jsor/doctrine-postgis#36), but never really understood the cause. Maybe the problem is in ORMSchemaEventSubscriber For the extra lines in down(), it seems related to postgis/docker-postgis#187

---

For the debugger, I'm not an example to follow, but I don't use one. I use old-school var_dump/print_r 😓

If you want to setup XDebug, AFAIK you will need to modify the Dockerfile to enable it. I found this but I only searched 30 seconds. The Docker image is not intended for production usage, so it's totally fine if you enable XDebug, and maybe this will motivate me to do things the right way (i.e use XDebug).

Did we really remove XDebug? I don't remember, but maybe it was because Docker on Mac is not especially fast, and XDebug makes it even slower.

[sheridanvk]

Sorry for the slow response, work is very busy during the week! This all sounds fine and totally makes sense :)

Re XDebug, I just spotted this commit, but I'm realising now that's in what I assume is the production build, so not related to this.

[alexsegura]

Hi @sheridanvk, hope you are doing well.

Are still motivated to tackle this issue? If this can help, @maxi3390 has added XDebug to the Docker stack (#1765) 🙂

[sheridanvk]

Ahh thank you so much for checking in! I am definitely, and having XDebug will be a huge help. I think I will have time next week, does that sound ok?

[alexsegura]

Cool, anytime is ok 🙂

About this comment, it was done in #1744