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

Set up email template infrastructure #4369

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Set up email template infrastructure #4369

merged 2 commits into from
Mar 27, 2024

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Mar 26, 2024

References:

Jira: MNTOR-3036

This bundles up the MJML work we've done so far in atomic commits (5b95d9c for the email-specific bits, rather than the preparatory Fluent work) to merge into main. I'll submit a followup PR to start using this for an actual email (behind a flag).

See an example render in Storybook.

This further refines the synchronous FTL loading to allow loading
FTL files for multiple locales. This makes it useful to not just
tests/Storybook, but also e.g. for rendering email templates.

At some point, we should probably rename this from `mockL10n` to
something that implies production use as well, and possibly see if
the website's l10n code (in /src/app/functions/server/10n.ts)
could/should also build on top of it.
@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Mar 26, 2024
@Vinnl Vinnl requested a review from rhelmer March 26, 2024 12:33
@Vinnl Vinnl self-assigned this Mar 26, 2024
import { getSpecificL10nSync } from "../app/functions/server/mockL10n";

export function getEmailL10n(
subscriber: SubscriberRow,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not critical, but I wonder if instead of using SubscriberRow we should have more constrained types - e.g. we could Pick as a way to allow-list. There are properties in SubscriberRow we would never want to appear in an email.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great point. I've implemented that in the followup PR at #4370.

import { getSpecificL10nSync } from "../app/functions/server/mockL10n";

export function getEmailL10n(
subscriber: SubscriberRow,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use a more specific type, e.g. using Pick? SubscriberRow is fairly broad and contains data we wouldn't want to appear in an email.

args: {
subscriber: {
signup_language: "en",
} as SubscriberRow,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above re: SubscriberRow, I know we tend to pass this around but I think allow-listing the properties would be prevent someone from accidentally exposing something in the email that shouldn't be there.

Copy link
Collaborator

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! I have a suggestion to scope down the SubscriberRow type but I don't think it's blocking, would like to hear what you think about that.

@Vinnl Vinnl merged commit 158ad02 into main Mar 27, 2024
13 checks passed
@Vinnl Vinnl deleted the MNTOR-3036-mjml branch March 27, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants