-
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] Enable Kibana feature controls (RBAC)configuration by space for Endpoint management #193003
Changes from all commits
ffe877b
5a1a2f5
87cd5ca
bb08b05
cf67151
ef505c4
07c7cec
e2f8baf
6edb3ed
a595479
f679b57
dede372
53ea5fa
0baf5d9
befe460
8d874a8
fb0c0bd
724e085
995cc8a
52e90cf
2c07483
10aa2cc
af63994
720353b
1895340
d8d91d0
d7d2d21
9b6cdaa
51455db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ import { | |
EuiIconTip, | ||
EuiText, | ||
} from '@elastic/eui'; | ||
import React from 'react'; | ||
import React, { useCallback, useMemo } from 'react'; | ||
|
||
import { i18n } from '@kbn/i18n'; | ||
import type { | ||
|
@@ -33,10 +33,23 @@ interface Props { | |
privilegeIndex: number; | ||
onChange: (selectedPrivileges: string[]) => void; | ||
disabled?: boolean; | ||
categoryId?: string; | ||
} | ||
|
||
export const SubFeatureForm = (props: Props) => { | ||
const groupsWithPrivileges = props.subFeature.getPrivilegeGroups(); | ||
const subFeatureNameTestId = useMemo(() => { | ||
// Removes anything that is not a Number, Letter or space and replaces it with _ | ||
return props.subFeature.name.toLowerCase().replace(/[^\w\d]/g, '_'); | ||
}, [props.subFeature.name]); | ||
const getTestId = useCallback( | ||
(suffix: string = '') => { | ||
return `${props.categoryId ? `${props.categoryId}_` : ''}${ | ||
props.featureId | ||
}_${subFeatureNameTestId}${suffix ? `_${suffix}` : ''}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I always opt against having generators for test-ids because it's almost impossible to find the element from the web console in the codebase. What do you say if we hardcode the test-ids? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually of the opposite opinion. Having static test ids in some cases makes it harder to find the elements you are after, especially when a component is reused multiple times on the page - as is the case here. Static only make sense (to me) when a component is not reused on a page (is only ever rendered once) I rather keep it as is, unless you have a better way to select elements on the page in a more efficient way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whenever I want to debug functionality, I get the data-test-id from devconsole. If it was hardcoded I could just use Ctrl+F and search for it, with the prefixes generators etc it's almost impossible - and have to look for chunks and later confirm if I found a correct prefix. However, I don't insist, just sharing personal opinion :P It can stay this way 👍 Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So yeah, I do think we differ in the way we approach test development. If I'm working on e2e tests, I do in fact use the browser's developer tools to grab the I will leave it as is since you are ok with it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course, all good 👍 thanks for the consideration :) |
||
}, | ||
[props.categoryId, props.featureId, subFeatureNameTestId] | ||
); | ||
|
||
const getTooltip = () => { | ||
if (!props.subFeature.privilegesTooltip) { | ||
|
@@ -55,6 +68,7 @@ export const SubFeatureForm = (props: Props) => { | |
type="iInCircle" | ||
color="subdued" | ||
content={tooltipContent} | ||
anchorProps={{ 'data-test-subj': getTestId('nameTooltip') }} | ||
/> | ||
); | ||
}; | ||
|
@@ -63,11 +77,11 @@ export const SubFeatureForm = (props: Props) => { | |
return null; | ||
} | ||
return ( | ||
<EuiFlexGroup alignItems="center"> | ||
<EuiFlexGroup alignItems="center" data-test-subj={getTestId()}> | ||
<EuiFlexItem grow={3}> | ||
<EuiFlexGroup gutterSize="none" direction="column"> | ||
<EuiFlexItem> | ||
<EuiText size="s"> | ||
<EuiText size="s" data-test-subj={getTestId('name')}> | ||
{props.subFeature.name} {getTooltip()} | ||
</EuiText> | ||
</EuiFlexItem> | ||
|
@@ -85,7 +99,9 @@ export const SubFeatureForm = (props: Props) => { | |
)} | ||
</EuiFlexGroup> | ||
</EuiFlexItem> | ||
<EuiFlexItem grow={2}>{groupsWithPrivileges.map(renderPrivilegeGroup)}</EuiFlexItem> | ||
<EuiFlexItem grow={2} data-test-subj={getTestId('privilegeGroup')}> | ||
{groupsWithPrivileges.map(renderPrivilegeGroup)} | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
); | ||
|
||
|
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 wonder if it wouldn't be more readable if you iterate over
securitySubFeaturesList
instead of wrapping each subFeature inenableSpaceAwarenessIfNeeded
. What do you think?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 guess. It accomplishes the same thing, but I'm open to changing it.
what do you find about this implementation that makes it less readable?
I did try to make the function names clear as to what they do and also opted to do it this way only because it gives us some flexibility if we ever have a case where a specific feature privilege does require it to be space agonistic.
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 was thinking more about doing it in one place insterad of passing to all separately:
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... Not sure if its better - I don't like that additional iteration by using
.map()
and recreating the entire structure again.I'm going to leave it as is for now.