[Security Solution][Trusted Apps] New ArtifactEntryCard and refactor of Trusted Apps list to use it
#111051
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nents + new ActionsContextMenu component + add context menu to card
…mponents and refactoring
paul-tavares
added
release_note:enhancement
v8.0.0
Team:Defend Workflows
|
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
paul-tavares
commented
Sep 8, 2021
| `; | ||
|
|
||
| export interface ArtifactEntryCardProps extends CommonProps { | ||
| item: AnyArtifact; |
There was a problem hiding this comment.
Although this component accepts either an ExceptionListItem (also used by Event Filters) or Trusted app item, the entire component still needs some work to be able to truly support exceptions (ex. criteria conditions and per-policy defintions)
paul-tavares
commented
Sep 8, 2021
| } | ||
| `; | ||
|
|
||
| export interface ArtifactEntryCardProps extends CommonProps { |
There was a problem hiding this comment.
FYI: when you set the data-test-subj that value is used through out the sub-components as the prefix 🎉
paul-tavares
commented
Sep 8, 2021
| dataTestSubj: 'policyDetailsBackLink', | ||
| }} | ||
| /> | ||
| <BackToExternalAppButton {...backLinkOptions} data-test-subj="policyDetailsBackLink" /> |
There was a problem hiding this comment.
HeaderLinkBack (prior component) does not really play well with our route paths that are generated out of the routing.ts module because it does support a pageId of the top-level security app, so I replaced it with the one we had in our common components.
dasansol92
reviewed
Sep 9, 2021
There was a problem hiding this comment.
Left few comments but it looks awesome! This new artifacts card component and his subcomponents will allow us to do a bunch of amazing things!! 🔥
As a suggestion, I think this pr could be split in smaller ones, it's a bit hard to review a 46 files pr (I'm the first one doing this kind of big pr's...). Maybe a first pr with the artifacts card and subcomponents, then the changes and refactors and then the last one with the change in the trusted apps grid to use the new cards. Thoughts? 😄
| </EuiFlexItem> | ||
| <EuiFlexItem className="eui-textTruncate" data-test-subj={getTestId('value')}> | ||
| <TextValueDisplay bold> | ||
| <FormattedRelativePreferenceDate value={date} dateFormat="M/D/YYYY" /> |
There was a problem hiding this comment.
I'm wondering about this. Should we use date location instead of enforce this format?
There was a problem hiding this comment.
I think I took a note on this to talk to Bonnie about it. The format (I believe) does get adjusted for countries where the day comes before the month. Its just that in the UX design mocks she does not want the long date + time to be displayed - only the short date.
| updated_at, | ||
| updated_by, | ||
| description, | ||
| entries: (entries as unknown) as ArtifactInfo['entries'], |
There was a problem hiding this comment.
Do we need to do something special for nested conditions? I know this is not needed on this iteration, just curious
There was a problem hiding this comment.
not yet. that is part of the work that will be needed once we work on adding this to Event Filters (and possibly exceptions).
| useEffect(() => { | ||
| if (pageCount > 0 && pageCount < (pagination?.pageIndex || 0) + 1) { | ||
| if (!loading && pageCount > 0 && pageCount < (pagination?.pageIndex || 0) + 1) { |
| @@ -226,6 +231,18 @@ export const listOfPolicies: ( | |||
| return isLoadedResourceState(policies) ? policies.data.items : []; | |||
| }); | |||
|
|
|||
| export const mapOfPoliciesById: ( | |||
There was a problem hiding this comment.
Maybe starting this selector with get -> getMapOfPoliciesById
| >['onChange'] = useTrustedAppsNavigateCallback(({ pageIndex, pageSize }) => ({ | ||
| page_index: pageIndex, | ||
| page_size: pageSize, | ||
| })); | ||
|
|
||
| const handleArtifactCardProps = useMemo(() => { |
There was a problem hiding this comment.
If this is going to return a function that needs to be memoized, should it be useCallback instead of useMemo?
There was a problem hiding this comment.
If I use useCallback then I will not be able to keep private state - below, cachedCardProps is a private scooped object that the memoized function uses. That was the only reason I used useMemo()
|
@dasansol92 yeah, sorry about the big PR. I should have kept it smaller and created (as you suggest) smaller PRs for the refactoring. It might take me a bit of time to split this up now just because the card itself also uses some of the components that were refactored out of Endpoint list. Do you (others) rather have this be shrunken down? |
joeypoon
approved these changes
Sep 9, 2021
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Alerting API Integration Tests.x-pack/test/alerting_api_integration/spaces_only/tests/actions/enqueue·ts.alerting api integration spaces only Actions enqueue should never leaved a failed task, even if max attempts is reachedStandard OutStack TraceMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 9, 2021
…r of Trusted Apps list to use it (elastic#111051) * New `ArtifactEntryCard` component * Refactored ContextMenuItemNavByRouter and moved it to top-level components + new ActionsContextMenu component + add context menu to card * Refactor Trusted App grid to use new ArtifactEntryCard * new Trusted Apps generator + refactor existing of TA script to use it * policy details support for custom back link * bug fix: paginated content should not trigger a change to adjust paging settings unless loading is done
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
Sep 10, 2021
…r of Trusted Apps list to use it (#111051) (#111811) * New `ArtifactEntryCard` component * Refactored ContextMenuItemNavByRouter and moved it to top-level components + new ActionsContextMenu component + add context menu to card * Refactor Trusted App grid to use new ArtifactEntryCard * new Trusted Apps generator + refactor existing of TA script to use it * policy details support for custom back link * bug fix: paginated content should not trigger a change to adjust paging settings unless loading is done Co-authored-by: Paul Tavares <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ArtifactEntryCardcomponent for displaying artifacts (used only with Trusted Apps as of this PR)setupFleetForEndpoints()data loader to return the packages that were installedFormattedRelativePreferenceDatecommon component now allows for override of date formatActionsContextMenucomponent for displaying action menus behind an Icon button (lifted from Endpoint list along withContextMenuWithRouterSupport. Will refactor Endpoint list at a later time to use it)PaginatedContentthat was causing the pagination data to be adjusted while component was stillloadingChecklist