Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] website_slides: translate course invite into recipients' language #162237

Conversation

naja628
Copy link
Contributor

@naja628 naja628 commented Apr 17, 2024

Problem:
when adding attendees to a course, the sent email is not translated into the recipient's language.

Fix:
Upon sending, translate the email on a per recipient basis, but only when the final message to sent is exactly the same as the template. (i.e. the body was not edited from the dialog box)

This is a compromise to permit emails being translated while not breaking the following 2 current features:

  • there can be many recipients (with different languages)
  • the body is pre-filled with a template but can be edited further by the user directly from the dialog box.

opw-3862369


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Apr 17, 2024

@naja628
Copy link
Contributor Author

naja628 commented Apr 17, 2024

Targeting saas-16.3, because it's the earliest supported versions where it "automatically" works, but the fix looks like it would be feasible as early as 15.0 if needed.

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 17, 2024
Copy link
Contributor

@nle-odoo nle-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

subject = self._render_field('subject', slide_channel_partner.ids)[slide_channel_partner.id]
body = self._render_field('body', slide_channel_partner.ids)[slide_channel_partner.id]
set_lang, lang_opt = {}, {}
if (self.body_has_template_value):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary parenthesis

Suggested change
if (self.body_has_template_value):
if self.body_has_template_value:

also maybe it's not necessary to check, since _render_field as far as I can read will only use the template translation if the body/subject has not changed:

if translation_asked and equality:
template = self.template_id.sudo() if call_sudo else self.template_id
return template._render_field(
template_field, *args, **kwargs,
)
record = self.sudo() if call_sudo else self
return super(MailComposerMixin, record)._render_field(field, *args, **kwargs)

and for the second part:

self.env['ir.qweb']._render(email_layout_xmlid, template_ctx, engine='ir.qweb', minimal_qcontext=True, raise_if_not_found=False, **lang_opt)

having it translated seems to make sense.

Copy link
Contributor Author

@naja628 naja628 Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The body_has_template_value check is necessary because if the subject is unchanged, but the body is, the subject will still be translated (but not the body).

Actually, it probably breaks the other way around (changed subject, unchanged body). I assumed body_has_template_value would also check the subject but I guess not. I will check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, makes sense then if we want in this case to have everything in same language

body = self._render_field('body', slide_channel_partner.ids)[slide_channel_partner.id]
set_lang, lang_opt = {}, {}
if (self.body_has_template_value):
lang = self._render_field('lang', slide_channel_partner.ids)[slide_channel_partner.id]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could use _render_lang instead, I guess this does the same:

Suggested change
lang = self._render_field('lang', slide_channel_partner.ids)[slide_channel_partner.id]
lang = self._render_lang(slide_channel_partner.ids)[slide_channel_partner.id]

@naja628 naja628 force-pushed the saas-16.3-opw-3862369-elearning_invites_not_translated-naja branch from 09ca817 to 09cecbe Compare April 18, 2024 13:25
@naja628 naja628 marked this pull request as ready for review April 18, 2024 15:03
@C3POdoo C3POdoo requested a review from a team April 18, 2024 15:04
Copy link
Member

@Julien00859 Julien00859 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice feature, let's code a non-regression test and merge it :)

@naja628 naja628 force-pushed the saas-16.3-opw-3862369-elearning_invites_not_translated-naja branch 3 times, most recently from bf0329a to 838f6ad Compare May 6, 2024 14:28
@naja628
Copy link
Contributor Author

naja628 commented May 6, 2024

The test is a bit janky, since we rely on _prepare_mail_values behaving a specific way, and since the way we get languages to work in a test environment is a bit weird, but I'm not sure how to make it better.

@naja628 naja628 force-pushed the saas-16.3-opw-3862369-elearning_invites_not_translated-naja branch from 838f6ad to 55e3444 Compare May 6, 2024 15:21
Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zboing, same some light, went to see if there were cookies, added some comments

addons/website_slides/tests/test_slide_channel.py Outdated Show resolved Hide resolved
addons/website_slides/tests/test_slide_channel.py Outdated Show resolved Hide resolved
body = self._render_field('body', slide_channel_partner.ids)[slide_channel_partner.id]
set_lang, lang_opt = {}, {}
subject_same_as_template = (self.template_id and self.subject == self.template_id.subject)
if self.body_has_template_value and subject_same_as_template:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we could split (don't have to have both identical to check for translations, each one is separate)

Copy link
Contributor Author

@naja628 naja628 May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello :)


Indeed we should check the returned result directly, here it is a bit indirect. Note that you can create a small template and its translation in the test, so that we don't rely on existing data.

I wanted to do something like this, but I'm not sure how I would achieve this.
As far as I can tell, the translations are stored in the db as jsonb, and the orm does not really provide a way to write to them, so we would have to do some raw SQL queries in the test? (I guess I'm missing something)


Note that we could split (don't have to have both identical to check for translations, each one is separate)

Do you mean that if (for example) the subject is unchanged but the body is, we still want the subject to be translated? I just assumed this was not what we wanted, but I guess it kinda makes sense/would simplify the code. (I think then we could completely remove the checks)


Side note: invite tests are inside 'test_attendee.py' test file (see notably 'test_invite_to_course'), so would add the test in that file to keep them in the same file :)

Will do :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to do something like this, but I'm not sure how I would achieve this.

Just copy/paste the french text from the email. No code.

Or follow tde's advice to create a dedicated template inside the test, provide with the translation in the test and just assertEqual that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then you rely on data ... and actual translation (you could have modified it in your db). Would say to create a small template and its translation so that the test is deterministic. To update it in new version, just update its body in a given environment should do the trick.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tde-banana-odoo I wanted to r+ this one but I see that this question has been left open:

Note that we could split (don't have to have both identical to check for translations, each one is separate)

Do you mean that if (for example) the subject is unchanged but the body is, we still want the subject to be translated? I just assumed this was not what we wanted, but I guess it kinda makes sense/would simplify the code. (I think then we could completely remove the checks)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the end what we want is to try to use translations if possible :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tde-banana-odoo Hello :)
I made the change

@naja628 naja628 force-pushed the saas-16.3-opw-3862369-elearning_invites_not_translated-naja branch from 55e3444 to addee18 Compare May 8, 2024 08:54
@naja628 naja628 requested a review from Julien00859 May 8, 2024 11:38
Copy link
Member

@Julien00859 Julien00859 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Problem:
when adding attendees to a course, the sent email is not translated into
the recipient's language.

Fix:
Upon sending, translate the email on a per recipient basis,
but **only when** the final message to sent is exactly the same
as the template. (i.e. the body was not edited from the dialog box)

This is a compromise to permit emails being translated while not
breaking the following 2 current features:
* there can be many recipients (with different languages)
* the body is pre-filled with a template but can be edited further
  by the user directly from the dialog box.

opw-3862369
@naja628 naja628 force-pushed the saas-16.3-opw-3862369-elearning_invites_not_translated-naja branch from 150582c to 7b9a398 Compare May 13, 2024 09:20
@nle-odoo
Copy link
Contributor

robodoo delegate+

@naja628
Copy link
Contributor Author

naja628 commented May 14, 2024

@robodoo r+

robodoo pushed a commit that referenced this pull request May 14, 2024
Problem:
when adding attendees to a course, the sent email is not translated into
the recipient's language.

Fix:
Upon sending, translate the email on a per recipient basis,
but **only when** the final message to sent is exactly the same
as the template. (i.e. the body was not edited from the dialog box)

This is a compromise to permit emails being translated while not
breaking the following 2 current features:
* there can be many recipients (with different languages)
* the body is pre-filled with a template but can be edited further
  by the user directly from the dialog box.

opw-3862369

closes #162237

Signed-off-by: Nathaniel Jacques (naja) <naja@odoo.com>
@robodoo robodoo closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants