-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: so empty string can not be used as default value #2757
fix: so empty string can not be used as default value #2757
Conversation
NV-1598 🐛 Bug Report: Template variables are given a default empty string value even when nothing is entered in the default value form
📜 DescriptionWhen creating a template, even when you do not set a default variable, the value is set to an empty string. 👟 Reproduction stepsCreate a template and use a variable named 👍 Expected behaviorNovu should not overwrite with an empty string if there is no default value set. 👎 Actual Behavior with ScreenshotsNote that when inspecting the "payload" in the activity feed Novu will have sent an empty string as a default value for You can also check by getting the template from the API and seeing this in the variables:
|
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.
Looks great, Is it possible to add a test for this functionality?
...src/app/message-template/usecases/create-message-template/create-message-template.usecase.ts
Outdated
Show resolved
Hide resolved
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.
🌟
expect(result.variables?.length).to.eql(1); | ||
expect(result.variables?.at(0)?.defaultValue).to.eql('test'); |
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.
Shouldn't the message be created with the variable even if the default value is false? And we just shouldn't override the sent message with this information?
Because this variable is still was used in the template (if that's the usage of the variables array)
And only during trigger we should filter out to not override it with a string. Or maybe I'm missing something @davidsoderberg
What change does this PR introduce?
so empty string can not be used as default value
Why was this change needed?
To be more logical with how we handle default values