[Security Solution][Detections] Rule forms cleanup #76138

rylnd · 2020-08-27T18:58:10Z

Summary

In preparation for new rule types/fields, and in response to improvements to the form library, this adds types to the majority of our form code.

Besides the bugfixes,the main behavioral change here is that it is no longer possible to navigate between invalid tabs on the Rule Edit page, and you must instead make a tab valid before moving away. This is how the Create page currently behaves, and so behavior (and implementation) are now consolidated between the two pages. edit: I've reverted this behavior but changed the invalid callout to display on navigation between tabs (previously it was only shown when you clicked Save. Once we have UX for displaying validation errors to the user on both Create and Edit, we can persist invalid data in a consistent manner.

"Features":

adds types our form state/helpers
- (we had a lot of implicit anys and ass in there)
adds regression assertions to an existing "create rule" cypress test (7fa8bca)
Removes intermediate state in our form components
- Summary: the parent holds the data to be persisted, while the form lib holds the data as the user interacts with the form; most of the local state was there to work around issues with the form lib (e.g. memoization, which is now fixed)
- We persist form hooks (RuleStepsFormHooks) in the parent so that we can validate and store the active step's data
- We persist VALID form data (RuleStepsFormData) in the parent in order to repopulate a form that has previously unmounted
  - If a form is unmounted or a form's field is unmounted, the form hook loses knowledge of the form/field
Removes uses of FormDataProvider
- These were used to react to form changes, and typically used local state to track the old value to compare.
- In (almost) all of our current uses, we only care about current and default values, never the "previous value," and so we were able to reimplement the same functionality with useFormData and useEffect.

"Bugfixes":

Fixes moving away from the Actions step (b174270)
Fixes bug moving between steps of the Edit form (4995e81)
- Consequence here is that if a user navigates away from an invalid step, their modifications will be lost
Fixes "link" between default severity and default risk score, where changing the former updated the latter (0972e16)
Removes some unnecessary ML API calls (aac72c9)

Unaddressed issues/notes

With the severity/risk score link back in place, there's now an interesting UI oddity: the default severity is low, but the default risk score is 50. However, the default mapped risk score for low is 21. This might be fine, I just wanted to bring it up as a potential discrepancy.
I believe that there may be a validation bug in the form library. Update: fixed with [Form lib] Fix regression on field not being validated after reset to its default value. #76379
1. Have a text field with an emptyField validation (e.g. the Rule Name field)
2. leave the field blank, submit the form, observe the validation error
3. enter text to remove the validation error
4. delete text
5. submit the form and see that a "" has been persisted to that field
  Update: should be fixed with [Form lib] Fix regression on field not being validated after reset to its default value. #76379

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Digging through the git history, it looks like this was replaced with the isUpdateView prop at some point. There's a small chance that we're indirectly leveraging the effect of this value being changed, but I think we're safe.

We have lots of anys and unknowns in here, this is my attempt to fix that. I started by defining better types for the state/refs in our parent component; everything else mostly flowed out of that: * Step components now type their form hook for their step's data * Removes lots of unneeded `as` casts * Renames uses of `accordionId` with `step` and `activeStep`, since they are also values of our `RuleStep` enum * Step components now export their default values * The data flow is simpler when the parent passes these values in, rather than trying to merge props with some internal defaults. * The internal defaulting is still there, but I think it'll be unnecessary once I audit the `edit` forms. I've only done this work for the Define step for now, the rest are next.

Now that the create step is passing in the default values, we no longer need to merge with internal state. The one exception is the default indexes; since we need that data for our "reset to default indexes" behavior, we'll keep that functionality within our DefineStep component.

We don't gain much by forcing the parent to pass in default values. The slightly cleaner types are not worth the burden to the parent; instead, we add a type guard to be used in our parent to ensure our values are present before working with them. Previously, we were circumventing this logic with an `as` cast.

These are arrays of strings, so a shallow comparison should suffice here. Also reorders conditions to short-circuit on simple booleans first.

There's currently a bug on master where returning to a previous form step does not populate its previous values. After some investigation it appears that this is due to form values being reset on submission (form.reset()). Previously, we kept a separate copy of data in each step's state, and had a useEffect that would repopulate the form's values if they ever became out of sync. Once that was removed I believe this bug was introduced. For now the fix is effectively reimplementing the above behavior, albeit a little more elegantly with `reset()`. If we can restructure the form logic to only require the form data at the end (big if), then we can remove the need to "repopulate" these values to the form. For now, this does remove the local copy of data in the step component as I believe that is no longer needed. Data is stored in the parent, copied/modified to the form, and pushed back up when one clicks on to the next step.

The linter was complaining because it didn't think that `typedUseKibana` was a hook. But it is, and we should name it as such.

Things still aren't quite working, state gets lost when moving through steps but I believe this is addressed in an outstanding PR so I'm not sweating it right now. * Removes as much state in Step components as possible * We shouldn't need this as the form holds all the state as well. If we need to "watch" for a change, we can subscribe to the form's observable to replace FormDataProvider and local state (TODO) * Removes setting of default values in form components * I believe that this is redundant with defaultValues provided to useForm, but I need to verify.

* Removes redundant uses of field's defaultValue * Most are redundant with the form's defaultValue; added calls to field.setValue in cases where the default is generated internally * Removes calls to reset() after submitting * These were needed due to a bug in the form lib; once elastic#75796 is merged these will no longer be necessary.

This exists identically earlier in the component; I'm guessing it was the result of a bad merge conflict.

* Makes data structures more similar to rule creation form * Adds type guards for asserting which step is "active" * Simplifies logic around the active tabe/step/form

* Removes use of wait() in favor of act() * Fixes mock call assertion now that we're no longer setting our data to null (which was a now-unnecessary form lib workaround).

We never send our actions data back down to the actions form, so it was lost if you went to a previous step. Since the actions UI still had any connectors you created, you merely had to reselect the throttle and the connector, but this prevents you from having to do that.

Asserts that our rule form repopulates with the provided values when going back to a previous step. This is to cover a regression that was not caught by CI (but which has now been fixed).

* Validation and data collection are performed in the parent, not the step component * Step component provides a form ref and notifies the parent when it's being submitted; the rest is the parent's responsibility * Renames some internal helper functions to be more declarative: submitStep, editStep, etc.

If the active step form is invalid we will receive no data, so we must not persist that lest the form blows up on absent values when we later navigate back.

These exercise functionality that was moved into the parent, so they need a new home.

rylnd · 2020-09-01T05:48:48Z

@patrykkopycinski @sebelga I'm seeing some odd behavior, described in the first "Unaddressed issues/notes" above. I haven't yet determined whether it's a bug with the form lib or merely with our forms, but it's present on both this branch and master. If either of you has a moment to 👀 that issue and provide some guidance it would be much appreciated.

sebelga · 2020-09-01T08:51:53Z

@rylnd Having a look...

x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx

sebelga · 2020-09-01T09:35:01Z

@rylnd I saw what it is. I introduced a regression in my last PR and will revert it asap! Sorry for this and thanks for raising it 👍

elasticmachine · 2020-09-03T17:04:13Z

Pinging @elastic/siem (Team:SIEM)

* Form hooks now _always_ return the form's data, regardless of validity * Edit Rule now: * persists invalid data * submits both the active form and the destination form on navigation. This is necessary to refresh validations on the destination form, since the form lib assumes a newly-mounted form is valid * simplifies "invalid tab" logic to be derived from our persisted data

If the rule is immutable, they can only edit actions.

spong · 2020-09-04T17:29:08Z

No need to address here (so much goodness already! :), but wanted to comment on the standardization of the disabled/loading state of fields. Looks like we're closer than before, but there are still a few fields that don't disable when loading/saving. This may be particularly hard to get the Actions component to match since we're pulling in the alerting component full sail, but we should be able to get the others to match since we own those components. In the screenshots below, the arrows point to the fields that don't disable on load/save:

Definition

About

Schedule (As above, probably not much we can do here unless the alerting component supports the loading/disabled state)

Actions

spong · 2020-09-04T17:41:07Z

x-pack/plugins/security_solution/public/detections/components/rules/next_step/index.tsx

@@ -9,7 +9,7 @@ import { EuiHorizontalRule, EuiFlexGroup, EuiFlexItem, EuiButton } from '@elasti
 import * as RuleI18n from '../../../pages/detection_engine/rules/translations';

 interface NextStepProps {
- onClick: () => Promise<void>;
+ onClick: () => void;


Nice catch!

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/data.tsx

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/index.tsx

Conflicts: x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/create/helpers.ts x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/create/index.tsx x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.tsx

Fixed by upstream elastic#76471

spong · 2020-09-04T18:13:18Z

x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx

+ const [{ throttle: formThrottle }] = (useFormData({
+ form,
+ watch: ['throttle'],
+ }) as unknown) as [Partial<ActionsStepRule>];


These always get a double-take from me when I see them, but understand with the removal of FormDataProvider.

Yeah, I've discussed this with @sebelga and I'm hopeful that we can get a generic type for our use case (where form data F is identical to output data T.

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/helpers.tsx

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/types.ts

Since the default severity is 'low,' these two defaults now coincide.

spong

Checked out, tested locally, and performed code review. So much goodness here @rylnd, fantastic work with the cleanup effort! 🙂 Transforming that tech debt into a tech investment with all the type and control flow enhancements you packed in here. I've left a few nits (mostly about starting to use our common types if we can), but nothing worth holding this PR up. LGTM! 👍 🚀 🎉

rylnd · 2020-09-04T19:29:40Z

Good catch on the SeverityValue redundancy. Fixed in b24742c.

kibanamachine · 2020-09-04T21:17:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b24742c

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
securitySolution	1951	+1	1950

async chunks size

id	value	diff	baseline
securitySolution	9.9MB	+1.7KB	9.9MB

History

💚 Build #72740 succeeded e45c128
💚 Build #72718 succeeded 7d359ed
💚 Build #72404 succeeded db36f40
💚 Build #72355 succeeded 33cdeef
💚 Build #72121 succeeded 36ba03f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Remove unused isNew field Digging through the git history, it looks like this was replaced with the isUpdateView prop at some point. There's a small chance that we're indirectly leveraging the effect of this value being changed, but I think we're safe. * WIP: Making rule form type safe We have lots of anys and unknowns in here, this is my attempt to fix that. I started by defining better types for the state/refs in our parent component; everything else mostly flowed out of that: * Step components now type their form hook for their step's data * Removes lots of unneeded `as` casts * Renames uses of `accordionId` with `step` and `activeStep`, since they are also values of our `RuleStep` enum * Step components now export their default values * The data flow is simpler when the parent passes these values in, rather than trying to merge props with some internal defaults. * The internal defaulting is still there, but I think it'll be unnecessary once I audit the `edit` forms. I've only done this work for the Define step for now, the rest are next. * Make defaultValues a required prop of the define step Now that the create step is passing in the default values, we no longer need to merge with internal state. The one exception is the default indexes; since we need that data for our "reset to default indexes" behavior, we'll keep that functionality within our DefineStep component. * Refactor rule creation forms to not require default values We don't gain much by forcing the parent to pass in default values. The slightly cleaner types are not worth the burden to the parent; instead, we add a type guard to be used in our parent to ensure our values are present before working with them. Previously, we were circumventing this logic with an `as` cast. * Remove unnecessary "deep" comparison These are arrays of strings, so a shallow comparison should suffice here. Also reorders conditions to short-circuit on simple booleans first. * Make StepRuleDescription generic on its schema * Fixes bug introduced by form lib updates There's currently a bug on master where returning to a previous form step does not populate its previous values. After some investigation it appears that this is due to form values being reset on submission (form.reset()). Previously, we kept a separate copy of data in each step's state, and had a useEffect that would repopulate the form's values if they ever became out of sync. Once that was removed I believe this bug was introduced. For now the fix is effectively reimplementing the above behavior, albeit a little more elegantly with `reset()`. If we can restructure the form logic to only require the form data at the end (big if), then we can remove the need to "repopulate" these values to the form. For now, this does remove the local copy of data in the step component as I believe that is no longer needed. Data is stored in the parent, copied/modified to the form, and pushed back up when one clicks on to the next step. * Rename typed hook to obviate eslint exception The linter was complaining because it didn't think that `typedUseKibana` was a hook. But it is, and we should name it as such. * WIP: Fixing type errors in the other form steps Things still aren't quite working, state gets lost when moving through steps but I believe this is addressed in an outstanding PR so I'm not sweating it right now. * Removes as much state in Step components as possible * We shouldn't need this as the form holds all the state as well. If we need to "watch" for a change, we can subscribe to the form's observable to replace FormDataProvider and local state (TODO) * Removes setting of default values in form components * I believe that this is redundant with defaultValues provided to useForm, but I need to verify. * More form cleanup * Removes redundant uses of field's defaultValue * Most are redundant with the form's defaultValue; added calls to field.setValue in cases where the default is generated internally * Removes calls to reset() after submitting * These were needed due to a bug in the form lib; once elastic#75796 is merged these will no longer be necessary. * Fix some leftover type errors * Remove duplicated useEffect hook This exists identically earlier in the component; I'm guessing it was the result of a bad merge conflict. * Fix Rule edit form * Makes data structures more similar to rule creation form * Adds type guards for asserting which step is "active" * Simplifies logic around the active tabe/step/form * Fixes About Step jest tests * Removes use of wait() in favor of act() * Fixes mock call assertion now that we're no longer setting our data to null (which was a now-unnecessary form lib workaround). * Fix bug with going to a previous step after editing actions We never send our actions data back down to the actions form, so it was lost if you went to a previous step. Since the actions UI still had any connectors you created, you merely had to reselect the throttle and the connector, but this prevents you from having to do that. * Add assertions to our rule creation test Asserts that our rule form repopulates with the provided values when going back to a previous step. This is to cover a regression that was not caught by CI (but which has now been fixed). * Simplify Rule Creation logic * Validation and data collection are performed in the parent, not the step component * Step component provides a form ref and notifies the parent when it's being submitted; the rest is the parent's responsibility * Renames some internal helper functions to be more declarative: submitStep, editStep, etc. * Don't persist empty form data when leaving a form step If the active step form is invalid we will receive no data, so we must not persist that lest the form blows up on absent values when we later navigate back. * Skip About Step tests for now These exercise functionality that was moved into the parent, so they need a new home. * Remove unnecessary calls to setValue * Instead of setting our kibana url after the form is created, we add it to the form's default state * We do not need to set the throttle field value, the field component already does this * Style: logic cleanup * Prevent users from navigating away from an invalid step on rule edit We can go against the form lib conventions and persist this invalid data ourselves on transition, but for now this brings the create/edit forms into alignment so that adding the aforementioned behavior should be nearly identical on both. * Display callout if attempting to navigate away from an invalid tab We already do this if you try to _submit_ the form, but not when you click on another tab. * Persist our form submit() rather than the entire form Making the entire stateful form a useEffect dependency was likely causing unnecessary render cycles. This may also have been part of why both the hooks and the data are refs instead of normal state; to prevent these rerenders. * Replace FormDataProvider with useFormData hook We have to do a type cast here because the hook's data is not typed, but other than that this is a solid win and cleans things up immensely; the side effects that result from these values changing are much more apparent (IMO). * Move fetch of fields data _after_ form initialization This ensures that our first fetch of fields will use the index patterns on the form, not necessarily the default ones. * Replace FormDataProvider on About step * This fixes a bug where changing the default severity no longer updated the default risk score. It looks like this was broken when the severity/riskScore overrides were added, and the values of these fields changed from primitives to objects. * Replace local state with useFormData By watching the value directly from the form we no longer have a need for local state, as we were just using it to determine whether the throttle had changed from the default. * Types cleanup Remove some unneeded casts, add some needed ones. * Rewrite About Step tests Rather than asserting that the form is invalid through the UI, we instead retrieve data out of the form hook and assert on that instead. * Add memoization back to StepRuleDescription I'm not sure that it's necessary, but best to leave it until we have time to audit/remove multiple of these. * Do not fetch ML Jobs if StepRuleDescription is not rendering ML Data We were incorrectly invoking the useSecurityJobs hook any time the StepRuleDescription component was rendered, regardless of whether we actually needed that data. This moves the useSecurityJobs hook into the component that uses it, MlJobDescription. If we end up having multiple of these on the page we can evaluate caching/sharing this data somehow, but for now this prevents: * 3x3=9 unnecessary ML calls on the Rule Create page. * 1x3=3 unnecessary ML calls on Rule Details * 1x3=3 unnecessary ML Calls on the Rule Edit page. * Fix bug where revisiting the About step could modify the user's Risk Score With the severity/risk score link back in place, there was a bug where a user who had previously set a manual risk score would have it rewritten on edit (or edit during rule creation). This was due to a poorly-written useEffect that basically said "if there is a severity, set a risk score." This has now been amended to say "if the user changes the severity, set a risk score." * Clean up About Step tests * We don't need act(), it's not doing anything. * We don't need to click Continue since we're just talking to the form hook * Fix local form data when form isn't mounted If the form isn't on the page (e.g. if we're read-only), then useFormData will return no values. In these cases, we can simply fall back to the initialState values, as they'll either be: the default values on a new form, or: the current values on an active create/edit form. Updates the manual type of useFormData to reflect this "maybe" fact. * Allow user to navigate between invalid tabs on Edit Rule * Form hooks now _always_ return the form's data, regardless of validity * Edit Rule now: * persists invalid data * submits both the active form and the destination form on navigation. This is necessary to refresh validations on the destination form, since the form lib assumes a newly-mounted form is valid * simplifies "invalid tab" logic to be derived from our persisted data * Fix logical error If the rule is immutable, they can only edit actions. * Remove unneeded eslint exception Fixed by upstream elastic#76471 * Make 21 the default risk score for a new rule Since the default severity is 'low,' these two defaults now coincide. * Remove duplicated type in favor of common one

* Remove unused isNew field Digging through the git history, it looks like this was replaced with the isUpdateView prop at some point. There's a small chance that we're indirectly leveraging the effect of this value being changed, but I think we're safe. * WIP: Making rule form type safe We have lots of anys and unknowns in here, this is my attempt to fix that. I started by defining better types for the state/refs in our parent component; everything else mostly flowed out of that: * Step components now type their form hook for their step's data * Removes lots of unneeded `as` casts * Renames uses of `accordionId` with `step` and `activeStep`, since they are also values of our `RuleStep` enum * Step components now export their default values * The data flow is simpler when the parent passes these values in, rather than trying to merge props with some internal defaults. * The internal defaulting is still there, but I think it'll be unnecessary once I audit the `edit` forms. I've only done this work for the Define step for now, the rest are next. * Make defaultValues a required prop of the define step Now that the create step is passing in the default values, we no longer need to merge with internal state. The one exception is the default indexes; since we need that data for our "reset to default indexes" behavior, we'll keep that functionality within our DefineStep component. * Refactor rule creation forms to not require default values We don't gain much by forcing the parent to pass in default values. The slightly cleaner types are not worth the burden to the parent; instead, we add a type guard to be used in our parent to ensure our values are present before working with them. Previously, we were circumventing this logic with an `as` cast. * Remove unnecessary "deep" comparison These are arrays of strings, so a shallow comparison should suffice here. Also reorders conditions to short-circuit on simple booleans first. * Make StepRuleDescription generic on its schema * Fixes bug introduced by form lib updates There's currently a bug on master where returning to a previous form step does not populate its previous values. After some investigation it appears that this is due to form values being reset on submission (form.reset()). Previously, we kept a separate copy of data in each step's state, and had a useEffect that would repopulate the form's values if they ever became out of sync. Once that was removed I believe this bug was introduced. For now the fix is effectively reimplementing the above behavior, albeit a little more elegantly with `reset()`. If we can restructure the form logic to only require the form data at the end (big if), then we can remove the need to "repopulate" these values to the form. For now, this does remove the local copy of data in the step component as I believe that is no longer needed. Data is stored in the parent, copied/modified to the form, and pushed back up when one clicks on to the next step. * Rename typed hook to obviate eslint exception The linter was complaining because it didn't think that `typedUseKibana` was a hook. But it is, and we should name it as such. * WIP: Fixing type errors in the other form steps Things still aren't quite working, state gets lost when moving through steps but I believe this is addressed in an outstanding PR so I'm not sweating it right now. * Removes as much state in Step components as possible * We shouldn't need this as the form holds all the state as well. If we need to "watch" for a change, we can subscribe to the form's observable to replace FormDataProvider and local state (TODO) * Removes setting of default values in form components * I believe that this is redundant with defaultValues provided to useForm, but I need to verify. * More form cleanup * Removes redundant uses of field's defaultValue * Most are redundant with the form's defaultValue; added calls to field.setValue in cases where the default is generated internally * Removes calls to reset() after submitting * These were needed due to a bug in the form lib; once #75796 is merged these will no longer be necessary. * Fix some leftover type errors * Remove duplicated useEffect hook This exists identically earlier in the component; I'm guessing it was the result of a bad merge conflict. * Fix Rule edit form * Makes data structures more similar to rule creation form * Adds type guards for asserting which step is "active" * Simplifies logic around the active tabe/step/form * Fixes About Step jest tests * Removes use of wait() in favor of act() * Fixes mock call assertion now that we're no longer setting our data to null (which was a now-unnecessary form lib workaround). * Fix bug with going to a previous step after editing actions We never send our actions data back down to the actions form, so it was lost if you went to a previous step. Since the actions UI still had any connectors you created, you merely had to reselect the throttle and the connector, but this prevents you from having to do that. * Add assertions to our rule creation test Asserts that our rule form repopulates with the provided values when going back to a previous step. This is to cover a regression that was not caught by CI (but which has now been fixed). * Simplify Rule Creation logic * Validation and data collection are performed in the parent, not the step component * Step component provides a form ref and notifies the parent when it's being submitted; the rest is the parent's responsibility * Renames some internal helper functions to be more declarative: submitStep, editStep, etc. * Don't persist empty form data when leaving a form step If the active step form is invalid we will receive no data, so we must not persist that lest the form blows up on absent values when we later navigate back. * Skip About Step tests for now These exercise functionality that was moved into the parent, so they need a new home. * Remove unnecessary calls to setValue * Instead of setting our kibana url after the form is created, we add it to the form's default state * We do not need to set the throttle field value, the field component already does this * Style: logic cleanup * Prevent users from navigating away from an invalid step on rule edit We can go against the form lib conventions and persist this invalid data ourselves on transition, but for now this brings the create/edit forms into alignment so that adding the aforementioned behavior should be nearly identical on both. * Display callout if attempting to navigate away from an invalid tab We already do this if you try to _submit_ the form, but not when you click on another tab. * Persist our form submit() rather than the entire form Making the entire stateful form a useEffect dependency was likely causing unnecessary render cycles. This may also have been part of why both the hooks and the data are refs instead of normal state; to prevent these rerenders. * Replace FormDataProvider with useFormData hook We have to do a type cast here because the hook's data is not typed, but other than that this is a solid win and cleans things up immensely; the side effects that result from these values changing are much more apparent (IMO). * Move fetch of fields data _after_ form initialization This ensures that our first fetch of fields will use the index patterns on the form, not necessarily the default ones. * Replace FormDataProvider on About step * This fixes a bug where changing the default severity no longer updated the default risk score. It looks like this was broken when the severity/riskScore overrides were added, and the values of these fields changed from primitives to objects. * Replace local state with useFormData By watching the value directly from the form we no longer have a need for local state, as we were just using it to determine whether the throttle had changed from the default. * Types cleanup Remove some unneeded casts, add some needed ones. * Rewrite About Step tests Rather than asserting that the form is invalid through the UI, we instead retrieve data out of the form hook and assert on that instead. * Add memoization back to StepRuleDescription I'm not sure that it's necessary, but best to leave it until we have time to audit/remove multiple of these. * Do not fetch ML Jobs if StepRuleDescription is not rendering ML Data We were incorrectly invoking the useSecurityJobs hook any time the StepRuleDescription component was rendered, regardless of whether we actually needed that data. This moves the useSecurityJobs hook into the component that uses it, MlJobDescription. If we end up having multiple of these on the page we can evaluate caching/sharing this data somehow, but for now this prevents: * 3x3=9 unnecessary ML calls on the Rule Create page. * 1x3=3 unnecessary ML calls on Rule Details * 1x3=3 unnecessary ML Calls on the Rule Edit page. * Fix bug where revisiting the About step could modify the user's Risk Score With the severity/risk score link back in place, there was a bug where a user who had previously set a manual risk score would have it rewritten on edit (or edit during rule creation). This was due to a poorly-written useEffect that basically said "if there is a severity, set a risk score." This has now been amended to say "if the user changes the severity, set a risk score." * Clean up About Step tests * We don't need act(), it's not doing anything. * We don't need to click Continue since we're just talking to the form hook * Fix local form data when form isn't mounted If the form isn't on the page (e.g. if we're read-only), then useFormData will return no values. In these cases, we can simply fall back to the initialState values, as they'll either be: the default values on a new form, or: the current values on an active create/edit form. Updates the manual type of useFormData to reflect this "maybe" fact. * Allow user to navigate between invalid tabs on Edit Rule * Form hooks now _always_ return the form's data, regardless of validity * Edit Rule now: * persists invalid data * submits both the active form and the destination form on navigation. This is necessary to refresh validations on the destination form, since the form lib assumes a newly-mounted form is valid * simplifies "invalid tab" logic to be derived from our persisted data * Fix logical error If the rule is immutable, they can only edit actions. * Remove unneeded eslint exception Fixed by upstream #76471 * Make 21 the default risk score for a new rule Since the default severity is 'low,' these two defaults now coincide. * Remove duplicated type in favor of common one

* master: [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751) Remove legacy deprecation adapter (elastic#76753) [Security Solution][Detections] Rule forms cleanup (elastic#76138) Adds back in custom images for reporting + tests (elastic#76810) [APM] Fix overlapping transaction names (elastic#76083) [Ingest Pipelines] Add descriptions for ingest processors A-D (elastic#75975)

elasticmachine · 2021-09-23T14:26:22Z

Pinging @elastic/security-solution (Team: SecuritySolution)

rylnd added 14 commits August 27, 2020 12:29

Remove unused isNew field

c5c1675

Digging through the git history, it looks like this was replaced with the isUpdateView prop at some point. There's a small chance that we're indirectly leveraging the effect of this value being changed, but I think we're safe.

Remove unnecessary "deep" comparison

7c3a807

These are arrays of strings, so a shallow comparison should suffice here. Also reorders conditions to short-circuit on simple booleans first.

Make StepRuleDescription generic on its schema

2b66a8f

Rename typed hook to obviate eslint exception

fdf2cf4

The linter was complaining because it didn't think that `typedUseKibana` was a hook. But it is, and we should name it as such.

Fix some leftover type errors

8bc8141

Remove duplicated useEffect hook

a61744e

This exists identically earlier in the component; I'm guessing it was the result of a bad merge conflict.

Fix Rule edit form

d80998a

* Makes data structures more similar to rule creation form * Adds type guards for asserting which step is "active" * Simplifies logic around the active tabe/step/form

Fixes About Step jest tests

ad8aa60

* Removes use of wait() in favor of act() * Fixes mock call assertion now that we're no longer setting our data to null (which was a now-unnecessary form lib workaround).

rylnd added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 27, 2020

rylnd self-assigned this Aug 27, 2020

rylnd added 5 commits August 27, 2020 17:45

Add assertions to our rule creation test

7fa8bca

Asserts that our rule form repopulates with the provided values when going back to a previous step. This is to cover a regression that was not caught by CI (but which has now been fixed).

Don't persist empty form data when leaving a form step

4995e81

If the active step form is invalid we will receive no data, so we must not persist that lest the form blows up on absent values when we later navigate back.

Skip About Step tests for now

453ef00

These exercise functionality that was moved into the parent, so they need a new home.

sebelga reviewed Sep 1, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx Outdated Show resolved Hide resolved

sebelga reviewed Sep 1, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx Outdated Show resolved Hide resolved

sebelga mentioned this pull request Sep 1, 2020

[Form lib] Fix regression on field not being validated after reset to its default value. #76379

Merged

Merge branch 'master' into rule_form_cleanup_and_prep

33cdeef

rylnd marked this pull request as ready for review September 3, 2020 17:04

rylnd requested review from a team as code owners September 3, 2020 17:04

rylnd added 2 commits September 3, 2020 14:14

Fix logical error

db36f40

If the rule is immutable, they can only edit actions.

spong reviewed Sep 4, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/data.tsx Outdated Show resolved Hide resolved

spong reviewed Sep 4, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/index.tsx Show resolved Hide resolved

rylnd added 2 commits September 4, 2020 13:08

Remove unneeded eslint exception

7d359ed

Fixed by upstream elastic#76471

spong reviewed Sep 4, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/helpers.tsx Outdated Show resolved Hide resolved

spong reviewed Sep 4, 2020

View reviewed changes

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/types.ts Outdated Show resolved Hide resolved

Make 21 the default risk score for a new rule

e45c128

Since the default severity is 'low,' these two defaults now coincide.

spong approved these changes Sep 4, 2020

View reviewed changes

Remove duplicated type in favor of common one

b24742c

rylnd merged commit 1b9030f into elastic:master Sep 4, 2020

rylnd deleted the rule_form_cleanup_and_prep branch September 4, 2020 21:18

rylnd mentioned this pull request Sep 4, 2020

[7.x] [Security Solution][Detections] Rule forms cleanup (#76138) #76827

Merged

rylnd mentioned this pull request Sep 9, 2020

[Security Solution][Detections] Integration test for Editing a Rule #77090

Merged

2 tasks

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Rule forms cleanup #76138

[Security Solution][Detections] Rule forms cleanup #76138

rylnd commented Aug 27, 2020 •

edited

Loading

rylnd commented Sep 1, 2020

sebelga commented Sep 1, 2020

sebelga commented Sep 1, 2020

elasticmachine commented Sep 3, 2020

spong commented Sep 4, 2020 •

edited

Loading

spong Sep 4, 2020

spong Sep 4, 2020

rylnd Sep 4, 2020

spong left a comment

rylnd commented Sep 4, 2020

kibanamachine commented Sep 4, 2020

elasticmachine commented Sep 23, 2021

[Security Solution][Detections] Rule forms cleanup #76138

[Security Solution][Detections] Rule forms cleanup #76138

Conversation

rylnd commented Aug 27, 2020 • edited Loading

Summary

"Features":

"Bugfixes":

Unaddressed issues/notes

Checklist

For maintainers

rylnd commented Sep 1, 2020

sebelga commented Sep 1, 2020

sebelga commented Sep 1, 2020

elasticmachine commented Sep 3, 2020

spong commented Sep 4, 2020 • edited Loading

spong Sep 4, 2020

Choose a reason for hiding this comment

spong Sep 4, 2020

Choose a reason for hiding this comment

rylnd Sep 4, 2020

Choose a reason for hiding this comment

spong left a comment

Choose a reason for hiding this comment

rylnd commented Sep 4, 2020

kibanamachine commented Sep 4, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

History

elasticmachine commented Sep 23, 2021

rylnd commented Aug 27, 2020 •

edited

Loading

spong commented Sep 4, 2020 •

edited

Loading