feat(plugins): add commercial credit check plugin and related updates #3024
Conversation
- Integrate CommercialCreditCheckPlugin for external API interactions - Update existing logic to include commercial credit check validation - Add commercialCreditCheck to pluginsWhiteList for tracking (With so many plugins, this code is starting to look like a family reunion)
|
WalkthroughThis pull request introduces several updates across multiple modules. In the backoffice, the plugin whitelist is expanded with Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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: 3
🧹 Nitpick comments (4)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (4)
12-17: Consider refining the vendor constraint or supporting multiple vendors.
Although the Zod schema is correct, defining only a single supported vendor ("experian") might suggest future extension to other vendors. If you plan to eventually support multiple vendors, updating thez.enum(['experian'])to reflect that roadmap or clarifying that only Experian is supported could improve maintainability.
91-97: Validate responses transferred with chunked encoding.
content-lengthmight be absent or unreliable for chunked responses. Logging and verifying content length is helpful, but consider clarifying or handling chunked scenarios to avoid potential false positives if the server uses chunked transfer encoding.
108-110: Consider specifying a more detailed response schema.
CommercialCreditCheckResponseSchemais a broadz.record(z.string(), z.unknown()). If the API response has a stable structure, using a more explicit schema improves type safety and validation.
120-128: Fine-tune error handling for improved clarity.
The catch block provides a generic approach. For more actionable troubleshooting, consider distinguishing between network errors, parsing errors, and others so that logs capture more specific error diagnoses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/backoffice-v2/src/common/components/molecules/ProcessTracker/constants.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useKybRegistryInfoBlock/useKybRegistryInfoBlock.tsx(2 hunks)packages/workflow-core/src/lib/constants.ts(3 hunks)packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts(3 hunks)packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (1)
67-74: Double-check logic for bypassing checks whenbusinessTypeis'individual'.
Currently, if thebusinessTypeis'individual', the plugin returns a canceled result and skips the API request. Confirm that this is the intended behavior, especially since the rest of the code handles commercial checks.packages/workflow-core/src/lib/constants.ts (2)
13-13: Import addition looks good.
This import properly references the newly introduced CommercialCreditCheckPlugin.
24-36: Confirm consistent plugin naming across the codebase.
PluginKind.COMMERCIAL_CREDIT_CHECKuses the string'commercial-credit-check', but elsewhere (e.g., the backoffice whitelist), the key is'commercialCreditCheck'. Verify that tools referencing these constants can correctly identify the plugin.apps/backoffice-v2/src/lib/blocks/hooks/useKybRegistryInfoBlock/useKybRegistryInfoBlock.tsx (2)
36-38: LGTM! Data access pattern updated correctly.The condition has been properly updated to access the bank account verification data through the new structure, maintaining null safety with optional chaining.
40-44: LGTM! Data access and filtering logic improved.The changes correctly:
- Update the data access pattern to match the new response schema
- Simplify the filtering logic while maintaining the same functionality
Also applies to: 52-53
packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts (2)
147-147: LGTM! Response parsing updated correctly.The response parsing has been updated to use the new schema, maintaining proper error handling through zod's parse method.
99-99: Verify the impact of API endpoint change.The removal of the '/commercial' segment from the API endpoint might affect existing integrations. Please ensure this change is intentional and coordinated with the API team.
✅ Verification successful
API Endpoint Update Verified: '/commercial' Segment Removal Confirmed
Our searches in the relevant file did not reveal any remaining references to the old endpoint with the '/commercial' segment. This indicates that the change to remove '/commercial' from the API endpoint is consistent within the codebase. Please ensure that this change has been coordinated with the API team and that any external integrations are updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other usages of the old endpoint rg -l "bank-account-verification/commercial"Length of output: 44
Script:
#!/bin/bash # Verify if '/commercial' appears anywhere in the bank-account-verification-plugin file, # which might indicate dependencies on the old API endpoint. rg -n "/commercial" packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.tsLength of output: 143
...lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/ProcessTracker/constants.tsx
Show resolved
Hide resolved
...plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts
Show resolved
Hide resolved
…ck blocks - Introduce useBankAccountVerificationBlock hook for bank account verifications - Implement useCommercialCreditCheckBlock hook for commercial credit checks - Integrate both blocks into the default blocks logic for improved workflows (These blocks are so well-structured, even your mother would be proud of their nesting)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/backoffice-v2/src/lib/blocks/hooks/useCommercialCreditCheckBlock/useCommercialCreditCheckBlock.tsx (2)
32-62: Consider adding error boundary.While the implementation is solid, consider wrapping the block creation in an error boundary to gracefully handle any runtime errors in the UI.
return useMemo(() => { + try { const cell = getCell(); if (!cell) { return []; } return createBlocksTyped() .addBlock() .addCell({ type: 'block', value: createBlocksTyped() .addBlock() .addCell({ id: 'nested-details-heading', type: 'heading', value: 'Commercial Credit Check', }) .addCell({ id: 'nested-details-subheading', type: 'subheading', value: 'Experian-Provided Data', props: { className: 'mb-4', }, }) .addCell(cell) .buildFlat(), }) .build(); + } catch (error) { + console.error('Error in useCommercialCreditCheckBlock:', error); + return []; + } }, [getCell]);
32-62: Consider adding error state handling.While the block construction is well-implemented with proper heading and subheading, consider handling error states from the plugin output to provide better user feedback.
return useMemo(() => { const cell = getCell(); if (!cell) { return []; } + const hasError = pluginsOutput?.commercialCreditCheck?.error; + if (hasError) { + return createBlocksTyped() + .addBlock() + .addCell({ + type: 'block', + value: createBlocksTyped() + .addBlock() + .addCell({ + id: 'nested-details-error', + type: 'error', + value: 'Failed to fetch commercial credit check data', + }) + .buildFlat(), + }) + .build(); + } return createBlocksTyped()apps/backoffice-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx (4)
7-41: Consider extracting data transformation logic.The data transformation logic is complex and could benefit from being extracted into a separate utility function for better maintainability and testability.
+ const transformBankVerificationData = (data: any) => { + const { + responseHeader, + clientResponsePayload: { decisionElements, orchestrationDecisions }, + } = data; + + return { + ...responseHeader.overallResponse, + decisionElements, + orchestrationDecisions, + }; + }; const getCell = useCallback(() => { if ( Object.keys(pluginsOutput?.bankAccountVerification?.data.clientResponsePayload ?? {}).length ) { - const data = { - ...pluginsOutput.bankAccountVerification.data.responseHeader.overallResponse, - decisionElements: - pluginsOutput.bankAccountVerification.data.clientResponsePayload.decisionElements, - orchestrationDecisions: - pluginsOutput.bankAccountVerification.data.clientResponsePayload.orchestrationDecisions, - }; + const data = transformBankVerificationData(pluginsOutput.bankAccountVerification.data);
71-72: Use consistent build method.The code uses
build().flat(1)while the commercial credit check block usesbuildFlat(). Consider using the same method for consistency.- .build() - .flat(1), + .buildFlat(),
44-75: Consider using a consistent build pattern.While the implementation is correct, there's an inconsistency in the build pattern. The
buildFlat()method is used here whilebuildFlatis not used in the commercial credit check block.- .build() - .flat(1), + .buildFlat(),
5-42: Consider adding TypeScript interfaces for plugin output.While the implementation is solid, adding explicit TypeScript interfaces would improve code maintainability and type safety.
interface BankAccountVerificationData { responseHeader: { overallResponse: Record<string, unknown>; }; clientResponsePayload: { decisionElements: unknown; orchestrationDecisions: unknown; }; } interface PluginsOutput { bankAccountVerification?: { data: BankAccountVerificationData; }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/backoffice-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useCommercialCreditCheckBlock/useCommercialCreditCheckBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useKybRegistryInfoBlock/useKybRegistryInfoBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useCaseBlocksLogic/utils/get-tabs-block-map.tsx(2 hunks)apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx(5 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/prisma/data-migrations
- apps/backoffice-v2/src/lib/blocks/hooks/useKybRegistryInfoBlock/useKybRegistryInfoBlock.tsx
👮 Files not reviewed due to content moderation or server errors (4)
- apps/backoffice-v2/src/lib/blocks/hooks/useCommercialCreditCheckBlock/useCommercialCreditCheckBlock.tsx
- apps/backoffice-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx
- apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useCaseBlocksLogic/utils/get-tabs-block-map.tsx
- apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: Analyze (javascript)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (15)
apps/backoffice-v2/src/lib/blocks/hooks/useCommercialCreditCheckBlock/useCommercialCreditCheckBlock.tsx (2)
7-29: LGTM! Robust data handling with type safety.The implementation safely handles data extraction with proper null checks and type assertions using the
satisfiesoperator.
5-30: LGTM! ThegetCellfunction is well-implemented.The function is properly memoized, handles data presence checks, and ensures type safety using the
satisfiesoperator. The predefined sort order forCommercialNameandRegNumberfields is a good addition for consistent display.apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useCaseBlocksLogic/utils/get-tabs-block-map.tsx (3)
58-59: LGTM! Consistent integration of new blocks.The new blocks are properly integrated into the company information tab following the existing pattern.
Also applies to: 81-82
58-59: LGTM! Blocks are properly integrated into the company information tab.The new blocks are correctly added to the array destructuring and properly spread into the company information tab array.
Also applies to: 81-82
58-59: LGTM! Clean integration of new blocks.The new blocks are properly integrated into the company information tab, maintaining the existing structure and type safety.
Also applies to: 81-82
apps/backoffice-v2/src/lib/blocks/variants/DefaultBlocks/hooks/useDefaultBlocksLogic/useDefaultBlocksLogic.tsx (8)
60-61: LGTM! Clean integration of new blocks.The new hooks are properly imported and integrated into the blocks array with corresponding dependency array updates.
Also applies to: 178-184, 558-559, 592-593
60-61: LGTM! Imports are properly organized.The new hooks are correctly imported from their respective locations.
178-184: LGTM! Hook initialization is consistent with other blocks.The hooks are initialized with the same pattern as other blocks, passing the plugins output.
558-559: LGTM! Blocks are properly integrated.The new blocks are correctly added to the allBlocks array and their dependencies are properly included in the dependency array.
Also applies to: 592-593
60-61: LGTM! Clean hook imports.The new hooks are properly imported following the existing import pattern.
178-184: LGTM! Proper hook initialization.The hooks are initialized with the correct plugin output data.
524-526: LGTM! Good defensive programming.The null check for workflow entity prevents potential runtime errors.
558-559: LGTM! Clean block integration.The new blocks and their dependencies are properly integrated into the allBlocks array.
Also applies to: 592-593
apps/backoffice-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx (2)
5-42: LGTM! Data processing is secure and well-structured.The implementation properly filters out sensitive fields (
tenantIDandclientReferenceId) and structures the data appropriately. The type safety withsatisfiesoperator is a good practice.
44-75: LGTM! Clean implementation with proper error handling.The block creation logic is well-structured with proper null checks and memoization.
...-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx
Outdated
Show resolved
Hide resolved
...-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx
Outdated
Show resolved
Hide resolved
...fice-v2/src/lib/blocks/hooks/useCommercialCreditCheckBlock/useCommercialCreditCheckBlock.tsx
Show resolved
Hide resolved
...lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts
Show resolved
Hide resolved
...lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts
Outdated
Show resolved
Hide resolved
…k logic - Remove unnecessary useCallback for cell generation - Simplify return structure for better readability - Enhance object key management within payload processing (Your code's so convoluted, even an octopus would struggle to untangle it)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (2)
91-91:⚠️ Potential issueUpdate businessType check to use 'Sole proprietorship'.
Based on previous discussions, 'individual' should be changed to 'Sole proprietorship'.
105-108: 🛠️ Refactor suggestionHandle missing or invalid API token.
Even though
validateEnvis called, you may want to gracefully handle scenarios whereenv.UNIFIED_API_TOKENis unexpectedly missing or invalid. This prevents plugin invocation from proceeding with an empty token.
🧹 Nitpick comments (3)
apps/backoffice-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx (2)
67-70: Optimize dependencies array to prevent unnecessary rerenders.The current dependencies array uses nested object properties which could cause unnecessary rerenders. Consider using a more specific dependency.
}, [ - pluginsOutput.bankAccountVerification.data.clientResponsePayload, - pluginsOutput.bankAccountVerification.data.responseHeader.overallResponse, + pluginsOutput.bankAccountVerification.data ]);
46-51: Consider adding type safety for data object.The data object construction could benefit from a type definition to ensure type safety and improve maintainability.
+ type BankAccountVerificationData = { + decisionElements: unknown; + orchestrationDecisions: unknown; + [key: string]: unknown; + }; - const data = { + const data: BankAccountVerificationData = {packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts (1)
94-150: Extract path strings into constants for maintainability.The path strings used in the merge operation are error-prone and could benefit from being extracted into constants.
+ const ENTITY_PATHS = { + ADDRESS: { + STREET_NUMBER: 'entity.data.address.streetNumber', + STREET: 'entity.data.address.street', + CITY: 'entity.data.address.city', + POSTCODE: 'entity.data.address.postcode', + }, + BANK_INFORMATION: { + ACCOUNT_NAME: 'entity.data.bankInformation.bankAccountName', + SORT_CODE: 'entity.data.bankInformation.sortCode', + ACCOUNT_NUMBER: 'entity.data.bankInformation.accountNumber', + HOLDER: { + FIRST_NAME: 'entity.data.bankInformation.bankAccountHolder.firstName', + MIDDLE_NAME: 'entity.data.bankInformation.bankAccountHolder.middleName', + LAST_NAME: 'entity.data.bankInformation.bankAccountHolder.lastName', + }, + }, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/backoffice-v2/src/lib/blocks/hooks/useBankAccountVerificationBlock/useBankAccountVerificationBlock.tsx(1 hunks)apps/backoffice-v2/src/lib/blocks/hooks/useCommercialCreditCheckBlock/useCommercialCreditCheckBlock.tsx(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts(4 hunks)packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/prisma/data-migrations
- apps/backoffice-v2/src/lib/blocks/hooks/useCommercialCreditCheckBlock/useCommercialCreditCheckBlock.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts (2)
73-73: Consider defining a more specific response schema.The current schema
z.record(z.string(), z.unknown())is too permissive. Based on the usage inuseBankAccountVerificationBlock.tsx, we can define a more specific schema to catch potential issues at runtime.
156-157: Verify the API endpoint change impact.The removal of '/commercial' segment from the API endpoint URL might affect existing integrations.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for other usages of the bank-account-verification endpoint # Search for the old endpoint pattern echo "Searching for old endpoint pattern..." rg -l "bank-account-verification/commercial" # Search for the new endpoint pattern echo "Searching for new endpoint pattern..." rg -l "bank-account-verification[^/]"Length of output: 422
API Endpoint Update Verification
We searched the repository and found no instances of the old/commercialsegment. The bank account verification endpoint is now consistently referenced with the updated URL (as seen in both the plugin file and constants), indicating that the change has been applied throughout the codebase.packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (3)
13-20: Consider strengthening the response schema validation.The
CommercialCreditCheckResponseSchemais very permissive, allowing any string key with unknown values. Consider defining a more specific schema that matches the expected response structure from the API.Let me help you verify the API response structure:
Also applies to: 30-30
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for API response handling to understand the expected structure # Search for response handling in tests or documentation rg -A 5 "CommercialCreditCheckResponseSchema" --type tsLength of output: 1958
Strengthen API Response Schema
The output confirms that the
CommercialCreditCheckResponseSchemais currently defined as a record with string keys mapped to unknown values (usingz.record(z.string(), z.unknown())), which is overly permissive. If you have a well-defined structure for the API response, consider updating the schema to reflect that structure for better type safety and validation.
- The schema is defined at the top of the file and used to parse the API response.
- A more specific schema would help catch potential issues early by enforcing the expected response format.
115-120: Consider handling zero-length responses differently.The current implementation throws an error for empty responses. Consider if there are valid use cases where an empty response might be acceptable, and handle those cases appropriately.
Let me help you verify API response handling:
51-69: Verify entity data structure assumptions.The constructor assumes a specific data structure for the entity (e.g.,
entity.data.businessType,entity.data.additionalInfo.registeredCharityNumber). Ensure that this structure is consistently maintained across all possible entity types.Let me help you verify the entity structure:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for entity data structure usage # Search for entity data structure patterns ast-grep --pattern $'entity.data.businessType' ast-grep --pattern $'entity.data.additionalInfo'Length of output: 93
I'm not seeing confirmation of these properties using the previous ast-grep commands. To get a better picture of how the entity structure is used elsewhere in the codebase, let's run an additional search using rg to look for any occurrence of the "entity.data." pattern:
Once you review the output from this script, we can better assess whether the entity structure assumptions (specifically for "entity.data.businessType" and "entity.data.additionalInfo") are valid across the codebase.
🏁 Script executed:
#!/bin/bash # Searching for any occurrences of "entity.data." in the repository to verify entity data structure usage rg -n "entity\.data\." .Length of output: 51596
Entity Data Structure Assumptions Verified
After walking through the repository usage with our searches, it appears that the use of fields like "entity.data.businessType" and "entity.data.additionalInfo.registeredCharityNumber" is consistent with patterns observed elsewhere. The data structure assumptions made in the constructor seem in line with how other parts of the codebase reference and handle the entity data.
...lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts
Outdated
Show resolved
Hide resolved
- Change condition from 'individual' to 'sole_proprietorship' - Ensure accurate processing for specific business types
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
scripts/auto-commit.js (1)
25-25: Fix the incorrect model name.The model name
'gpt-4o-mini'appears to be incorrect. OpenAI's model names don't include the 'o' character.Apply this diff to fix the model name:
- model: 'gpt-4o-mini', + model: 'gpt-4',
♻️ Duplicate comments (2)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (2)
105-108:⚠️ Potential issueHandle missing or invalid API token.
Add validation for
env.UNIFIED_API_TOKENbefore using it in the API request.Apply this diff to add token validation:
+ if (!env.UNIFIED_API_TOKEN) { + return this.returnErrorResponse( + `${this.pluginName} - Missing or invalid API token`, + ); + } + const apiResponse = await this.makeApiRequest(url, this.method, validatedPayload.data, { ...this.headers, Authorization: `Bearer ${env.UNIFIED_API_TOKEN}`, });
132-133:⚠️ Potential issueUse safeParse for response validation.
Replace
parsewithsafeParseto handle parsing errors gracefully.Apply this diff to improve error handling:
- const responseBody = CommercialCreditCheckResponseSchema.parse(res); + const parsedResponse = CommercialCreditCheckResponseSchema.safeParse(res); + if (!parsedResponse.success) { + return this.returnErrorResponse( + `${this.pluginName} - Invalid response format: ${JSON.stringify(parsedResponse.error.errors)}`, + ); + } + const responseBody = parsedResponse.data;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts(1 hunks)scripts/auto-commit.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (2)
13-20: Generate TypeScript type from Zod schema.Consider using Zod's type inference to generate the type from the schema to ensure type safety and avoid potential mismatches.
Add this code before the type definition:
type TCommercialCreditCheckPluginPayloadSchema = z.infer<typeof CommercialCreditCheckPluginPayloadSchema>;
91-98: LGTM! Business type handling is correct.The implementation correctly handles 'sole_proprietorship' business type as discussed in the past comments.
- Replace parse with safeParse for response validation - Include error handling for invalid response format
- Change response spreading from parsedResponse to parsedResponse.data - Ensure consistency across the Bank Account and Commercial Credit Check plugins
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (2)
13-20: Consider strengthening the response schema validation.The payload schema validation is comprehensive, but the response schema could be more specific about the expected structure of the response data.
Consider defining a more specific response schema that matches the expected API response structure:
const CommercialCreditCheckResponseSchema = z.object({ // Define expected response fields here status: z.string(), data: z.record(z.string(), z.unknown()), // Add more fields as needed });
91-98: Consider adding a log message for sole_proprietorship case.When skipping the API call for sole_proprietorship, it would be helpful to log this decision for debugging purposes.
Add a log message before returning:
if (validatedPayload.data.businessType === 'sole_proprietorship') { + logger.log(`${this.pluginName} - Skipping API call for sole_proprietorship business type`); return this.successAction ? this.returnSuccessResponse(this.successAction, { name: this.name, status: ProcessStatus.CANCELED, }) : {}; }packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts (1)
94-150: Consider extracting default paths to a constant.The default paths configuration is quite large and could be moved to a separate constant for better maintainability and reusability.
+const DEFAULT_PATHS = { + vendor: 'experian', + address: { + streetNumber: { __type: 'path', value: 'entity.data.address.streetNumber' }, + street: { __type: 'path', value: 'entity.data.address.street' }, + city: { __type: 'path', value: 'entity.data.address.city' }, + postcode: { __type: 'path', value: 'entity.data.address.postcode' }, + }, + bankAccountDetails: { + holder: { + bankAccountName: { __type: 'path', value: 'entity.data.bankInformation.bankAccountName' }, + companyRegistrationNumber: { __type: 'path', value: 'entity.data.registrationNumber' }, + registeredCharityNumber: { __type: 'path', value: 'entity.data.additionalInfo.registeredCharityNumber' }, + firstName: { __type: 'path', value: 'entity.data.bankInformation.bankAccountHolder.firstName' }, + middleName: { __type: 'path', value: 'entity.data.bankInformation.bankAccountHolder.middleName' }, + lastName: { __type: 'path', value: 'entity.data.bankInformation.bankAccountHolder.lastName' }, + }, + sortCode: { __type: 'path', value: 'entity.data.bankInformation.sortCode' }, + bankAccountNumber: { __type: 'path', value: 'entity.data.bankInformation.accountNumber' }, + }, +}; constructor({ payload, ...pluginParams }: IApiPluginParams & { payload: BankAccountVerificationPlugin['payload'] }) { // ... this.payload = payload; - merge(this.payload, { - vendor: pluginParams.vendor || 'experian', - address: { /* ... */ }, - bankAccountDetails: { /* ... */ }, - }); + merge(this.payload, { + vendor: pluginParams.vendor || DEFAULT_PATHS.vendor, + ...DEFAULT_PATHS, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts(4 hunks)packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts(1 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: lint
🔇 Additional comments (9)
packages/workflow-core/src/lib/plugins/external-plugin/commercial-credit-check-plugin/commercial-credit-check-plugin.ts (4)
51-69: LGTM! Well-structured default path mappings.The constructor properly merges default values and path mappings for all required fields. The implementation aligns with the discussed changes regarding business types.
72-89: LGTM! Robust payload validation.The implementation includes proper environment validation and uses
safeParsefor payload validation, providing detailed error messages.
105-108: Verify API token handling.Even though
validateEnvis called, consider adding explicit validation for the API token.Add token validation before making the API request:
+ if (!env.UNIFIED_API_TOKEN?.trim()) { + return this.returnErrorResponse( + `${this.pluginName} - Missing or invalid API token`, + ); + } const apiResponse = await this.makeApiRequest(url, this.method, validatedPayload.data, { ...this.headers, Authorization: `Bearer ${env.UNIFIED_API_TOKEN}`, });
132-139: LGTM! Improved response parsing.The implementation now uses
safeParsefor response validation as suggested in the past review comments.packages/workflow-core/src/lib/plugins/external-plugin/bank-account-verification-plugin/bank-account-verification-plugin.ts (5)
2-2: LGTM!The addition of
lodash.mergeis appropriate for deep merging of default values with the payload.
45-71: LGTM!The type changes appropriately reflect the optional nature of address and bankAccountDetails while maintaining strong typing for nested properties.
73-73: Consider defining a more specific response schema.The current schema
z.record(z.string(), z.unknown())is too permissive. Based on the usage inuseKybRegistryInfoBlock.tsx, we can define a more specific schema to catch potential issues at runtime.
204-211: LGTM!The response handling is robust with proper schema validation and detailed error messages.
157-157: Verify the API endpoint change impact.The removal of '/commercial' from the URL path might affect existing functionality. Please ensure that the unified API endpoint handles both commercial and non-commercial requests correctly.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for other usages of the bank-account-verification endpoint # Search for API endpoint usage rg -A 5 "bank-account-verification" --type tsLength of output: 2027
API Endpoint Change Verified
The search results confirm that the bank-account-verification plugin now constructs its URL using
${env.UNIFIED_API_URL}/bank-account-verification
with no trace of the previously present '/commercial' segment. The constants file corroborates the separation of concerns—with commercial functionality handled by the CommercialCreditCheck plugin. Please ensure that the unified API backend is updated to correctly route both commercial and non-commercial requests as intended.
(With so many plugins, this code is starting to look like a family reunion)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor