-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Endpoint][EventFilters] Port Event Filters to use ArtifactListPage
component
#130995
[Security Solution][Endpoint][EventFilters] Port Event Filters to use ArtifactListPage
component
#130995
Conversation
ca3b209
to
8a83f20
Compare
ArtifactListPage
componentArtifactListPage
component
6577bff
to
e5bc6e8
Compare
2800a4c
to
a1a6b21
Compare
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.
this approval is for functionality upon checking out the PR and running it. Will leave the code comments to the other devs
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.
This looks awesome! A lot of new awesome code and very good cleaning stuff! Thanks for doing all of this. I let you some comments/questions/suggestions, let me know what do you think 🙂
}); | ||
}); | ||
|
||
describe('Policy section with downgraded license', () => { |
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.
Should we move these tests to the effect scope ones here?
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.
Good idea. Although, there are a similar set of tests also in TA form. I think let's leave it here for now and refactor the tests when we extract common form elements into one component.
!!exception.entries.length || false | ||
); | ||
// compute this for initial render only | ||
const existingComments = useMemo<ExceptionListItemSchema['comments']>( |
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 about having this as a default value of the exception? For example, having the eventFilter
as state instead of being a useMemo
, so you can define the default value when creating it with the existing comments:
const [eventFilter, setEventFilter] = useState(exception);
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 tried that earlier when I got started with the form but that doesn't work . The AddExceptionComments
expects the exception item comments to also have updated_by
, created_by
keys that the form added comments do not have.
Thus, I went with this approach where we pick out the saved comments once on the initial render and do not update it until the event filter is created with new comments.
x-pack/plugins/security_solution/public/management/pages/event_filters/view/components/form.tsx
Outdated
Show resolved
Hide resolved
); | ||
}, [hasNameError, exception.entries]); | ||
|
||
const processChanged = useCallback( |
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.
this function name got me confused, why processChanged
? Can we change it by something with event
or exception
?
Also, if you decide to have the exception in a state, you could just setException({...changes})
and then have a useEffect
to call onChange
every time exception
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.
I see. I went with this particular name cause I want to process
/compute any change
s that happened with the exception. This is also named notifyOfChange
in other forms I think. We can agree on a single name and keep it consistent in all the forms.
x-pack/plugins/security_solution/public/management/pages/event_filters/view/components/form.tsx
Outdated
Show resolved
Hide resolved
...c/management/pages/policy/view/artifacts/delete_modal/policy_artifacts_delete_modal.test.tsx
Outdated
Show resolved
Hide resolved
review changes
…of github.com:ashokaditya/kibana into task/olm-3093-move-event-filters-to-artifactsListPage
review changes
…-ref HEAD~1..HEAD --fix'
…of github.com:ashokaditya/kibana into task/olm-3093-move-event-filters-to-artifactsListPage
review changes
review changes
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.
Investigations code owner file changes. LGTM!
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.
Awesome! Thanks for all the effort here 🚢
Would you mind adding a FTR here for event filters? It can be done in another pr if you want 🙂
4dd3828
to
6717ae9
Compare
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @ashokaditya |
Summary
Refactors endpoint event filters page to use
ArtifactListPage
Checklist
Delete any items that are not applicable to this PR.