-
Notifications
You must be signed in to change notification settings - Fork 15
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
Cleanup abilities/permissions #2734
Conversation
e70f8cd
to
f53662a
Compare
f53662a
to
6d67b71
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.
Hey man LGTM! Great work!
@@ -208,6 +211,7 @@ describe('HostsLists component', () => { | |||
hostsList: { | |||
hosts: [host], | |||
}, | |||
user: { abilities: [{ name: 'all', resource: 'all' }] }, |
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 repeat this very often, an idea could be to define a global const object something like
const superAdminAbilities = { abilities: [{ name: 'all', resource: 'all' }] }
or const superAdmin
and reuse it --> user: superAdmin,
But i see why we reuse it again and again, so only just an idea :)
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.
@EMaksy Yeah...
This is true.
I like the idea of having a constant for this, so we can reuse in tests and storybook.
As we have some already going work in parallel, do you mind if we wait until all the frontend PRs are merged to apply this change?
This way, we don't have any sort of conflicts.
Basically, we do a big search/replace once we have all the features done
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, sure!
We could even define some more user ability roles, so in future if something changes we need to change it only at one place and not everywhere.
But yeah, let's finish up everything else first!
6d67b71
to
beb7e7d
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.
LGTM
Just a nitpick, if you want you can add a controller test that accepts the request when the abilities are present.
Other then all:all ability, we have that in the setup function, but I think it's valuable, pretty easy to create a user on demand with the right ability and test it.
It serves also as docs purpose about the controller usage
Wdyt?
Description
Add host/application instance/database instance cleanup abilities.
The
Clean up
is only available when the user has the proper abilities attached.Even though we have a lot of new lines, almost everything is boilerplate and
userAbilities
movement in the frontend.