-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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(alert-rules): Set the correct default value for new SelectControl #28804
Conversation
initialVal = isNaN(parseInt(data[name] as string, 10)) | ||
? data[name] | ||
: parseInt(data[name] as string, 10); |
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.
@getsentry/core-ui It looks like the new SelectControl options (rather than choices) are pretty strict about numbers in a different way than was already done. Was this intentional?
We store all these fields as strings, but when we select, we store them as numbers so that mismatch didn't allow default values to appear
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.
cc @davidenwang
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.
ahhh shoot didn't know about this restriction with SelectControl options, thanks for fixing @leeandher
@@ -71,7 +71,9 @@ class RuleNode extends React.Component<Props> { | |||
initialVal = fieldConfig.choices[0][0]; | |||
} | |||
} else { | |||
initialVal = data[name]; | |||
initialVal = isNaN(parseInt(data[name] as string, 10)) |
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.
@leeandher I don't see name
defined in either IssueAlertRuleAction or IssueAlertRuleCondition types. What is it supposed to be?
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.
@NisanthanNanthakumar It's the name of the dynamic fields that are custom to the integration (e.g. 'workplace' for slack, 'account' or 'service' for pagerduty)
Fixes a bug preventing defaults from showing up in Select Control dropdowns:
Before:

After:

Demo
Workflow is unaffected:
https://user-images.githubusercontent.com/35509934/134570235-412ae4ff-90d9-4ef5-83c3-07b125bfd73c.mov
Updates are reflected and deleting still works:
https://user-images.githubusercontent.com/35509934/134570483-95af5754-12e5-4c6b-933a-27e6dffedd41.mov