feat(dashboard): restore email editor 'for' block

[ChmaraX]

What changed? Why was the change needed?

• DX: iterable needs to be referenced in full, e.g. when looping over {{payload.comments}}, item is {{payload.comments.id}}
• the preview always generates 3 example items in the array
• I've tried to restore the functionality with the least amount of code change, but in my opinion heavy refactoring is required for the entire preview

High-level overview of how the parsing flow goes so we can reference it later when refactoring:

for-loop.mov

[linear]

NV-5034 🐛 Fix preview of Maily For Block

NV-5138 API(email-for): Expand Maily Content JSON to TipTap JSON

[netlify]

✅ Deploy Preview for dev-web-novu ready!

Name	Link
🔨 Latest commit	9c8d4a5
🔍 Latest deploy log	https://app.netlify.com/sites/dev-web-novu/deploys/67867809d4881d000968df6a
😎 Deploy Preview	https://deploy-preview-7483.dashboard.novu-staging.co
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[netlify]

✅ Deploy Preview for dashboard-v2-novu-staging ready!

Name	Link
🔨 Latest commit	9c8d4a5
🔍 Latest deploy log	https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/678678093c45d10009f9d9b1
😎 Deploy Preview	https://deploy-preview-7483.dashboard-v2.novu-staging.co
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[djabarovgeorge]

Amazing work, i left a couple of comments about my opinion of responsibility spread in the code and the meaning of it.

[djabarovgeorge]

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.

[ChmaraX]

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 }}

[pkg-pr-new]

Open in Stackblitz

@novu/client

npm i https://pkg.pr.new/novuhq/novu/@novu/client@7483

@novu/node

npm i https://pkg.pr.new/novuhq/novu/@novu/node@7483

@novu/notification-center

npm i https://pkg.pr.new/novuhq/novu/@novu/notification-center@7483

@novu/headless

npm i https://pkg.pr.new/novuhq/novu/@novu/headless@7483

novu

npm i https://pkg.pr.new/novuhq/novu@7483

@novu/providers

npm i https://pkg.pr.new/novuhq/novu/@novu/providers@7483

@novu/shared

npm i https://pkg.pr.new/novuhq/novu/@novu/shared@7483

commit: 9c8d4a5

[SokratisVidros]

Show is an Attribute, not a node. So to be precise we should name this as handleShowAttr

[djabarovgeorge]

even more precise would be handleNodeByShowAttr because here we decide if to show a Node based on an attribute.

[ChmaraX]

I have to disagree here, I think this way the naming is more common and understandable.

We are processing nodes and the for and show are a specific type of nodes. We are not processing the attribute only, we are processing an entire node which has attr = "for", similar as step.type = "email".

When you first time look at this method, its clear that if the node is of type for you do something with the node - you dont need to know about some internal property name such as attr or anything:

  private async processNode(node: TipTapNode, variables: FullPayloadForRender, parent?: TipTapNode): Promise<void> {
    if (this.hasShow(node)) {
      await this.handleShowNode(node, variables, parent);
    }

    if (this.hasEach(node)) {
      await this.handleEachNode(node, variables, parent);
    }
  }

[SokratisVidros]

How about handleEach, handleShow?

[djabarovgeorge]

the validation became even more complex, this is fine for now but i wonder if we could make the validation work without this change.

[djabarovgeorge]

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.

Suggested change

	const iterableArray = this.getValueByPath(variables, iterablePath) as [{ [key: string]: unknown }];
	const iterableArray = await parseLiquid(iterablePath as string, variables);

let me know what you think.

[ChmaraX]

In this case, iterableArray accepts and returns this:

// variables, iterablePath = "payload.comments"
const iterableArray = this.getValueByPath(variables, iterablePath) as [{ [key: string]: unknown }];
/**
* iterableArray = [
*  {
*    name: '{{payload.comments.name}}',
* },
* {
*   name: '{{payload.comments.name}}',
* },
* {
*   name: '{{payload.comments.name}}',
* }
*]
**/

What you are suggesting, would return just the iterable path string, not the actual array:

const iterableArray = await parseLiquid(iterablePath as string, variables);
// iterableArray = payload.comments

[djabarovgeorge]

not sure if i understand it should return an array.

Editor:

for block {{ payload.comments }}

Variable:

{"payload" : {
    "comments": [
      " a_1 ",
      " a_2 ",
      " a_3 "
    ]
  }
}

[ChmaraX]

It currently doesnt work like this because we dont have for block {{ payload.comments }} anywhere in the maily json, we have just each: {{ payload.comments}}. That said its interesting idea to explore I will have a look if we can do this separately.

[djabarovgeorge]

for semantics representation aside, this one should work, doesn't it?
we have each: {{ payload.comments}}
with data variables: {"payload":{"comments":[" a_1 "," a_2 "," a_3 "]}}
once we execute const iterableArray = await parseLiquid(iterablePath as string, variables);
we should get the array [" a_1 "," a_2 "," a_3 "]

[djabarovgeorge]

what do you think about?

Suggested change

	private addIndexesToPlaceholders(nodes: TipTapNode[], iterablePath: string, index: number): TipTapNode[] {
	private addIndexesToLiquidOutput(nodes: TipTapNode[], iterablePath: string, index: number): TipTapNode[] {

[ChmaraX]

But the placeholders ({{payload.something}}) are not liquid output right? They are more Liquid input, since the parsing by Liquid happens later and this is usecase is an input for that:

 const parsedTipTap = await this.parseTipTapNodeByLiquid(expandedMailyContent, renderCommand);

[djabarovgeorge]

Liquid JS calls the value with filters wrapped with {{...}} an Output i using Output instead of placeholder would help us unify and help us to understand each other better.

and the main logic in this function is
newNode.text = nodePlaceholder.replace(iterablePath, ${iterablePath}[${index}]);
which creates a liquid js Output.
Let me know what you think about it.

In addition, apologies for missing this earlier I noticed that we are using string.replace. Instead, we should use wrapInLiquidOutput whenever we have a Maily fallback attribute.

[djabarovgeorge]

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.