From 6112e6e105e8434085137c11ef5f56a04bfdee36 Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 20 Oct 2021 17:13:24 -0700 Subject: [PATCH] fix(cmp_alerts): Change the default timeWindow and changeDelta options for change alerts --- .../incidentRules/ruleConditionsForm.tsx | 41 ++++++++++++------- .../alerts/incidentRules/ruleForm/index.tsx | 26 +++++++++--- .../constants/changeAlerts.tsx | 2 +- .../views/alerts/issueRuleEditor/index.tsx | 10 ++++- .../alerts/issueRuleEditor/ruleNodeList.tsx | 12 ++++-- 5 files changed, 66 insertions(+), 25 deletions(-) diff --git a/static/app/views/alerts/incidentRules/ruleConditionsForm.tsx b/static/app/views/alerts/incidentRules/ruleConditionsForm.tsx index 6fc3f3a3aef1db..364122d699860d 100644 --- a/static/app/views/alerts/incidentRules/ruleConditionsForm.tsx +++ b/static/app/views/alerts/incidentRules/ruleConditionsForm.tsx @@ -55,9 +55,11 @@ type Props = { onFilterSearch: (query: string) => void; alertType: AlertType; dataset: Dataset; + timeWindow: number; comparisonType: AlertRuleComparisonType; onComparisonTypeChange: (value: AlertRuleComparisonType) => void; onComparisonDeltaChange: (value: number) => void; + onTimeWindowChange: (value: number) => void; comparisonDelta?: number; allowChangeEventTypes?: boolean; }; @@ -107,7 +109,7 @@ class RuleConditionsForm extends React.PureComponent { } return Object.entries(options).map(([value, label]) => ({ - value, + value: parseInt(value, 10), label, })); } @@ -144,8 +146,10 @@ class RuleConditionsForm extends React.PureComponent { onFilterSearch, allowChangeEventTypes, alertType, + timeWindow, comparisonType, comparisonDelta, + onTimeWindowChange, onComparisonDeltaChange, onComparisonTypeChange, dataset, @@ -391,19 +395,20 @@ class RuleConditionsForm extends React.PureComponent { /> )} {timeWindowText && {timeWindowText}} - ({ + ...provided, + minWidth: 130, + maxWidth: 300, + }), }} options={this.timeWindowOptions} required isDisabled={disabled} - getValue={value => Number(value)} - setValue={value => `${value}`} + value={timeWindow} + onChange={({value}) => onTimeWindowChange(value)} inline={false} flexibleControlStateSize /> @@ -411,15 +416,21 @@ class RuleConditionsForm extends React.PureComponent { {comparisonType === AlertRuleComparisonType.CHANGE && ( {t(' compared to ')} - ({ + ...provided, + marginLeft: space(1), + }), + control: (provided: {[x: string]: string | number | boolean}) => ({ + ...provided, + minWidth: 500, + maxWidth: 1000, + }), }} value={comparisonDelta} - onChange={onComparisonDeltaChange} + onChange={({value}) => onComparisonDeltaChange(value)} options={COMPARISON_DELTA_OPTIONS} required={comparisonType === AlertRuleComparisonType.CHANGE} /> diff --git a/static/app/views/alerts/incidentRules/ruleForm/index.tsx b/static/app/views/alerts/incidentRules/ruleForm/index.tsx index 4f1f56734ac38e..85e12ad6db8558 100644 --- a/static/app/views/alerts/incidentRules/ruleForm/index.tsx +++ b/static/app/views/alerts/incidentRules/ruleForm/index.tsx @@ -385,7 +385,14 @@ class RuleFormContainer extends AsyncComponent { handleFieldChange = (name: string, value: unknown) => { if ( - ['dataset', 'eventTypes', 'timeWindow', 'environment', 'aggregate'].includes(name) + [ + 'dataset', + 'eventTypes', + 'timeWindow', + 'environment', + 'aggregate', + 'comparisonDelta', + ].includes(name) ) { this.setState({[name]: value}); } @@ -435,7 +442,8 @@ class RuleFormContainer extends AsyncComponent { const {organization, params, rule, onSubmitSuccess, location, sessionId} = this.props; const {ruleId} = this.props.params; - const {resolveThreshold, triggers, thresholdType, comparisonDelta, uuid} = this.state; + const {resolveThreshold, triggers, thresholdType, comparisonDelta, uuid, timeWindow} = + this.state; // Remove empty warning trigger const sanitizedTriggers = triggers.filter( @@ -473,6 +481,7 @@ class RuleFormContainer extends AsyncComponent { resolveThreshold: isEmpty(resolveThreshold) ? null : resolveThreshold, thresholdType, comparisonDelta, + timeWindow, }, { referrer: location?.query?.referrer, @@ -562,8 +571,11 @@ class RuleFormContainer extends AsyncComponent { handleComparisonTypeChange = (value: AlertRuleComparisonType) => { const comparisonDelta = - value === AlertRuleComparisonType.COUNT ? undefined : this.state.comparisonDelta; - this.setState({comparisonType: value, comparisonDelta}); + value === AlertRuleComparisonType.COUNT + ? undefined + : this.state.comparisonDelta ?? 10080; + const timeWindow = this.state.comparisonDelta ? this.state.timeWindow : 60; + this.setState({comparisonType: value, comparisonDelta, timeWindow}); }; handleComparisonDeltaChange = (value: number) => { @@ -741,10 +753,14 @@ class RuleFormContainer extends AsyncComponent { allowChangeEventTypes={isCustomMetric || dataset === Dataset.ERRORS} alertType={isCustomMetric ? 'custom' : alertType} dataset={dataset} + timeWindow={timeWindow} comparisonType={comparisonType} comparisonDelta={comparisonDelta} onComparisonTypeChange={this.handleComparisonTypeChange} - onComparisonDeltaChange={this.handleComparisonDeltaChange} + onComparisonDeltaChange={value => + this.handleFieldChange('comparisonDelta', value) + } + onTimeWindowChange={value => this.handleFieldChange('timeWindow', value)} /> {t('Set thresholds to trigger alert')} {triggerForm(hasAccess)} diff --git a/static/app/views/alerts/issueRuleEditor/constants/changeAlerts.tsx b/static/app/views/alerts/issueRuleEditor/constants/changeAlerts.tsx index c5cd7ef80d3ff5..6c71c994dbf932 100644 --- a/static/app/views/alerts/issueRuleEditor/constants/changeAlerts.tsx +++ b/static/app/views/alerts/issueRuleEditor/constants/changeAlerts.tsx @@ -6,7 +6,7 @@ export const CHANGE_ALERT_CONDITION_IDS = [ export const CHANGE_ALERT_PLACEHOLDERS_LABELS = { 'sentry.rules.conditions.event_frequency.EventFrequencyCondition': - 'Number of errors in an issue is...', + 'Number of events in an issue is...', 'sentry.rules.conditions.event_frequency.EventUniqueUserFrequencyCondition': 'Number of users affected by an issue is...', 'sentry.rules.conditions.event_frequency.EventFrequencyPercentCondition': diff --git a/static/app/views/alerts/issueRuleEditor/index.tsx b/static/app/views/alerts/issueRuleEditor/index.tsx index 1525e38346cd81..2f408e3b904339 100644 --- a/static/app/views/alerts/issueRuleEditor/index.tsx +++ b/static/app/views/alerts/issueRuleEditor/index.tsx @@ -382,13 +382,21 @@ class IssueRuleEditor extends AsyncView { getInitialValue = (type: ConditionOrActionProperty, id: string) => { const configuration = this.state.configs?.[type]?.find(c => c.id === id); + + const hasChangeAlerts = + configuration?.id && + this.props.organization.features.includes('change-alerts') && + CHANGE_ALERT_CONDITION_IDS.includes(configuration.id); + return configuration?.formFields ? Object.fromEntries( Object.entries(configuration.formFields) // TODO(ts): Doesn't work if I cast formField as IssueAlertRuleFormField .map(([key, formField]: [string, any]) => [ key, - formField?.initial ?? formField?.choices?.[0]?.[0], + hasChangeAlerts && key === 'interval' + ? '1h' + : formField?.initial ?? formField?.choices?.[0]?.[0], ]) .filter(([, initial]) => !!initial) ) diff --git a/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx b/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx index 6d9dbe6cda8599..8a9b6daa39c828 100644 --- a/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx +++ b/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx @@ -55,7 +55,7 @@ class RuleNodeList extends React.Component { | IssueAlertRuleConditionTemplate | null | undefined => { - const {nodes, items, organization} = this.props; + const {nodes, items, organization, onPropertyChange} = this.props; const node = nodes ? nodes.find(n => n.id === id) : null; if (!node) { @@ -95,6 +95,13 @@ class RuleNodeList extends React.Component { }; if (item.comparisonType === 'percent') { + if (!item.comparisonInterval) { + // comparisonInterval value in IssueRuleEditor state + // is undefined even if initial value is defined + // can't directly call onPropertyChange, because + // getNode is called during render + setTimeout(() => onPropertyChange(itemIdx, 'comparisonInterval', '1w')); + } changeAlertNode = { ...changeAlertNode, formFields: { @@ -102,8 +109,7 @@ class RuleNodeList extends React.Component { comparisonInterval: { type: 'choice', choices: COMPARISON_INTERVAL_CHOICES, - // comparisonInterval initial value isn't on the item, so it needs to be selected by user - initial: 'select', + initial: '1w', }, }, };