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

feat(assign-dropdown): add role filtering for assignees #2860

Merged
merged 6 commits into from
Nov 21, 2024
Merged

Conversation

alonp99
Copy link
Collaborator

@alonp99 alonp99 commented Nov 20, 2024

  • Introduce filterUsersByRole utility to exclude assignees by roles
  • Update AssignDropdown and AlertsAssignDropdown to utilize filtering
  • Set default excluded role to 'viewer' in relevant components

(Your role filtering is so selective, it's like a bouncer at a VIP club)

Summary by CodeRabbit

  • New Features

    • Introduced an excludedRoles property to the AssignDropdown and AlertsAssignDropdown components, allowing users to filter out specific roles from the dropdown options.
    • Enhanced filtering logic to prioritize authenticated users while excluding specified roles from the assignee list.
    • Added a roles property to the UsersListSchema validation schema for improved user role validation.
  • Bug Fixes

    • Improved the handling of user roles in the dropdown components to ensure accurate filtering.
  • Documentation

    • Updated prop signatures to reflect the addition of the excludedRoles property in relevant components.

- Introduce filterUsersByRole utility to exclude assignees by roles
- Update AssignDropdown and AlertsAssignDropdown to utilize filtering
- Set default excluded role to 'viewer' in relevant components

(Your role filtering is so selective, it's like a bouncer at a VIP club)
Copy link
Contributor

coderabbitai bot commented Nov 20, 2024

Walkthrough

The changes involve modifications to the AssignDropdown, AlertsAssignDropdown, and the introduction of a new utility function filterUsersByRole. The AssignDropdown component now accepts an optional excludedRoles property to filter out specific user roles. The filterUsersByRole function filters users based on their roles, returning those who do not have any excluded roles. The AlertsAssignDropdown component also implements the new filtering logic, ensuring that the dropdown options reflect the updated role-based filtering.

Changes

File Change Summary
apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx Updated assignees type in IAssignDropdownProps, added excludedRoles, and modified filtering logic.
apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts Introduced filterUsersByRole function to filter users based on excluded roles and added type TUserRole.
apps/backoffice-v2/src/pages/Entity/components/Case/Case.Actions.tsx Updated AssignDropdown to include excludedRoles prop with value ['viewer'].
apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx Added optional excludedRoles prop and updated logic to filter assignees using filterUsersByRole.
apps/backoffice-v2/src/domains/users/validation-schemas.ts Added roles property to UsersListSchema to validate user roles.
services/workflows-service/src/user/user.controller.internal.ts Updated UserControllerInternal to include roles in the select clause for list and create methods.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant AssignDropdown
    participant FilterUtility

    User->>AssignDropdown: Selects roles to exclude
    AssignDropdown->>FilterUtility: filterUsersByRole(assignees, excludedRoles)
    FilterUtility-->>AssignDropdown: Returns filtered users
    AssignDropdown->>User: Displays filtered dropdown options
Loading

🐰 "In the dropdown, roles we can exclude,
To make selections, oh so shrewd!
With filters in place, choices abound,
A hop and a skip, the best roles found!
Let’s celebrate this change with glee,
For happy users, just wait and see!" 🐇

Possibly related PRs

  • Backoffice UI improvements #2827: The changes in Case.Actions.tsx include the addition of the excludedRoles prop to the AssignDropdown component, which is directly related to the modifications made in the main PR that also introduces the excludedRoles property in the AssignDropdown component.

Suggested reviewers

  • MatanYadaev

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 56ca421 and 0f484db.

📒 Files selected for processing (1)
  • apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

changeset-bot bot commented Nov 20, 2024

⚠️ No Changeset found

Latest commit: 0f484db

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts (1)

9-15: Improve error handling and type safety

The current implementation has several areas for improvement:

  1. No validation for excludedRoles array
  2. Redundant Array.isArray check
  3. Implicit type handling for roles property

Consider this improved implementation:

 export const filterUsersByRole = (
   users: Array<Partial<TAuthenticatedUser>>,
   excludedRoles: TUserRole[],
 ) => {
+  if (!Array.isArray(excludedRoles)) {
+    throw new Error('excludedRoles must be an array');
+  }
+
   return users.filter(user => {
     if (!user) return false;
 
-    if (!('roles' in user)) return true;
+    const userRoles = user.roles;
+    if (!userRoles) return true;
 
-    return !excludedRoles.some(role => Array.isArray(user.roles) && user.roles.includes(role));
+    return !excludedRoles.some(role => userRoles.includes(role));
   });
 };
apps/backoffice-v2/src/pages/Entity/components/Case/Case.Actions.tsx (1)

61-61: Consider extracting the hardcoded role value to a constant.

While the implementation is correct, the hardcoded string 'viewer' should be extracted to a constant or configuration value for better maintainability and reusability.

+ const EXCLUDED_ROLES = ['viewer'] as const;
  
  export const Actions: FunctionComponent<IActionsProps> = ({
    // ...
    <AssignDropdown
      // ...
-     excludedRoles={['viewer']}
+     excludedRoles={EXCLUDED_ROLES}
    />
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c25d4a3 and b573a23.

📒 Files selected for processing (4)
  • apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx (3 hunks)
  • apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts (1 hunks)
  • apps/backoffice-v2/src/pages/Entity/components/Case/Case.Actions.tsx (1 hunks)
  • apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx (1 hunks)
🔇 Additional comments (11)
apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts (2)

5-16: Verify integration points and similar utilities

Let's ensure this utility doesn't duplicate existing functionality and is properly integrated.

✅ Verification successful

Let me try again with corrected commands to gather the necessary information.


Let me check the implementation of the components using this utility to understand the integration better.


Function is properly integrated and no duplicate utilities exist

The filterUsersByRole utility is:

  • Used consistently in both AssignDropdown components for filtering assignees based on roles
  • Integrated correctly with proper type handling and memoization
  • No duplicate or similar role-filtering utilities exist in the codebase
  • Implementation follows the same pattern in both components, suggesting standardized usage
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for similar filtering utilities
echo "Searching for similar role filtering utilities..."
rg -t typescript "filter.*Role|filterBy.*Role|filterUser.*Role"

# Check usage of the new function
echo "Checking usage of filterUsersByRole..."
rg -t typescript "filterUsersByRole"

# Look for other role-based filtering patterns
echo "Checking existing role-based filtering patterns..."
ast-grep --pattern 'filter($$$).some(role => $$$)'

Length of output: 532


Script:

#!/bin/bash
# Check for similar filtering utilities
echo "Searching for similar role filtering utilities..."
rg "filter.*Role|filterBy.*Role|filterUser.*Role"

# Check usage of the new function
echo "Checking usage of filterUsersByRole..."
rg "filterUsersByRole"

# Look for other role-based filtering patterns
echo "Checking for similar array filtering patterns..."
rg "\.filter.*\.some.*role"

Length of output: 2127


Script:

#!/bin/bash
# Check the implementation of AssignDropdown components
echo "Checking AssignDropdown implementations..."
cat "apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx"
cat "apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx"

Length of output: 6616


3-3: 🛠️ Refactor suggestion

Consider expanding TUserRole type definition

The current type definition seems too restrictive by only including 'viewer'. Consider expanding it to include other possible roles in the system for better type safety and future extensibility.

-export type TUserRole = 'viewer';
+export type TUserRole = 'viewer' | 'admin' | 'manager'; // Add other relevant roles

Let's verify what other roles exist in the system:

apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx (5)

6-6: LGTM! Import changes are well-organized.

The new utility import follows the project's domain-driven structure.


14-15: LGTM! Interface changes maintain backward compatibility.

The optional excludedRoles prop with proper typing ensures type safety while maintaining compatibility with existing usage.


23-28: LGTM! Sorting logic properly integrated with filtering.

The changes maintain the existing sorting behavior while correctly incorporating the new filtered assignees list. The dependency array is properly updated to reflect the new dependencies.


15-15: Verify default role exclusion implementation.

The PR summary mentions setting a default excluded role to 'viewer', but this component uses an empty array as default. Verify if this is intentional or if the default should be set here.

✅ Verification successful

Empty default for excludedRoles is correct

The empty array default for excludedRoles in AlertsAssignDropdown is intentional. The component is used in two different contexts:

  1. In AlertsHeader.tsx where no role exclusion is needed (uses default empty array)
  2. In Case.Actions.tsx where 'viewer' role is explicitly excluded

This flexible implementation allows the component to be reused in different contexts with different role exclusion requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other instances of AlertsAssignDropdown usage to verify role exclusion
rg -t typescript "AlertsAssignDropdown.*excludedRoles" -A 3

# Search for similar components that might implement the default viewer role
rg -t typescript "excludedRoles.*=.*\['viewer'\]" -A 3

Length of output: 194


Script:

#!/bin/bash
# Search for AlertsAssignDropdown usage with excludedRoles
rg "AlertsAssignDropdown.*excludedRoles"

# Search for similar components with default viewer role
rg "excludedRoles.*=.*\['viewer'\]"

# Look for the component's usage in the codebase
rg "<AlertsAssignDropdown"

# Check PR summary or related configuration files
fd "alerts.*config" --type f --exec cat {}

Length of output: 396


Script:

#!/bin/bash
# Let's check the AlertsHeader implementation to see how excludedRoles is used
cat "apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsHeader/AlertsHeader.tsx"

# Also check the Case.Actions.tsx where we found the default viewer role
cat "apps/backoffice-v2/src/pages/Entity/components/Case/Case.Actions.tsx"

Length of output: 9334


16-19: Verify type compatibility between TUsers and TAuthenticatedUser.

The type casting assignees as Array<Partial<TAuthenticatedUser>> suggests a potential type mismatch. This could lead to runtime errors if the types are incompatible.

✅ Verification successful

Based on the gathered information, I can now provide a final response:

Type casting is safe and intentional in this context.

The type casting from TUsers to Array<Partial<TAuthenticatedUser>> is valid because:

  1. TUsers is defined as a Zod array schema that includes all the fields required by TAuthenticatedUser (id, fullName, avatarUrl)
  2. The filterUsersByRole function explicitly expects Array<Partial<TAuthenticatedUser>> and is used consistently across the codebase
  3. The same pattern is used in multiple components (AlertsAssignDropdown, AssignDropdown) indicating this is an established pattern
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for TUsers type definition to verify compatibility
ast-grep --pattern 'type TUsers = $_'

# Search for TAuthenticatedUser usage in similar contexts
rg -t typescript "TAuthenticatedUser" -A 3

Length of output: 16296


Script:

#!/bin/bash
# Search for TAuthenticatedUser type definition
ast-grep --pattern 'type TAuthenticatedUser = $_'

# Search for TUsers type definition and usage
rg "TUsers" -A 3 --type ts

# Search for filterUsersByRole function to understand the expected type
ast-grep --pattern 'filterUsersByRole($_)'

Length of output: 21428


Script:

#!/bin/bash
# Search for UsersListSchema to understand TUsers type definition
ast-grep --pattern 'const UsersListSchema = $_'

# Search for filterUsersByRole implementation
rg "filterUsersByRole" -B 2 -A 5 --type ts

Length of output: 23044

apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx (3)

10-10: LGTM! Clean interface extension

The new import and interface changes are well-structured and properly typed.

Also applies to: 20-20


37-43: LGTM! Proper memoization maintained

The sorting logic is correctly updated to work with filtered assignees while maintaining performance through proper memoization.


10-10: Verify the filterUsersByRole implementation

Let's ensure the filtering utility is properly implemented and consistently used across the codebase.

Also applies to: 32-32

✅ Verification successful

Implementation verified and properly used

The filterUsersByRole utility is correctly implemented and used consistently across the codebase. It is:

  • Properly exported and typed with TUserRole
  • Used in both AssignDropdown and AlertsAssignDropdown components with consistent parameter types
  • Implements safe null checks and role filtering logic
  • Correctly handles edge cases (missing roles property, null users)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the implementation of filterUsersByRole
echo "Checking filterUsersByRole implementation:"
ast-grep --pattern 'export const filterUsersByRole = $_' -A 10

# Check for other usages of the utility
echo "\nChecking other usages of filterUsersByRole:"
rg "filterUsersByRole" --type typescript

Length of output: 71615


Script:

#!/bin/bash
# Let's check the implementation and usages with ripgrep
echo "Checking filterUsersByRole implementation and usages:"
rg "filterUsersByRole" -t ts -A 5 -B 5

# Also check for the file content directly
echo "\nChecking the specific file content:"
cat apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts

Length of output: 7926

apps/backoffice-v2/src/pages/Entity/components/Case/Case.Actions.tsx (1)

61-61: Verify type safety and integration.

Let's verify the type safety of the new prop and its integration with the existing assignment logic.

✅ Verification successful

Type safety and integration are properly implemented

The excludedRoles prop is correctly typed and integrated:

  • The AssignDropdown component properly defines excludedRoles?: TUserRole[] in its props interface
  • TUserRole type explicitly includes 'viewer' as a valid role
  • The filterUsersByRole utility function correctly handles the excluded roles filtering
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify type definitions and usage patterns

# Check if AssignDropdown component's props interface includes excludedRoles
ast-grep --pattern 'interface $interface {
  $$$
  excludedRoles?: string[];
  $$$
}'

# Check for any other usages of AssignDropdown with excludedRoles
rg -A 2 '<AssignDropdown.*excludedRoles'

# Check for role-related type definitions
ast-grep --pattern 'type $type = $$$"viewer"$$$'

Length of output: 181


Script:

#!/bin/bash
# Let's try a different approach to verify the type safety

# First, let's find the AssignDropdown component definition
fd -e tsx -e ts "AssignDropdown"

# Then, let's look for role-related type definitions or constants
rg -l "type.*Role|enum.*Role|roles.*=.*viewer"

# Finally, let's check the specific file for the component's usage context
cat "apps/backoffice-v2/src/pages/Entity/components/Case/Case.Actions.tsx"

Length of output: 5025


Script:

#!/bin/bash
# Let's check the AssignDropdown component definition to verify type safety
cat "apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx"

# Let's also check if there are any role-related utilities or types that might be used
cat "apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts"

Length of output: 4030

- Add check for users input to ensure it is an array
- Update validation schema to include roles as an optional array

(your input validation is so lax, it might as well accept a pizza order)
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
apps/backoffice-v2/src/domains/users/validation-schemas.ts (1)

15-15: LGTM! Consider adding documentation for the roles field.

The implementation is flexible, allowing both predefined and custom roles. However, adding JSDoc comments explaining the role values and their implications would improve maintainability.

+      /** 
+       * User roles that determine access levels and filtering behavior.
+       * Predefined values: 'viewer', 'admin'
+       * Custom string roles are also supported for extensibility.
+       */
       roles: z.array(z.union([z.enum(['viewer', 'admin']), z.string()])).optional(),
services/workflows-service/src/user/user.controller.internal.ts (2)

Line range hint 1-64: LGTM with suggestions for role-based access control

The changes appropriately support the new role filtering feature while maintaining existing security controls. Consider documenting the role-based access control (RBAC) design decisions in the API documentation for future maintainability.

Consider implementing a dedicated RBAC service or middleware to centralize role-based access control logic as the application's role-based features grow.


Validation for roles property needs improvement

Based on the code analysis:

  • The UserCreateDto has a roles property typed as InputJsonValue with basic array validation (@IsArray() and @IsString({ each: true }))
  • There's no enum-based validation or specific role value constraints
  • The type InputJsonValue is just an alias for non-null JSON values, providing no role-specific validation

Recommendations:

  • Define an enum or constant for valid role values
  • Add enum-based validation using @IsEnum() or @IsIn() decorators
  • Consider using a more specific type than InputJsonValue for roles
🔗 Analysis chain

Line range hint 46-64: Verify role validation in UserCreateDto

The create method now returns role information, but we should ensure proper validation exists in UserCreateDto to prevent invalid role assignments.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for role validation in DTOs and related files

# Look for role validation in DTOs
rg -A 5 'class.*UserCreateDto'

# Check for role-related validation pipes or decorators
rg -A 5 '@IsEnum.*Role|@IsIn.*Role'

Length of output: 2118


Script:

#!/bin/bash
# Let's check the content of UserCreateDto and related files to understand role handling

# Check the complete UserCreateDto implementation
cat services/workflows-service/src/user/dtos/user-create.ts

# Look for role-related types or enums
rg -l "enum.*Role|type.*Role"

# Search for role-related validation or transformation logic in pipes
rg -l "Role.*Pipe|Transform.*Role"

Length of output: 1424


Script:

#!/bin/bash
# Let's check the role type definition and validation

# Check the InputJsonValue type definition
cat services/workflows-service/src/types.ts

# Look for role constants or enums in the codebase
rg -A 5 "const.*Role.*=|enum.*Role.*{" 

# Check the user service implementation for role handling
cat services/workflows-service/src/user/user.service.ts

Length of output: 3402


Script:

#!/bin/bash
# Let's check the role constants and repository implementation

# Fix the role pattern search
rg -A 5 "enum Role|const.*ROLE"

# Check the user repository implementation
cat services/workflows-service/src/user/user.repository.ts

# Check for role-related constants
rg -A 5 "roles.*=.*\[|ROLES.*=.*\{"

Length of output: 3901

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b573a23 and d5fcca7.

📒 Files selected for processing (3)
  • apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts (1 hunks)
  • apps/backoffice-v2/src/domains/users/validation-schemas.ts (1 hunks)
  • services/workflows-service/src/user/user.controller.internal.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts
🔇 Additional comments (2)
apps/backoffice-v2/src/domains/users/validation-schemas.ts (1)

15-15: Verify role values consistency across the codebase.

Let's ensure the predefined role values ('viewer', 'admin') are consistently used throughout the codebase.

✅ Verification successful

Let me gather more information about role definitions and enums in the codebase.


Let me try one more search to find all role-related type definitions and usages.


Based on the gathered information, I can now provide a final response:

Role values are consistent but used in different contexts

The codebase shows two distinct role systems that don't conflict with each other:

  1. User system roles ('viewer', 'admin') in apps/backoffice-v2/src/domains/users/validation-schemas.ts
  2. Profile-related roles ('ubo', 'director', 'representative') in apps/backoffice-v2/src/pages/Profiles/Individuals/components/ProfilesTable/columns.tsx

The 'viewer' and 'admin' roles in the validation schema are correctly defined and consistently used for user authentication/authorization purposes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for inconsistencies in role values across the codebase

# Search for role-related string literals
echo "Searching for role definitions and usage..."
rg -g '!*.{json,lock}' -A 2 "('viewer'|'admin'|role.*enum)" --type ts

# Search for potential role definitions in constants or enums
echo "Searching for role enums or constants..."
ast-grep --pattern 'enum $_ {
  $$$
  viewer = $_,
  $$$
}'

Length of output: 4366


Script:

#!/bin/bash
# Search for role-related enums and type definitions
echo "Searching for role-related type definitions..."
ast-grep --pattern 'type $_ = $$$"viewer"$$$'

echo "Searching for role constants..."
rg -g '!*.{json,lock}' "(?:const|enum).+(?:Role|ROLE)" --type ts -A 3

echo "Searching for role usage in schemas..."
rg -g '!*.{json,lock}' "z\.enum\(\[.*role.*\]\)" --type ts -A 2

Length of output: 20630


Script:

#!/bin/bash
# Search for all role-related definitions and usages
echo "Searching for role-related code..."
rg -g '!*.{json,lock}' "(?i)role.*=.*(?:'viewer'|'admin')|type.*Role.*=|interface.*Role.*{" --type ts -A 3

echo "Searching for role-related imports and exports..."
rg -g '!*.{json,lock}' "(?i)import.*Role|export.*Role" --type ts -A 1

Length of output: 3210

services/workflows-service/src/user/user.controller.internal.ts (1)

39-39: Verify security implications of exposing user roles

The addition of roles to the select clause in a non-admin-guarded endpoint needs security review. While this aligns with the PR's role filtering objectives, consider whether exposing role information to all authenticated users follows security best practices.

✅ Verification successful

Let me gather more information about the specific endpoint and its usage context.


Let me try one more verification to check the list endpoint implementation and its context.


Exposing roles in list endpoint is acceptable for this internal API

Based on the verification results, the security concern is addressed by the following findings:

  • The controller is marked with @swagger.ApiExcludeController() and is in the internal namespace, indicating it's not publicly exposed
  • The list endpoint already implements proper access control by filtering users based on project IDs
  • The roles field exposure aligns with the PR's objective for role-based filtering
  • The create endpoint that also exposes roles is properly guarded with @UseGuards(AdminAuthGuard)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for role-based access controls and usage patterns

# Check if roles are exposed in other public endpoints
ast-grep --pattern 'roles: true' 

# Check for any existing role-based security patterns
rg -A 5 '@UseGuards.*Admin.*Guard'

Length of output: 3305


Script:

#!/bin/bash
# Check the list endpoint implementation and its guards
ast-grep --pattern 'async list($$$) {
  $$$
}'  services/workflows-service/src/user/user.controller.internal.ts

# Check for any auth guards on the controller class level
rg -B 5 'export class.*Controller' services/workflows-service/src/user/user.controller.internal.ts

# Check if roles are used in any filtering logic
rg -A 5 'roles.*filter' services/workflows-service/src/user

Length of output: 460


Script:

#!/bin/bash
# Check the entire user controller file content to understand the context better
cat services/workflows-service/src/user/user.controller.internal.ts

# Check for any role-based filtering in the service layer
rg -A 5 'findMany.*where.*role' services/workflows-service/src/user

Length of output: 2273

- Change assignee prop type from TAssignee to TAuthenticatedUser
- Streamline filtering logic in both AssignDropdown and AlertsAssignDropdown

(Your code's type definitions are so convoluted, they could use a user manual)
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx (2)

31-34: Add runtime safety checks for assignees array.

The filtering logic assumes all items in assignees array are valid objects. This could cause runtime errors if the array contains null/undefined values.

Consider adding runtime safety:

   const filteredAssignees = useMemo(
-    () => filterUsersByRole(assignees, excludedRoles),
+    () => filterUsersByRole(assignees?.filter(Boolean) ?? [], excludedRoles),
     [assignees, excludedRoles],
   );

38-42: Simplify array operations and sorting logic.

The current implementation uses redundant optional chaining and could be simplified for better readability.

Consider this cleaner implementation:

-      filteredAssignees
-        ?.slice()
-        ?.sort((a, b) =>
-          a?.id === authenticatedUserId ? -1 : b?.id === authenticatedUserId ? 1 : 0,
-        ),
+      [...(filteredAssignees ?? [])].sort((a, b) => 
+        Number(b?.id === authenticatedUserId) - Number(a?.id === authenticatedUserId)
+      ),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d5fcca7 and 56ca421.

📒 Files selected for processing (2)
  • apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx (3 hunks)
  • apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx (1 hunks)
🔇 Additional comments (5)
apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx (4)

15-18: LGTM! Well-structured filtering implementation

The filtering logic is efficiently implemented using useMemo with appropriate dependencies.


6-6: Verify role validation in filterUsersByRole utility

Since this component handles user roles, we should ensure proper role validation is implemented in the utility function.

✅ Verification successful

Role validation implementation is secure and properly implemented

The filterUsersByRole utility implements proper role validation with the following security measures:

  • Input validation checking for array type and null/undefined users
  • Safe handling of users without roles property
  • Proper array checking before role comparison
  • Correct implementation of role exclusion logic using some and includes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check implementation of filterUsersByRole
ast-grep --pattern 'export const filterUsersByRole = ($params) => {
  $$$
}'

Length of output: 78


Script:

#!/bin/bash
# Let's try a different approach to find the filterUsersByRole implementation
# First, let's search for the file containing the function
fd "filter-users-by-role" --type f

# Then let's examine its content
rg "filterUsersByRole" -A 10

Length of output: 6785


Script:

#!/bin/bash
# Let's examine the complete implementation of filter-users-by-role.ts
cat apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts

Length of output: 534


21-27: LGTM! Sorting behavior preserved with filtered assignees

The changes correctly maintain the authenticated user prioritization while working with the filtered list.

✅ Verification successful

The previous pattern might have failed due to syntax. Let's try these alternative approaches to verify the sorting behavior consistency.


Let's try to locate and examine the specific file and its sorting implementation directly.


Sorting implementation verified and correctly implemented

The sorting logic in AlertsAssignDropdown.tsx is properly implemented to prioritize the authenticated user by:

  • Using optional chaining for safe property access
  • Placing authenticated user first (returning -1 when matching)
  • Correctly depending on both filteredAssignees and authenticatedUserId
  • Maintaining a stable sort for non-authenticated users (returning 0)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for consistent sorting behavior across components
ast-grep --pattern 'sort((a, b) => 
  $$$authenticatedUserId$$$
)'

Length of output: 68


Script:

#!/bin/bash
# Let's try a different pattern to find sorting implementations
ast-grep --pattern 'sort((a, b) => a?.id === $_ ? -1 : b?.id === $_ ? 1 : 0)'

# Also search for similar sorting patterns using ripgrep
rg "sort\(\(.*\)\s*=>\s*.*authenticatedUserId" -A 2

Length of output: 133


Script:

#!/bin/bash
# Search for the file and its history to understand the sorting implementation
fd "AlertsAssignDropdown.tsx" --type f

# Look for any sorting implementations in this specific file
rg "sort\(" --context 3 apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx

Length of output: 512


13-14: Consider setting default excludedRoles to ['viewer']

The PR objective mentions defaulting to excluding 'viewer' role, but the component sets an empty array as default. Consider aligning with the intended behavior.

-}> = ({ assignees, authenticatedUserId, isDisabled, onAssigneeSelect, excludedRoles = [] }) => {
+}> = ({ assignees, authenticatedUserId, isDisabled, onAssigneeSelect, excludedRoles = ['viewer'] }) => {
apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx (1)

10-10: Verify the filterUsersByRole utility implementation.

Let's ensure the utility function is properly implemented and consistently used across the codebase.

✅ Verification successful

The filterUsersByRole utility is properly implemented and used consistently

The utility is correctly implemented in apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts and is being used consistently in both:

  • apps/backoffice-v2/src/common/components/atoms/AssignDropdown/AssignDropdown.tsx
  • apps/backoffice-v2/src/pages/TransactionMonitoringAlerts/components/AlertsAssignDropdown/AlertsAssignDropdown.tsx

The implementation includes proper type safety with TUserRole type and handles edge cases appropriately.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the utility implementation and its usages
echo "Searching for filterUsersByRole implementation..."
ast-grep --pattern 'export const filterUsersByRole = $_'

echo "\nSearching for filterUsersByRole usages..."
rg "filterUsersByRole" --type typescript

Length of output: 17759


Script:

#!/bin/bash
# Let's try a different approach to find the implementation and usages

# Search for the file containing filterUsersByRole
echo "Searching for files containing filterUsersByRole..."
fd "filter-users-by-role" --type f

# Search for imports and usages
echo "\nSearching for filterUsersByRole usages..."
rg "filterUsersByRole" -t ts -t tsx

# Search for the type definition
echo "\nSearching for TUserRole definition..."
rg "type TUserRole" -t ts -t tsx

Length of output: 527


Script:

#!/bin/bash
# Let's examine the implementation and usages with different flags

# Check the implementation
echo "Examining filterUsersByRole implementation..."
cat "apps/backoffice-v2/src/domains/users/utils/filter-users-by-role.ts"

# Search for usages without file type flags
echo "\nSearching for filterUsersByRole usages..."
rg "filterUsersByRole"

# Search for type definition without file type flags
echo "\nSearching for TUserRole definition..."
rg "type TUserRole"

Length of output: 1751

…e.ts

Co-authored-by: Tomer Shvadron <tomers@ballerine.com>
@alonp99 alonp99 enabled auto-merge (squash) November 21, 2024 12:43
@alonp99 alonp99 merged commit ca5299c into dev Nov 21, 2024
10 checks passed
@alonp99 alonp99 deleted the BAL-3102 branch November 21, 2024 12:51
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 this pull request may close these issues.

2 participants