Bug: refactor: change create and update role apis to respond with role info




diff --git a/crates/api_models/src/events/user_role.rs b/crates/api_models/src/events/user_role.rs
index aee2b3fb66c..0d42d1de7d6 100644
--- a/crates/api_models/src/events/user_role.rs
+++ b/crates/api_models/src/events/user_role.rs
@@ -3,7 +3,8 @@ use common_utils::events::{ApiEventMetric, ApiEventsType};
 use crate::user_role::{
     role::{
         CreateRoleRequest, GetRoleFromTokenResponse, GetRoleRequest, ListRolesResponse,
-        RoleInfoResponse, RoleInfoWithPermissionsResponse, UpdateRoleRequest,
+        RoleInfoResponse, RoleInfoWithGroupsResponse, RoleInfoWithPermissionsResponse,
+        UpdateRoleRequest,
     },
     AcceptInvitationRequest, AuthorizationInfoResponse, DeleteUserRoleRequest,
     TransferOrgOwnershipRequest, UpdateUserRoleRequest,
@@ -21,5 +22,6 @@ common_utils::impl_misc_api_event_type!(
     UpdateRoleRequest,
     ListRolesResponse,
     RoleInfoResponse,
-    GetRoleFromTokenResponse
+    GetRoleFromTokenResponse,
+    RoleInfoWithGroupsResponse
 );
diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs
index c3d534f8860..6aa226cd933 100644
--- a/crates/router/src/core/user_role/role.rs
+++ b/crates/router/src/core/user_role/role.rs
@@ -57,14 +57,18 @@ pub async fn create_role(
     state: AppState,
     user_from_token: UserFromToken,
     req: role_api::CreateRoleRequest,
-) -> UserResponse<()> {
+) -> UserResponse<role_api::RoleInfoWithGroupsResponse> {
     let now = common_utils::date_time::now();
-    let role_name = RoleName::new(req.role_name)?.get_role_name();
+    let role_name = RoleName::new(req.role_name)?;
 
-    if req.groups.is_empty() {
-        return Err(UserErrors::InvalidRoleOperation.into())
-            .attach_printable("Role groups cannot be empty");
-    }
+    utils::user_role::validate_role_groups(&req.groups)?;
+    utils::user_role::validate_role_name(
+        &state,
+        &role_name,
+        &user_from_token.merchant_id,
+        &user_from_token.org_id,
+    )
+    .await?;
 
     if matches!(req.role_scope, RoleScope::Organization)
         && user_from_token.role_id != consts::user_role::ROLE_ID_ORGANIZATION_ADMIN
@@ -73,19 +77,11 @@ pub async fn create_role(
             .attach_printable("Non org admin user creating org level role");
     }
 
-    utils::user_role::is_role_name_already_present_for_merchant(
-        &state,
-        &role_name,
-        &user_from_token.merchant_id,
-        &user_from_token.org_id,
-    )
-    .await?;
-
-    state
+    let role = state
         .store
         .insert_role(RoleNew {
             role_id: generate_id_with_default_len("role"),
-            role_name,
+            role_name: role_name.get_role_name(),
             merchant_id: user_from_token.merchant_id,
             org_id: user_from_token.org_id,
             groups: req.groups,
@@ -98,7 +94,14 @@ pub async fn create_role(
         .await
         .to_duplicate_response(UserErrors::RoleNameAlreadyExists)?;
 
-    Ok(ApplicationResponse::StatusOk)
+    Ok(ApplicationResponse::Json(
+        role_api::RoleInfoWithGroupsResponse {
+            groups: role.groups,
+            role_id: role.role_id,
+            role_name: role.role_name,
+            role_scope: role.scope,
+        },
+    ))
 }
 
 // TODO: To be deprecated once groups are stable
@@ -260,15 +263,11 @@ pub async fn update_role(
     user_from_token: UserFromToken,
     req: role_api::UpdateRoleRequest,
     role_id: &str,
-) -> UserResponse<()> {
-    let role_name = req
-        .role_name
-        .map(RoleName::new)
-        .transpose()?
-        .map(RoleName::get_role_name);
+) -> UserResponse<role_api::RoleInfoWithGroupsResponse> {
+    let role_name = req.role_name.map(RoleName::new).transpose()?;
 
     if let Some(ref role_name) = role_name {
-        utils::user_role::is_role_name_already_present_for_merchant(
+        utils::user_role::validate_role_name(
             &state,
             role_name,
             &user_from_token.merchant_id,
@@ -277,6 +276,10 @@ pub async fn update_role(
         .await?;
     }
 
+    if let Some(ref groups) = req.groups {
+        utils::user_role::validate_role_groups(groups)?;
+    }
+
     let role_info = roles::RoleInfo::from_role_id(
         &state,
         role_id,
@@ -290,23 +293,16 @@ pub async fn update_role(
         && user_from_token.role_id != consts::user_role::ROLE_ID_ORGANIZATION_ADMIN
     {
         return Err(UserErrors::InvalidRoleOperation.into())
-            .attach_printable("Non org admin user creating org level role");
+            .attach_printable("Non org admin user changing org level role");
     }
 
-    if let Some(ref groups) = req.groups {
-        if groups.is_empty() {
-            return Err(UserErrors::InvalidRoleOperation.into())
-                .attach_printable("role groups cannot be empty");
-        }
-    }
-
-    state
+    let updated_role = state
         .store
         .update_role_by_role_id(
             role_id,
             RoleUpdate::UpdateDetails {
                 groups: req.groups,
-                role_name,
+                role_name: role_name.map(RoleName::get_role_name),
                 last_modified_at: common_utils::date_time::now(),
                 last_modified_by: user_from_token.user_id,
             },
@@ -316,5 +312,12 @@ pub async fn update_role(
 
     blacklist::insert_role_in_blacklist(&state, role_id).await?;
 
-    Ok(ApplicationResponse::StatusOk)
+    Ok(ApplicationResponse::Json(
+        role_api::RoleInfoWithGroupsResponse {
+            groups: updated_role.groups,
+            role_id: updated_role.role_id,
+            role_name: updated_role.role_name,
+            role_scope: updated_role.scope,
+        },
+    ))
 }
diff --git a/crates/router/src/services/authorization/roles/predefined_roles.rs b/crates/router/src/services/authorization/roles/predefined_roles.rs
index 9accf094ea4..568af6e19fd 100644
--- a/crates/router/src/services/authorization/roles/predefined_roles.rs
+++ b/crates/router/src/services/authorization/roles/predefined_roles.rs
@@ -26,7 +26,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::OrganizationManage,
             ],
             role_id: consts::user_role::ROLE_ID_INTERNAL_ADMIN.to_string(),
-            role_name: "Internal Admin".to_string(),
+            role_name: "internal_admin".to_string(),
             scope: RoleScope::Organization,
             is_invitable: false,
             is_deletable: false,
@@ -46,7 +46,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_INTERNAL_VIEW_ONLY_USER.to_string(),
-            role_name: "Internal View Only".to_string(),
+            role_name: "internal_view_only".to_string(),
             scope: RoleScope::Organization,
             is_invitable: false,
             is_deletable: false,
@@ -73,7 +73,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::OrganizationManage,
             ],
             role_id: consts::user_role::ROLE_ID_ORGANIZATION_ADMIN.to_string(),
-            role_name: "Organization Admin".to_string(),
+            role_name: "organization_admin".to_string(),
             scope: RoleScope::Organization,
             is_invitable: false,
             is_deletable: false,
@@ -100,7 +100,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsManage,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_ADMIN.to_string(),
-            role_name: "Admin".to_string(),
+            role_name: "admin".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -120,7 +120,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_VIEW_ONLY.to_string(),
-            role_name: "View Only".to_string(),
+            role_name: "view_only".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -139,7 +139,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_IAM_ADMIN.to_string(),
-            role_name: "IAM".to_string(),
+            role_name: "iam".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -159,7 +159,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsManage,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_DEVELOPER.to_string(),
-            role_name: "Developer".to_string(),
+            role_name: "developer".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -180,7 +180,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_OPERATOR.to_string(),
-            role_name: "Operator".to_string(),
+            role_name: "operator".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -198,7 +198,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_CUSTOMER_SUPPORT.to_string(),
-            role_name: "Customer Support".to_string(),
+            role_name: "customer_support".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs
index 8ae65ce86e0..bb83b82f2da 100644
--- a/crates/router/src/utils/user_role.rs
+++ b/crates/router/src/utils/user_role.rs
@@ -1,10 +1,12 @@
 use api_models::user_role as user_role_api;
+use common_enums::PermissionGroup;
 use error_stack::ResultExt;
 
 use crate::{
     core::errors::{UserErrors, UserResult},
     routes::AppState,
-    services::authorization::permissions::Permission,
+    services::authorization::{permissions::Permission, roles},
+    types::domain,
 };
 
 impl From<Permission> for user_role_api::Permission {
@@ -40,23 +42,44 @@ impl From<Permission> for user_role_api::Permission {
     }
 }
 
-pub async fn is_role_name_already_present_for_merchant(
+pub fn validate_role_groups(groups: &[PermissionGroup]) -> UserResult<()> {
+    if groups.is_empty() {
+        return Err(UserErrors::InvalidRoleOperation.into())
+            .attach_printable("Role groups cannot be empty");
+    }
+
+    if groups.contains(&PermissionGroup::OrganizationManage) {
+        return Err(UserErrors::InvalidRoleOperation.into())
+            .attach_printable("Organization manage group cannot be added to role");
+    }
+
+    Ok(())
+}
+
+pub async fn validate_role_name(
     state: &AppState,
-    role_name: &str,
+    role_name: &domain::RoleName,
     merchant_id: &str,
     org_id: &str,
 ) -> UserResult<()> {
-    let role_name_list: Vec<String> = state
+    let role_name_str = role_name.clone().get_role_name();
+
+    let is_present_in_predefined_roles = roles::predefined_roles::PREDEFINED_ROLES
+        .iter()
+        .any(|(_, role_info)| role_info.get_role_name() == role_name_str);
+
+    // TODO: Create and use find_by_role_name to make this efficient
+    let is_present_in_custom_roles = state
         .store
         .list_all_roles(merchant_id, org_id)
         .await
         .change_context(UserErrors::InternalServerError)?
         .iter()
-        .map(|role| role.role_name.to_owned())
-        .collect();
+        .any(|role| role.role_name == role_name_str);
 
-    if role_name_list.contains(&role_name.to_string()) {
+    if is_present_in_predefined_roles || is_present_in_custom_roles {
         return Err(UserErrors::RoleNameAlreadyExists.into());
     }
+
     Ok(())
 }

## Type of Change
<!-- Put an `x` in the boxes that apply -->

- [ ] Bugfix
- [ ] New feature
- [ ] Enhancement
- [x] Refactoring
- [ ] Dependency updates
- [ ] Documentation
- [ ] CI/CD

## Description
<!-- Describe your changes in detail -->
This PR modifies create and update role APIs. The APIs will now respond with the role created/updated respectively.
This PR also changes the names of predefined roles to snake case.

### Additional Changes

- [x] This PR modifies the API contract
- [ ] This PR modifies the database schema
- [ ] This PR modifies application configuration/environment variables

<!--
Provide links to the files with corresponding changes.

Following are the paths where you can find config files:
1. `config`
2. `crates/router/src/configs`
3. `loadtest/config`
-->


## Motivation and Context
<!--
Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

If you don't have an issue, we'd recommend starting with one first so the PR
can focus on the implementation (unless it is an obvious bug or documentation fix
that will have little conversation).
-->
closes #3895 

## How did you test it?
<!--
Did you write an integration/unit/API test to verify the code changes?
Or did you test this change manually (provide relevant screenshots)?
-->
1. Create role
	- `organization_manage` cannot be sent in groups.
	- `role_name` should not be present in any other roles that merchant has access to.
	- Non `org_admin` won't be able to create a role with `role_scope` as `organization`
	- `groups` should not be empty.
	```
	curl --location 'http://localhost:8080/user/role' \
	--header 'Content-Type: application/json' \
	--header 'Authorization: Bearer JWT' \
	--data '{
	  "role_name": "admin",
	  "groups": ["users_manage"],
	  "role_scope": "organization"
	}'
	```
	- Response:
		```
		{
		    "role_id": "role_KLw0K0psguhVX8FhvIuv",
		    "groups": [
		        "users_manage"
		    ],
		    "role_name": "admin1",
		    "role_scope": "organization"
		}
		```
2. Update role
	- `organization_manage` cannot be sent in groups.
	- `role_name` should not be present in any other roles that merchant has access to.
	- Non `org_admin` won't be able to update a role with `role_scope` as `organization`
	- `groups` should not be empty.
	```
	curl --location 'http://localhost:8080/user/role/role_KLw0K0psguhVX8FhvIuv' \
	--header 'Content-Type: application/json' \
	--header 'Authorization: Bearer JWT' \
	--data '{
	  "groups": ["users_view"]
	}'
	```
	- Response:
		```
		{
		    "role_id": "role_KLw0K0psguhVX8FhvIuv",
		    "groups": [
		        "users_view"
		    ],
		    "role_name": "admin1",
		    "role_scope": "organization"
		}
		```

## Checklist
<!-- Put an `x` in the boxes that apply -->

- [x] I formatted the code `cargo +nightly fmt --all`
- [x] I addressed lints thrown by `cargo clippy`
- [x] I reviewed the submitted code
- [ ] I added unit tests for my changes where possible
- [ ] I added a [CHANGELOG](/CHANGELOG.md) entry if applicable
