[Security Solution] [Endpoint] Ability to assign existing event filter to the policy flyout #121964

dasansol92 · 2021-12-23T15:07:26Z

Summary

For maintainers

This was checked for breaking API changes and was labeled appropriately

…empty state message

…lter_to_the_policy_flyout-2029

dasansol92 · 2021-12-23T15:08:01Z

x-pack/plugins/security_solution/public/management/pages/event_filters/store/middleware.ts

-      'updated_at',
-      'updated_by',
-    ].forEach((field) => {
-      delete updatedCommentsEntry[field as keyof UpdateExceptionListItemSchema];


Moved from the middleware to the service as the service is used in other places (with react-query)

dasansol92 · 2021-12-23T15:08:31Z

...ublic/management/pages/policy/view/artifacts/assignable/policy_artifacts_assignable_list.tsx

+    return (
+      <>
+        {isListLoading && (
+          <EuiProgress size="xs" color="primary" data-test-subj="artifactsAssignableListLoader" />


Added a progress bar like we have in the other search list areas

dasansol92 · 2021-12-23T15:10:48Z

x-pack/plugins/security_solution/public/management/pages/policy/view/event_filters/hooks.ts

+  );
+}
+
+export function useBulkUpdateEventFilters(


Generic bulk update action. Using this, we will be able to use the bulk API once is done without changing the components logic, only this hook

elasticmachine · 2021-12-23T15:11:40Z

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

academo · 2021-12-27T10:34:12Z

@elasticmachine merge upstream

…lter_to_the_policy_flyout-2029

paul-tavares · 2021-12-27T14:19:23Z

x-pack/plugins/security_solution/public/management/pages/event_filters/service/index.ts

+      'updated_at',
+      'updated_by',
+    ].forEach((field) => {
+      delete exception[field as keyof UpdateExceptionListItemSchema];


Can we change the implementation so that we don't mutate the object that was passed on input to this method. I think that maybe this might work:

async updateOne( { created_at, created_by, created_at, creatd_by, list_id, tie_breaker_id, updated_at, updated_by, ...exception }: Immutable<UpdateExceptionListItemSchema> ): Promise<ExceptionListItemSchema> {

paul-tavares · 2021-12-27T14:26:49Z

...ion/public/management/pages/policy/view/event_filters/flyout/policy_event_filters_flyout.tsx

+    const [currentFilter, setCurrentFilter] = useState<string>('');
+
+    const bulkUpdateMutation = useBulkUpdateEventFilters({
+      onUpdateSuccess: (updatedExceptions: ExceptionListItemSchema[]) => {


I have not worked with react query, so I'm not sure about this - but should these all be memoized? are they triggering unnecessary react render cycles by having new instances created everytime?

No, internally react-query is already taking care of this.

…lter_to_the_policy_flyout-2029

…_policy_flyout-2029' of github.com:dasansol92/kibana into feat/olm-ability_to_assign_existing_event_filter_to_the_policy_flyout-2029

kibana-ci · 2022-01-04T11:49:40Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2834	2836	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	4.6MB	4.6MB	+6.2KB

History

💔 Build #15592 failed ddf4d36
💚 Build #15084 succeeded fb13b97
💚 Build #15013 succeeded 776ed8e

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

paul-tavares

left some comments and am good if you need to merge this in order to make progress and address them in a subsequent PR.

paul-tavares · 2022-01-04T16:29:07Z

...ublic/management/pages/policy/view/event_filters/flyout/policy_event_filters_flyout.test.tsx

+    ...exception,
+    tags: [...testTags, `policy:${policy.id}`],
+  };
+  // Clean unnecessary fields for update action


is this the same code as the service file? if so, maybe create a utility that does this (TA has a similar one I think) and reuse it.

paul-tavares · 2022-01-04T16:30:55Z

...ublic/management/pages/policy/view/event_filters/flyout/policy_event_filters_flyout.test.tsx

+  UpdateExceptionListItemSchema,
+} from '@kbn/securitysolution-io-ts-list-types';
+
+const endpointGenerator = new EndpointDocGenerator('seed');


Just FYI: This is fine being defined as a singleton, but note that if a new test case is inserted between others, you could get test failures because the data generated will then differ for the tests cases that are present after the one inserted.

My suggestion would be to initialize it for every test via beforeEach()

paul-tavares · 2022-01-04T16:33:14Z

...ublic/management/pages/policy/view/event_filters/flyout/policy_event_filters_flyout.test.tsx

+    sort_order: undefined,
+  },
+});
+const emptyList = {


can we avoid using mutable constants in the tests - can you convert this to a function that returns the object?

paul-tavares · 2022-01-04T16:41:06Z

...ublic/management/pages/policy/view/event_filters/flyout/policy_event_filters_flyout.test.tsx

+        renderResult = mockedContext.render(
+          <PolicyEventFiltersFlyout policyItem={policy} onClose={onCloseMock} />
+        );
+        await waitFor(mockedApi.responseProvider.eventFiltersList);


I'm not sure this is doing what you expect it to be done. What is the intent - to wait for the data to be returned? If so, then you need to check for when it is actually called. maybe something like this:

await waitFor( () => expect(mockedApi.responseProvider.eventFiltersList).toHaveBeenCalled() );

yeah. exactly. It was what I was looking for, thanks!

dasansol92 · 2022-01-04T16:52:11Z

@paul-tavares Thanks for the suggestions, I will address it in a subsequent PR since I need some of the code here to move on with other issues

dasansol92 added 5 commits December 22, 2021 17:10

Displays assignable artifact list and allow selection. Also displays …

67e63f7

…empty state message

Adds mutation to update event filters with new tags

5e6479a

Shows confirm toast when action has ben done correctly

d512fe7

Merge branch 'main' into feat/olm-ability_to_assign_existing_event_fi…

3e8c441

…lter_to_the_policy_flyout-2029

Fix ui/ux loading states and added unit test for flyout

3fa7fdf

dasansol92 added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.1.0 labels Dec 23, 2021

dasansol92 commented Dec 23, 2021

View reviewed changes

Remove customqueryId

776ed8e

dasansol92 commented Dec 23, 2021

View reviewed changes

dasansol92 marked this pull request as ready for review December 23, 2021 15:11

dasansol92 requested a review from a team as a code owner December 23, 2021 15:11

dasansol92 requested review from academo and paul-tavares December 23, 2021 15:11

academo approved these changes Dec 24, 2021

View reviewed changes

Merge branch 'main' into feat/olm-ability_to_assign_existing_event_fi…

fb13b97

…lter_to_the_policy_flyout-2029

paul-tavares reviewed Dec 27, 2021

View reviewed changes

dasansol92 added 3 commits January 4, 2022 09:36

Merge branch 'main' into feat/olm-ability_to_assign_existing_event_fi…

a42ba1d

…lter_to_the_policy_flyout-2029

Merge branch 'feat/olm-ability_to_assign_existing_event_filter_to_the…

0e2613b

…_policy_flyout-2029' of github.com:dasansol92/kibana into feat/olm-ability_to_assign_existing_event_filter_to_the_policy_flyout-2029

Use new object reference befor update it

ddf4d36

dasansol92 requested a review from paul-tavares January 4, 2022 08:57

Fixes unit test

6a23f6d

paul-tavares approved these changes Jan 4, 2022

View reviewed changes

dasansol92 merged commit 14b8e12 into elastic:main Jan 4, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jan 4, 2022

dasansol92 mentioned this pull request Jan 5, 2022

[Security Solution] [Endpoint] Allows user remove event filter from policy using a modal #122315

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] [Endpoint] Ability to assign existing event filter to the policy flyout #121964

[Security Solution] [Endpoint] Ability to assign existing event filter to the policy flyout #121964

dasansol92 commented Dec 23, 2021

dasansol92 Dec 23, 2021

dasansol92 Dec 23, 2021

dasansol92 Dec 23, 2021

academo Dec 24, 2021

elasticmachine commented Dec 23, 2021

academo commented Dec 27, 2021

paul-tavares Dec 27, 2021

paul-tavares Dec 27, 2021

academo Dec 31, 2021

kibana-ci commented Jan 4, 2022

paul-tavares left a comment

paul-tavares Jan 4, 2022

paul-tavares Jan 4, 2022

paul-tavares Jan 4, 2022

paul-tavares Jan 4, 2022

dasansol92 Jan 4, 2022

dasansol92 commented Jan 4, 2022

[Security Solution] [Endpoint] Ability to assign existing event filter to the policy flyout #121964

[Security Solution] [Endpoint] Ability to assign existing event filter to the policy flyout #121964

Conversation

dasansol92 commented Dec 23, 2021

Summary

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 23, 2021

academo commented Dec 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jan 4, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasansol92 commented Jan 4, 2022