Skip to content
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

fix(api-service): Workflow editor improvements #7682

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

SokratisVidros
Copy link
Contributor

What changed? Why was the change needed?

This PR ensures that all the used subscriber fields in a workflow are rendered in the Trigger page in the "Send to" section

Screenshots

Copy link

netlify bot commented Feb 7, 2025

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit e3272ec
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/67abca011bfa60000824dd9e
😎 Deploy Preview https://deploy-preview-7682.dashboard.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

netlify bot commented Feb 7, 2025

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

Name Link
🔨 Latest commit e3272ec
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/67abca01fa4750000814b215
😎 Deploy Preview https://deploy-preview-7682.dashboard-v2.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@SokratisVidros SokratisVidros changed the title Workflow editor improvements fix(api-service): Workflow editor improvements Feb 7, 2025
Comment on lines 127 to 133
if (variables.subscriber.isOnline) {
toSchema.properties!.avatar = { type: 'boolean', default: true };
}

if (variables.subscriber.isLastOnline) {
toSchema.properties!.avatar = { type: 'string', format: 'date-time', default: new Date().toISOString() };
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something is odd with these two, we set the avatar property...
despite that, why do we want to allow setting these two values in the form? they are controlled by WS service

Copy link
Member

@BiswaViraj BiswaViraj Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we shouldn't allow setting the online fields in the to schema.
WS controls these.
I am working on it now, and will remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about the test data during preview. We need to give users the ability to test these values especially if they use them in Show conditions.

Return all subscriber fields as the test data.to field based on the variables used in the workflow and the steps that the workflow includes.

That is, an SMS step requires a phone, an Email step an email and any other field should be present in the To test data object if it's used as a variable in the content of the workflow steps.
Copy link

pkg-pr-new bot commented Feb 11, 2025

Open in Stackblitz

@novu/client

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

@novu/headless

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

@novu/node

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

@novu/notification-center

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

novu

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

@novu/providers

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

@novu/shared

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

commit: e3272ec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants