feat(dashboard): restore email editor 'for' block #7483
Conversation
✅ 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. |
| private async traverseAndProcessNodes( | ||
| node: TipTapNode, | ||
| parentNode?: TipTapNode | ||
| ): Promise<TipTapNode> { | ||
| if (node.content) { | ||
| const processedContent: TipTapNode[] = []; | ||
| for (const innerNode of node.content) { | ||
| const processed = await this.processShowAndForControls(variables, innerNode, parentNode); | ||
| if (processed) { | ||
| processedContent.push(processed); | ||
| variables: Record<string, unknown>, | ||
| parent?: TipTapNode | ||
| ): Promise<void> { | ||
| const queue: Array<{ node: TipTapNode; parent?: TipTapNode }> = [{ node, parent }]; | ||
|
|
||
| while (queue.length > 0) { | ||
| const current = queue.shift()!; | ||
| await this.processNode(current.node, variables, current.parent); | ||
|
|
||
| if (current.node.content) { | ||
| for (const childNode of current.node.content) { | ||
| queue.push({ node: childNode, parent: current.node }); | ||
| } | ||
| } | ||
| node.content = processedContent; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reworked the previously recursive function to something easier to read using queue.
| private addIndexesToPlaceholders(nodes: TipTapNode[], iterablePath: string, index: number): TipTapNode[] { | ||
| return nodes.map((node) => { | ||
| const newNode: TipTapNode = { ...node }; | ||
|
|
||
| const parsedShowIfValue = await parseLiquid(showIfKey as string, variables); | ||
| const showIfValueBoolean = | ||
| typeof parsedShowIfValue === 'boolean' ? parsedShowIfValue : this.stringToBoolean(parsedShowIfValue); | ||
| if (this.isAVariableNode(newNode)) { | ||
| const nodePlaceholder = newNode.text as string; | ||
|
|
||
| /** | ||
| * example: | ||
| * iterablePath = payload.comments | ||
| * nodePlaceholder = {{ payload.comments.author }} | ||
| * text = {{ payload.comments[0].author }} | ||
| */ | ||
| newNode.text = nodePlaceholder.replace(iterablePath, `${iterablePath}[${index}]`); | ||
| newNode.type = 'text'; // set 'variable' to 'text' to for Liquid to recognize it | ||
| } else if (newNode.content) { | ||
| newNode.content = this.addIndexesToPlaceholders(newNode.content, iterablePath, index); | ||
| } | ||
|
|
||
| if (!showIfValueBoolean) { | ||
| this.removeNodeFromParent(node, parentNode); | ||
| } else { | ||
| delete node.attrs[MailyAttrsEnum.SHOW_IF_KEY]; | ||
| } | ||
| return newNode; | ||
| }); |
There was a problem hiding this comment.
Instead of replacing for loop values directly in this usecase as it was before, we create just placeholders {{ payload.comments[0].author }} and leave the actual value substitution for Liquid parser down the line
| @@ -157,27 +214,7 @@ export class ExpandEmailEditorSchemaUsecase { | |||
| } | |||
|
|
|||
| private isAVariableNode(newNode: TipTapNode): newNode is TipTapNode & { attrs: { id: string } } { | |||
| return newNode.type === 'payloadValue' && newNode.attrs?.id !== undefined; | |||
There was a problem hiding this comment.
No idea what is payloadValue but I was not able to find it anywhere in the TipTapNode
| @@ -132,10 +132,13 @@ export const Maily = (props: MailyProps) => { | |||
| return dedupAndSortVariables(filteredVariables, queryWithoutSuffix); | |||
| } | |||
|
|
|||
| const iterableName = editor?.getAttributes('for')?.each; | |||
There was a problem hiding this comment.
for now the iterable name is the full path, e.g. - we can polish this in separate PR, it was too difficult for me to change all that at once
for each "payload.comments"
{{payload.comments.id}}
ChmaraX
changed the title
| @@ -39,12 +39,12 @@ export class HydrateEmailSchemaUseCase { | |||
| index: number | |||
| ) { | |||
| content[index] = { | |||
| type: 'text', | |||
| type: 'variable', | |||
There was a problem hiding this comment.
was there a specific reason for this change? When parsing the variable type from Maily, we need to convert it to text because Liquid is responsible for variable parsing.
in addition, this helps us avoid any side effects from Maily's variable parsing.
There was a problem hiding this comment.
This is required for the 'for' logic down the line. The output from this hydrate schema usecase looks something like this:
{
"type": "for",
"content": [
{
"type": "paragraph",
"content": [
{
"type": "text",
"text": "comments: "
},
{
"type": "variable", <--- This needs to be variable not text
"text": "{{ payload.comments.text }}"
}
],
"attrs": {
"textAlign": "left"
}
}
],
}If it would be replaced by type: text here, I wouldn't be able to know which node is variable and which text, therefore I wouldn't be able to transform the variable here from {{ payload.comments.text }} to {{ payload.comments[0].text }}
| const tipTapNode = isStringTipTapNode(controlValue) ? JSON.parse(controlValue) : controlValue; | ||
| const iterableArrayPaths = this.extractIterableArrayPaths(tipTapNode); | ||
| const variablesExampleResult = this.buildVariablesExample(iterableArrayPaths, validVariableNames); |
There was a problem hiding this comment.
this is not the right place to build the for variables. please let me know if there was any objective to push this logic here.
id argue that the responsibility of creating the variables under buildVariables.
please note that once transformMailyContentToLiquid executes the following transformation
From:
### for block {{ payload.comments }}
{{ payload.comments.postBody | upcase}}
To:
{{ payload.comments[0].postBody | upcase }}
you will get those variables out of the box under validVariableNames in line 87.
The left part in this use case will be to multiply the arrays by 3.
Additional key points why this is an issue.
- we are building payload schema on fly based on control value, where we reuse transformMailyContentToLiquid, not adding this transformation logic there will create incorrect payload schema of object instead of a list
- we are reusing the buildVariables in order to build the
PayloadforTest Workflow, meaning that now it will generate an object instead of an array
There was a problem hiding this comment.
Changed as we discussed
There was a problem hiding this comment.
Also I noticed we are doing this:
-
In step preview, we send the payload example in HTTP response to client.
-
In trigger, we first encode the payload to JSONSchema, then on client we decode the JSONSchema back to payload in
create-mock-object-from-schema. Its difficult to preserve all the properties of the payload in the schema (e.g. 3x example elements in array).
Not sure but maybe we can simplify this by sending the payload directly to trigger as well.
| }, | ||
| }; | ||
| }); | ||
| const content = node.content.map((contentNodeChild) => processNode(contentNodeChild)); |
There was a problem hiding this comment.
As mentioned above here we should
Transform From:
### for block {{ payload.comments }}
{{ payload.comments.postBody | upcase}}
To:
{{ payload.comments[0].postBody | upcase }}
There was a problem hiding this comment.
Changed as we discussed
|
|
||
| private getResolvedValueForPlaceholder( | ||
| masterPayload: PreviewPayload, | ||
| node: TipTapNode & { | ||
| attrs: { each: string }; | ||
| }, | ||
| itemPointerToDefaultRecord: Record<string, string>, | ||
| placeholderAggregation: PlaceholderAggregation | ||
| ) { | ||
| let resolvedValueIfFound = this.getValueByPath(masterPayload, node.attrs.each); | ||
|
|
||
| if (!resolvedValueIfFound) { | ||
| resolvedValueIfFound = [ | ||
| this.buildElement(itemPointerToDefaultRecord, '1'), | ||
| this.buildElement(itemPointerToDefaultRecord, '2'), | ||
| ]; | ||
| } | ||
| placeholderAggregation.nestedForPlaceholders[`{{${node.attrs.each}}}`] = | ||
| this.buildNestedVariableRecord(itemPointerToDefaultRecord); | ||
|
|
||
| return resolvedValueIfFound; | ||
| } | ||
|
|
||
| private buildNestedVariableRecord(itemPointerToDefaultRecord: Record<string, string>) { | ||
| const transformedObj: Record<string, string> = {}; | ||
|
|
||
| Object.entries(itemPointerToDefaultRecord).forEach(([key, value]) => { | ||
| transformedObj[value] = value; | ||
| }); | ||
|
|
||
| return transformedObj; | ||
| } | ||
| private collectAllItemPlaceholders(nodeExt: TipTapNode) { | ||
| const payloadValues = {}; | ||
| const traverse = (node: TipTapNode) => { | ||
| if (node.type === 'for') { | ||
| return; | ||
| } | ||
| if (this.isPayloadValue(node)) { | ||
| const { id } = node.attrs; | ||
| payloadValues[`${node.attrs.id}`] = node.attrs.fallback || `{{item.${id}}}`; | ||
| } | ||
| if (node.content && Array.isArray(node.content)) { | ||
| node.content.forEach(traverse); | ||
| } | ||
| }; | ||
| nodeExt.content?.forEach(traverse); | ||
|
|
||
| return payloadValues; | ||
| } | ||
|
|
||
| private getValueByPath(masterPayload: Record<string, any>, placeholderRef: string): any { | ||
| const keys = placeholderRef.split('.'); | ||
|
|
||
| return keys.reduce((currentObj, key) => { | ||
| if (currentObj && typeof currentObj === 'object' && key in currentObj) { | ||
| return currentObj[key]; | ||
| } | ||
|
|
||
| return undefined; | ||
| }, masterPayload); | ||
| } | ||
|
|
||
| private buildElement(itemPointerToDefaultRecord: Record<string, string>, suffix: string) { | ||
| const mockPayload: Record<string, any> = {}; | ||
| Object.keys(itemPointerToDefaultRecord).forEach((key) => { | ||
| const keys = key.split('.'); | ||
| let current = mockPayload; | ||
| keys.forEach((innerKey, index) => { | ||
| if (!current[innerKey]) { | ||
| current[innerKey] = {}; | ||
| } | ||
| if (index === keys.length - 1) { | ||
| current[innerKey] = itemPointerToDefaultRecord[key] + suffix; | ||
| } else { | ||
| current = current[innerKey]; | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| return mockPayload; | ||
| } | ||
|
|
||
| private isPayloadValue(node: TipTapNode): node is { type: 'payloadValue'; attrs: { id: string; fallback?: string } } { | ||
| return !!(node.type === 'payloadValue' && node.attrs && typeof node.attrs.id === 'string'); | ||
| } |
There was a problem hiding this comment.
not needed since we are removing related stuff here: #7484
@novu/client
@novu/headless
@novu/notification-center
@novu/node
novu
@novu/providers
@novu/shared
commit: |
| } | ||
|
|
||
| return { | ||
| hydratedEmailSchema: emailBody, | ||
| placeholderAggregation, | ||
| placeholderAggregation, // TODO: remove this |
There was a problem hiding this comment.
Useless since we are removing this here: #7484
We can remove this line after the mentioned PR is done, I'm keeping it here to not break types down the line.
There was a problem hiding this comment.
@djabarovgeorge please make sure we merge the above ASAP, preferably today.
| @@ -28,60 +33,98 @@ export class ExpandEmailEditorSchemaUsecase { | |||
| return hydratedEmailSchema; | |||
| } | |||
|
|
|||
| private async processShowAndForControls( | |||
| variables: Record<string, unknown>, | |||
| private async processSpecialNodeTypes(variables: Record<string, unknown>, rootNode: TipTapNode): Promise<TipTapNode> { | |||
There was a problem hiding this comment.
Something is a bit off with naming. Is the first parameter Novu Variables? Isn't fullPayloadForRender of this type?
There was a problem hiding this comment.
correct Novu Variables and fullPayloadForRender are equal.
| } | ||
|
|
||
| private hasEach(node: TipTapNode): node is TipTapNode & { attrs: { each: unknown } } { | ||
| return !!(node.attrs && 'each' in node.attrs); | ||
| private async handleShowNode( |
There was a problem hiding this comment.
Show is an Attribute, not a node. So to be precise we should name this as handleShowAttr
There was a problem hiding this comment.
even more precise would be handleNodeByShowAttr because here we decide if to show a Node based on an attribute.
| return !!(node.attrs && 'each' in node.attrs); | ||
| private async handleShowNode( | ||
| node: TipTapNode & { attrs: { showIfKey: unknown } }, | ||
| variables: Record<string, unknown>, |
There was a problem hiding this comment.
Ditto about adding stricter typing to variables.
| } | ||
|
|
||
| private hasShow(node: TipTapNode): node is TipTapNode & { attrs: { [MailyAttrsEnum.SHOW_IF_KEY]: string } } { | ||
| return node.attrs?.[MailyAttrsEnum.SHOW_IF_KEY] !== undefined; | ||
| private async handleEachNode( |
There was a problem hiding this comment.
As discussed above
| private async handleEachNode( | |
| private async handleEachAttr( |
| private hasShow(node: TipTapNode): node is TipTapNode & { attrs: { [MailyAttrsEnum.SHOW_IF_KEY]: string } } { | ||
| return node.attrs?.[MailyAttrsEnum.SHOW_IF_KEY] !== undefined; | ||
| private async handleEachNode( | ||
| node: TipTapNode & { attrs: { each: unknown } }, |
There was a problem hiding this comment.
Ditto about enhancing the TipTapNode shared type
There was a problem hiding this comment.
I believe in this context we are talking about enhancing Maily JSONContent (TipTap based) opinionated type
| return node.attrs?.[MailyAttrsEnum.SHOW_IF_KEY] !== undefined; | ||
| private async handleEachNode( | ||
| node: TipTapNode & { attrs: { each: unknown } }, | ||
| variables: Record<string, unknown>, |
There was a problem hiding this comment.
Ditto on stronger types on this.
| } | ||
|
|
||
| private hasEach(node: TipTapNode): node is TipTapNode & { attrs: { each: unknown } } { | ||
| return !!(node.attrs && 'each' in node.attrs); |
There was a problem hiding this comment.
| return !!(node.attrs && 'each' in node.attrs); | |
| return !!(node.attrs && MailyAttrsEnum.EACH_KEY in node.attrs); |
| } | ||
|
|
||
| return { | ||
| hydratedEmailSchema: emailBody, | ||
| placeholderAggregation, | ||
| placeholderAggregation, // TODO: remove this |
There was a problem hiding this comment.
@djabarovgeorge please make sure we merge the above ASAP, preferably today.
There was a problem hiding this comment.
Does this change add support for arrays? can we have a small test in liquid-parser.spec.ts for this new logic?
There was a problem hiding this comment.
the validation became even more complex, this is fine for now but i wonder if we could make the validation work without this change.
|
|
||
| return this.regularExpansion(eachObject, templateContent); | ||
| } | ||
| const iterableArray = this.getValueByPath(variables, iterablePath) as [{ [key: string]: unknown }]; |
There was a problem hiding this comment.
here we do not need to getValueByPath, id suggest doing the following:
go to variableAttributeConfig
add a new entry, { attr: MailyAttrsEnum.EACH, flag: MailyAttrsEnum.EACH }
the result will be that in the pipeline we will wrap each attribute (node.attrs.each) with {{...}}
then you could get the value from the variable using liquid js.
| const iterableArray = this.getValueByPath(variables, iterablePath) as [{ [key: string]: unknown }]; | |
| const iterableArray = await parseLiquid(iterablePath as string, variables); |
let me know what you think.
| if (showIfKey === undefined) { | ||
| return; | ||
| } | ||
| private addIndexesToPlaceholders(nodes: TipTapNode[], iterablePath: string, index: number): TipTapNode[] { |
There was a problem hiding this comment.
what do you think about?
| private addIndexesToPlaceholders(nodes: TipTapNode[], iterablePath: string, index: number): TipTapNode[] { | |
| private addIndexesToLiquidOutput(nodes: TipTapNode[], iterablePath: string, index: number): TipTapNode[] { |
| } | ||
| } | ||
|
|
||
| function duplicateArrayItems(obj: Record<string, any>, size = 3): void { |
There was a problem hiding this comment.
What do you think about moving this logic to the "generate preview" use case or making it configurable? Since this multiplication is only needed for previews right now, it could be triggered on demand. In the future, we could include it in the trigger tests if necessary.
There was a problem hiding this comment.
Amazing work, looks super clean! please go over the comments
as a side thought, what do we think about adding indexes tp the preview? At first glance, it’s hard to understand that each value in the payload corresponds to a separate line in the preview, especially since the strings are identical.
What changed? Why was the change needed?
{{payload.comments}}, item is{{payload.comments.id}}High-level overview of how the parsing flow goes so we can reference it later when refactoring:
for-loop.mov