Signup page leftovers #2849
Signup page leftovers #2849
Conversation
- Clean up the workflow configuration schema by removing the createCollectionFlowToken option - Update various workflow definition files to eliminate obsolete token settings (your code was so cluttered, it looked like a toddler's art project on a bad day)
WalkthroughThis pull request introduces several modifications across various files, primarily focusing on the removal of the Changes
Possibly related PRs
Suggested reviewers
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 (
|
|
- Remove unnecessary intermediate variable in buildCollectionFlowState - Reorganize import statements for better readability - Add a count method in WorkflowTokenService for token existence checks (Your code is so bloated, it could use a good diet and some introspection)
- Replace relative imports with absolute paths for better readability - Add additional providers to the TokenAuthModule for expanded functionality (You’re like a pathing guide in a maze – leading the way, but it’s still a bit of a headache)
- Replace ambiguous service variable with workflowService - Update all references to maintain consistency - Enhance code readability and prevent confusion (Your variable names are like trying to find a needle in a haystack without the hay)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
services/workflows-service/src/common/guards/token-guard/token-auth.module.ts (1)
16-28: Consider documenting the module's expanded responsibilities.The module has grown to include several new providers beyond its original token authentication scope. While this might be intentional as part of the workflow configuration cleanup, it would be helpful to document why these services are needed in the token authentication context.
Consider adding a module-level JSDoc comment explaining the expanded responsibilities:
+/** + * TokenAuthModule handles authentication through tokens and manages related + * workflow configurations. It provides integration with customer management, + * UI definitions, and project scoping to support the authentication process. + */ @Module({packages/common/src/utils/collection-flow/build-collection-flow-state.ts (2)
39-43: LGTM! Consider adding type assertion for additionalInformation.The simplified return statement improves code clarity while maintaining the same functionality.
Consider adding explicit type assertion for additionalInformation to ensure type safety:
return { config, state, - additionalInformation: inputConfig.additionalInformation || {}, + additionalInformation: inputConfig.additionalInformation || {} as TCollectionFlow['additionalInformation'], };
Line range hint
24-24: Consider using proper logging infrastructure.The console.log statement might not be suitable for production code. Consider using a proper logging infrastructure with appropriate log levels.
- console.log('Collection Flow Context built progress state: ', progressState); + logger.debug('Collection Flow Context built progress state', { progressState });services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts (1)
27-32: LGTM! The count method is well-implemented.The implementation correctly maintains multi-tenancy by enforcing the projectId filter and uses type-safe Prisma arguments.
Consider adding JSDoc documentation to describe the method's purpose, parameters, and return value:
+ /** + * Counts workflow tokens for a specific project + * @param args - Prisma count arguments for filtering and options + * @param projectId - Project identifier for scoping the count + * @returns Promise resolving to the count of matching tokens + */ async count(args: Prisma.WorkflowRuntimeDataTokenCountArgs, projectId: TProjectId) {services/workflows-service/src/workflow/workflow.controller.external.ts (4)
384-396: Enhance error message clarityThe error message could be more specific about what type of token was not found.
- throw new NotFoundException(`No token was found for ${workflowRuntimeDataId}`); + throw new NotFoundException(`No collection flow token was found for workflow runtime ID: ${workflowRuntimeDataId}`);
Line range hint
419-421: Consider extracting magic numbers to constantsThe expiry calculation contains magic numbers. Consider extracting these to named constants for better maintainability.
+ const DEFAULT_TOKEN_EXPIRY_DAYS = 30; + const MILLISECONDS_PER_DAY = 24 * 60 * 60 * 1000; + - const expiresAt = new Date(Date.now() + (expiry || 30) * 24 * 60 * 60 * 1000); + const expiresAt = new Date(Date.now() + (expiry || DEFAULT_TOKEN_EXPIRY_DAYS) * MILLISECONDS_PER_DAY);
517-521: Consider extracting workflow runtime retrieval to a separate methodThe workflow runtime retrieval logic could be extracted to improve readability and reusability.
+ private async getLockedWorkflowRuntime(id: string, transaction: any) { + return await this.workflowService.getWorkflowRuntimeDataByIdAndLockUnscoped({ + id, + transaction, + }); + } + - const workflowRuntime = - await this.workflowService.getWorkflowRuntimeDataByIdAndLockUnscoped({ - id: params.id, - transaction, - }); + const workflowRuntime = await this.getLockedWorkflowRuntime(params.id, transaction);
Line range hint
532-554: Consider consolidating event emissionsThe two sequential event emissions share similar structure and could be consolidated into a single helper method to reduce code duplication.
+ private async emitWorkflowEvent( + id: string, + name: string, + payload: any, + projectId: string, + transaction: any + ) { + await this.workflowService.event( + { + id, + name, + ...(payload && { payload }), + }, + [projectId], + projectId, + transaction, + ); + } + - await this.workflowService.event( - { - id: params.id, - name: BUILT_IN_EVENT.DEEP_MERGE_CONTEXT, - payload: { - newContext: context, - arrayMergeOption: ARRAY_MERGE_OPTION.REPLACE, - }, - }, - [workflowRuntime.projectId], - workflowRuntime.projectId, - transaction, - ); - - await this.workflowService.event( - { - id: params.id, - name: params.event, - }, - [workflowRuntime.projectId], - workflowRuntime.projectId, - transaction, - ); + await this.emitWorkflowEvent( + params.id, + BUILT_IN_EVENT.DEEP_MERGE_CONTEXT, + { + newContext: context, + arrayMergeOption: ARRAY_MERGE_OPTION.REPLACE, + }, + workflowRuntime.projectId, + transaction + ); + + await this.emitWorkflowEvent( + params.id, + params.event, + null, + workflowRuntime.projectId, + transaction + );services/workflows-service/src/auth/workflow-token/workflow-token.service.ts (1)
51-61: Simplify complex type assertions for better maintainabilityThe type assertions in the construction of
stepsare complex and might lead to issues if the structure ofuiDefinition.definitionchanges. This can affect readability and maintainability.Consider defining a specific interface for
uiDefinition.definitionand refactoring the code to reduce the need for multiple type casts. Here's an example:interface UiDefinition { definition: Record<string, Record<string, unknown>>; } // Then, use the interface without type assertions steps: uiDefinition?.definition ? getOrderedSteps( uiDefinition.definition, { finalStates: [...WORKFLOW_FINAL_STATES] }, ).map(stepName => ({ stateName: stepName, })) : [],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
packages/common/src/schemas/documents/workflow/config-schema.ts(0 hunks)packages/common/src/utils/collection-flow/build-collection-flow-state.ts(1 hunks)services/workflows-service/scripts/workflows/dynamic-ui-workflow.ts(0 hunks)services/workflows-service/scripts/workflows/kyb-kyc-workflow-definition.ts(0 hunks)services/workflows-service/scripts/workflows/ui-definition/kyb-with-associated-companies/definition/compose-child-associated-company-definition.ts(0 hunks)services/workflows-service/scripts/workflows/ui-definition/kyb-with-associated-companies/definition/compose-kyb-with-associated-companies-definition.ts(0 hunks)services/workflows-service/scripts/workflows/ui-definition/kyb-with-associated-companies/definition/compose-kyc-child-workflow-definition.ts(0 hunks)services/workflows-service/src/auth/auth.module.ts(1 hunks)services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts(2 hunks)services/workflows-service/src/auth/workflow-token/workflow-token.service.ts(2 hunks)services/workflows-service/src/common/guards/token-guard/token-auth.module.ts(1 hunks)services/workflows-service/src/workflow/schemas/zod-schemas.ts(0 hunks)services/workflows-service/src/workflow/workflow-runtime-data.repository.ts(12 hunks)services/workflows-service/src/workflow/workflow.controller.external.ts(14 hunks)services/workflows-service/src/workflow/workflow.service.ts(0 hunks)
💤 Files with no reviewable changes (8)
- packages/common/src/schemas/documents/workflow/config-schema.ts
- services/workflows-service/scripts/workflows/dynamic-ui-workflow.ts
- services/workflows-service/scripts/workflows/kyb-kyc-workflow-definition.ts
- services/workflows-service/scripts/workflows/ui-definition/kyb-with-associated-companies/definition/compose-child-associated-company-definition.ts
- services/workflows-service/scripts/workflows/ui-definition/kyb-with-associated-companies/definition/compose-kyb-with-associated-companies-definition.ts
- services/workflows-service/scripts/workflows/ui-definition/kyb-with-associated-companies/definition/compose-kyc-child-workflow-definition.ts
- services/workflows-service/src/workflow/schemas/zod-schemas.ts
- services/workflows-service/src/workflow/workflow.service.ts
🔇 Additional comments (14)
services/workflows-service/src/auth/auth.module.ts (1)
Line range hint 7-11: LGTM! Import paths standardization looks good.
The conversion from relative to absolute imports using the '@/' prefix improves maintainability and makes the code more resilient to file relocations.
services/workflows-service/src/common/guards/token-guard/token-auth.module.ts (2)
3-13: LGTM! Clean import organization using absolute paths.
The imports are well-organized and consistently use absolute paths, which is good for maintainability and prevents path resolution issues.
16-28: Verify potential circular dependencies with the new providers.
While the addition of new providers enhances the module's capabilities, the introduction of multiple interdependent services (Customer, ApiKey, UiDefinition, etc.) increases the complexity of the dependency graph.
Let's verify there are no circular dependencies:
✅ Verification successful
Let me analyze the dependencies further to check for potential circular dependencies.
Based on the analysis of the dependencies, I can now provide a final response:
No circular dependencies detected in the token-auth.module providers
The verification shows that:
- CustomerService depends on ApiKeyService but not vice versa
- UiDefinitionService only depends on its repository and WorkflowRuntimeDataRepository
- None of the services import TokenAuthModule, preventing circular dependencies
- ProjectScopeService appears to be a standalone service without dependencies on other providers
The dependency graph is acyclic and the providers are properly layered, making it safe to include them in the token-auth.module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies between the newly added services
# Search for imports of TokenAuthModule or its exports in the new service files
echo "Checking for potential circular dependencies..."
rg -l "TokenAuthModule|WorkflowTokenRepository|WorkflowTokenService|TokenAuthGuard" \
$(fd -t f "$(rg -l "CustomerService|ApiKeyService|UiDefinitionService|ProjectScopeService" -g "*.ts")")
# Check if any of the new services import each other
echo "Checking for cross-dependencies between new services..."
for service in "CustomerService" "ApiKeyService" "UiDefinitionService" "ProjectScopeService"; do
echo "Dependencies for $service:"
rg "import.*$service" -g "*.ts"
done
Length of output: 22708
Script:
#!/bin/bash
# Check if any of the newly added services import TokenAuthModule or its components
echo "Checking if new services import TokenAuthModule or its components..."
rg "import.*\{.*TokenAuthModule.*\}" services/workflows-service/src/customer/customer.service.ts services/workflows-service/src/customer/api-key/api-key.service.ts services/workflows-service/src/ui-definition/ui-definition.service.ts services/workflows-service/src/project/project-scope.service.ts
# Check if CustomerService has any circular dependencies with ApiKeyService
echo "Checking CustomerService and ApiKeyService dependencies..."
ast-grep --pattern 'class CustomerService {
$$$
constructor($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class ApiKeyService {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Check if UiDefinitionService has any circular dependencies
echo "Checking UiDefinitionService dependencies..."
ast-grep --pattern 'class UiDefinitionService {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Check if ProjectScopeService has any circular dependencies
echo "Checking ProjectScopeService dependencies..."
ast-grep --pattern 'class ProjectScopeService {
$$$
constructor($$$) {
$$$
}
$$$
}'
Length of output: 20173
services/workflows-service/src/auth/workflow-token/workflow-token.repository.ts (1)
27-32: Verify the usage of the new count method in workflow configurations.
Since this PR involves removing createCollectionFlowToken, let's verify how this new count method integrates with the updated workflow configurations.
✅ Verification successful
The count method is properly integrated and used in the workflow token creation flow
The verification shows that the count method is correctly used within the WorkflowTokenService.create() method to check for existing tokens before creating a new one. This is part of the token management flow and not related to the removed createCollectionFlowToken. The method serves a different purpose and is working as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usages of the new count method in workflow-related files
rg -l "workflowToken.*count" --type typescript
# Check for any remaining references to createCollectionFlowToken
rg "createCollectionFlowToken" --type typescript
Length of output: 167
Script:
#!/bin/bash
# Search for potential usages of the new count method in workflow-related files
rg -l "workflowToken.*count"
# Look for workflow token related files and their content
fd "workflow.*token" -t f -X cat
# Check for any remaining references to createCollectionFlowToken
rg "createCollectionFlowToken"
Length of output: 8536
services/workflows-service/src/workflow/workflow-runtime-data.repository.ts (4)
26-26: LGTM! Improved naming convention
The rename from prisma to prismaService better follows Angular/NestJS naming conventions and makes the dependency injection more explicit.
32-32: LGTM! Consistent parameter updates
The default transaction parameters have been consistently updated across all methods to use this.prismaService, maintaining alignment with the constructor change while preserving the correct type signatures.
Also applies to: 64-64, 88-88, 136-136, 151-151, 168-168
50-52: LGTM! Consistent service reference updates
All method bodies have been updated to use this.prismaService, maintaining consistency with the constructor change while preserving the original functionality.
Also applies to: 58-58, 202-209, 261-272, 279-282, 288-291, 398-398
Line range hint 184-190: Verify the existence of custom PostgreSQL function
The code uses a custom PostgreSQL function jsonb_deep_merge_with_options. Let's verify its existence in the database schema.
✅ Verification successful
Custom PostgreSQL function is properly defined and maintained
The jsonb_deep_merge_with_options function is properly defined in the database schema through two migrations:
- Initial implementation in migration
20230723221706_add_jsonb_deep_merge_with_options - Bug fix for handling primitives in migration
20231122094527_fix_jsonb_deep_merge_with_options_primitives_bug
The function is implemented in PLV8 (JavaScript in PostgreSQL), supports multiple array merging strategies ('concat', 'by_id', 'by_index', 'replace'), and handles edge cases appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the custom PostgreSQL function exists in the migration files
# Search for the function definition in migration files
rg -l "jsonb_deep_merge_with_options" --type sql
# Search for function usage in other files
ast-grep --pattern 'jsonb_deep_merge_with_options'
Length of output: 330
Script:
#!/bin/bash
# Let's check the content of these migration files to verify the function definition
# Check the initial migration that added the function
cat services/workflows-service/prisma/migrations/20230723221706_add_jsonb_deep_merge_with_options/migration.sql
# Check the fix migration
cat services/workflows-service/prisma/migrations/20231122094527_fix_jsonb_deep_merge_with_options_primitives_bug/migration.sql
Length of output: 7504
services/workflows-service/src/workflow/workflow.controller.external.ts (2)
Line range hint 55-60: LGTM! Clear and descriptive service naming
The renaming from service to workflowService improves code clarity by making the service's purpose explicit.
408-417: LGTM! Improved error handling for workflow runtime data validation
The added try-catch block provides better error handling when validating workflow runtime data existence.
services/workflows-service/src/auth/workflow-token/workflow-token.service.ts (4)
3-10: Imports are appropriate and necessary for added functionality
The additional imports bring in the necessary types and services for the new logic in the WorkflowTokenService. This ensures all dependencies are properly declared.
14-19: Constructor correctly injects required services
The updated constructor includes additional services that are needed for the enhanced create method functionality. Dependency injection is properly used.
107-112: New count method aligns with service design and enhances functionality
The addition of the count method in WorkflowTokenService provides a useful abstraction over the repository layer, allowing for counting tokens based on specific criteria.
74-82: Ensure existing context properties are not overwritten unintentionally
When merging new properties into context, there is a possibility that existing properties could be overwritten. Specifically, confirm that adding collectionFlow and updating metadata does not overwrite any important data in the existing context.
Run the following script to check for any usages of collectionFlow and metadata in context handling:
services/workflows-service/src/auth/workflow-token/workflow-token.service.ts
Show resolved
Hide resolved
…UserController - Add imports for UiDefinitionRepository and ApiKey services - Include new dependencies in module testing setup - Create a new UI definition instance as part of the test setup (your tests are more complex than explaining the plot of Inception)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts (2)
77-81: Consider grouping related providers togetherWhile the additions are correct, consider grouping related providers together for better readability. For example, group UI-related providers (UiDefinitionService, UiDefinitionRepository) and Customer-related providers (CustomerService, ApiKeyService, ApiKeyRepository) consecutively.
- UiDefinitionService, - UiDefinitionRepository, - CustomerService, - ApiKeyService, - ApiKeyRepository, + // UI Definition related + UiDefinitionService, + UiDefinitionRepository, + // Customer related + CustomerService, + ApiKeyService, + ApiKeyRepository,
Database cleanup issue is caused by commented-out transaction control
The database cleanup issue is caused by the commented-out transaction control in the database helper. The helper attempts to delete data without proper session replication role settings, which can cause referential integrity issues during cleanup.
Specific issues in
database-helper.ts:
- Line 19:
SET session_replication_role = replicais commented out- Line 24:
SET session_replication_role = DEFAULTis commented outThese settings are crucial for properly bypassing referential constraints during cleanup operations, which explains why some tests can't be run due to cleanup issues.
🔗 Analysis chain
Line range hint
192-192: Address the database cleanup issueThe TODO comment indicates an unresolved database cleanup issue that's preventing additional test cases from running. This should be prioritized as it affects test coverage and reliability.
Let's check for similar cleanup issues in other test files:
Would you like help investigating and resolving the database cleanup issue? I can help create a GitHub issue to track this task.
Let me gather more context about this test file and related test files to better understand the cleanup issue.
Let me check the test helpers to understand the database cleanup implementation since the issue seems to be related to that.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar DB cleanup related comments rg "TODO.*[Dd]([Bb]|atabase).*cleanup" --type tsLength of output: 216
Script:
#!/bin/bash # Get the content of the test file to understand the cleanup context cat services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts # Look for similar integration test files in the same directory fd ".*\.intg\.test\.ts$" services/workflows-service/src/collection-flow/controllers/Length of output: 9466
Script:
#!/bin/bash # Check the database helper implementation cat services/workflows-service/src/test/helpers/database-helper.ts # Look for other test files using these helpers rg "cleanupDatabase|tearDownDatabase" --type tsLength of output: 7472
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts(4 hunks)services/workflows-service/src/workflow/workflow.service.unit.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- services/workflows-service/src/workflow/workflow.service.unit.test.ts
🔇 Additional comments (2)
services/workflows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts (2)
40-42: LGTM: Clean import additions
The new imports are properly organized and use absolute paths consistently.
105-105: LGTM: Consistent repository initialization
The uiDefinitionRepository initialization follows the established pattern and is properly typed.
...lows-service/src/collection-flow/controllers/collection-flow.no-user.controller.intg.test.ts
Show resolved
Hide resolved
…service - Add integration test for UI definition repository - Update results validation with environment variables (These tests are so robust, they can survive a zombie apocalypse… but maybe not a spelling mistake)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/collection-flow/collection-flow.service.intg.test.ts (1)
236-257: Enhance test coverage for context structureThe expected context structure is well-defined, but consider adding:
- Tests for missing or invalid environment variables
- Validation of URL formats in metadata
- Edge cases for missing customer display name
Example test cases to add:
it('should handle missing environment variables', async () => { // Mock env with undefined values // Verify error handling or fallback values }); it('should validate URL formats', async () => { // Test with invalid URL formats in env // Verify validation or sanitization }); it('should handle missing customer display name', async () => { // Create customer without displayName // Verify fallback behavior });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
services/workflows-service/src/collection-flow/collection-flow.service.intg.test.ts(5 hunks)
🔇 Additional comments (2)
services/workflows-service/src/collection-flow/collection-flow.service.intg.test.ts (2)
40-40: LGTM: Clean setup of new dependencies
The addition of the env import and UiDefinitionRepository is well-structured and follows the existing pattern of dependency injection in the test setup.
Also applies to: 125-125, 164-164
208-216: Verify uiContext value and consider additional test cases
While the UI definition creation is correctly implemented, consider:
- Verifying if 'collection_flow' is the correct constant for uiContext
- Adding test cases for invalid/missing UI definitions to ensure proper error handling
✅ Verification successful
'collection_flow' is a valid uiContext value
The value 'collection_flow' is correctly used as it's one of the two valid enum values ('back_office' | 'collection_flow') defined in the UiDefinitionModel. The test implementation is consistent with the model's constraints.
Regarding test coverage:
- The current minimal test case is sufficient for basic functionality
- Additional test cases for invalid UI definitions would be handled by class-validator decorators (@IsString, @isObject, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of the uiContext value to verify consistency
rg -l "collection_flow" --type ts
Length of output: 1838
Script:
#!/bin/bash
# Check the UI context definition in the model and common constants
echo "=== Checking UI Definition model ==="
cat services/workflows-service/src/ui-definition/ui-definition.model.ts
echo -e "\n=== Checking common constants ==="
cat packages/common/src/consts/index.ts
Length of output: 6135
(your code was so cluttered, it looked like a toddler's art project on a bad day)
Summary by CodeRabbit
Release Notes
New Features
createCollectionFlowTokenin specific workflow definitions.Bug Fixes
Refactor
createCollectionFlowTokenproperty from various workflow definitions, adjusting configuration options.Chores