feat(aci): Add validation to threshold inputs (#96375)

scttcper · web-flow · commit 6021f68992af · 2025-07-25T11:18:23.000-07:00
diff --git a/static/app/components/workflowEngine/form/control/index.stories.tsx b/static/app/components/workflowEngine/form/control/index.stories.tsx
@@ -16,7 +16,7 @@ export default Storybook.story('Form Controls', story => {
 
       <Form hideFooter>
         <Flex direction="column" gap="xl">
-          <PriorityControl minimumPriority={DetectorPriorityLevel.LOW} />
+          <PriorityControl minimumPriority={DetectorPriorityLevel.MEDIUM} />
         </Flex>
       </Form>
     </Fragment>
diff --git a/static/app/components/workflowEngine/form/control/priorityControl.spec.tsx b/static/app/components/workflowEngine/form/control/priorityControl.spec.tsx
@@ -14,37 +14,36 @@ describe('PriorityControl', function () {
     const formModel = new FormModel({
       initialData: {
         ...DEFAULT_THRESHOLD_METRIC_FORM_DATA,
-        [METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel]: DetectorPriorityLevel.LOW,
+        [METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel]: DetectorPriorityLevel.MEDIUM,
       },
     });
     render(
       <Form model={formModel} hideFooter>
-        <PriorityControl minimumPriority={DetectorPriorityLevel.LOW} />
+        <PriorityControl minimumPriority={DetectorPriorityLevel.MEDIUM} />
       </Form>
     );
 
     expect(await screen.findByText('Above 0ms')).toBeInTheDocument();
-    expect(screen.getByLabelText('Medium threshold')).toBeInTheDocument();
     expect(screen.getByLabelText('High threshold')).toBeInTheDocument();
   });
 
   it('allows configuring priority', async function () {
     const formModel = new FormModel({
       initialData: {
         ...DEFAULT_THRESHOLD_METRIC_FORM_DATA,
-        [METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel]: DetectorPriorityLevel.LOW,
+        [METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel]: DetectorPriorityLevel.MEDIUM,
       },
     });
     render(
       <Form model={formModel} hideFooter>
-        <PriorityControl minimumPriority={DetectorPriorityLevel.LOW} />
+        <PriorityControl minimumPriority={DetectorPriorityLevel.MEDIUM} />
       </Form>
     );
-    expect(await screen.findByRole('button', {name: 'Low'})).toBeInTheDocument();
+    expect(await screen.findByRole('button', {name: 'Med'})).toBeInTheDocument();
     expect(screen.getByText('Med')).toBeInTheDocument();
     expect(screen.getByText('High')).toBeInTheDocument();
 
-    await userEvent.click(screen.getByRole('button', {name: 'Low'}));
+    await userEvent.click(screen.getByRole('button', {name: 'Med'}));
     await userEvent.click(await screen.findByRole('option', {name: 'High'}));
     expect(formModel.getValue(METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel)).toBe(
       DetectorPriorityLevel.HIGH
@@ -53,25 +52,22 @@ describe('PriorityControl', function () {
     expect(screen.getAllByRole('button')).toHaveLength(1);
   });
 
-  it('allows configuring medium and high thresholds', async function () {
+  it('allows configuring high thresholds', async function () {
     const formModel = new FormModel({
       initialData: {
         ...DEFAULT_THRESHOLD_METRIC_FORM_DATA,
-        [METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel]: DetectorPriorityLevel.LOW,
+        [METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel]: DetectorPriorityLevel.MEDIUM,
       },
     });
     render(
       <Form model={formModel} hideFooter>
-        <PriorityControl minimumPriority={DetectorPriorityLevel.LOW} />
+        <PriorityControl minimumPriority={DetectorPriorityLevel.MEDIUM} />
       </Form>
     );
-    const medium = screen.getByLabelText('Medium threshold');
-    await userEvent.type(medium, '4');
 
     const high = screen.getByLabelText('High threshold');
     await userEvent.type(high, '5');
 
-    expect(formModel.getValue(METRIC_DETECTOR_FORM_FIELDS.mediumThreshold)).toBe('4');
     expect(formModel.getValue(METRIC_DETECTOR_FORM_FIELDS.highThreshold)).toBe('5');
   });
 
@@ -97,4 +93,39 @@ describe('PriorityControl', function () {
     expect(screen.getByRole('option', {name: 'High'})).toBeInTheDocument();
     expect(screen.queryByRole('option', {name: 'Low'})).not.toBeInTheDocument();
   });
+
+  it('validates that medium threshold is lower than high threshold', async function () {
+    const formModel = new FormModel({
+      initialData: {
+        ...DEFAULT_THRESHOLD_METRIC_FORM_DATA,
+        [METRIC_DETECTOR_FORM_FIELDS.initialPriorityLevel]: DetectorPriorityLevel.MEDIUM,
+        [METRIC_DETECTOR_FORM_FIELDS.conditionValue]: '10', // This is the medium threshold
+      },
+    });
+
+    render(
+      <Form model={formModel} hideFooter>
+        <PriorityControl minimumPriority={DetectorPriorityLevel.MEDIUM} />
+      </Form>
+    );
+
+    // Only high threshold field should be visible when initial priority is MEDIUM
+    const highField = screen.getByLabelText('High threshold');
+    expect(screen.queryByLabelText('Medium threshold')).not.toBeInTheDocument();
+
+    // Test invalid case: high (5) <= medium (10)
+    await userEvent.clear(highField);
+    await userEvent.type(highField, '5');
+
+    expect(formModel.getError(METRIC_DETECTOR_FORM_FIELDS.highThreshold)).toBe(
+      'High threshold must be higher than medium threshold'
+    );
+
+    // Test valid case: high (15) > medium (10)
+    await userEvent.clear(highField);
+    await userEvent.type(highField, '15');
+
+    // Validation should clear error when high > medium
+    expect(formModel.getError(METRIC_DETECTOR_FORM_FIELDS.highThreshold)).toBeFalsy();
+  });
 });
diff --git a/static/app/components/workflowEngine/form/control/priorityControl.tsx b/static/app/components/workflowEngine/form/control/priorityControl.tsx
@@ -1,5 +1,6 @@
 import {useContext} from 'react';
 import styled from '@emotion/styled';
+import toNumber from 'lodash/toNumber';
 
 import {GroupPriorityBadge} from 'sentry/components/badge/groupPriority';
 import {CompactSelect} from 'sentry/components/core/compactSelect';
@@ -16,11 +17,12 @@ import {
   DETECTOR_PRIORITY_LEVEL_TO_PRIORITY_LEVEL,
   DetectorPriorityLevel,
 } from 'sentry/types/workflowEngine/dataConditions';
+import type {MetricDetectorFormData} from 'sentry/views/detectors/components/forms/metric/metricFormData';
 import {
   METRIC_DETECTOR_FORM_FIELDS,
   useMetricDetectorFormField,
 } from 'sentry/views/detectors/components/forms/metric/metricFormData';
-import {getStaticDetectorThresholdSuffix} from 'sentry/views/detectors/utils/metricDetectorSuffix';
+import {getMetricDetectorSuffix} from 'sentry/views/detectors/utils/metricDetectorSuffix';
 
 const priorities = [
   DetectorPriorityLevel.LOW,
@@ -42,17 +44,13 @@ const conditionKindAndTypeToLabel: Record<
   },
 };
 
-function ThresholdPriority() {
+function ThresholdPriority({thresholdSuffix}: {thresholdSuffix: string}) {
   const conditionType = useMetricDetectorFormField(
     METRIC_DETECTOR_FORM_FIELDS.conditionType
   );
   const conditionValue = useMetricDetectorFormField(
     METRIC_DETECTOR_FORM_FIELDS.conditionValue
   );
-  const aggregate = useMetricDetectorFormField(
-    METRIC_DETECTOR_FORM_FIELDS.aggregateFunction
-  );
-  const thresholdSuffix = getStaticDetectorThresholdSuffix(aggregate);
 
   return (
     <div>
@@ -63,17 +61,13 @@ function ThresholdPriority() {
   );
 }
 
-function ChangePriority() {
+function ChangePriority({thresholdSuffix}: {thresholdSuffix: string}) {
   const conditionType = useMetricDetectorFormField(
     METRIC_DETECTOR_FORM_FIELDS.conditionType
   );
   const conditionValue = useMetricDetectorFormField(
     METRIC_DETECTOR_FORM_FIELDS.conditionValue
   );
-  const aggregate = useMetricDetectorFormField(
-    METRIC_DETECTOR_FORM_FIELDS.aggregateFunction
-  );
-  const thresholdSuffix = getStaticDetectorThresholdSuffix(aggregate);
 
   return (
     <div>
@@ -83,8 +77,54 @@ function ChangePriority() {
   );
 }
 
+function createValidationError(field: string, message: string): [string, string] {
+  return [field, message];
+}
+
+function validateThresholdOrder(
+  value: number,
+  reference: number,
+  conditionType: DataConditionType,
+  isGreaterExpected: boolean
+): boolean {
+  if (conditionType === DataConditionType.GREATER) {
+    return isGreaterExpected ? value > reference : value < reference;
+  }
+  // For LESS condition type, logic is inverted
+  return isGreaterExpected ? value < reference : value > reference;
+}
+
+function validateHighThreshold({
+  form,
+}: {
+  form: MetricDetectorFormData;
+  id: string;
+}): Array<[string, string]> {
+  const highNum = toNumber(form.highThreshold);
+  const conditionNum = toNumber(form.conditionValue);
+  const {conditionType} = form;
+
+  if (!conditionType) {
+    return [];
+  }
+
+  if (
+    Number.isFinite(highNum) &&
+    Number.isFinite(conditionNum) &&
+    !validateThresholdOrder(highNum, conditionNum, conditionType, true)
+  ) {
+    const message = t(
+      'High threshold must be %s than medium threshold',
+      conditionType === DataConditionType.GREATER ? t('higher') : t('lower')
+    );
+    return [createValidationError(METRIC_DETECTOR_FORM_FIELDS.highThreshold, message)];
+  }
+
+  return [];
+}
+
 interface PriorityControlProps {
-  minimumPriority: DetectorPriorityLevel;
+  minimumPriority: DetectorPriorityLevel.MEDIUM | DetectorPriorityLevel.HIGH;
 }
 
 export default function PriorityControl({minimumPriority}: PriorityControlProps) {
@@ -100,7 +140,7 @@ export default function PriorityControl({minimumPriority}: PriorityControlProps)
   const aggregate = useMetricDetectorFormField(
     METRIC_DETECTOR_FORM_FIELDS.aggregateFunction
   );
-  const thresholdSuffix = getStaticDetectorThresholdSuffix(aggregate);
+  const thresholdSuffix = getMetricDetectorSuffix(detectionType, aggregate);
 
   if (detectionType === 'dynamic') {
     return null;
@@ -111,47 +151,31 @@ export default function PriorityControl({minimumPriority}: PriorityControlProps)
       <PrioritizeRow
         left={
           <Flex align="center" direction="column">
-            {detectionType === 'static' ? <ThresholdPriority /> : <ChangePriority />}
+            {detectionType === 'static' ? (
+              <ThresholdPriority thresholdSuffix={thresholdSuffix} />
+            ) : (
+              <ChangePriority thresholdSuffix={thresholdSuffix} />
+            )}
             <SecondaryLabel>({t('issue created')})</SecondaryLabel>
           </Flex>
         }
         right={<InitialPrioritySelect minimumPriority={minimumPriority} />}
       />
-      {priorityIsConfigurable(initialPriorityLevel, DetectorPriorityLevel.MEDIUM) && (
+      {initialPriorityLevel === DetectorPriorityLevel.MEDIUM && (
         <PrioritizeRow
           left={
             <Flex align="center" gap="md">
               <SmallNumberField
-                alignRight
-                inline
-                hideLabel
-                flexibleControlStateSize
-                size="sm"
-                suffix={thresholdSuffix}
-                placeholder="0"
-                name={METRIC_DETECTOR_FORM_FIELDS.mediumThreshold}
-                aria-label={t('Medium threshold')}
-              />
-              <div>{conditionKindAndTypeToLabel[detectionType][conditionType!]}</div>
-            </Flex>
-          }
-          right={<GroupPriorityBadge showLabel priority={PriorityLevel.MEDIUM} />}
-        />
-      )}
-      {priorityIsConfigurable(initialPriorityLevel, DetectorPriorityLevel.HIGH) && (
-        <PrioritizeRow
-          left={
-            <Flex align="center" gap="md">
-              <SmallNumberField
-                alignRight
-                inline
+                inline={false}
                 hideLabel
                 flexibleControlStateSize
                 size="sm"
                 suffix={thresholdSuffix}
                 placeholder="0"
                 name={METRIC_DETECTOR_FORM_FIELDS.highThreshold}
                 aria-label={t('High threshold')}
+                validate={validateHighThreshold}
+                required
               />
               <div>{conditionKindAndTypeToLabel[detectionType][conditionType!]}</div>
             </Flex>
@@ -163,13 +187,6 @@ export default function PriorityControl({minimumPriority}: PriorityControlProps)
   );
 }
 
-function priorityIsConfigurable(
-  initialPriorityLevel: DetectorPriorityLevel,
-  targetPriority: DetectorPriorityLevel
-): boolean {
-  return targetPriority > initialPriorityLevel;
-}
-
 function PrioritizeRow({left, right}: {left: React.ReactNode; right: React.ReactNode}) {
   return (
     <Row>
@@ -258,7 +275,7 @@ const Cell = styled('div')`
 `;
 
 const SmallNumberField = styled(NumberField)`
-  width: 3.5rem;
+  width: 6rem;
   padding: 0;
   & > div {
     padding-left: 0;
diff --git a/static/app/views/detectors/components/details/metric/sidebar.tsx b/static/app/views/detectors/components/details/metric/sidebar.tsx
@@ -54,7 +54,10 @@ function DetectorPriorities({detector}: {detector: MetricDetector}) {
       typeof condition.comparison === 'number'
         ? String(condition.comparison)
         : String(condition.comparison || '0');
-    const thresholdSuffix = getMetricDetectorSuffix(detector);
+    const thresholdSuffix = getMetricDetectorSuffix(
+      detector.config?.detectionType || 'static',
+      detector.dataSources[0].queryObj?.snubaQuery?.aggregate || 'count()'
+    );
 
     return `${typeLabel} ${comparisonValue}${thresholdSuffix}`;
   };
@@ -87,7 +90,10 @@ function DetectorResolve({detector}: {detector: MetricDetector}) {
   const mainCondition = conditions.find(
     condition => condition.conditionResult !== DetectorPriorityLevel.OK
   );
-  const thresholdSuffix = getMetricDetectorSuffix(detector);
+  const thresholdSuffix = getMetricDetectorSuffix(
+    detector.config?.detectionType || 'static',
+    detector.dataSources[0].queryObj?.snubaQuery?.aggregate || 'count()'
+  );
 
   const description = getResolutionDescription({
     detectionType,
diff --git a/static/app/views/detectors/components/detectorLink.tsx b/static/app/views/detectors/components/detectorLink.tsx
@@ -81,14 +81,17 @@ function DetailItem({children}: {children: React.ReactNode}) {
 }
 
 function MetricDetectorConfigDetails({detector}: {detector: MetricDetector}) {
-  const type = detector.config.detectionType;
+  const detectionType = detector.config.detectionType;
   const conditions = detector.conditionGroup?.conditions;
   if (!conditions?.length) {
     return null;
   }
 
-  const unit = getMetricDetectorSuffix(detector);
-  switch (type) {
+  const unit = getMetricDetectorSuffix(
+    detectionType,
+    detector.dataSources[0].queryObj?.snubaQuery?.aggregate || 'count()'
+  );
+  switch (detectionType) {
     case 'static': {
       const text = conditions
         .map(condition => formatCondition({condition, unit}))
@@ -112,7 +115,7 @@ function MetricDetectorConfigDetails({detector}: {detector: MetricDetector}) {
     case 'dynamic':
       return <DetailItem>{t('Dynamic')}</DetailItem>;
     default:
-      unreachable(type);
+      unreachable(detectionType);
       return null;
   }
 }
diff --git a/static/app/views/detectors/edit.spec.tsx b/static/app/views/detectors/edit.spec.tsx
@@ -126,7 +126,7 @@ describe('DetectorEdit | Metric Detector', () => {
               queryType: 0,
             },
             conditionGroup: {
-              conditions: [{comparison: 8, conditionResult: 50, type: 'gt'}],
+              conditions: [{comparison: 8, conditionResult: 75, type: 'gt'}],
               logicType: 'any',
             },
             config: {detectionType: 'static', thresholdPeriod: 1},
diff --git a/static/app/views/detectors/utils/metricDetectorSuffix.spec.tsx b/static/app/views/detectors/utils/metricDetectorSuffix.spec.tsx
diff --git a/static/app/views/detectors/utils/metricDetectorSuffix.tsx b/static/app/views/detectors/utils/metricDetectorSuffix.tsx
diff --git a/tests/js/fixtures/dataConditions.ts b/tests/js/fixtures/dataConditions.ts
