Skip to content
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

Add event for AccessControl._setRoleAdmin #2210

Closed
nventuro opened this issue Apr 22, 2020 · 6 comments · Fixed by #2214
Closed

Add event for AccessControl._setRoleAdmin #2210

nventuro opened this issue Apr 22, 2020 · 6 comments · Fixed by #2214

Comments

@nventuro
Copy link
Contributor

Whenever a role's admin is set by _setRoleAdmin, we should emit an event indicating this happened.

Not only is this useful to react to admin changes, it will also help in dynamic role schemes, where a role hierarchy may be set up without any account having the roles.

@Skyge
Copy link
Contributor

Skyge commented Apr 24, 2020

That is good, how about the name of this new event is NewRoleAdmin or something else?

@nventuro
Copy link
Contributor Author

We name events as verbs in the past tense, so it'd have to be something like RoleAdminChanged. I now wonder if _setRoleAdmin shouldn't be _changeRoleAdmin: 'set' might give the impression of it being a one-off thing.

@julianmrodri
Copy link
Contributor

@nventuro I would like to tackle this if you are OK. Not sure whats the expected timeline but can take a look during the weekend if required. Can do just the event or also the renaming if we agree. Let me know, just looking on how to contribute :)

@nventuro
Copy link
Contributor Author

Go for it! The event alone should be fine, it'll be a straightforward PR :)

@julianmrodri
Copy link
Contributor

@nventuro Just submitted PR. Ran the tests and lintern and seems to be fine. I dont think we have to modify the CHANGELOG. Please let me know how it looks. Thanks!

@Skyge
Copy link
Contributor

Skyge commented Apr 24, 2020

I now wonder if _setRoleAdmin shouldn't be _changeRoleAdmin: 'set' might give the impression of it being a one-off thing.

As far as I am concerned, the function name is OK, the 'set' also means it can 'change', of course, in order to make it more clear, we can add some comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants