Conversation
7f98f1f to
1ad2a77
Compare
There was a problem hiding this comment.
I would expect some permissons check for edit_hosts before showing it in menu
There was a problem hiding this comment.
Other cases do not show the item in menu when user do not have the permissions, please do it in same way, having item disabled could be confusing (some of the items are disabled when it needs selection etc)
pr-processor
Bot
added
Waiting on contributor
Needs re-review
and removed
Not yet reviewed
labels
|
@Lukshio thank you, updated. I'm just unsure about the permissions. (No component in the codebase checks the |
|
|
Lukshio
left a comment
Lukshio
left a comment
There was a problem hiding this comment.
@Lukshio thank you, updated. I'm just unsure about the permissions. (No component in the codebase checks the
edit_hostsin the frontend.)
We use permission check often e.g.
https://github.com/theforeman/foreman/blob/c74b70ed8ea3d2fbdb206a636d9c528578eddc01/webpack/assets/javascripts/react_app/components/HostsIndex/index.js#L52
Also, what do you think about "As you can see, the new UI is now missing the table with hosts - do you think it's important and should be added?" We can ask Maria from UX about that.
All updated modals are without tables, so I would leave it like that
fix false success commit:
Controller returned HTTP redirects for both success and errors and showed success even when validation failed.
Solution: Detect AJAX requests (request.xhr?) and return JSON with the correct status code instead of redirects.
I would introduce new API controller, because later the legacy page will be deleted and it will be cleaner variant.
| const hasPermission = | ||
| userPermissions.has('assign_policies') && userPermissions.has('edit_hosts'); |
There was a problem hiding this comment.
Please use consts from core
cc892d1 to
33343cd
Compare
| export const fetchPolicies = () => | ||
| APIActions.get({ | ||
| url: foremanUrl('/api/v2/compliance/policies'), | ||
| key: POLICIES_KEY, |
There was a problem hiding this comment.
| key: POLICIES_KEY, | |
| key: POLICIES_KEY, | |
| params: { per_page: 'all' }, |
i would include all, I think that otherwise it will use per_page from user settings
| ouiaId="bulk-compliance-policy-modal-confirm-button" | ||
| variant="primary" | ||
| onClick={handleConfirm} | ||
| isDisabled={policyId === ''} |
There was a problem hiding this comment.
Add please condition to prevent double submit
| ouiaId="bulk-compliance-policy-toggle" | ||
| onClick={onToggleClick} | ||
| isExpanded={policySelectOpen} | ||
| style={{ width: '500px' }} |
There was a problem hiding this comment.
Do not use hardcoded values
9bea594 to
f346d0b
Compare
f346d0b to
76ed1f6
Compare
There was a problem hiding this comment.
Other cases do not show the item in menu when user do not have the permissions, please do it in same way, having item disabled could be confusing (some of the items are disabled when it needs selection etc)
There was a problem hiding this comment.
Please add test for forbidden user
This branch has not been deployed
2 participants
QUESTION: As you can see, the new UI is now missing the table with hosts - do you think it's important and should be added? Also, in the new UI, after submitting the modal, the hosts are still selected in the table, so the "Remember hosts selection..." tick is not needed.
Change in the behaviour when assigning a policy that is already assigned:
--- Legacy:
Error: cannot assign to centos-vm.usersys.redhat.com, all assigned policies must be deployed in the same way, check 'deploy by' for each assigned policy--- Now:
Successfully assigned compliance policy to selected hostsLegacy UI:
New UI:
