Role.getRoles() to return only role names #2975
Conversation
|
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
@slnode ok to test |
|
@ebarault thank you for the pull request. Your proposal makes sense to me, however I am concerned about backwards compatibility. Can we implement your enhancement behind a flag, so that users can decide whether to use current behaviour (by default) or get role names only (when the flag is set)? One options is to add a new optional parameter Roles.getRoles(ctx, cb); // works as before
Roles.getRoles(ctx, { returnRoleNames: true }, cb); // the new way |
|
@bajtos : makes sense. Renamed the option 'returnOnlyRoleNames'. Rollbacked original test, and added a new one with the option. please have a look |
|
related #2420 |
There was a problem hiding this comment.
One small nitpick, the rest looks good to me.
After you address my comment, could you please squash all commits into a single one and provide a descriptive error message following our 50/72 convention?
| @@ -254,6 +254,20 @@ describe('role model', function() { | |||
| }, | |||
| function(next) { | |||
| Role.getRoles( | |||
| {principalType: RoleMapping.USER, principalId: user.id}, | |||
| {'returnOnlyRoleNames': true}, | |||
There was a problem hiding this comment.
I think the quotes are not needed, can you remove please?
|
Actually, let me make those changes for you, to get the patch landed sooner. |
Currently the return type of Role.getRoles() method is inconsistent: role names are returned for smart roles and role ids are returned for static roles (configured through user-role mapping). This commit adds a new option to Role.getRoles() allowing the caller to request role names to be returned for all types of roles.
bajtos
force-pushed
the
getRoles-to-return-only-role-names
branch
from
6a26cee
to
b0d6c4a
Compare
|
thanks for finalizing @bajtos |
|
Landed, thank you for the contribution! |
Description
Currently the return type of
Role.getRoles()method is inconsistent: role names are returned for smart roles and role ids are returned for static roles (configured through user-role mapping)This PR proposes to return only role names in
Role.getRoles()Tests have been updated.
Related issues
Checklist
guide
@bajtos, @superkhau could you please have a look?