fix(api-service): email 'if' condition truthy values #7551
Conversation
| @@ -241,7 +241,7 @@ export function mergeCommonObjectKeys( | |||
| sourceValue as Record<string, unknown> | |||
| ); | |||
| } else { | |||
| merged[key] = sourceValue ?? targetValue; | |||
| merged[key] = key in source ? sourceValue : targetValue; | |||
There was a problem hiding this comment.
Check presence of key in payload instead of a value - If I put null in preview payload as a value of some attribute (e.g. to check ifShowKey condition), I want it to not get it overwritten by placeholder string
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| return value.toLowerCase() === 'true'; | ||
| } | ||
| private stringToBoolean(value: string): boolean { | ||
| const normalized = value.toLowerCase().trim(); |
There was a problem hiding this comment.
value.toLowerCase() could throw an exception if we value null/undefined/false/true and on everything that is not a string etc..
There was a problem hiding this comment.
It will always be a string, the function stringToBoolean accepts only string as an argument.
The reason is because parseLiquid() function returns always string:
private async evaluateShowCondition(
variables: FullPayloadForRender,
node: MailyJSONContent & { attrs: { [MailyAttrsEnum.SHOW_IF_KEY]: string } }
): Promise<boolean> {
const { [MailyAttrsEnum.SHOW_IF_KEY]: showIfKey } = node.attrs;
const parsedShowIfValue = await this.parseLiquid(showIfKey, variables);
return this.stringToBoolean(parsedShowIfValue);
}|
|
||
| return false; | ||
| try { | ||
| return Boolean(JSON.parse(value)); |
There was a problem hiding this comment.
what is the usecase for executing JSON.parse?
There was a problem hiding this comment.
If I would remove the JSON.parse and just do Boolean(value), some originally falsy values (like zeroes in the example) would resolve to true because it would be treated a string, but they should be false.
Boolean('0') // returns true (incorrect: because `0` as a number is falsy)
Boolean(JSON.parse('0')) // returns false
Boolean('0e0') // returns true
Boolean(JSON.parse('0e0')) // returns false
Boolean('-0') // returns true
Boolean(JSON.parse('-0')) // returns false
...
The issue we currently have is that the liquid parser always returns string, we can't distinguish if user typed 100 or "100" in the payload, we always assume it was number for now.
There was a problem hiding this comment.
shouldn't we also pass here the normalized value? also in the catch block
| { value: 'true', desc: 'string "true"' }, | ||
| { value: 'yes', desc: 'string "yes"' }, | ||
| { value: {}, desc: 'empty object' }, | ||
| { value: [], desc: 'empty array' }, |
There was a problem hiding this comment.
missing the tests where the normalization will come into play
What changed? Why was the change needed?
Evaluate true and false conditions from payload based on JS evaluation.