-
Notifications
You must be signed in to change notification settings - Fork 64
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
fix(ESSNTL-5239): Improve RBAC check for bulk group actions #1974
Conversation
9963d4e
to
0931068
Compare
0931068
to
d78c08a
Compare
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.
Nice work overall @gkarat
I have found a few comments which I am not sure if you intended to keep (marked them with 'here')
Also, I think you could be using the template literal alongside with the ternary operator to shorten the code and remove some code duplication :)
I have added an example of how I thought it could go
In the file: src/components/InventoryGroups/Modals/RemoveHostsFromGroupModal.js
- line 42
- line 20
src/components/InventoryGroups/Modals/RemoveHostsFromGroupModal.js
Outdated
Show resolved
Hide resolved
|
||
it('can bulk remove from the permitted group', () => { | ||
cy.get(ROW).find('[type="checkbox"]').eq(0).click(); | ||
// TODO: implement ouia selector for this component |
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.
here
.contains('Remove from group') | ||
.shouldHaveAriaEnabled(); | ||
cy.get(ROW).find('[type="checkbox"]').eq(1).click(); | ||
// TODO: implement ouia selector for this component |
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.
here
it('can bulk remove from group together with ungroupped hosts', () => { | ||
cy.get(ROW).find('[type="checkbox"]').eq(0).click(); | ||
cy.get(ROW).find('[type="checkbox"]').eq(3).click(); | ||
// TODO: implement ouia selector for this component |
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.
here
it('can bulk add hosts to the permitted group', () => { | ||
cy.get(ROW).find('[type="checkbox"]').eq(3).click(); | ||
cy.get(ROW).find('[type="checkbox"]').eq(4).click(); | ||
// TODO: implement ouia selector for this component |
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.
here
it('cannot bulk add to group if groupped hosts selected', () => { | ||
cy.get(ROW).find('[type="checkbox"]').eq(0).click(); | ||
cy.get(ROW).find('[type="checkbox"]').eq(3).click(); | ||
// TODO: implement ouia selector for this component |
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.
here
const hostsInGroup = hosts.filter(({ groups }) => groups.length > 0); | ||
const groupName = hostsInGroup[0].groups[0].name; | ||
|
||
return hostsInGroup.length === 1 |
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 could also put all the checks inside the string, using a Template Literal with the ternary operator, and by that remove some code duplication :)
Something like this:
const numOfSystems = hostsInGroup.length;
return {
onSuccess: {
title: `${numOfSystems} system${numOfSystems===1 ? " was" : "s were"} removed from ${groupName}`,
},
onError: {
title: `Failed to remove ${numOfSystems} system${numOfSystems===1 ? "" : "s"} from ${groupName}`,
},
};
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 this is the variant that used to be here. But I decided to rework it for better code readability (even though it's not following DRY 100%).
component: componentTypes.PLAIN_TEXT, | ||
name: 'warning-message', | ||
label: | ||
hostsInGroup.length === 1 ? ( |
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.
These lines can also be shortened using the template literal with the ternary operator 👍🏼
Something like this:
const numOfSystems = hostsInGroup.length;
const systemName = hostsInGroup[0].display_name;
// in the returned JSX (line 19):
label:
<Text>
<strong>{numOfSystems === 1 ? systemName : numOfSystems}</strong>{numOfSystems === 1 ? "" : "
systems"} will no longer be part of <strong>{groupName}</strong> and {numOfSystems === 1 ? "its" :
"their"} configuration will be impacted.
</Text>
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.
Thank you. But still, I would like to stick to the current variant (see #1974 (comment) comment)
🎉 This PR is included in version 1.35.11 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Implements https://issues.redhat.com/browse/ESSNTL-5239.
Must be merged after #1973 and RedHatInsights/frontend-components#1885.
This makes sure that both bulk actions (Remove from group and Add to group) on the /inventory page comply with the RBAC requirements: specifically that the table no longer checks on the global level permissions.
How to test
inventory:groups:write
permission only to specific group