Sync new customers from the WF service to the Unified API #2786
Conversation
|
WalkthroughThis pull request introduces several updates to the workflows service. The version of the service is incremented to Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 8
🧹 Outside diff range and nitpick comments (13)
services/workflows-service/src/business-report/business-report.module.ts (1)
18-21: Reminder: Consider follow-up tasks for the new external controller.With the addition of
BusinessReportControllerExternal, please ensure the following:
- Update API documentation to reflect any new endpoints.
- Verify that appropriate authentication and authorization are in place for the new external routes.
- If this introduces breaking changes to the API, consider versioning and communicate changes to API consumers.
- Update any relevant tests to cover the new controller.
services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts (4)
120-122: LGTM:createCustomermethod implemented correctly.The
createCustomermethod is well-implemented, following RESTful API practices for creating a new resource. The use of theCustomertype ensures type safety for the payload.Consider adding specific error handling for this method to provide more detailed error information or perform custom error handling if needed. For example:
public async createCustomer(payload: Customer) { try { return await this.axiosInstance.post('/customers', payload); } catch (error) { this.logger.error('Error creating customer', error); throw error; } }
124-126: LGTM:updateCustomermethod implemented correctly.The
updateCustomermethod is well-implemented, following RESTful API practices for updating an existing resource. The use of theCustomertype ensures type safety for the payload.Consider adding specific error handling for this method to provide more detailed error information or perform custom error handling if needed. For example:
public async updateCustomer(id: string, payload: Customer) { try { return await this.axiosInstance.put(`/customers/${id}`, payload); } catch (error) { this.logger.error(`Error updating customer with id ${id}`, error); throw error; } }
128-130: LGTM:deleteCustomermethod implemented correctly.The
deleteCustomermethod is well-implemented, following RESTful API practices for deleting an existing resource.Consider adding specific error handling for this method to provide more detailed error information or perform custom error handling if needed. For example:
public async deleteCustomer(id: string) { try { return await this.axiosInstance.delete(`/customers/${id}`); } catch (error) { this.logger.error(`Error deleting customer with id ${id}`, error); throw error; } }
120-130: LGTM: Customer management methods added successfully.The addition of
createCustomer,updateCustomer, anddeleteCustomermethods enhances the functionality of theUnifiedApiClientclass, providing essential CRUD operations for customer management. The implementation is consistent across all three methods.Consider adding
getCustomerandlistCustomersmethods to complete the set of CRUD operations:public async getCustomer(id: string) { return await this.axiosInstance.get(`/customers/${id}`); } public async listCustomers(params?: { limit?: number; offset?: number }) { return await this.axiosInstance.get('/customers', { params }); }This would provide a comprehensive set of customer management operations in the
UnifiedApiClientclass.services/workflows-service/package.json (1)
Line range hint
1-100: Consider performing a dependency audit and cleanupThe
package.jsonfile contains a large number of dependencies. Consider the following suggestions:
- Run
npm auditto check for any security vulnerabilities in the dependencies.- Use a tool like
npm-check-updatesto identify outdated packages that could be updated.- Review the list of dependencies and devDependencies to remove any that are no longer used in the project.
- Consider standardizing the version specifiers (e.g., using
^for all non-critical dependencies) for consistency.These steps can help maintain a cleaner and more secure dependency tree.
Also applies to: 101-200
services/workflows-service/src/business-report/business-report.controller.external.ts (3)
9-12: Review Swagger decorators on an excluded controllerSince the controller is excluded from Swagger documentation using
@ApiExcludeController(), the other Swagger decorators (@ApiBearerAuth(),@ApiTags('Business Reports')) will not have any effect.Consider removing these unnecessary decorators to keep the code clean:
-@ApiBearerAuth() -@ApiTags('Business Reports') ... -@ApiExcludeController() +@ApiExcludeController()
11-11: Reevaluate the route path for the controllerMapping the controller to
'external/business-reports'while usingAdminAuthGuardsuggests that it's intended for internal use by administrators. Using the term'external'in the route might be misleading.Consider changing the route to better reflect its purpose and enhance clarity:
-@Controller('external/business-reports') +@Controller('admin/business-reports')Or, if it's meant for external admins:
-@Controller('external/business-reports') +@Controller('external/admin/business-reports')
21-29: Handle potential performance issues with deep nested includesThe
findManyquery includes nested relations (projectandcustomer). Deep nesting can lead to performance issues due to complex SQL queries.Consider selecting only the necessary fields or using lazy loading if supported. Alternatively, fetch related data in separate queries if appropriate.
Example:
return await this.prisma.businessReport.findMany({ select: { id: true, name: true, project: { select: { id: true, name: true, customer: { select: { id: true, name: true, }, }, }, }, }, });services/workflows-service/src/customer/customer.service.ts (1)
83-92: Consider using static import for 'p-retry'Using dynamic
import('p-retry')within theretryfunction may introduce unnecessary overhead every time the function is called. Importingp-retrystatically at the top of the file can improve performance.Apply this change:
+ import pRetry from 'p-retry'; const retry = async (fn: () => Promise<unknown>) => { - const { default: pRetry } = await import('p-retry'); return await pRetry(fn, { retries: 5, randomize: true, minTimeout: 100, maxTimeout: 10_000, }); };services/workflows-service/src/customer/customer.repository.ts (3)
2-2: Question on the necessity of importingPrismaClientThe
PrismaClientis imported but not used directly in this file. SincePrismaServiceextendsPrismaClient, and you already havePrismaTransactionfor transaction types, consider removing the unusedPrismaClientimport to keep the code clean.
Line range hint
12-125: Action Required: Update method calls to includetransactionwhere necessaryWith the addition of the
transactionparameter to several methods, please ensure that all calls to these methods are updated accordingly. Missing thetransactionparameter in calls that require transactional context might lead to inconsistent data states.
111-111: Type Assertion in Update MethodIn the
updateByIdmethod, there's a type assertion:return transaction.customer.update<T & { where: { id: string } }>({ ... });Ensure that this type assertion is necessary and doesn't mask any underlying type issues. If possible, refactor to eliminate the need for explicit type assertions to maintain type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
- services/workflows-service/package.json (1 hunks)
- services/workflows-service/src/business-report/business-report.controller.external.ts (1 hunks)
- services/workflows-service/src/business-report/business-report.module.ts (1 hunks)
- services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts (2 hunks)
- services/workflows-service/src/customer/customer.repository.ts (3 hunks)
- services/workflows-service/src/customer/customer.service.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
services/workflows-service/src/business-report/business-report.module.ts (2)
18-18: LGTM: New controller import added correctly.The import statement for
BusinessReportControllerExternalfollows the existing naming conventions and import structure of the file.
21-21: LGTM: New controller added to the module.The
BusinessReportControllerExternalhas been correctly added to thecontrollersarray in the@Moduledecorator.To ensure that the new controller is properly implemented and doesn't conflict with existing routes, let's verify its contents:
services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts (1)
4-4: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include the
Customertype from@prisma/client, which is consistent with the new customer management methods added to the class.services/workflows-service/package.json (2)
Line range hint
3-3: LGTM: Version updateThe version update to
0.7.54follows semantic versioning conventions. This patch version increment suggests bug fixes or minor changes have been made.
100-100: Verify usage of new dependencyThe addition of
p-retry(^6.2.0) is a good choice for improving the reliability of promise-based operations. However, please ensure that this library is actually used in the codebase to avoid unnecessary dependencies.To verify the usage of
p-retry, run the following script:services/workflows-service/src/business-report/business-report.controller.external.ts (1)
17-17: Ensure proper authentication and authorizationUsing
@UseGuards(AdminAuthGuard)secures the endpoint, but verify that the guard correctly checks for admin privileges and that unauthorized access is appropriately handled.Run the following script to check for other endpoints using
AdminAuthGuardand ensure consistency:✅ Verification successful
AdminAuthGuard is correctly implemented and consistently applied
The
AdminAuthGuardeffectively checks for admin privileges across all secured endpoints, and unauthorized access is appropriately handled.
- Verified in the following files:
workflow.controller.internal.tsalert.controller.internal.tsuser.controller.internal.tscustomer.controller.internal.tsbusiness-report.controller.external.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of AdminAuthGuard to verify its implementation and consistency. # Search for AdminAuthGuard usage in the codebase rg 'AdminAuthGuard' --type ts -A 5 -B 2Length of output: 10872
services/workflows-service/src/customer/customer.service.ts (2)
7-8: Imports added correctlyThe imports for
UnifiedApiClientandPrismaServiceare correctly added and will allow for proper interaction with the external API and database transactions.
15-15: PrismaService injected successfullyThe
PrismaServiceis properly injected into the constructor, enabling transactional database operations.services/workflows-service/src/customer/customer.repository.ts (4)
4-4: Verify the definition and usage ofPrismaTransactionEnsure that
PrismaTransactionfrom@/typesaccurately represents the Prisma transaction client. This verification is important for type safety when thetransactionparameter is used across methods.
12-14: Enhancement: Added transaction support tocreatemethodAdding the
transactionparameter to thecreatemethod enhances flexibility by allowing database operations to be part of a transaction context when needed. This is a good practice for maintaining data consistency.
106-111: Consistent transaction handling inupdateByIdmethodIncluding the
transactionparameter and utilizing it in theupdateByIdmethod aligns with transactional best practices. This consistency across methods ensures that updates can be properly managed within transactions.
123-125: Addition of transaction parameter indeleteByIdmethodBy introducing the
transactionparameter to thedeleteByIdmethod and using it for the delete operation, you're enabling transactional control over deletions. This is beneficial for operations that require atomicity.
services/workflows-service/src/business-report/business-report.controller.external.ts
Show resolved
Hide resolved
services/workflows-service/src/business-report/business-report.controller.external.ts
Show resolved
Hide resolved
services/workflows-service/src/business-report/business-report.controller.external.ts
Show resolved
Hide resolved
services/workflows-service/src/business-report/business-report.controller.external.ts
Show resolved
Hide resolved
services/workflows-service/src/business-report/business-report.controller.external.ts
Show resolved
Hide resolved
services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Improvements
Dependency Updates