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

setup localization for twenty-emails #9806

Conversation

DeepaPrasanna
Copy link
Contributor

@DeepaPrasanna DeepaPrasanna commented Jan 22, 2025

One of the steps to address #8128

How to test:
Please change the locale in the settings and click on change password button. A password reset email in the preferred locale will be sent.

image
image

Todo:

  • Remove the hardcoded locales for invitation, warn suspended workspace email, clean suspended workspace emails
  • Need to test invitation, email verification, warn suspended workspace email, clean suspended workspace emails
  • The duration variable 5 minutes is always in english. Do we need to do something about that? It does seems odd in case of chinese translations.

Notes:

  • Only tested the password reset , password update notify templates.
  • Cant test email verification due to error during sign up Internal server error: New workspace setup is disabled

@@ -0,0 +1,18 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I see Babel in the emails folder. I think it's best to use SWC but it seemed to use Babel. But you didn't migrate twenty-emails to SWC here did you? I'm surprised it works. Want to take a shot at removing Babel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I didn't migrated to SWC. I chose to go creating .swcrc because the vite.config.ts already imported React from @vitejs/plugin-react-swc
I will remove babel and will check how it goes then.

@charlesBochet
Copy link
Member

@DeepaPrasanna Thanks a lot for the PR! Would you be able to rebase on main to fix conflicts and to address @FelixMalfait feedbacks?

@DeepaPrasanna DeepaPrasanna marked this pull request as ready for review January 27, 2025 08:21
@DeepaPrasanna DeepaPrasanna marked this pull request as draft January 27, 2025 08:25
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR implements comprehensive email template localization for Twenty, supporting multiple languages (en, fr, pt, de, it, es, zh-Hans, zh-Hant) through the Lingui i18n framework.

  • Added loadAndActivateLocale utility in /packages/twenty-emails/src/utils/loadAndActivateLocale.ts for dynamic locale loading and activation
  • Hardcoded 'en' locale in server services needs to be replaced with user preferences from workspace settings
  • Duration strings (e.g., "5 minutes") remain in English across all translations, requiring localization
  • Email subjects in server services are still hardcoded in English and need localization
  • Some special characters in translation files show encoding issues (e.g., 'í' instead of 'í')

💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!

36 file(s) reviewed, 45 comment(s)
Edit PR Review Bot Settings | Greptile

}>;

export const BaseEmail = ({ children, width }: BaseEmailProps) => {
export const BaseEmail = ({ children, width, locale }: BaseEmailProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: locale prop is now required but no default value provided - could cause runtime errors if not passed

{children}
</Container>
</Html>
<I18nProvider i18n={i18n}>
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: i18n instance needs to be initialized and configured before being used in I18nProvider

<I18nProvider i18n={i18n}>
<Html lang={locale}>
<BaseHead />
<Container width={width || 290}>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: width default of 290 should be defined as a constant at the top level

Comment on lines +8 to +13
{
"runtimeModules": {
"i18n": ["@lingui/core", "i18n"],
"trans": ["@lingui/react", "Trans"]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: indentation is inconsistent with the rest of the file (extra space before opening brace)

const helloString = userName?.length > 1 ? `Dear ${userName}` : 'Dear';
await loadAndActivateLocale(locale);

const helloString = userName?.length > 1 ? t`Dear ${userName}` : t`Dear`;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: string interpolation in t macro may not work as expected - consider using Trans component instead for variable interpolation

@@ -0,0 +1 @@
/*eslint-disable*/import type{Messages}from"@lingui/core";export const messages=JSON.parse("{\"A7zEzZ\":[[\"daysLeft\",\"plural\",{\"one\":[\"day left\"],\"other\":[\"days left\"]}]],\"0JB+B5\":[[\"daysLeft\",\"plural\",{\"one\":[\"No need for concern, though! Simply create or edit a record within the next \",[\"remainingDays\"],\" day to retain access.\"],\"other\":[\"No need for concern, though! Simply create or edit a record within the next \",[\"remainingDays\"],\" days to retain access.\"]}]],\"RPHFhC\":[\"Confirm your email address\"],\"791xh3\":[\"Dead Workspaces 😵 that should be deleted\"],\"JRzgV7\":[\"Dear\"],\"Lm5BBI\":[\"Dear \",[\"userName\"]],\"tGme7M\":[\"has invited you to join a workspace called \"],\"uzTaYi\":[\"Hello\"],\"r0OJlV\":[\"Hello \",[\"userName\"]],\"vq4f+3\":[\"Inactive Workspace 😴\"],\"Y4Nk3c\":[\"It appears that there has been a period of inactivity on your<0>\",[\"workspaceDisplayName\"],\"</0> workspace.<1/><2/>Please note that the account is due for deactivation soon, and all associated data within this workspace will be deleted.<3/><4/>\"],\"PviVyk\":[\"Join your team on Twenty\"],\"ja42wz\":[\"List of <0>workspaceIds</0> inactive since at least <1>\",[\"minDaysSinceInactive\"],\" days</1>:\"],\"ogtYkT\":[\"Password updated\"],\"OfhWJH\":[\"Reset\"],\"RE5NiU\":[\"Reset your password 🗝\"],\"7yDt8q\":[\"Thanks for registering for an account on Twenty! Before we get started, we just need to confirm that this is you. Click above to verify your email address.\"],\"wSOsS+\":[\"This is a confirmation that password for your account (\",[\"email\"],\") was successfully changed on \",[\"formattedDate\"],\".<0/><1/>If you did not initiate this change, please contact your workspace owner immediately.\"],\"RGs4CL\":[\"This link is only valid for the next \",[\"duration\"],\". If the link does not\\n work, you can use the login verification link directly:<0/>\"],\"2oA637\":[\"This link is only valid for the next \",[\"duration\"],\". If the link does not work, you can use the login verification link directly:<0/>\"],\"wCKkSr\":[\"Verify Email\"]}")as Messages;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The file uses escaped newlines '\n' in some translations which may cause formatting inconsistencies. Consider using template literals or removing manual line breaks

Comment on lines 421 to 424
const html = await render(emailTemplate);
const text = await render(emailTemplate, {
plainText: true,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

style: render() calls should be wrapped in try/catch to handle potential rendering errors

@@ -55,15 +55,14 @@ export class EmailVerificationService {

const emailData = {
link: verificationLink.toString(),
locale: 'en',
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: locale should not be hardcoded to 'en' - it should be obtained from user preferences or settings

@@ -296,14 +296,13 @@ export class WorkspaceInvitationService {
lastName: sender.lastName,
},
serverUrl: this.environmentService.get('SERVER_URL'),
locale: 'en',
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: locale should not be hardcoded to 'en' - it should be passed from the user's preferences or system settings

@DeepaPrasanna DeepaPrasanna force-pushed the feat/setup-localization-for-twenty-emails branch from 22bd29a to 7e2da25 Compare January 27, 2025 10:14
Update version of @react-email/components and @react-email/render
Install packege to solve dynamic imports
add extract and compile commands, set up lingui config
use lingui macros for translations
translations added for de, en, es, fr, it, pt, zh-Hans, zh-Hant
workspaceId passed to fetch user's workspace locale
add await and remove {pretty :true} to fix this issue resend/react-email#1802
removed package vite-plugin-dynamic-import, removed await while calling email templates, updated loadAndActivateLocale to have static imports
@DeepaPrasanna DeepaPrasanna force-pushed the feat/setup-localization-for-twenty-emails branch from 9b13d4f to 18ad5f6 Compare January 28, 2025 14:31
downgrades @react-email/render version from 1.0.4 to 0.0.17 to fix dynamic imports during jest tests
update dto, mutation query to accept locale for updatePassword functionality
pass locale via header to sendEmailPasswordResetLink
@@ -336,6 +337,7 @@ export class AuthResolver {
@Mutation(() => EmailPasswordResetLink)
async emailPasswordResetLink(
@Args() emailPasswordResetInput: EmailPasswordResetLinkInput,
@Context() context: I18nContext,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context is undefined when this is called during logged out scenario. Need to address that

@FelixMalfait FelixMalfait marked this pull request as ready for review February 2, 2025 11:30
@FelixMalfait FelixMalfait force-pushed the feat/setup-localization-for-twenty-emails branch from 4079895 to d251812 Compare February 2, 2025 11:32
@FelixMalfait FelixMalfait force-pushed the feat/setup-localization-for-twenty-emails branch from d251812 to ca1fad3 Compare February 2, 2025 11:37
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the provided information, I'll focus on the most recent changes and avoid repeating points from the previous review.

This PR continues the email template localization implementation with additional language support and infrastructure changes.

  • Added Lingui configuration in /packages/twenty-emails/lingui.config.js with TranslationIO service integration when API key is present
  • Added BaseEmail component locale prop requirement in /packages/twenty-emails/src/components/BaseEmail.tsx for consistent language handling
  • Inconsistent character encoding in translation files (e.g., 'ç' appearing as 'ç') needs to be fixed
  • Formal/informal address inconsistencies in German and Spanish translations should be standardized

The implementation is progressing well but requires attention to encoding and translation consistency issues before completion.

51 file(s) reviewed, 63 comment(s)
Edit PR Review Bot Settings | Greptile

}
},
"engines": {
"node": "^18.17.1",
"node": "^18.17.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Extra space after ^18.17.1 should be removed


export default defineConfig({
sourceLocale: 'en',
locales: Object.values(APP_LOCALES),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Using Object.values() on APP_LOCALES could cause issues if the order of locales matters for fallback chains. Consider explicitly listing locales in desired order.

<Title value="Reset your password 🗝" />
<CallToAction href={link} value="Reset" />
<BaseEmail locale={locale}>
<Title value={t`Reset your password 🗝`} />
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The emoji in the title string may cause display issues in some email clients. Consider using a Unicode emoji or removing it for better compatibility.

@@ -17,20 +18,24 @@ export const CleanSuspendedWorkspaceEmail = ({
const helloString = userName?.length > 1 ? `Hello ${userName}` : 'Hello';
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: helloString needs to be wrapped in <Trans> component for localization

@@ -17,20 +18,24 @@ export const CleanSuspendedWorkspaceEmail = ({
const helloString = userName?.length > 1 ? `Hello ${userName}` : 'Hello';

return (
<BaseEmail width={333}>
<BaseEmail width={333} locale="en">
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: locale="en" should not be hardcoded - should be passed as a prop from parent component

Suggested change
<BaseEmail width={333} locale="en">
<BaseEmail width={333} locale={locale}>

plugins: [['@lingui/swc-plugin', {}]],
}),
lingui({
configPath: path.resolve(__dirname, './lingui.config.ts'),
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: lingui.config.ts referenced here but the actual file is lingui.config.js

Comment on lines +38 to +44
...Object.values(APP_LOCALES).reduce(
(acc, locale) => ({
...acc,
[`locales/generated/${locale}`]: `src/locales/generated/${locale}.ts`,
}),
{},
),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider adding type safety for APP_LOCALES values and validation that the generated files exist

Comment on lines 34 to 37
return {
...serverMessages,
...emailMessages,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: mergeMessages could silently overwrite server messages with email messages due to spread operator order. Consider adding conflict detection/warning

...emailMessages,
};
}

async onModuleInit() {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: onModuleInit lacks error handling for failed message loading or merging

Comment on lines 778 to 782
async resolveTranslatableString(
fieldMetadata: FieldMetadataDTO,
labelKey: 'label' | 'description',
locale: string | undefined,
locale: keyof typeof APP_LOCALES | undefined,
): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding validation to ensure locale exists in APP_LOCALES before activating it

@FelixMalfait FelixMalfait force-pushed the feat/setup-localization-for-twenty-emails branch from bd7a5d6 to c25ea73 Compare February 2, 2025 19:53
@FelixMalfait
Copy link
Member

Thanks a lot @DeepaPrasanna! I'll merge even if it's not 100% finished (we need to implement the part that switches the locale), to avoid further conflicts

@FelixMalfait FelixMalfait merged commit 39e7f6c into twentyhq:main Feb 2, 2025
48 of 49 checks passed
Copy link

github-actions bot commented Feb 2, 2025

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5591:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5562:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5571:7)�[39m
danger-results://tmp/danger-results-cde4e9f0.json

Generated by 🚫 dangerJS against c25ea73

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants