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

Translation with params not included in payload generation #5302

Conversation

ainouzgali
Copy link
Contributor

What change does this PR introduce?

Params used within a translation are not being recognized as workflow variables and are not included in the created payload trigger.
example: {{i18n "test.key" var=payload-var}} => payload-var should be included in the trigger snippet :
novu.trigger(......, payload: { payload-var: ...})

Screen Shot 2024-03-14 at 12 11 25 Screen Shot 2024-03-14 at 12 11 48

Why was this change needed?

Other information (Screenshots)

Copy link

linear bot commented Mar 14, 2024

Copy link

netlify bot commented Mar 14, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 1b17e45
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/65f821bc80dda40008259f7f
😎 Deploy Preview https://deploy-preview-5302--dev-web-novu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -33,7 +33,7 @@ export class UpdateMessageTemplate {
updatePayload.name = command.name;
}

if (command.content !== null) {
if (command.content !== null || command.content !== undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to a different PR, added it here after a comment there

Comment on lines +13 to +29
const pairVariables = bod
.filter((body) => body.type === 'HashPair')
.flatMap((body) => {
const varName = body.value?.original as string;

if (!shouldAddVariable(varName)) {
return [];
}

return {
type: TemplateVariableTypeEnum.STRING,
name: body.value?.original as string,
defaultValue: '',
required: false,
};
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be used in the future for other handlebars helpers that we might want to allow variables assignments in.

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 should extract it to a reusable function then?


return {
type: TemplateVariableTypeEnum.STRING,
name: body.value?.original as string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: body.value?.original as string,
name: varName,

Comment on lines +13 to +29
const pairVariables = bod
.filter((body) => body.type === 'HashPair')
.flatMap((body) => {
const varName = body.value?.original as string;

if (!shouldAddVariable(varName)) {
return [];
}

return {
type: TemplateVariableTypeEnum.STRING,
name: body.value?.original as string,
defaultValue: '',
required: false,
};
});

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 should extract it to a reusable function then?

if (body.path.original === HandlebarHelpersEnum.I18N) {
if (body.path?.original === HandlebarHelpersEnum.I18N) {
if (body.hash?.pairs) {
return getTemplateVariables(body.hash.pairs);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we need a recurrent call here?
Can't we just use the extracted above function (lines 13-29) to get the variables?

Copy link
Contributor Author

@ainouzgali ainouzgali Mar 18, 2024

Choose a reason for hiding this comment

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

Currently, yes, you are right @LetItRock . But when we have time, in the cooldown maybe I want to go back to it and add support for sub expressions. In i18n, but also for general use. And that will need the recurrent calls.
For example:
{{titlecase (uppercase msg) }} (not a useful example, but just to show my point)
or for i18n
{{i18n "group.key" var=(titlecase msg) }}

msg in both examples would not be recognized as a variable, and in the first example it would set uppercase as a variable instead.

I do have something working for those states, but it requires some more work. I didn't want to over do it with the time as I feel its not the biggest priority

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, makes sense now :)

…ror-jsonparseanonymous

Unhandled Error: SyntaxError JSON.parse(<anonymous>)
@ainouzgali ainouzgali requested a review from LetItRock March 18, 2024 12:13
@ainouzgali ainouzgali merged commit a64176f into next Mar 19, 2024
31 checks passed
@ainouzgali ainouzgali deleted the nv-3550-translation-with-params-not-included-in-payload-generation branch March 19, 2024 08:15
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.

2 participants