-
Notifications
You must be signed in to change notification settings - Fork 465
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(webapp): Handle empty state for webhook settings coming over the wire #3010
fix(webapp): Handle empty state for webhook settings coming over the wire #3010
Conversation
} | ||
|
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.
would it make sense to have those settings always returned by the api?
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.
@TBonnin I'm pretty sure we could always return them. We would have to either:
- pre-set an
ExternalWebhook
in the DB when we create the environment - remove the
id
field from thewebhook_settings
part of the response so we can send an empty state without an id
Technically we could make the id nullable but it seems like it'd be better just to not leak that id since we don't use it to update the webhook anyway.
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.
I would be tempted to go for option 2 if we are actually not using this internal id anywhere. Approving but up to you to stick with this frontend change if you prefer this option
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.
@TBonnin I'll let this sit until tomorrow and we can see what @bodinsamuel says about it. I 100% agree with you about choosing option 2 though.
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.
I would go for 1
personally, no changes are required and feels normal to me to pre-populate data
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.
I'm going to merge this as-is, and I logged a ticket to come back to for resolving this discussion, but I think this PR solves an existing problem the fastest way we can right now.
…tings-values-when-switching
Describe your changes
When webhook settings come back over the wire as null, which happens when they've never been configured, we were just skipping updating them, which caused stale data from an environment change in the UI. This fixes that!
Issue ticket number and link
https://linear.app/nango/issue/NAN-2104/inconsistent-environment-settings-values-when-switching-environments
Checklist before requesting a review (skip if just adding/editing APIs & templates)