Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Security Solution][Detections] Rule forms cleanup (#76138)
* 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- Loading branch information
