-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(api): revert preview tests that was deleted #7237
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. |
body: 'This is a legal placeholder with a pipe [{{payload.variableName | upcase}}the pipe should show in the preview]', | ||
}, | ||
}; | ||
const previewResponseDto = await generatePreview( | ||
workflowsClient, | ||
workflowId, | ||
stepDatabaseId, | ||
requestDto, | ||
'email' | ||
); | ||
expect(previewResponseDto.result!.preview).to.exist; | ||
if (previewResponseDto.result!.type !== 'sms') { | ||
throw new Error('Expected sms'); | ||
} | ||
expect(previewResponseDto.result!.preview.body).to.contain('{{PAYLOAD.VARIABLENAME | UPCASE}}'); | ||
expect(previewResponseDto.previewPayloadExample).to.exist; | ||
expect(previewResponseDto?.previewPayloadExample?.payload?.variableName).to.equal( | ||
'{{payload.variableName | upcase}}' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a suggestion; in this comment, I want to highlight current potential issues we are facing.
Currently, the code is extracting and building the payload schema (used for validation and sanitation in PrepareAndValidateContentUsecase
) based on the new variable extraction powered by LiquidJS. This is important because we should rely more on LiquidJS, and this is a good example of why.
In the code below, my suggestion is an old test we had that used an invalid filter not supported by LiquidJS, leading to false positive test results.
body: 'This is a legal placeholder with a pipe [{{payload.variableName | upcase}}the pipe should show in the preview]', | |
}, | |
}; | |
const previewResponseDto = await generatePreview( | |
workflowsClient, | |
workflowId, | |
stepDatabaseId, | |
requestDto, | |
'email' | |
); | |
expect(previewResponseDto.result!.preview).to.exist; | |
if (previewResponseDto.result!.type !== 'sms') { | |
throw new Error('Expected sms'); | |
} | |
expect(previewResponseDto.result!.preview.body).to.contain('{{PAYLOAD.VARIABLENAME | UPCASE}}'); | |
expect(previewResponseDto.previewPayloadExample).to.exist; | |
expect(previewResponseDto?.previewPayloadExample?.payload?.variableName).to.equal( | |
'{{payload.variableName | upcase}}' | |
); | |
body: 'This is a legal placeholder with a pipe [{{payload.variableName | upper}}the pipe should show in the preview]', | |
}, | |
}; | |
const previewResponseDto = await generatePreview( | |
workflowsClient, | |
workflowId, | |
stepDatabaseId, | |
requestDto, | |
'email' | |
); | |
expect(previewResponseDto.result!.preview).to.exist; | |
if (previewResponseDto.result!.type !== 'sms') { | |
throw new Error('Expected sms'); | |
} | |
expect(previewResponseDto.result!.preview.body).to.contain('{{payload.variableName | upper}}'); | |
expect(previewResponseDto.previewPayloadExample).to.exist; | |
expect(previewResponseDto?.previewPayloadExample?.payload?.variableName).to.equal( | |
'{{payload.variableName | upper}}' | |
); |
What changed? Why was the change needed?
we had this task https://github.com/novuhq/novu/pull/7137/files where we added payload variable extraction for framework workflows and fixed preview-related issues.
As part of this PR, we removed some tests related to these
issues
.In this PR I reverted the tests that were deleted from
generate.preview.e2e.ts
and moved them toworkflow.controller.e2e.ts
, where the issue with the response return exists.Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer