-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] User can add new event filter from event filter list #98118
[Security Solution] User can add new event filter from event filter list #98118
Conversation
…state for OS. Add created entry to the entries list
…e and path management
70a7c3d
to
dd7e246
Compare
@elasticmachine merge upstream |
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
6571bee
to
74f9c91
Compare
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
@@ -51,57 +51,66 @@ export interface AuthenticatedElasticUser { | |||
} | |||
|
|||
export const useCurrentUser = (): AuthenticatedElasticUser | null => { | |||
const isMounted = useRef(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 🚢
85b78d8
to
5b3e7e5
Compare
@elasticmachine merge upstream |
…from_event_filter_list-955
expected head sha didn’t match current head ref. |
…from_event_filter_list-955
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Left some comments, but they are not critical enought to hold merging this in.
x-pack/plugins/security_solution/public/management/pages/event_filters/state/index.ts
Show resolved
Hide resolved
...gins/security_solution/public/management/pages/event_filters/view/components/empty/index.tsx
Outdated
Show resolved
Hide resolved
import { EuiButton, EuiEmptyPrompt } from '@elastic/eui'; | ||
import { FormattedMessage } from '@kbn/i18n/react'; | ||
|
||
const EmptyPrompt = styled(EuiEmptyPrompt)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the default max-width is too short for the text inside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would not set the max-width but rather let the text wrap instead. Attempting to make the element wide enough to accommodate the english version of the text show is not a good approach because the text can be localized and thus the length of the message might change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally right!
...ecurity_solution/public/management/pages/event_filters/view/components/flyout/index.test.tsx
Show resolved
Hide resolved
...ins/security_solution/public/management/pages/event_filters/view/event_filters_list_page.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/management/pages/event_filters/view/hooks.ts
Outdated
Show resolved
Hide resolved
…lter_list-955' of github.com:dasansol92/kibana into feature/olm-user_can_add_new_event_filter_from_event_filter_list-955
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 🚢 🚢
Thanks for the changes. Looks good to merge in my opinion. I need this PR in order to make some progress with mine, since this one already puts in place some of the pieces I need
@@ -118,6 +119,26 @@ const normalizeTrustedAppsPageLocation = ( | |||
} | |||
}; | |||
|
|||
const normalizeEventFiltersPageLocation = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 I wonder if we can create a generic utility that could work for any use case in parsing/normalizing url params, so that we did not have to keep cloning this code. maybe a utility that takes in the defaults
++ the actual (aka: location
) value and returns back the location type of information. Likely would be a TS generic
function so that it can work across use cases.
…ist (elastic#98118) * New flyout with event filters form * Changes on event filters form to allow OS selector. Add new error on state for OS. Add created entry to the entries list * Fixes typo * Adds empty page with an add button that opens flyout. Alos added route and path management * Fixes type and adds a TODO comment. Also removes ESlit rule for useCallback deps * Fixes unit test. Adds consts for default page size and page index * Fixes warning state update on an unmounted component * Fixes infinite useEffect loop useFetchIndex hook because non memoized value * Adds policy:all to eventFilter.tag and disables or button on exception builder * Changes component name and simplify hook using useCallback without a custom callback Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…ist (#98118) (#98604) * New flyout with event filters form * Changes on event filters form to allow OS selector. Add new error on state for OS. Add created entry to the entries list * Fixes typo * Adds empty page with an add button that opens flyout. Alos added route and path management * Fixes type and adds a TODO comment. Also removes ESlit rule for useCallback deps * Fixes unit test. Adds consts for default page size and page index * Fixes warning state update on an unmounted component * Fixes infinite useEffect loop useFetchIndex hook because non memoized value * Adds policy:all to eventFilter.tag and disables or button on exception builder * Changes component name and simplify hook using useCallback without a custom callback Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…ist (elastic#98118) * New flyout with event filters form * Changes on event filters form to allow OS selector. Add new error on state for OS. Add created entry to the entries list * Fixes typo * Adds empty page with an add button that opens flyout. Alos added route and path management * Fixes type and adds a TODO comment. Also removes ESlit rule for useCallback deps * Fixes unit test. Adds consts for default page size and page index * Fixes warning state update on an unmounted component * Fixes infinite useEffect loop useFetchIndex hook because non memoized value * Adds policy:all to eventFilter.tag and disables or button on exception builder * Changes component name and simplify hook using useCallback without a custom callback Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
This includes a new flyout displayed on administration/event_filters tab when trying to add/update an event filter item.
Once an entry is created, it is added to the entries list (redux store).
This flyout allow user edit the OS type on event filter form.
It also includes unit tests.
Needs
eventFilteringEnabled
feature flag enabled.Checklist
Delete any items that are not applicable to this PR.
For maintainers