Feat: Enable activated alert creation via alert rule form #68959

nhsiehgit · 2024-04-16T04:48:56Z

Modifies the AlertRule form to add options for activated rule creation

Apr-15-2024.21-50-31.mp4

NOTE: GET api's are still filtering out all activated alert rules so this details page is not rendering anything yet

saponifi3d · 2024-04-16T14:56:50Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+    );
+  }
+
+  renderMonitorTypeSelect() {


rather than following the render* pattern here, could we just create a new functional component instead?

😢 was considering it...

but rather than having 2 render patterns, I figured we should keep it consistent.

hopefully we'll eventually rip this all out anyways

saponifi3d · 2024-04-16T14:57:22Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+              }
+            >
+              <strong>Continuous</strong>
+              <div>Continuously monitor trends for the metrics outlined below</div>


nit: might be easier to add t functions now vs later.

saponifi3d · 2024-04-16T14:58:17Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+                  Monitor{' '}
+                  <SelectControl
+                    name="activationCondition"
+                    styles={{


I'd recommend pulling these styles out and creating a styled SelectControl instead of inlining them here.

Ah

So this is a symptom of us creating our wild wacky components :(
In order to attach styles to the children elements, there's no easy way to do so other than utilizing SelectControls' dumb way of styling 💢

saponifi3d · 2024-04-16T14:58:31Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+                        minWidth: 200,
+                        maxWidth: 300,


:( I may opt to ignore these nits 😬

for now, not trying to make things good, just make them work

I don't see any use case for reusability here :/

saponifi3d · 2024-04-16T14:59:52Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+                      {
+                        value: ActivationConditionType.RELEASE_CREATION,
+                        label: t('New Release'),
+                      },
+                      {
+                        value: ActivationConditionType.DEPLOY_CREATION,
+                        label: t('New Deploy'),
+                      },


nit: move these to constants

saponifi3d · 2024-04-16T15:07:15Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+      onTimeWindowChange,
+      project,
+      monitorType,
+    } = this.props;


we're using a lot of props here - i wonder if we could use hooks or reduce the prop drilling 🤔

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

saponifi3d · 2024-04-16T15:14:47Z

static/app/views/alerts/rules/metric/ruleForm.tsx

@@ -1108,6 +1182,9 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
              onTimeWindowChange={value => this.handleFieldChange('timeWindow', value)}
              disableProjectSelector={disableProjectSelector}
              isExtrapolatedChartData={isExtrapolatedChartData}
+              monitorType={monitorType}
+              activationCondition={activationCondition}
+              onMonitorTypeSelect={this.handleMonitorTypeSelect}


unrelated to this change

we should make a tech debt item to completely refactor this form... 1200 lines and there's a lot of improvements we could do to this, from a technical standpoint alone.

Oh yea absolutely.

I hate everything about this form :|

saponifi3d · 2024-04-16T15:16:21Z

static/app/views/alerts/rules/metric/ruleForm.tsx

+    _onSubmitSuccess,
+    _onSubmitError,


should we optionally invoke these?

saponifi3d · 2024-04-16T15:17:18Z

static/app/views/alerts/rules/metric/ruleForm.tsx

+  };
+
+  handleSubmit = async (
+    _data: Partial<MetricRule>,


is this because the ruleForm isn't using the Form class to maintain state?

I have no idea why :/

I think this only changed because lint rules or something 🤷

saponifi3d

can we include some tests to ensure we don't have any side effects with the new states? (and then maybe some around the new props?)

codecov · 2024-04-17T22:43:36Z

Bundle Report

Changes will increase total bundle size by 3.97kB ⬆️

Bundle name	Size	Change
app-webpack-bundle-array-push	26.17MB	3.97kB ⬆️

saponifi3d

If possible, it'd be great to get some tests included here. feel free to treat the comments as options (i found a few other places where t functions might be handy and added them as diffs for 1 click adds 😎)

saponifi3d · 2024-04-17T23:16:13Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+                })
+              }
+            >
+              <strong>Continuous</strong>


nit:

Suggested change

<strong>Continuous</strong>

<strong>{t('Continuous')</strong>

saponifi3d · 2024-04-17T23:17:12Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+              <strong>Conditional</strong>
+              {monitorType === MonitorType.ACTIVATED ? (
+                <ActivatedAlertFields>
+                  Monitor{' '}


nit:

Suggested change

Monitor{' '}

{`${t('Monitor')} '}

saponifi3d · 2024-04-17T23:18:01Z

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

+                    inline={false}
+                    flexibleControlStateSize
+                  />{' '}
+                  for{' '}


Suggested change

for{' '}

{`${t('for')} `}

saponifi3d · 2024-04-17T23:22:12Z

static/app/types/alerts.tsx

+  RELEASE_CREATION = 0,
+  DEPLOY_CREATION = 1,


Do the ints map to the database storage level? if so 👍 if not, maybe we could use strings to define these so 0 won't evaluate as falsy.

yessir they do indeed

nhsiehgit · 2024-04-18T03:44:04Z

Yea - i agree with the general sentiment around testing.

For this change in particular, i've already included a test to assert the form is able to submit with an activation condition.

Otherwise i'm not sure what else you'd like to write a test for unless you're looking to rebuild and re-test every component here unfortunately.
These components in particular are just generally gross and lack adequate testing, and i don't think it's worth the effort to push for those changes here.

We can hash out a separate ticket to clean this up if you feel strongly about it

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 16, 2024

vercel bot deployed to Preview April 16, 2024 04:50 View deployment

nhsiehgit requested a review from saponifi3d April 16, 2024 04:51

saponifi3d reviewed Apr 16, 2024

View reviewed changes

nhsiehgit force-pushed the activated_alerts_ui branch from 3d3a911 to 83ebe8c Compare April 16, 2024 21:25

vercel bot deployed to Preview April 16, 2024 21:27 View deployment

saponifi3d reviewed Apr 17, 2024

View reviewed changes

nhsiehgit added 2 commits April 17, 2024 14:48

enable activated alert creation via alert rule form

64490d9

nit changes

54cf7ca

nhsiehgit marked this pull request as ready for review April 17, 2024 22:36

nhsiehgit requested review from a team as code owners April 17, 2024 22:36

nhsiehgit force-pushed the activated_alerts_ui branch from 83ebe8c to 54cf7ca Compare April 17, 2024 22:36

vercel bot deployed to Preview April 17, 2024 22:40 View deployment

add test for activated alert save

0759946

vercel bot deployed to Preview April 17, 2024 23:19 View deployment

saponifi3d approved these changes Apr 17, 2024

View reviewed changes

nits

59775b1

vercel bot deployed to Preview April 18, 2024 03:40 View deployment

nhsiehgit merged commit 6d88cff into master Apr 18, 2024
41 checks passed

nhsiehgit deleted the activated_alerts_ui branch April 18, 2024 15:09

github-actions bot locked and limited conversation to collaborators May 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Enable activated alert creation via alert rule form #68959

Feat: Enable activated alert creation via alert rule form #68959

nhsiehgit commented Apr 16, 2024 •

edited

Loading

saponifi3d Apr 16, 2024

nhsiehgit Apr 16, 2024

saponifi3d Apr 16, 2024

saponifi3d Apr 16, 2024

nhsiehgit Apr 16, 2024

saponifi3d Apr 16, 2024

nhsiehgit Apr 16, 2024

saponifi3d Apr 16, 2024

saponifi3d Apr 16, 2024

saponifi3d Apr 16, 2024 •

edited

Loading

nhsiehgit Apr 16, 2024

saponifi3d Apr 16, 2024

saponifi3d Apr 16, 2024

nhsiehgit Apr 16, 2024

saponifi3d left a comment •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

saponifi3d left a comment

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

saponifi3d Apr 17, 2024

nhsiehgit Apr 18, 2024

nhsiehgit commented Apr 18, 2024

	<strong>Continuous</strong>
	<strong>{t('Continuous')</strong>

Feat: Enable activated alert creation via alert rule form #68959

Feat: Enable activated alert creation via alert rule form #68959

Conversation

nhsiehgit commented Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 17, 2024 • edited Loading

Bundle Report

saponifi3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhsiehgit commented Apr 18, 2024

nhsiehgit commented Apr 16, 2024 •

edited

Loading

saponifi3d Apr 16, 2024 •

edited

Loading

saponifi3d left a comment •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading