[Endpoint] add policy details route #59951
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
kevinlog
commented
Mar 11, 2020
| @@ -43,3 +44,24 @@ export const policyListMiddlewareFactory: MiddlewareFactory<PolicyListState> = c | |||
| } | |||
| }; | |||
| }; | |||
|
|
|||
| export const policyDetailsMiddlewareFactory: MiddlewareFactory<PolicyDetailsState> = coreStart => { | |||
There was a problem hiding this comment.
@paul-tavares @parkiino @oatkiller was thinking that we'll keep all state separate from the overall policy list since Policy Details will be it's own route and page.
ManagementList keeps the details in state under the overall list data because it's a flyout and a query param:
Currently, I have PolicyDetails separate in state:
If we agree that we should keep PolicyDetails separate in state, then I'll go ahead and break out all of this code to be in its own files, etc. Let me know your thoughts otherwise.
There was a problem hiding this comment.
I would say 'go w/ it'. Typescript should make refactoring things like this a lot simpler.
There was a problem hiding this comment.
i think that makes sense too, since it's its own page
kevinlog
commented
Mar 11, 2020
|
|
||
| export const selectPolicyItems = (state: PolicyListState) => state.policyItems; | ||
|
|
||
| export const selectPolicyDetails = (state: PolicyDetailsState) => state.policyItem; | ||
|
|
||
| export const selectPolicyIdFromParams: (state: PolicyDetailsState) => string = createSelector( |
There was a problem hiding this comment.
Just gets the job done right now, we should break out the location state to another location in state as discussed in previous threads by @paul-tavares and @peluja1012
https://github.com/elastic/kibana/pull/58308/files#r385053154
Then we could also put selectors like this in a more generic place
kevinlog
commented
Mar 11, 2020
| items: PolicyData[]; | ||
| } | ||
|
|
||
| let savedFakePolicyData: SavedFakePolicyData = { items: [] }; |
There was a problem hiding this comment.
hacky stuff for the fake data, just wanna maintain the random set of data so I can use ids from the URL to find a Policy record
kevinlog
commented
Mar 11, 2020
| }; | ||
| } | ||
|
|
||
| if (action.type === 'userChangedUrl') { |
There was a problem hiding this comment.
It's also probably time to break this cycle and put location in a more generic place in state
x-pack/plugins/endpoint/public/applications/endpoint/store/policy_details/selectors.ts
Show resolved
Hide resolved
| export const getFakeDatasourceApiResponse = async (page: number, pageSize: number) => { | ||
| await new Promise(resolve => setTimeout(resolve, 500)); | ||
|
|
||
| // Emulates the API response - see PR: | ||
| // https://github.com/elastic/kibana/pull/56567/files#diff-431549a8739efe0c56763f164c32caeeR25 | ||
| return { | ||
| const policyData = { | ||
| items: Array.from({ length: pageSize }, (x, i) => ({ |
There was a problem hiding this comment.
Maybe extract the iterator function here to a top-level function generateSinglePolicyData(name) and then you can use it in getFakeDatasourceDetailsApiResponse() without the hackery 😃 ?
There was a problem hiding this comment.
I know I can do that, I was mostly using the hackery to so that when I click on a Policy name, the same name will show up on the Policy details (from the same dataset). I'll adjust the code though so that this isn't so hacky.
There was a problem hiding this comment.
Ahh. Ok. In that case, then maybe tie the id to the name in the generator? (instead of random strings, maybe use the pageNumber + pageSize to get consistent results?
If too much trouble, then just leave it as it - it is "fake" data 😆
There was a problem hiding this comment.
made to be less hacky now :)
| // Emulates the API response - see PR: | ||
| // https://github.com/elastic/kibana/pull/56567/files#diff-431549a8739efe0c56763f164c32caeeR25 | ||
|
|
||
| if (savedFakePolicyData.items) { |
There was a problem hiding this comment.
If you extract that function above that generate a single Policy, then here you can use it like:
return {
...generateSinglePolicyData('some name');
...{ id }
}| import { selectPolicyDetails } from '../../store/policy_details/selectors'; | ||
|
|
||
| export const PolicyDetails = React.memo(() => { | ||
| usePageId('policyDetailsPage'); |
There was a problem hiding this comment.
I don't think we are using this anymore (@peluja1012 right?).
I know Policy List has it, but the PR I was working (currently on hold) removes use of this hook in favor of userNavigagtedToPage store action that is dispatched every time the URL changes.
There was a problem hiding this comment.
removed, following a similar method as management and alerts lists now
| @@ -84,6 +85,10 @@ const renderFormattedNumber = (value: number, _item: PolicyData) => ( | |||
| </TruncateText> | |||
| ); | |||
|
|
|||
| const renderPolicyNameLink = (value: string, _item: PolicyData) => { | |||
| return <Link to={`/policy/${_item.id}`}>{value + ' '}</Link>; | |||
There was a problem hiding this comment.
Should use EuiLink. Can use the onClick event along with react-router-dom useHistory() to to push the route and then ev.preventDefault().
From your last PR, @oatkiller had suggested a generic method/hook? that we could add to our /lib/ and then re-use with Eui Components that act like navigation links. Maybe its time to add it to our code base.
There was a problem hiding this comment.
@paul-tavares using EuiLink now. I tried implementing the hook, but haven't quite gotten it to work yet. I created this ticket last week: https://github.com/elastic/endpoint-app-team/issues/230
I can take that on next and isolate the effort and update all Links. Are you OK with that?
| @@ -337,7 +337,7 @@ export type ResolverEvent = EndpointEvent | LegacyEndpointEvent; | |||
| /** | |||
| * The PageId type is used for the payload when firing userNavigatedToPage actions | |||
| */ | |||
| export type PageId = 'alertsPage' | 'managementPage' | 'policyListPage'; | |||
| export type PageId = 'alertsPage' | 'managementPage' | 'policyListPage' | 'policyDetailsPage'; | |||
There was a problem hiding this comment.
I don't think you need this - see comment further below
kevinlog
force-pushed
the
task/add-policy-details-route
branch
from
e6703ab to
ddff980
Compare
kevinlog
added
release_note:skip
|
Pinging @elastic/endpoint-management (Team:Endpoint Management) |
|
Pinging @elastic/endpoint-app-team (Feature:Endpoint) |
kevinlog
changed the title
kevinlog
changed the title
paul-tavares
approved these changes
Mar 13, 2020
|
|
||
| return ( | ||
| <EuiLink | ||
| onClick={(event: React.MouseEvent) => { |
There was a problem hiding this comment.
should also add href so that user can open in new window/tab or copy url. maybe on next PR :)
There was a problem hiding this comment.
@paul-tavares I implemented this locally, looks like EuiLink doesn't accept both href and onClick
We'll have to revisit that, I'll talk with EUI team
There was a problem hiding this comment.
oh yeah - I remember hitting that recently in ingest. Fortunately, I ended up not needing it there since that project is using hash router.
/cc me on the discussion with EUI if you remember
There was a problem hiding this comment.
yeah i had to do something like this for the endpoint list, which i put right above the eui component
// eslint-disable-next-line @elastic/eui/href-or-on-click
parkiino
reviewed
Mar 13, 2020
| ); | ||
| }; | ||
|
|
||
| const renderPolicyNameLink = (value: string, _item: PolicyData) => { |
There was a problem hiding this comment.
i noticed this in Paul's code too, what does the _item do?
There was a problem hiding this comment.
it gives you access to the all of the data that the table columns are based on, in this case the PolicyData
There was a problem hiding this comment.
to expand on @parkiino 's comment. The confusion might be in that in my commit I was actually NOT using the item data (thus why I named it with a _), but needed to define the parameter in order to get around a TS error - thread here (slack EUI channel) for reference
There was a problem hiding this comment.
@kevinlog do you remember when i had that type error on the endpoint details pr? Is this another way to solve that?
parkiino
approved these changes
Mar 13, 2020
peluja1012
reviewed
Mar 13, 2020
| * Policy list store state | ||
| */ | ||
| export interface PolicyDetailsState { | ||
| /** Array of policy items */ |
There was a problem hiding this comment.
Is is supposed to be an array of PolicyData or a single one?
There was a problem hiding this comment.
a single one, you caught my c/p. Corrected.
peluja1012
reviewed
Mar 13, 2020
| if (policyItem) { | ||
| return <span data-test-subj="policyDetailsName">{policyItem.name}</span>; | ||
| } else { | ||
| return <span data-test-subj="policyDetailsNotFound">Policy Not Found</span>; |
There was a problem hiding this comment.
We should i18n this message
peluja1012
approved these changes
Mar 13, 2020
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Alerting API Integration Tests.x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/alerts·ts.alerting api integration security and spaces enabled Alerting alerts space_1_all at space1 should schedule task, run alert and schedule actions when appropriateStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
kibanamachine
added
the
backport missing
|
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. |
kibanamachine
removed
the
backport missing
kevinlog
added a commit
that referenced
this pull request
Mar 16, 2020
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
Implements https://github.com/elastic/endpoint-app-team/issues/245'
Adds the policy details route. It currently operates on fake data that will take any ID passed in and create a Policy details page with a title. This is meant to be a starting point so that we can start building forms for the Policy details page.
The functional tests are light as we aren't dealing with real data now. We just navigate to the page and verify that it loads.
There are also some issues that need to be addressed regarding URLs and routing. I listed them at the bottom and will take them on next. I'd like to merge this so that we can continue development on the Policy forms. I'll address the debt in parallel.
https://github.com/elastic/endpoint-app-team/issues/269
https://github.com/elastic/endpoint-app-team/issues/230
https://github.com/elastic/endpoint-app-team/issues/268
Checklist
Delete any items that are not applicable to this PR.
For maintainers