-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(users): add profile level custom role #6380
base: main
Are you sure you want to change the base?
Conversation
Changed Files
|
@@ -21,6 +22,7 @@ pub struct RoleInfoWithGroupsResponse { | |||
pub groups: Vec<PermissionGroup>, | |||
pub role_name: String, | |||
pub role_scope: RoleScope, | |||
pub entity_type: Option<EntityType>, |
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.
Why entity_type is option here? We have it non option in DB, right?
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.
Because in the request its optional , so thought in response also it should be optional
.or(dsl::scope.eq(RoleScope::Merchant)) | ||
.or(dsl::scope.eq(RoleScope::Profile)), | ||
) | ||
.filter(dsl::entity_type.eq_any(entity_in_vec)) |
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.
Just for clarity, In case when is_lineage data_required
true
case this query will return same result as we will getting without applying this eq_any
filter?
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.
Yes
user_from_token.org_id, | ||
user_from_token.merchant_id, | ||
), | ||
request.entity_type.is_none(), | ||
None, | ||
) | ||
.await | ||
.change_context(UserErrors::InternalServerError) | ||
.attach_printable("Failed to get roles")?, | ||
// TODO: Populate this from Db function when support for profile id and profile level custom roles is added |
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 can remove this todo now?
@@ -0,0 +1,2 @@ | |||
-- Your SQL goes here | |||
ALTER TABLE roles ADD profile_id Varchar(64); |
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.
nitpick:
- To keep drop.sql and up.sql consistent we can either use COLUMN keyword in both, or drop it for both, since it is optional.
- We can also use IF EXISTs and IF NOT EXISTs (Optional change)
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.
Are you sure that old db functions will work without any issues with the new data?
if !(user_entity_type >= role_entity_type | ||
&& user_entity_type >= requestor_entity_from_role_scope | ||
&& requestor_entity_from_role_scope >= role_entity_type) |
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.
Can we split this conditional.
|
Type of Change
Description
Earlier we only had access to create Merchant level custom role at Org and Merchant scope . But after this user can be able to create custom role at Profile level at Organization , Merchant and Profile scope
Additional Changes
Motivation and Context
Closes 6488
How did you test it?
Create Custom role at profile level
Response :
To create a profile level custom role , the following scenarios the operation should be allowed
Create Custom role at merchant level
Response :
To create a merchant level custom role , the following scenarios the operation should be allowed
Checklist
cargo +nightly fmt --all
cargo clippy