-
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] Creates generic policy tab artifact component to be used for all of our artifacts #126685
Conversation
…component_to_be_used_for_all_of_our_artifacts-3173
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.
some initial feedback. Did not get very far (stoped at policy_artifacts_layout.tsx
). will pick it up again tomorrow
x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/constants.ts
Show resolved
Hide resolved
...tion/public/management/pages/policy/view/artifacts/empty/use_policy_artifacts_empty_hooks.ts
Outdated
Show resolved
Hide resolved
...tion/public/management/pages/policy/view/artifacts/empty/use_policy_artifacts_empty_hooks.ts
Outdated
Show resolved
Hide resolved
…an artifact_layout unit test
…component_to_be_used_for_all_of_our_artifacts-3173
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
onSettled: () => { | ||
queryClient.invalidateQueries(['list', apiClient]); | ||
onCancel(); | ||
}, |
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.
Just want to make sure this code is correct. As I understand it, onSettled
is called everytime - regardless of success or error. So even if it errors, you are invalidating queries and you are calling onCancel()
(why call onCancel on a failure?).
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.
You are right, we should keep the query cache and don't close the modal if something has failed. Thanks!
policyName: string; | ||
apiClient: ExceptionsListApiClient; | ||
exception: ExceptionListItemSchema; | ||
onCancel: () => void; |
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.
Looking at the code below, I wonder if this should be named onClose
. It seems to be used in both success and failure cases and when user cancels the modal
const handleModalConfirm = useCallback(() => { | ||
const modifiedException = { | ||
...exception, | ||
tags: exception.tags.filter((tag) => tag !== `policy:${policyId}`), |
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.
can you use the const
for the by policy prefix instead of hardcoding policy:
}, | ||
onSettled: () => { | ||
queryClient.invalidateQueries(['list', apiClient]); | ||
onClose(); |
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.
same here
let mockedApi: ReturnType<typeof eventFiltersListQueryHttpMock>; | ||
let history: AppContextTestRender['history']; | ||
|
||
const eventFiltersLabels = { |
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.
can you convert this to a function that returns the object, so that we avoid sharing state between tests
[navigateCallback] | ||
); | ||
|
||
const handleOnExpandCollapse: ArtifactCardGridProps['onExpandCollapse'] = ({ |
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.
for this one and more below - should wrap them in useCallback()
}, [artifacts?.data.length, labels]); | ||
|
||
const artifactCardPolicies = useEndpointPoliciesToArtifactPolicies(policiesRequest.data?.items); | ||
const provideCardProps: ArtifactCardGridProps['cardComponentProps'] = (artifact) => { |
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 have a hook in my ArtifactListPage
that provides this. Wondering if we should promote it and just reuse 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.
Is this one useArtifactCardPropsProvider
? I think we should do a bit of refactor then to add the privilege checks conditions.
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, that one. Let's hold off on using them, since some refactoring is needed.
defaultMessage: 'Error while attempt to remove event filter', | ||
} | ||
), | ||
flyoutWarningCalloutMessage: i18n.translate( |
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.
Wondering if this one should be a function that accepts on input the max number of items to display - since you created a const. then use that in the message
getExceptionsListApiClient={() => TrustedAppsApiClient.getInstance(http)} | ||
searcheableFields={[...TRUSTED_APPS_SEARCHABLE_FIELDS]} |
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 these be memoized in order to avoid unnecessary re-renders?
(same comment below)
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 don't think so, this is from a static import. I'm destructuring it because it's a readonly type. I can just define it as readonly on the component itself.
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.
Ok. Just realize that because you are passing a new function everytime to getExceptionsListApiClient
and a new Array to searcheableFields
, the component will always trigger a re-render (props strict equality ( ===
) will report 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.
oh yes, we can use useCallback
for the getExceptionsListApiClient
function
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.
and maybe use instead of creating a new array and spreading the values, just cast the type?
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.
the searchableFields destructured array is fixed in this commit d99ba65
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.
About the getExceptionsListApiClient
, all of this code is inside a useMemo
, do you think it's necessary to have functions outside using useCallback
?
if (!ExceptionsListApiClient.instance.has(listId)) { | ||
if ( | ||
!ExceptionsListApiClient.instance.has(listId) || | ||
ExceptionsListApiClient.instance.get(listId)?.http !== http |
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 don't think this is working. http
is not public on the instance.
Maybe (instead of exposing http
on the class, we add a method - something like: isHttp(coreHtttp): boolean { return this.http === coreHttp}
Also, can you add a test for it.
deleteModalErrorMessage: i18n.translate( | ||
'xpack.securitySolution.endpoint.policy.artifacts.list.removeDialog.errorToastTitle', | ||
{ | ||
defaultMessage: 'Error while attempt to remove artifact', |
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 think this and all the other translations with attempt
should be attempting
…component_to_be_used_for_all_of_our_artifacts-3173
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.
Left a few comments, but this looks good to go from my perspective.
🚢 it
page: number; | ||
perPage: number; | ||
policies: string[]; | ||
filter?: string; |
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.
Sooooo... I changed this hook in my PR as well and:
- Moved the order of the input props around to: apiClient, options, searchableFields and then query options.
- made
searchableFields
optional -defaults to theDEFAULT_EXCEPTION_LIST_ITEM_SEARCHABLE_FIELDS
- made
options
be aPartial<>
instead of forcing the user to define every attribute. Looks like this in mine:
export function useListArtifact(
exceptionListApiClient: ExceptionsListApiClient,
options: Partial<{
filter: string;
page: number;
perPage: number;
policies: string[];
}> = {},
searchableFields: MaybeImmutable<string[]> = DEFAULT_EXCEPTION_LIST_ITEM_SEARCHABLE_FIELDS,
customQueryOptions: UseQueryOptions<FoundExceptionListItemSchema, HttpFetchError> = {}
): QueryObserverResult<FoundExceptionListItemSchema, HttpFetchError> {
const {
filter = '',
page = MANAGEMENT_DEFAULT_PAGE + 1,
perPage = MANAGEMENT_DEFAULT_PAGE_SIZE,
policies,
} = options;
I made these changes in order to keep the hook's primary responsibility (to get a list of data out) simple and down to a minimal set of input args needed to use it.
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.
done: 63f8f9a
filter?: string; | ||
page?: number; | ||
perPage?: number; | ||
policies?: string[]; |
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'm wondering if we should also support a string
for policies so that it is easier to just pass in the URL param value. that being said, once I work on a better URL param parser hook, this might be a non-issue as that "parser" would likely take care of it.
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.
At this hook level I think is better to have an array and not a string. This can avoid us having wrong strings wit wrong structures coming from url. Agree this can change once we have a policy hook for parsing
FoundExceptionListItemSchema, | ||
HttpFetchError | ||
> = DEFAULT_OPTIONS, | ||
customQueryIds: string[] = [] |
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.
You are introducing this just so that you can invalidate queries?
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.
Exactly!
if (!ExceptionsListApiClient.instance.has(listId)) { | ||
if ( | ||
!ExceptionsListApiClient.instance.has(listId) || | ||
!ExceptionsListApiClient.instance.get(listId)?.isHttp(http) |
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 don't think you need the ?
here now that you added the method.
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.
ExceptionsListApiClient.instance.get(listId)
can be undefined at this point
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.
Ohhh. I see now. TS is not smart enough to know that .has()
being true
will cause .get()
to return the instance.
…ctions inside a 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.
Looks good to 🚢 . I have a suggestion for file renames and some minor typos.
import { APP_UI_ID } from '../../../../../../../common/constants'; | ||
import { EventFiltersPageLocation } from '../../../../event_filters/types'; | ||
import { TrustedAppsListPageLocation } from '../../../../trusted_apps/state'; |
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.
For consistency, I would suggest we remove this folder state
and move the files out and rename them like the other artifacts.
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.
will do this in a subsequent pr if you don't mind 🙂
if (!artifacts) { | ||
return; | ||
} | ||
const artifactssToUpdate: ExceptionListItemSchema[] = []; |
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.
nitpick: typo
const artifactssToUpdate: ExceptionListItemSchema[] = []; | |
const artifactsToUpdate: ExceptionListItemSchema[] = []; |
return null; | ||
} | ||
|
||
// there are no artifacts assignable to this policy |
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.
🔥
); | ||
} | ||
|
||
// there are no results for the current search |
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.
🔥
interface PolicyArtifactsFlyoutProps { | ||
policyItem: ImmutableObject<PolicyData>; | ||
apiClient: ExceptionsListApiClient; | ||
searcheableFields: string[]; |
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.
nitpick: typo
searcheableFields: string[]; | |
searchableFields: string[]; |
isLoading: isLoadingArtifacts, | ||
isRefetching: isRefetchingArtifacts, |
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.
🔥
values: { policyName }, | ||
}), | ||
flyoutSearchPlaceholder: i18n.translate( | ||
'xpack.securitySolution.endpoint.policy.artifacts.layout.searh.label', |
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.
'xpack.securitySolution.endpoint.policy.artifacts.layout.searh.label', | |
'xpack.securitySolution.endpoint.policy.artifacts.layout.search.label', |
values: { policyName }, | ||
}), | ||
flyoutSearchPlaceholder: i18n.translate( | ||
'xpack.securitySolution.endpoint.policy.eventFilters.layout.searh.label', |
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.
'xpack.securitySolution.endpoint.policy.eventFilters.layout.searh.label', | |
'xpack.securitySolution.endpoint.policy.eventFilters.layout.search.label', |
}, | ||
customQueryOptions: UseQueryOptions<FoundExceptionListItemSchema, HttpFetchError> | ||
searcheableFields: MaybeImmutable<string[]> = DEFAULT_EXCEPTION_LIST_ITEM_SEARCHABLE_FIELDS, |
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.
nitpick: typo
searcheableFields: MaybeImmutable<string[]> = DEFAULT_EXCEPTION_LIST_ITEM_SEARCHABLE_FIELDS, | |
searchableFields: MaybeImmutable<string[]> = DEFAULT_EXCEPTION_LIST_ITEM_SEARCHABLE_FIELDS, |
@ashokaditya Thanks for all the suggestions and typo's. Everything is in this commit: c5e2545 |
@elasticmachine merge upstream |
…component_to_be_used_for_all_of_our_artifacts-3173
…o_be_used_for_all_of_our_artifacts-3173' of github.com:dasansol92/kibana into feat/olm-create_generic_policy_tab_artifact_component_to_be_used_for_all_of_our_artifacts-3173
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…move-pdf-generation-to-screenshotting * 'main' of github.com:elastic/kibana: (62 commits) [Lens] Drop partial buckets option (elastic#127153) chore(NA): remove unused translation xpack.ml.management.jobsSpacesList.objectNoun from fr-FR (elastic#127457) Add data to user details page (elastic#127019) [Fleet] Make upload and registry package info consistent (elastic#126915) [Reporting] Capture browser errors (elastic#127135) Initial readme commit with some stub articles (elastic#127420) skip flaky suite (elastic#121482) skip flaky suite (elastic#127416) Tests to ensure Kibana is handling multi-space import of saved objects correctly (elastic#127229) [Aggs] remove toAngularJson (elastic#127267) [i18n] Integrate 8.2.0 Translations (elastic#127309) [Security Solution] [Endpoint] Creates generic policy tab artifact component to be used for all of our artifacts (elastic#126685) [Kibana React] Fix Page Template `solutionNav` propagation (elastic#127140) [Cases] Export getRelatedCases API from cases client (elastic#127065) [Cloud Posture]add support for sorting benchmark page (elastic#126983) [User experience] Fix filters for the app (elastic#127295) [Fleet] Fix timeserie dimension mapping (elastic#127328) [data view mgmt] fix data view name wrap (elastic#127319) [kbn/optimizer] extract string diffing logic (elastic#127394) [ResponseOps] Add pagination and sorting to the alerts search strategy (elastic#126813) ... # Conflicts: # x-pack/plugins/screenshotting/common/errors.ts # x-pack/plugins/screenshotting/common/index.ts # x-pack/plugins/screenshotting/server/screenshots/observable.ts
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
It includes a generic component that renders policy artifact tab depending on artifact type. This component will allow us add features easily and faster across all tabs and add new artifact types.
Usage within
PolicyTabs
component:For maintainers