Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Exceptions] - Updates autocomplete validation for numbers and boolean #74561

Merged
merged 16 commits into from
Oct 7, 2020

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Aug 6, 2020

Summary

This PR chips ⛏️ a bit at some stricter value validations that have been discussed. Further validation is needed, but this adds some more basic validation.

  • Current: if selected field is of type boolean users can add custom values in combo box
    • Now: if selected field is of type boolean users can only select true or false
  • Current: if selected field is of type number (that is kibana type number) user can input any values
    • Now: if selected field is of type number and no autocomplete suggestions are available, number input is used to restrict users
  • Current: for operator match_any it's conducting an autocomplete search after each selection resulting in some jumpy/weird behavior
    • Now: only conducts autocomplete search on initial field selection and if user enters value to search
  • Current: only validations on type date
    • Now: validation on type (Kibana type) date, number
  • Current: input would show red when there was an error but user could still submit
    • Now: submit button is disabled if error exists

TODO

  • can continue improving validation, this was just adding some basics

Boolean

boolean

Number w/ autocomplete values available

number_values_available

Number w/o autocomplete values available

number_no_values

Match any jitter

Before

match_any_jitter

After

match_any_jitter_fix

Validation (disabling submit)

validation

Checklist

Delete any items that are not applicable to this PR.

@yctercero yctercero marked this pull request as ready for review August 11, 2020 20:24
@yctercero yctercero requested review from a team as code owners August 11, 2020 20:24
@yctercero yctercero self-assigned this Aug 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@yctercero
Copy link
Contributor Author

@elasticmachine merge upstream

@spong
Copy link
Member

spong commented Aug 14, 2020

Looks like when inputting an invalid value after a valid value has been selected the valid value persists and is what is submitted. Perhaps this invalid input should clear the field's value?

Empty -> Valid -> Invalid

Empty -> Invalid

@spong
Copy link
Member

spong commented Aug 14, 2020

Just a bounds nit (so no need to fix here), but seems the field is truncated and displayed in scientific notation and the number picker no longer functions. This doesn't appear to cause any issues though, as modifying & re-saving the exception keeps the original value.

Also note, number validation doesn't take effect for is one of or is not one of operators. I don't think we can get the number picker to work with the multi-select, but looks like we can at least limit to numbers as per the ComboBox EUI docs (Custom options only, with validation).

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked out locally and desk tested, and performed code review. Great cleanup here @yctercero, and even more tests to boot! 🎉 Left a couple nits and notes about some behaviors I was seeing (address if you'd like), but this LJLTM (Looks Just Lovely To Me :)! 👍

@yctercero yctercero force-pushed the exception_value_validation branch from a442647 to d31743f Compare August 19, 2020 18:29
@yctercero
Copy link
Contributor Author

@spong Thanks so much for the 👀 . I went back and fixed the various things you noted. I made the input for boolean types into a select so now user can't even type anything in there. I also added validation for numbers when operator is one of. Lastly, updated to show errors below inputs and disable submit button if any errors exist. Let me know what you think!

@spong
Copy link
Member

spong commented Aug 19, 2020

Seeing an issue where switching fields (string->boolean) when validation is failed results in newly selected values from validatin:

@spong
Copy link
Member

spong commented Aug 19, 2020

We're in super-nit territory at this point so feel free to ignore.... 😅

When changing focus the validation error will bounce between empty and type errors. I suppose my expectation would be for it stay not a valid number, but understand that the value isn't set (to fix that other issue above :), so definitely not a big deal. There's always UI/UX cornercases floating around validation, but you've covered all the main cases and gotchas, so we're already in a really good spot with all the improvements you've made in this PR -- many thanks @yctercero!! 🙂

I could see this going either way, but when a validation error is displayed it increases the height of the exception entry and will move the trash icon down to keep it vertically centered. May want to keep vertically centered with the value input field
(unless this is necessary to support smaller viewports).

@@ -110,8 +110,11 @@ export const EditExceptionModal = memo(function EditExceptionModal({
>([]);
const { addError, addSuccess } = useAppToasts();
const { loading: isSignalIndexLoading, signalIndexName } = useSignalIndex();
const memoSignalIndexName = useMemo(() => (signalIndexName !== null ? [signalIndexName] : []), [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is working for me, good find 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
securitySolution 10.5MB 10.5MB +20.4KB

History

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

@yctercero yctercero merged commit 30695fb into elastic:master Oct 7, 2020
yctercero added a commit to yctercero/kibana that referenced this pull request Oct 7, 2020
… numbers and boolean (elastic#74561)

## Summary

This PR chips  a bit at some stricter value validations that have been discussed. Further validation is needed, but this adds some more basic validation.

- **Current:** if selected field is of type `boolean` users can add custom values in combo box
  - **Now:** if selected field is of type `boolean` users can only select `true` or `false`
- **Current:** if selected field is of type `number` (that is kibana type number) user can input any values
  - **Now:** if selected field is of type `number` and no autocomplete suggestions are available, number input is used to restrict users
- **Current:** for operator `match_any` it's conducting an autocomplete search after each selection resulting in some jumpy/weird behavior
  - **Now:** only conducts autocomplete search on initial field selection and if user enters value to search
- **Current:** only validations on type date
  - **Now:** validation on type (Kibana type) date, number
- **Current:** input would show red when there was an error but user could still submit
  - **Now:** submit button is disabled if error exists
yctercero added a commit that referenced this pull request Oct 7, 2020
… numbers and boolean (#74561) (#79808)

## Summary

This PR chips  a bit at some stricter value validations that have been discussed. Further validation is needed, but this adds some more basic validation.

- **Current:** if selected field is of type `boolean` users can add custom values in combo box
  - **Now:** if selected field is of type `boolean` users can only select `true` or `false`
- **Current:** if selected field is of type `number` (that is kibana type number) user can input any values
  - **Now:** if selected field is of type `number` and no autocomplete suggestions are available, number input is used to restrict users
- **Current:** for operator `match_any` it's conducting an autocomplete search after each selection resulting in some jumpy/weird behavior
  - **Now:** only conducts autocomplete search on initial field selection and if user enters value to search
- **Current:** only validations on type date
  - **Now:** validation on type (Kibana type) date, number
- **Current:** input would show red when there was an error but user could still submit
  - **Now:** submit button is disabled if error exists
@yctercero yctercero deleted the exception_value_validation branch October 14, 2020 11:59
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Security Solution rules and Detection Engine release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants