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(dashboard): Tweak arbitrary variable handling #7351

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

SokratisVidros
Copy link
Contributor

What changed? Why was the change needed?

  • A minor cleanup on empty JSON Schema
  • Treat inline variables from payload or subscriber.data as not required in Maily

Copy link

netlify bot commented Dec 23, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 26cdff6
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/676930cd48515d0008db126c
😎 Deploy Preview https://deploy-preview-7351.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.

properties: {},
additionalProperties: true,
};
return emptyJsonSchema();
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

label: null,
fallback: null,
showIfKey: null,
required: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

question: What the required prop controls here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ⚠️ icon in Maily jargon. For now, all of our variables should not be required.

Copy link
Contributor

Choose a reason for hiding this comment

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

But does is solve the actual problem then? Because the issue was happening when maily autocomplete returned "empty results" instead of allowing to add a new variable from the dropdown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A database migration solves the issue. I was torn between teaching the code to handle invalid payload vs just correcting the DB value and I did the latter as for code-first any user-defined payload schema should be accepted.

Copy link

netlify bot commented Dec 23, 2024

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

Name Link
🔨 Latest commit 26cdff6
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/676930cd08063c000849c80a
😎 Deploy Preview https://deploy-preview-7351.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 Tweak arbitrary variable handling fix(dashboard): Tweak arbitrary variable handling Dec 23, 2024
@SokratisVidros SokratisVidros merged commit 6ba756a into next Dec 23, 2024
39 of 40 checks passed
@SokratisVidros SokratisVidros deleted the tweak_arbitrary_variable_handling branch December 23, 2024 10:01
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.

2 participants