feat(api): preview usecase #7330
Conversation
# Conflicts: # apps/api/package.json # apps/api/src/app/environments-v1/usecases/output-renderers/render-email-output.usecase.ts # apps/api/src/app/workflows-v2/shared/schemas/email-control.schema.ts
# Conflicts: # apps/api/src/app/workflows-v2/shared/schemas/email-control.schema.ts
✅ 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. |
djabarovgeorge
changed the title
| const { fallback } = node.attrs; | ||
| const variableName = node.attrs.id; | ||
| const buildLiquidJSDefault = (mailyFallback: string) => (mailyFallback ? ` | default: '${mailyFallback}'` : ''); |
There was a problem hiding this comment.
Consider moving all the variable calculations from start to end into the buildLiquidJSVariable function defined once either on the class or as a single util. That is combine line 122 and 133.
There was a problem hiding this comment.
Let's also add an extra step of sanitization in the above function that caters for weird cases where the variableName ends with |.
There was a problem hiding this comment.
Lastly, this transformation should happena cross all steps, not just email.
There was a problem hiding this comment.
Consider moving all the variable calculations from start to end into the buildLiquidJSVariable function defined once either on the class or as a single util. That is combine line 122 and 133.
If you refer to the show resolver, i totally agree, this was postponed until we have full show support.
Let's also add an extra step of sanitization in the above function that caters for weird cases where the variableName ends with |.
I love the idea but i believe we can postpone such sanitization because liquid js does not care about filter suffixes.
Lastly, this transformation should happena cross all steps, not just email.
can you elaborate please what do you mean by this transformation
| const client = new Liquid(); | ||
| const templateString = client.parse(JSON.stringify(value)); | ||
| const parsedTipTap = await client.render(templateString, { | ||
| payload: renderCommand.fullPayloadForRender.payload, |
There was a problem hiding this comment.
Can we get rid of the fullPayloadForRender extra nesting? It will make the code way more beautiful.
There was a problem hiding this comment.
I believe we can, but this refactor is focused on doing the minimum needed to make the code work with what we currently have. We'll try to avoid major changes as much as possible.
The goal is to ensure the use cases—like sending a simple email, "show" component, then "for" components—work as expected with minimal refactoring. If that's not possible, we'll consider a bigger refactor.
Once we have a working solution, we can refactor further if needed. One potential improvement could be flattening the data as you suggested.
apps/api/src/app/workflows-v2/shared/schemas/email-control.schema.ts
Outdated
Show resolved
Hide resolved
libs/application-generic/src/utils/sanitize-preview-control-values.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # apps/api/src/app/workflows-v2/usecases/generate-preview/generate-preview.usecase.ts
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
libs/application-generic/src/utils/sanitize-preview-control-values.ts
Outdated
Show resolved
Hide resolved
| }; | ||
| } | ||
|
|
||
| function sanitizePush(controlValues: Record<string, unknown>) { |
There was a problem hiding this comment.
Would suggest using here typed control schemas from the framework
libs/application-generic/src/utils/sanitize-preview-control-values.ts
Outdated
Show resolved
Hide resolved
| if (typeof normalized === 'object' && normalized !== null) { | ||
| return Object.fromEntries( | ||
| Object.entries(normalized).filter(([_, value]) => value !== null), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Would suggest maybe to introduce function or add comment (a little hard to understand what it's doing)
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
@novu/client
@novu/js
@novu/framework
@novu/nest
@novu/headless
@novu/nextjs
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/react
@novu/react-native
@novu/shared
@novu/stateless
commit: |
What changed? Why was the change needed?
https://linear.app/novu/issue/NV-5064/api-preview-parse-tiptap-content-variables-extraction-on-the-new
https://linear.app/novu/issue/NV-5065/api-preview-remove-the-responsibility-of-parsing-variables-in-bridge
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer