-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(aci): Edit detector form #93579
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
Conversation
note for monday: might need to get the existing ids to update the detector?
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
# Conflicts: # static/app/views/detectors/components/forms/metricFormData.tsx # static/app/views/detectors/edit.tsx # static/app/views/detectors/new-settings.tsx
@@ -61,7 +61,7 @@ export interface MetricDetectorFormData | |||
MetricDetectorDynamicFormData { | |||
aggregate: string; | |||
environment: string; | |||
kind: 'threshold' | 'change' | 'dynamic'; | |||
kind: 'static' | 'percent' | 'dynamic'; |
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.
these are more in line with the values the backend is using
|
||
// Extract aggregate and visualize from the aggregate string | ||
// Format is typically "count(transaction.duration)" | ||
const aggregateMatch = snubaQuery?.aggregate?.match(/^(\w+)\(([^)]+)\)$/); |
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.
Will this always work? What are the other possible values?
I wonder if we should be using a parser for this or returning as an object form the backend? Work for later 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.
yeah let me dig up what we do in other places
const {mutateAsync: updateDetector} = useUpdateDetector(); | ||
|
||
const handleSubmit = useCallback<OnSubmitCallback>( | ||
async (data, _, __, ___, formModel) => { |
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.
gotta love functions with 5 arguments
...getNewMetricDetectorData(data as MetricDetectorFormData), | ||
}; | ||
|
||
const updatedDetector = await updateDetector(updatedData); |
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.
Does this handle errors?
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.
no :(
building off the work in #93380 this pr adds the ability to edit a dector.