-
Notifications
You must be signed in to change notification settings - Fork 224
WIP: RBAC UI - component steps progressbar #10496
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
base: master
Are you sure you want to change the base?
Conversation
|
👋 Hello! Thanks for contributing to our project. You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/10496/checks If you are unsure the failing tests are related to your code, you can check the "reference jobs". These are jobs that run on a scheduled time with code from master. If they fail for the same reason as your build, it means the tests or the infrastructure are broken. If they do not fail, but yours do, it means it is related to your code. Reference tests: KNOWN ISSUES Sometimes the build can fail when pulling new jar files from download.opensuse.org . This is a known limitation. Given this happens rarely, when it does, all you need to do is rerun the test. Sorry for the inconvenience. For more tips on troubleshooting, see the troubleshooting guide. Happy hacking! |
89d5409 to
661a3c1
Compare
|
Some notes after initial review:
Role list page:
Namespace list table:
Add users step & add users tab in edit:
|
7693eef to
9a5b589
Compare
e89309d to
a155669
Compare
67c2ef3 to
ef95129
Compare
Signed-off-by: Pascal Arlt <[email protected]>
Signed-off-by: Pascal Arlt <[email protected]>
…d on selected org
…disable parent if all children are disabled
ef95129 to
c4e32a5
Compare
Signed-off-by: Pascal Arlt <[email protected]>
c4e32a5 to
67c083d
Compare
cbbayburt
left a comment
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 very much @bisht-richa, @parlt91. It looks very good overall.
I've also deployed and tested it, and everything works as expected.
Apart from my inline comments, here's a few notes I had when testing, mostly on the UX:
- The text
[API]looks a bit raw on the table. Maybe we should pick an icon for this instead. - Bug: when I go back to the first step and remove an item from "Copy permissions from" control, the permissions list is not recalculated.
- After a series of searching and adding, it's easy to lose track of the overall selections. We need an overview of all the selections at the final step.
- When you search for a namespace, the natural next step is always expanding some rows. We can automatically expand the whole filtered list after a search, and reduce the necessary steps taken by the user.
- Apart from that, we could also use the usual "Expand/Collapse all" controls on the table.
| useEffect(() => { | ||
| if (!isLoading) { | ||
| setIsLoading(true); | ||
| getNamespaces(searchValue); | ||
| } | ||
| }, [searchValue]); |
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.
In this kind of search-as-you-type task, it's a good practice to limit the requests to the backend to prevent clogging up the network.
We already do this using the debounce utility from lodash. It wraps the method that does the API call and it waits for some milliseconds of inactivity before it calls the method. This way you avoid calling the API for every keystroke, and only do one call after the user pauses typing.
Here's an example usage (note the last arg: 100ms of wait time):
Lines 83 to 103 in fa55769
| const onSearch = useCallback( | |
| debounce((newSearch: string) => { | |
| channelProcessor.setSearch(newSearch).then((newRows) => { | |
| if (newRows) { | |
| setRows(newRows); | |
| } | |
| if (newSearch) { | |
| // Open all channels when search changes so visible child matches are also visible | |
| setOpenRows(new Set(newRows?.map((row) => row.base.id))); | |
| } else { | |
| // When change is cleared, close all besides the selected base | |
| const selectedBaseChannelId = channelProcessor.getSelectedBaseChannelId(); | |
| if (selectedBaseChannelId) { | |
| setOpenRows(new Set([selectedBaseChannelId])); | |
| } | |
| } | |
| }); | |
| }, 100), | |
| [] | |
| ); |
I think it should work if you wrap the getNamespace method like this and call it in useEffect.
| if (item.orgName === "-") { | ||
| return ( | ||
| <div className="btn-group"> | ||
| <Button className="btn-default btn-sm" icon="fa-user" /> |
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.
This button has no function. I think we don't need it at all. I don't even remember why this was here in the first place (also below in the else block).
| <LinkButton | ||
| className="btn-default btn-sm" | ||
| icon="fa-pencil" | ||
| href={`/rhn/manager/admin/access-control/show-access-group/${item.id}`} | ||
| /> |
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.
This button is missing a title
| .withVisibility(adminRoles.get("satellite"))) | ||
| .addChild(new MenuItem("Access Control") | ||
| .withPrimaryUrl("/rhn/manager/admin/access-control") |
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 think we can remove the item from here if there's no specific reason for it?
| @@ -0,0 +1 @@ | |||
| - Implemented a new StepsProgressBar component for managing RBAC. | |||
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.
The end user doesn't need to know about StepsProgressBar. Let's keep it simple. Maybe something like:
| - Implemented a new StepsProgressBar component for managing RBAC. | |
| - Implemented RBAC management UI |
| .createNativeQuery(""" | ||
| SELECT * | ||
| FROM access.namespace | ||
| WHERE :filter <% lower(namespace||description) |
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.
Maybe we should add a "." after the namespace (also to the index in the schema). Currently, "systemget_details" works, but "system.get_details" doesn't.
| INSERT INTO access.namespace (namespace, access_mode, description) | ||
| SELECT 'admin.access-control.access-group', 'R', 'List and detail custom access groups.' | ||
| WHERE NOT EXISTS (SELECT 1 FROM access.namespace WHERE namespace = 'admin.access-control.access-group' AND access_mode = 'R'); | ||
|
|
||
| INSERT INTO access.namespace (namespace, access_mode, description) | ||
| SELECT 'admin.access-control.access-group', 'W', 'Create, modify and delete custom access groups.' | ||
| WHERE NOT EXISTS (SELECT 1 FROM access.namespace WHERE namespace = 'admin.access-control.access-group' AND access_mode = 'W'); |
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.
We don't plan to expand this feature much further so I think only 2 levels are enough for the namespace. Also I suggested "access" to be consistent with the api.access namespace.
| INSERT INTO access.namespace (namespace, access_mode, description) | |
| SELECT 'admin.access-control.access-group', 'R', 'List and detail custom access groups.' | |
| WHERE NOT EXISTS (SELECT 1 FROM access.namespace WHERE namespace = 'admin.access-control.access-group' AND access_mode = 'R'); | |
| INSERT INTO access.namespace (namespace, access_mode, description) | |
| SELECT 'admin.access-control.access-group', 'W', 'Create, modify and delete custom access groups.' | |
| WHERE NOT EXISTS (SELECT 1 FROM access.namespace WHERE namespace = 'admin.access-control.access-group' AND access_mode = 'W'); | |
| INSERT INTO access.namespace (namespace, access_mode, description) | |
| SELECT 'admin.access', 'R', 'List and detail custom access groups.' | |
| WHERE NOT EXISTS (SELECT 1 FROM access.namespace WHERE namespace = 'admin.access-control.access-group' AND access_mode = 'R'); | |
| INSERT INTO access.namespace (namespace, access_mode, description) | |
| SELECT 'admin.access', 'W', 'Create, modify and delete custom access groups.' | |
| WHERE NOT EXISTS (SELECT 1 FROM access.namespace WHERE namespace = 'admin.access-control.access-group' AND access_mode = 'W'); |
Also: the data is missing in common/data/..
| @@ -0,0 +1 @@ | |||
| - Implemented a new StepsProgressBar component for managing RBAC. | |||
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.
The end user doesn't need to know about StepsProgressBar. Let's keep it simple. Maybe something like:
| - Implemented a new StepsProgressBar component for managing RBAC. | |
| - Implemented RBAC management UI |
What does this PR change?
GUI diff
Documentation
No documentation needed: add explanation. This can't be used if there is a GUI diff
No documentation needed: only internal and user invisible changes
Documentation issue was created: Link for SUSE Multi-Linux Manager contributors, Link for community contributors.
API documentation added: please review the Wiki page Writing Documentation for the API if you have any changes to API documentation.
(OPTIONAL) Documentation PR
DONE
Test coverage
ℹ️ If a major new functionality is added, it is strongly recommended that tests for the new functionality are added to the Cucumber test suite
No tests: add explanation
No tests: already covered
Unit tests were added
Cucumber tests were added
DONE
Links
Issue(s): #
Port(s): # add downstream PR(s), if any
Changelogs
Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository
If you don't need a changelog check, please mark this checkbox:
If you uncheck the checkbox after the PR is created, you will need to re-run
changelog_test(see below)Re-run a test
If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:
Before you merge
Check How to branch and merge properly!