Unsubscribe from Ongoing Merchant Monitoring #2799
Conversation
|
WalkthroughThe changes in this pull request involve an update to the subproject commit hash in Changes
Possibly related PRs
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 (
|
tomer-shvadron
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (4)
services/workflows-service/src/business/business.repository.ts (1)
Line range hint
102-107: Consider enhancing error handling for updates.While the transaction parameter update is correct, consider adding validation similar to the create method to ensure data integrity.
async updateById<T extends Omit<Prisma.BusinessUpdateArgs, 'where'>>( id: string, args: Prisma.SelectSubset<T, Omit<Prisma.BusinessUpdateArgs, 'where'>>, transaction: PrismaClient | PrismaTransaction = this.prismaService, ): Promise<BusinessModel> { + // Add validation if there's a corresponding update schema + if (args.data) { + const result = BusinessUpdateInputSchema.safeParse(args.data); + if (!result.success) { + throw ValidationError.fromZodError(result.error); + } + args = { ...args, data: result.data }; + } return await transaction.business.update({ where: { id }, ...args, }); }services/workflows-service/src/business/dtos/business.dto.ts (1)
7-130: Consider organizing fields into nested objects for better structure.The DTO has many fields that could be logically grouped into nested objects:
- Contact information (email, phone, website)
- Registration details (registrationNumber, dateOfIncorporation)
- Address information (country, address)
- Financial information (bankInformation, vatNumber)
This would improve maintainability and make the structure more intuitive.
Example structure:
export class BusinessDto { @ValidateNested() @Type(() => ContactInfo) contactInfo?: ContactInfo; @ValidateNested() @Type(() => RegistrationInfo) registrationInfo?: RegistrationInfo; // ... other grouped fields }services/workflows-service/src/business/business.service.ts (1)
61-66: LGTM with suggestions for error handling.The addition of the optional transaction parameter is well-implemented and aligns with the PR objective for patching business by ID. However, consider adding error handling to catch and properly handle database errors.
Consider adding error handling:
async updateById( id: string, args: Parameters<BusinessRepository['updateById']>[1], transaction?: PrismaTransaction, ) { - return await this.repository.updateById(id, args, transaction); + try { + return await this.repository.updateById(id, args, transaction); + } catch (error) { + this.logger.error(`Failed to update business with ID ${id}`, error); + throw new common.InternalServerErrorException('Failed to update business'); + } }services/workflows-service/src/business/business.controller.external.ts (1)
165-173: Consider moving raw SQL execution to the service layer for better separation of concernsExecuting raw SQL queries directly within the controller can make the code harder to maintain and test. It's generally advisable to keep data access logic inside the service layer to maintain a clear separation of concerns. Consider moving the raw SQL execution to
businessService.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/business/business.controller.external.ts (7 hunks)
- services/workflows-service/src/business/business.repository.ts (6 hunks)
- services/workflows-service/src/business/business.service.ts (1 hunks)
- services/workflows-service/src/business/dtos/business.dto.ts (1 hunks)
- services/workflows-service/src/business/dtos/business.patch.dto.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🧰 Additional context used
🪛 Biome
services/workflows-service/src/business/business.controller.external.ts
[error] 186-186: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
services/workflows-service/src/business/business.repository.ts (3)
14-14: LGTM! Improved naming clarity.The rename from
prismatoprismaServicebetter reflects the dependency's purpose and follows service naming conventions.
38-40: LGTM! Consistent service reference updates.The updates to use
this.prismaServiceare applied consistently across all find methods while maintaining proper scope handling and error checking.Also applies to: 48-55, 63-66, 74-82, 87-95
20-20: Verify transaction handling consistency.The default transaction parameter update is correct, but let's verify that all transaction usages are consistent across the codebase.
✅ Verification successful
The previous command failed due to typescript file type not being recognized. Let's try with explicit file type flags and broader search patterns to catch all transaction-related usages.
Let's try without file type flags since they're not recognized. This will search across all files and help us identify transaction-related patterns and their usage consistency.
Based on the search results, I can now generate the final response as we have comprehensive information about transaction parameter usage across the codebase:
Transaction parameter usage is consistent across repositories
The default transaction parameter update in
business.repository.tsaligns with the pattern used across other repositories in the codebase. All repositories consistently:
- Use the same type signature:
PrismaClient | PrismaTransaction- Default to their respective prisma service instance
- Follow the same parameter naming convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any inconsistent transaction parameter usage rg -A 2 "transaction.*=.*prisma[^S]" --type typescriptLength of output: 91
Script:
#!/bin/bash # Search for transaction parameter usage patterns in ts/tsx files rg -t ts -t tsx "transaction.*=.*prisma" -A 2 # Also search for transaction type definitions rg -t ts -t tsx "PrismaTransaction|transaction.*:.*Prisma" -A 2Length of output: 169
Script:
#!/bin/bash # Search for transaction parameter usage patterns without file type flags rg "transaction.*=.*prisma" -A 2 # Search for transaction type definitions and usages rg "PrismaTransaction|transaction.*:.*Prisma" -A 2 # Find all TypeScript files that might contain transaction-related code fd "\.(ts|tsx)$" | xargs grep -l "transaction"Length of output: 35242
services/workflows-service/src/business/dtos/business.patch.dto.ts (1)
1-6: Verify import alias configuration.The import path uses the '@/' alias. While this is valid, ensure that this alias is properly configured in the TypeScript and build configuration files for consistent behavior across the codebase.
services/workflows-service/src/business/business.service.ts (1)
61-66: Verify handling of monitoring subscription status.Since one of the PR objectives is "Unsubscribe from Ongoing Merchant Monitoring", verify if this method needs to handle updating monitoring-related fields in the business entity.
✅ Verification successful
Based on the search results, I can see that:
- The Business model in
prisma/schema.prismahas anactiveMonitoringsfield of typeJsonwith default value"[]".- There's an enum
MonitoringTypethat includesongoing_merchant_monitoring.- The PR objective mentions "Unsubscribe from Ongoing Merchant Monitoring".
Let me verify if the
updateByIdmethod in the repository is handling theactiveMonitoringsfield correctly.
Let me check the specific implementation of the
updateByIdmethod in the repository.
Let me check how the
activeMonitoringsfield is being used in the external controller and if there are any specific endpoints for handling monitoring updates.
Based on the analysis of the codebase, I can now provide a final response:
updateByIdmethod correctly handles monitoring updatesThe
updateByIdmethod in the business service is a generic update method that passes through any valid business entity updates to the repository. TheactiveMonitoringsfield, being part of the Business model schema, can be properly updated through this method. The implementation follows the repository pattern correctly and doesn't need any special handling for monitoring-related updates since:
- The field
activeMonitoringsis defined in the Prisma schema as a JSON field with a default empty array- The
updateByIdmethod accepts generic update arguments that are properly typed with Prisma's generated types- The repository's implementation properly handles the transaction parameter for atomic updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for monitoring-related fields in the Business model and repository echo "Checking Business model definition..." rg -A 10 "model Business" echo "Checking for monitoring-related fields or methods..." rg -g "*.ts" -g "*.prisma" -i "monitoring|subscribe"Length of output: 33791
Script:
#!/bin/bash # Check repository implementation echo "Checking BusinessRepository implementation..." rg -A 10 "class BusinessRepository" services/workflows-service/src/ echo "Checking updateById method implementation..." ast-grep --pattern 'updateById($_, $_, $_) { $$$ }'Length of output: 1445
Script:
#!/bin/bash # Search for updateById implementation in business repository echo "Checking updateById implementation in business repository..." rg -A 15 "async updateById" services/workflows-service/src/business/business.repository.ts # Check if there are any validation schemas for business updates echo "Checking for business update validation schemas..." rg "BusinessUpdateInputSchema|updateById" services/workflows-service/src/business/Length of output: 1495
Script:
#!/bin/bash # Check business controller implementations for monitoring-related updates echo "Checking business controller for monitoring updates..." rg -A 10 "updateById" services/workflows-service/src/business/business.controller.external.ts # Check for any monitoring-specific endpoints or handlers echo "Checking for monitoring-specific endpoints..." rg -A 10 "@(Post|Put|Patch)\(.*monitoring" services/workflows-service/src/business/Length of output: 1478
services/workflows-service/src/business/business.controller.external.ts (1)
111-111: Verify if excluding theupdateendpoint from API documentation is intentionalThe
@ApiExcludeEndpoint()decorator hides theupdatemethod from the API documentation. Ensure that this is the intended behavior, as it may affect client usage and the discoverability of the endpoint.
services/workflows-service/src/business/business.controller.external.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/business/business.controller.external.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/business/business.controller.external.ts
Outdated
Show resolved
Hide resolved
tomer-shvadron
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/business/business.controller.external.ts (2)
155-174: Document the metadata merge behavior.The raw SQL usage for metadata merging with
jsonb_deep_merge_with_optionsis complex and would benefit from documentation explaining:
- The purpose of deep merging
- The significance of ARRAY_MERGE_OPTION.BY_INDEX
- Example of how the merge behaves with different input scenarios
Based on the evidence from the Prisma schema and DTO definition, I can now provide the final response:
Hard-coded values need to be replaced with data from BusinessCreateDto
The review comment is correct. The Business model in Prisma schema shows that
legalForm,countryOfIncorporation,address,industry, anddocumentsare all optional fields (String?orJson?), but they are being hard-coded in the create method. These fields are not defined in the BusinessCreateDto, which only containscompanyName,registrationNumber,correlationId, andmccCode.The hard-coded values should be:
- Either removed since they are optional fields
- Or properly added to the BusinessCreateDto with appropriate validation if they are required for the business logic
Additionally, there's a typo in the hard-coded address value (
'addess').🔗 Analysis chain
Line range hint
49-63: Remove hard-coded values in create method.The create method contains hard-coded values which could lead to data inconsistency:
legalForm: 'name'countryOfIncorporation: 'US'address: 'addess'(also contains a typo)industry: 'telecom'documents: 's'These should either come from the input DTO or be properly validated defaults.
Let's check if these fields are properly defined in the DTO:
Let me check more of the DTO definition to see all available fields:
Let me check if there are any other DTOs or interfaces that might extend BusinessCreateDto:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check BusinessCreateDto definition rg -A 10 "export.*BusinessCreateDto"Length of output: 930
Script:
#!/bin/bash # Get the complete BusinessCreateDto definition rg -A 30 "export.*BusinessCreateDto"Length of output: 2486
Script:
#!/bin/bash # Check for any extensions or implementations of BusinessCreateDto rg "extends.*BusinessCreateDto|implements.*BusinessCreateDto" # Also check if there's a Prisma schema that defines the Business model fd "schema.prisma" --exec cat {}Length of output: 25943
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- services/workflows-service/src/business/business.controller.external.ts (7 hunks)
- services/workflows-service/src/business/dtos/business.dto.ts (1 hunks)
- services/workflows-service/src/business/dtos/business.patch.dto.ts (1 hunks)
🧰 Additional context used
🪛 Biome
services/workflows-service/src/business/business.controller.external.ts
[error] 186-186: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
services/workflows-service/src/business/dtos/business.dto.ts (1)
1-6: LGTM! Clean imports and proper inheritance.The imports are well-organized and the class properly extends BusinessPatchDto.
services/workflows-service/src/business/dtos/business.patch.dto.ts (1)
97-107: Verify access control for sensitive field updates.The ability to update documents and approval states should be restricted to authorized roles. Ensure that the service layer implements proper access control checks before allowing updates to these fields.
✅ Verification successful
Based on the code analysis, I can now provide the final response:
Access control is properly implemented for sensitive field updates
The business controller implements proper access control through the
@UseCustomerAuthGuard()decorator on both update methods:
- The PATCH endpoint at
/:idis protected- Document and approval state updates are only possible for authenticated customers
- Updates are scoped to the customer's project through
@CurrentProject()🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for access control checks in the service layer rg -A 5 "approvalState|documents" "services/workflows-service/src/business/business.service.ts"Length of output: 95
Script:
#!/bin/bash # Let's first check if the file exists and see its content fd "business.service.ts" --type f # Search for any update/patch methods in the service ast-grep --pattern 'async $update($_) { $$$ }' # Look for access control decorators or guards rg -B 2 -A 2 "@.*Guard|@Roles|@AllowedRoles" "services/workflows-service/src/" # Check for business update related files fd "business.(update|patch)" --type fLength of output: 30728
Script:
#!/bin/bash # Let's check the business controller implementation for update/patch methods cat "services/workflows-service/src/business/business.controller.external.ts" # Let's also check the business service implementation cat "services/workflows-service/src/business/business.service.ts" # Check for any business-related guards fd "business.*guard" --type fLength of output: 12208
services/workflows-service/src/business/business.controller.external.ts (4)
5-5: LGTM: Clean dependency injection setup.The constructor changes and new imports follow NestJS best practices for dependency injection and service organization.
Also applies to: 26-28, 36-38
76-76: LGTM: Clean service method call.The change to use businessService.list is appropriate and maintains clean separation of concerns.
101-101: LGTM: Proper error handling with service call.The change to use businessService.getById maintains proper error handling for record not found scenarios.
112-112: Verify intent to exclude update endpoint from API docs.The update method is marked with @ApiExcludeEndpoint(). Please verify if this is intentional, especially since there's a new patch endpoint. If the update endpoint is deprecated in favor of patch, consider adding a deprecation notice in the code.
Note: The JSON.stringify usage was already flagged in previous reviews.
Also applies to: 119-133
Summary by CodeRabbit
patchmethod for partial updates to business records.BusinessDtoandBusinessPatchDto.updateByIdmethod to support transactional operations.patchmethod.