[Endpoint] Host Details Policy Response Panel #63518
Merged
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
paul-tavares
added
Feature:Endpoint
|
Pinging @elastic/endpoint-management (Team:Endpoint Management) |
|
Pinging @elastic/endpoint-app-team (Feature:Endpoint) |
paul-tavares
added
the
release_note:skip
oatkiller
reviewed
Apr 15, 2020
| data-test-subj="hostnameCellLink" | ||
| href={'?' + policyResponseUri.search} | ||
| onClick={(ev: React.MouseEvent) => { | ||
| ev.preventDefault(); |
There was a problem hiding this comment.
should this use the same logic as we do here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/endpoint/public/applications/endpoint/view/hooks/use_navigate_to_app_event_handler.ts#L42
There was a problem hiding this comment.
No - its different here. In this case, its not a navigate to app, but rather a direct router history update. @kevinlog is assigned to issue 230 which will (I assume) add an additional hook similar to the one you referenced above, but that will use react-router's history.push.
There was a problem hiding this comment.
@paul-tavares gotcha.
re: using the same logic, I meant should ev.preventDefault() here be replaced with:
try {
if (onClick) {
onClick(ev);
}
} catch (error) {
ev.preventDefault();
throw error;
}
if (ev.defaultPrevented) {
return;
}
if (ev.button !== 0) {
return;
}
if (
ev.currentTarget instanceof HTMLAnchorElement &&
ev.currentTarget.target !== '' &&
ev.currentTarget.target !== '_self'
) {
return;
}
if (ev.metaKey || ev.altKey || ev.ctrlKey || ev.shiftKey) {
return;
}
ev.preventDefault();
There was a problem hiding this comment.
Yeah, I thought about that. I'm hoping we will have a true hook implemented soon so that we don't keep duplicating this type of code (increase tech. debt). There are a few links already doing this through out our codebase
@kevinlog you ok if I just go ahead and implement the hook for this?
There was a problem hiding this comment.
@paul-tavares yes, sounds good.
There was a problem hiding this comment.
Ok. If @oatkiller is ok with it, then I will work on that Issue next and will search/replace usages in our code base with the new callback.
| ({ children, backButton, ...otherProps }) => { | ||
| return ( | ||
| <StyledEuiFlyoutHeader hasBorder {...otherProps} className={backButton && `hasButtons`}> | ||
| {backButton && ( |
There was a problem hiding this comment.
what is the purpose of having a second back button?
There was a problem hiding this comment.
There is only 1 back button - if you look at the screen capture above, you can see the link in the sub-panel. perhaps if I rename the prop to leftLink it would be clearer?
There was a problem hiding this comment.
Also - FYI: I made this a separate component because I think we'll have more use cases we will need it - at least that seems to be true for Host details. Once we have that need, we should likely move the component to a higher directory.
There was a problem hiding this comment.
Sorry, I was thinking 'second' as in addition to the browser back button. does this do the same thing? or is it different
There was a problem hiding this comment.
does this do the same thing? or is it different
Yes and No :)
The link (in the current usage by Host Policy Response panel) will always take you to the Host Details panel. If user navigated to the Policy Response panel from the host details, then yes, the behaviour would be the same as the browser back button. But since this panel is driven by a URL param, the user might have also gotten there by using the URL directly - in that case, this link would (possibly) not match the behavior of the browser's back button.
paul-tavares
commented
Apr 15, 2020
| const store = appStoreFactory({ | ||
| coreStart, | ||
| depsStart, | ||
| additionalMiddleware: [middlewareSpy.actionSpyMiddleware], |
There was a problem hiding this comment.
@oatkiller could you review and comment on this change?
This goes along with a change to the appStoreMiddleware() (see further below). It enables us to inject the actionSpyMiddleware into the application store for testing purposes.
Let me know your thoughts
There was a problem hiding this comment.
I generally think this makes sense. One thought though:
When looking at the appStoreFactory, I see these comments:
/**
* Any additional Redux Middlewares
* (should only be used for testing - example: to inject the action spy middleware)
*/
// Additional Middleware should go last
...additionalMiddleware
Based on those, could we replace:
additionalMiddleware?: Array<ReturnType<typeof substateMiddlewareFactory>>;
with:
actionSpyMiddleware?: WhateverTheTypeShouldBe
Let me know your thoughts
There was a problem hiding this comment.
Thanks for the comments. Yeah, that makes sense. I assume that would be (from our types.ts) - ReturnType<MiddlewareFactory> so that it is correctly typed for the dispatch signature.
paul-tavares
commented
Apr 15, 2020
| /** | ||
| * Utilities for testing Redux middleware | ||
| */ | ||
| export interface MiddlewareActionSpyHelper<S = GlobalState> { |
There was a problem hiding this comment.
Moved this test utility under /store since its not specific to Host list and can be used by any of the test cases. Currently used in the mocks/app_context_render
…cy-Response-panel # Conflicts: # x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/index.test.ts # x-pack/plugins/endpoint/public/applications/endpoint/store/policy_list/test_mock_utils.ts
paul-tavares
commented
Apr 16, 2020
| if (action.type === actionType) { | ||
| watchers.delete(watch); | ||
| clearTimeout(timeout); | ||
| resolve((action as unknown) as ActionsMap<A>[typeof actionType]); |
There was a problem hiding this comment.
I had to this this casting here (which I think is safe being that we are inside of the if() clause) because I kept getting errors in TS around the types not matching. It might have something to do with the fact that the ActionWatcher is defined to take in AppAction (A), but the return value is actually from the ActionMap. They should match up (in my opinion), but they are not 😞
parkiino
reviewed
Apr 16, 2020
| } | ||
| `; | ||
|
|
||
| const BUTTON_CONTENT_PROPS = Object.freeze({ className: 'back-button-content' }); |
There was a problem hiding this comment.
question about why you have to object.freeze this object
There was a problem hiding this comment.
I am defining this constant at the module level and want to ensure it is never mutated accidentally, since that would impact future instantiations of the component.
Maybe I should have also used a type to set the value as Immutable.
parkiino
approved these changes
Apr 16, 2020
|
jenkins test this |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses·ts.detection engine api security and spaces enabled find_statuses should return a single rule status when a single rule is loaded from a find status with defaults addedStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
paul-tavares
added a commit
to paul-tavares/kibana
that referenced
this pull request
Apr 16, 2020
* Added link to Policy status that updates URL and show details panel * Custom Styled Flyout Panel sub-header component to display sub-headers * Move Middleware spy utils under `store/` for re-use * Changed `appStoreFactory()` to accept optional `additionalMiddleware` prop * `waitForAction` middleware test utility now return Action on Promise resolve * Updated PageView component to remove bottom margin
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Apr 17, 2020
* master: (40 commits) [APM]Upgrade apm-rum agent to latest version to fix full page reload (elastic#63723) add deprecation warning for legacy 3rd party plugins (elastic#62401) Migrate timelion vis (elastic#62819) Replacebad scope link with actual values (elastic#63444) Index pattern management UI -> TypeScript and New Platform Ready (create_index_pattern_wizard) (elastic#63111) [SIEM] Threat hunting enhancements: Filter for/out value, Show top field, Copy to Clipboard, Draggable chart legends (elastic#61207) [Maps] fix term join agg key collision (elastic#63324) [Ingest] Fix agent config key sorting (elastic#63488) [Monitoring] Fixed server response errors (elastic#63181) update elastic charts to 18.3.0 (elastic#63732) Start services (elastic#63720) [APM] Encode spaces when creating ML job (elastic#63683) Uptime 7.7 docs (elastic#62228) [DOCS] Updates remote cluster and ccr docs (elastic#63517) [Maps] Add 3rd party vector tile support (elastic#62084) [Endpoint][EPM] Retrieve Index Pattern from Ingest Manager (elastic#63016) [Endpoint] Host Details Policy Response Panel (elastic#63518) [Uptime] Certificate expiration threshold settings (elastic#63682) Refactor saved object types to use `namespaceType` (elastic#63217) [SIEM][CASE] Create comments sequentially (elastic#63692) ...
paul-tavares
added a commit
that referenced
this pull request
Apr 17, 2020
* Added link to Policy status that updates URL and show details panel * Custom Styled Flyout Panel sub-header component to display sub-headers * Move Middleware spy utils under `store/` for re-use * Changed `appStoreFactory()` to accept optional `additionalMiddleware` prop * `waitForAction` middleware test utility now return Action on Promise resolve * Updated PageView component to remove bottom margin
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
Converts the Policy Status on the host details panel to a link, that when clicked will show a Policy Response panel (empty at the moment). Enablement of this panel is driven by a new URL search param (
show).In addition, the following improvements were also done:
mocks/app_context_renderwas enhanced to use the Redux middleware Action spystore/(its not specific to Policy List and is now being used in the mock app context render)waitForAction()utility was modified to resolve with theactionthat was dispatchedHosts List
Checklist
Delete any items that are not applicable to this PR.