Add/Remove UBOs sign-off and pr comments #2915
Conversation
|
WalkthroughThe pull request introduces several enhancements across multiple applications and services, focusing on localization, UBO (Ultimate Beneficial Owners) management, and dependency updates. Key changes include adding a Changes
Suggested Reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
services/workflows-service/src/case-management/case-management.service.ts (1)
Based on the search results, I can now provide a final response:
Hardcoded workflow definition ID should be configurable
The hardcoded workflow definition ID
kyc_email_session_exampleappears to be used in test files and the actual implementation. While the codebase shows usage ofConfigServicein various places, there's no evidence of workflow definition IDs being configured through environment variables or configuration service.To improve maintainability and flexibility:
- Add a configuration entry in your environment configuration:
// config/workflow.config.ts export default () => ({ workflow: { uboWorkflowDefinitionId: process.env.UBO_WORKFLOW_DEFINITION_ID || 'kyc_email_session_example', }, });
- Update the case management service to use the configuration:
@Injectable() export class CaseManagementService { constructor( private readonly configService: ConfigService, // ... other dependencies ) {} async createUbo(/*...*/) { const workflowDefinitionId = this.configService.get<string>('workflow.uboWorkflowDefinitionId'); // ... rest of the method } }🔗 Analysis chain
Line range hint
102-144: Consider several improvements to the UBO creation logic.
- The workflow definition ID is hardcoded. Consider making it configurable through environment variables or configuration.
- The UBO input data lacks validation. Consider adding schema validation similar to the
validateEntitymethod.- The
uboparameter uses a loose type. Consider creating a strict interface for the UBO data structure.Here's a suggested approach for implementing these improvements:
// Create an interface for the UBO input interface UboInput { firstName: string; lastName: string; email: string; nationalId: string; ownershipPercentage?: number; percentageOfOwnership?: number; fullAddress?: string; nationality?: string; } // Add validation schema const uboSchema = { type: 'object', required: ['firstName', 'lastName', 'email', 'nationalId'], properties: { firstName: { type: 'string', minLength: 1 }, lastName: { type: 'string', minLength: 1 }, email: { type: 'string', format: 'email' }, nationalId: { type: 'string' }, ownershipPercentage: { type: 'number', minimum: 0, maximum: 100 }, percentageOfOwnership: { type: 'number', minimum: 0, maximum: 100 }, fullAddress: { type: 'string' }, nationality: { type: 'string' } } }; // Update the method signature and add validation async createUbo({ workflowId, ubo, projectId, }: { workflowId: string; ubo: UboInput; projectId: TProjectId; }) { // Validate UBO data const validate = ajv.compile(uboSchema); if (!validate(ubo)) { throw ValidationError.fromAjvError(validate.errors!); } // Rest of the method... const workflowDefinitionId = this.configService.get('UBO_WORKFLOW_DEFINITION_ID'); // ... }Let's verify the workflow definition ID usage across the codebase:
Hardcoded workflow ID should be configurable
The search results confirm that
kyc_email_session_exampleis used as a workflow definition ID across multiple test files and actual implementation. While it appears in test files which is expected, it's hardcoded in the actual implementation inservices/workflows-service/src/case-management/case-management.service.ts.Let's verify if there's any configuration mechanism already in place:
Let's try a different search strategy to understand the configuration patterns in the codebase:
Let's search for environment variables and configuration patterns related to workflow definitions:
Let's check for any configuration patterns in the environment files and workflow service:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded workflow definition IDs rg -A 2 "kyc_email_session_example"Length of output: 21881
Script:
#!/bin/bash # Search for workflow definition ID configuration rg -l "UBO_WORKFLOW_DEFINITION" # Search for workflow configuration patterns rg "workflowDefinitionId.*config"Length of output: 67
Script:
#!/bin/bash # Search for workflow configuration patterns in configuration files fd -e config.ts -e env -x cat {} # Search for workflow definition configuration patterns rg -A 3 "ConfigService|configuration" services/workflows-service/src/Length of output: 65848
Script:
#!/bin/bash # Search for workflow definition related environment variables rg -l "UBO_WORKFLOW|WORKFLOW_DEFINITION" services/workflows-service/ # Search for workflow definition configuration patterns in the case management service rg -A 3 "workflowDefinitionId.*=|const.*workflowDefinition" services/workflows-service/src/case-management/Length of output: 1251
Script:
#!/bin/bash # Search for environment files fd -e env -x cat {} # Search for workflow configuration patterns in workflow service rg -A 3 "workflowDefinitionId.*=|const.*workflow.*config" services/workflows-service/src/workflow/Length of output: 1919
🧹 Nitpick comments (8)
packages/ui/src/components/atoms/inputs/PhoneNumberInput/PhoneNumberInput.tsx (2)
31-31: Double-check the forced override usage.Using
!border-nonecan be effective, but it can also introduce unexpected specificity conflicts. Confirm that no other styles rely on maintaining a border in certain states.
35-35: Maintain consistency in border styling.The
!border-noneoverride on the button might conflict with the container’s border. Ensure consistent border handling across the container, input, and button for a seamless visual experience.apps/backoffice-v2/src/domains/ui-definition/hooks/queries/useTranslateUiDefinitionQuery/useTranslateUiDefinitionQuery.tsx (1)
8-12: Enhance usage documentation for locale parameter.Adding the
localeparameter is a great step for enabling internationalization. Consider documenting mandatory or optional usage details forlocaleand its expected values, to help future maintainers and callers of this hook.apps/backoffice-v2/src/domains/ui-definition/query-keys.ts (2)
8-12: Keep consistent naming.Including
localeis beneficial. However, ensure consistent naming across the codebase for parameters handling locale, e.g.locale,language, orlang, to avoid confusion.
19-19: Further refine the query key.Incorporating
localein the query key is correct. Make sure the locale doesn’t dramatically inflate caching if it can have many possible values. If that’s a concern, consider imposing constraints on valid locales or a fallback pattern.apps/backoffice-v2/src/domains/ui-definition/fetchers.ts (2)
11-15: Validate or document expected locale values.Including
localein the request is an excellent extension. It might be useful to validate or document that only specific locales (e.g., ISO 639-1 codes) are accepted by the endpoint to avoid potential server-side errors.
18-18: Consider fallback mechanism if translation fails.When building the endpoint with the
locale, consider providing a fallback or default locale if the server cannot handle specific locales. This helps avoid potential 404 or 500 responses when the locale is invalid.services/workflows-service/src/case-management/case-management.service.ts (1)
97-101: Enhance error message with more context.While the error handling is well-placed, consider making the error message more specific by including the workflow ID for better debugging.
- `Attempted to create a UBO to a parent workflow without a business`, + `Attempted to create a UBO for workflow '${workflowId}' which has no associated business`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/backoffice-v2/src/domains/ui-definition/fetchers.ts(1 hunks)apps/backoffice-v2/src/domains/ui-definition/hooks/queries/useTranslateUiDefinitionQuery/useTranslateUiDefinitionQuery.tsx(1 hunks)apps/backoffice-v2/src/domains/ui-definition/query-keys.ts(1 hunks)apps/backoffice-v2/src/domains/workflow-definitions/fetchers.ts(1 hunks)apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx(3 hunks)apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx(1 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)packages/react-pdf-toolkit/CHANGELOG.md(1 hunks)packages/react-pdf-toolkit/package.json(2 hunks)packages/ui/CHANGELOG.md(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/components/atoms/inputs/PhoneNumberInput/PhoneNumberInput.tsx(1 hunks)services/workflows-service/src/case-management/case-management.service.ts(1 hunks)services/workflows-service/src/workflow/schemas/zod-schemas.ts(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/ui/package.json
- packages/react-pdf-toolkit/CHANGELOG.md
- packages/ui/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- apps/kyb-app/package.json
- packages/react-pdf-toolkit/package.json
🔇 Additional comments (15)
packages/ui/src/components/atoms/inputs/PhoneNumberInput/PhoneNumberInput.tsx (1)
30-30: Ensure the container’s height remains consistent with the new input height.
You removed the h-9 class, which might affect component alignment if other parts of the UI expect this container’s height. Verify the visual consistency across all breakpoints and component states.
apps/backoffice-v2/src/domains/ui-definition/hooks/queries/useTranslateUiDefinitionQuery/useTranslateUiDefinitionQuery.tsx (1)
15-15: Confirm that all usage sites updated.
locale is now added to the query keys. Ensure all downstream calls and any parent hooks pass a valid locale or handle missing locales appropriately.
apps/backoffice-v2/src/domains/ui-definition/query-keys.ts (1)
22-22: Ensure robust error handling for localizations.
When calling translateUiDefinition, if a requested locale is unavailable, ensure that the server responds with either a default locale or a descriptive error, so that the UI can gracefully handle missing translations.
apps/backoffice-v2/src/domains/workflow-definitions/fetchers.ts (1)
59-67: Good addition for UBO creation configuration.
Defining ubos.create.enabled as an optional boolean is a flexible approach. If future expansions include additional UBO properties, consider grouping them under a deeper schema or separate domain object to keep the code organized and maintainable.
services/workflows-service/src/workflow/schemas/zod-schemas.ts (1)
84-92: Ensure consistent optional property usage.
Defining the ubos.create.enabled property as optional is a good approach for maintaining backward compatibility. The pattern is sound and aligns with the rest of the schema's optional properties.
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx (8)
28-28: Import statement LGTM.
This import statement is straightforward and does not introduce any issues.
30-36: Well-structured hook signature.
The new create property clearly conveys whether UBO creation is enabled. The destructuring approach is organized and readable.
44-44: Locale usage.
Using the useLocale hook to dynamically load translated forms is a clean approach to localization. Ensure adequate error or fallback handling in case of missing locales.
48-48: Proper usage of dynamic translation.
The code correctly supplies the locale to the query, providing a more flexible, multi-language setup.
131-131: Improved clarity in the removal confirmation message.
The updated removal message focuses on UBO data only, making the user flow more straightforward with less confusion about unused references.
216-217: Conditional rendering for creation.
Good job checking !create.enabled before showing the "Add UBO" button. This pattern ensures that the UI remains consistent with the configuration.
218-245: Manage UBO button logic.
The dialog logic for displaying and managing existing UBOs is well-structured. The fallback to the Add UBO button, if creation is enabled, maintains clear user flow.
246-271: Add UBO flow integrates smoothly.
Implementing the dynamic form is a smart approach. Consider adding validation messages or user-friendly error handling if submission fails.
apps/backoffice-v2/src/lib/blocks/components/KycBlock/hooks/useKycBlock/useKycBlock.tsx (1)
554-566: Contextual "Initiate KYC" call-to-action.
Placing the "Initiate KYC" button near the relevant data is intuitive for users. Maintain consistent styling and disable states to avoid accidental clicks when unavailable.
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (1)
342-347: Flexible UBO creation config.
Providing a default false value when enabled is undefined ensures the UI is robust. This pattern cleanly integrates with useManageUbosBlock.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx (2)
Line range hint
216-242: Consider using a more descriptive class name for the scroll areaWhile the functionality is correct, consider using a more semantic class name than
[&>div]:max-h-[73vh]to improve maintainability. Consider extracting this to a constant or CSS variable.- className: '[&>div]:max-h-[73vh]', + className: '[&>div]:max-h-[--ubo-table-max-height]',Add to your CSS variables:
:root { --ubo-table-max-height: 73vh; }
245-270: Maintain consistency in scroll area heightsThe ScrollArea height uses the same value (73vh) as the table. Consider using the same CSS variable suggested earlier for consistency.
- <ScrollArea orientation={'vertical'} className={'h-[73vh]'}> + <ScrollArea orientation={'vertical'} className={'h-[--ubo-table-max-height]'}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx(4 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (3)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx (3)
28-36: LGTM: Clean hook signature and import changes
The hook signature is well-typed with the new create configuration parameter, and the useLocale import is properly placed.
44-49: LGTM: Proper localization integration
The locale is correctly obtained from useLocale hook and properly passed to the translation query, enabling localized UBO forms.
130-132: LGTM: Clearer UBO removal confirmation message
The confirmation message has been simplified while maintaining clarity about the action's impact.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx (3)
55-61: Consider enhancing the success callbackWhile the state check is good, consider these improvements:
- Make the early return more explicit
- Add error handling for failed mutations
onSuccess: () => { if (!isAddingUbo) { - return; + return void 0; // explicit return } toggleOffIsAddingUbo(); + }, + onError: (error) => { + // Handle error appropriately + console.error('Failed to create UBO:', error); },
259-268: Consider extracting styles to constantsThe button styles contain magic strings that could be moved to constants for better maintainability.
+const BACK_BUTTON_STYLES = 'absolute left-4 top-4 rounded-sm p-0 opacity-70 transition-opacity d-4 hover:bg-transparent hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-slate-400 focus:ring-offset-2 data-[state=open]:bg-slate-100 dark:focus:ring-slate-400 dark:focus:ring-offset-slate-900 dark:data-[state=open]:bg-slate-800'; <Button type={'button'} variant={'ghost'} onClick={toggleOffIsAddingUbo} - className={ - 'absolute left-4 top-4 rounded-sm p-0 opacity-70 transition-opacity d-4 hover:bg-transparent hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-slate-400 focus:ring-offset-2 data-[state=open]:bg-slate-100 dark:focus:ring-slate-400 dark:focus:ring-offset-slate-900 dark:data-[state=open]:bg-slate-800' - } + className={BACK_BUTTON_STYLES} >
272-273: Consider making the viewport height configurableThe hardcoded
73vhvalue for the scroll area height could be made configurable for better reusability.+const SCROLL_AREA_HEIGHT = '73vh'; +const FORM_STYLES = '[&>div>fieldset>div:first-of-type]:py-0 [&>div]:py-0'; -<ScrollArea orientation={'vertical'} className={'h-[73vh]'}> +<ScrollArea orientation={'vertical'} className={`h-[${SCROLL_AREA_HEIGHT}]`}> <DynamicForm // ... - className={'[&>div>fieldset>div:first-of-type]:py-0 [&>div]:py-0'} + className={FORM_STYLES} />Also applies to: 279-279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx(6 hunks)
🔇 Additional comments (3)
apps/backoffice-v2/src/lib/blocks/hooks/useManageUbosBlock/useManageUbosBlock.tsx (3)
44-48: LGTM: Clean integration of localization support
The locale integration is well-implemented, using a centralized useLocale hook and properly passing the locale to the translation query.
Line range hint 227-254: LGTM: Well-structured conditional rendering
The conditional rendering based on create.enabled is well-implemented with proper accessibility attributes.
28-36: Verify all hook consumers for the breaking signature change
The hook signature has been modified to require a create parameter, which is a breaking change. Ensure all consumers of this hook have been updated accordingly.
✅ Verification successful
All consumers of useManageUbosBlock have been properly updated
The verification shows that there is only one consumer of the useManageUbosBlock hook in the codebase, located in useDefaultBlocksLogic.tsx. This consumer has been properly updated to provide the required create parameter with the enabled flag, sourcing its value from the workflow configuration:
useManageUbosBlock({
create: {
...workflow?.workflowDefinition?.config?.ubos?.create,
enabled: workflow?.workflowDefinition?.config?.ubos?.create?.enabled ?? false,
},
});The implementation correctly:
- Spreads any additional create configuration from the workflow
- Explicitly handles the
enabledflag with a fallback tofalse
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of useManageUbosBlock to verify they've been updated
ast-grep --pattern 'useManageUbosBlock($$$)'
Length of output: 956
* chore: update package versions and dependencies - Bump package versions across various modules - Update dependencies to the latest stable versions (With this many updates, your dependencies have more frequent flyer miles than I do) * feat(workflow-definition): add configuration examples for vendors - Include detailed configuration examples for various vendors - Improve clarity on vendor integration and usage (These examples are so detailed, they could serve as a user manual for a rocket) * bal 3191 (#2905) * refactor(merchant-monitoring): improve search and date filtering logic - Simplify search parameters handling across components - Integrate DateRangePicker for filtering reports by date range - Clean up redundant search schemas and unused imports (your code is now so tidy, it could host a top-tier cleaning service) * BAL 3197 - add merchant monitoring filters to UI (#2901) * feat(business-reports): add UI for filtering by merchant type - Update reportType to accept 'All' alongside existing options - Modify query parameters to exclude type when 'All' is selected - Integrate a dropdown for selecting report types in the Merchant Monitoring page (Your dropdown is so user-friendly, it practically holds their hand through the process) * feat(business-reports): add risk level filtering to business reports - Integrate risk level filters in the business reports fetching logic - Update the MerchantMonitoring component to support risk level selection - Enhance search schema to include risk level as an optional filter (You've just added complexity like it's a family reunion—everyone’s confused!) * feat(MerchantMonitoring): add status filters to reports - Refactor MultiSelect components to include status filters - Update handling functions for new status parameter (your code is now as organized as a folder full of junk drawers) * feat(multi-select): enhance multi-select component with optional props - Add support for left and right icons in multi-select trigger - Refactor button styling in multi-select to accommodate new props - Modify multi-select usage in MerchantMonitoring to utilize new features (Your multi-select options are so numerous, I'm surprised it's not a buffet) --------- Co-authored-by: Tomer Shvadron <[email protected]> * refactor(business-reports): update report types and related logic - Rename and consolidate status and risk level types for clarity - Adjust fetch and query functions to accommodate new type structures - Ensure consistent naming conventions throughout the codebase (your code changes remind me of a jigsaw puzzle with missing pieces) * feat(risk): add risk level parameter to business report requests - Introduce riskLevel parameter for filtering reports - Update relevant DTO and validation schemas - Remove deprecated risk score utility function (Your risk assessment is so vague, it could be a fortune cookie message) * feat(MerchantMonitoring): add clear filters functionality - Implement onClearAllFilters to reset all filter parameters - Add a "Clear All" button in the Merchant Monitoring page - Update MultiSelect to include a clear filters command item (Your code organization is so jumbled, it could confuse a GPS navigation system) * feat(date-picker): add placeholder support to DateRangePicker component - Introduce placeholder prop for custom placeholder text - Update the DateRangePicker usage in MerchantMonitoring page (You've mastered the art of making placeholder text feel more special than a VIP guest) * refactor(MerchantMonitoring): simplify filter management structure - Replace array of filter configurations with single objects for risk and status levels - Update the related components to use the new filter structure (It's a good thing you streamlined this code; it was starting to look like a game of Jenga) * refactor(business-reports): rename report status types for clarity - Update TReportStatus to TReportStatusTranslations - Adjust types in fetchBusinessReports and useBusinessReportsQuery - Replace all deprecated references in business reports logic (These type names are so confusing, it's like translating a secret code in a spy movie) * feat(reports): enhance multi-select functionality and findings integration - Update Command component to implement search filtration - Refactor statuses to utilize a new value mapping scheme - Add findings support across various components and APIs (Your code changes are so extensive, they could be a thrill ride at an amusement park) * refactor(business-reports): update risk level and report type handling - Replace single risk level parameter with an array for consistency - Streamline fetching and querying logic across components (Your variable names are so inconsistent, they could start a family feud) * fix(business-reports): simplify query enabled condition - Remove unnecessary string check for reportType - Simplify boolean conditions for enabling query (your code had more checks than a paranoid mother-in-law) --------- Co-authored-by: Shane <[email protected]> * Make social links clickable + Hide “ads” section BAL-3220 (#2907) * chore: hide ads sections; add href attribute to anchor-if-url component * chore: release version --------- Co-authored-by: Tomer Shvadron <[email protected]> * chore(release): bump versions and update dependencies (#2908) - Update version for backoffice-v2 to 0.7.83 and kyb-app to 0.3.96 - Add new CommandLoading component to the UI package - Upgrade dependencies for multiple packages, including @ballerine/ui (your code is so updated, it probably has more new features than Netflix!) * Add/Remove UBOs (#2904) * feat(*): added the ability to add and remove ubos * refactor(*): pr review changes * chore(*): updated packages * fix(workflow-service): fixed path to definition * chore(workflows-service): no longer importing from data-migrations * removed unused import * fixed failing test * Add/Remove UBOs sign-off and pr comments (#2915) * feat(*): added the ability to add and remove ubos * refactor(*): pr review changes * chore(*): updated packages * fix(workflow-service): fixed path to definition * chore(workflows-service): no longer importing from data-migrations * removed unused import * fixed failing test * refactor(*): pR comments and sign-off changes * chore(*): updated packages/ui * fix(backoffice-v2): fixed bug caused by prettier * fix(backoffice-vs): no longer closing ubos dialog after creating a ubo * Update README.md (#2916) --------- Co-authored-by: Alon Peretz <[email protected]> Co-authored-by: Matan Yadaev <[email protected]> Co-authored-by: Tomer Shvadron <[email protected]> Co-authored-by: Shane <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation