Store merchants in the Unified API DB #2976
Conversation
|
WalkthroughThis pull request introduces several modifications across the workflows service, focusing on business and customer-related functionality. The changes include updating the business repository, enhancing the business service with unified API client integration, adding new methods to controllers, and introducing more robust transaction and error handling. The modifications aim to improve data management, synchronization, and feature configuration for businesses and customers. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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: 5
🧹 Nitpick comments (4)
services/workflows-service/src/business/business.service.ts (1)
315-316: Use static import forp-retryinstead of dynamic importDynamically importing
p-retryinside theretryfunction can introduce unnecessary overhead. Consider importingp-retrystatically at the top of the file for better performance and readability.Apply this diff:
+ 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/common/utils/unified-api-client/unified-api-client.ts (2)
82-89: Add error handling for the API call increateOrUpdateBusinessThe
createOrUpdateBusinessmethod performs an API call without error handling. To improve resilience and debugging, wrap the API call in a try-catch block and handle exceptions appropriately.For example:
public async createOrUpdateBusiness(payload: BusinessPayload) { const formattedPayload = this.formatBusiness(payload); + try { return await this.axiosInstance.put( `/customers/${payload.customerId}/merchants/${payload.id}`, formattedPayload, ); + } catch (error) { + this.logger.error('Failed to create or update business', error); + throw error; + } }
91-113: Improve type safety and readability informatBusinessThe use of type assertions and nested ternary operators can impact code clarity and safety. Define a specific type for
metadataand refactor the logic to enhance readability and maintainability.Define an interface for
BusinessMetadata:+ interface BusinessMetadata { + featureConfig: TCustomerWithFeatures['features']; + lastOngoingReportInvokedAt: number; + }Refactor the
formatBusinessmethod:public formatBusiness(business: BusinessPayload) { - const metadata = business.metadata as unknown as { - featureConfig: TCustomerWithFeatures['features']; - lastOngoingReportInvokedAt: number; - } | null; + const metadata = business.metadata as BusinessMetadata | null; + let unsubscribedMonitoringAt: Date | null = null; + const ongoingReport = metadata?.featureConfig?.[FEATURE_LIST.ONGOING_MERCHANT_REPORT]; + if (ongoingReport?.disabledAt) { + unsubscribedMonitoringAt = new Date(ongoingReport.disabledAt); + } else if (ongoingReport?.enabled === false) { + unsubscribedMonitoringAt = new Date(); + } return { id: business.id, correlationId: business.correlationId, companyName: business.companyName, customerId: business.customerId, unsubscribedMonitoringAt: unsubscribedMonitoringAt?.toISOString() ?? null, createdAt: business.createdAt.toISOString(), updatedAt: business.updatedAt.toISOString(), }; }services/workflows-service/src/business/business.controller.internal.ts (1)
51-89: Verify error handling for the sync endpoint.The sync endpoint looks good but could benefit from error handling and pagination for large datasets.
Consider adding:
- Error handling for database queries
- Pagination to handle large datasets
- Rate limiting for the admin endpoint
@common.Get('sync') @common.UseGuards(AdminAuthGuard) @ApiExcludeEndpoint() async getAllBusinesses() { + try { const businesses = await this.repository.findManyUnscoped({ select: { id: true, createdAt: true, updatedAt: true, correlationId: true, companyName: true, metadata: true, project: { select: { customer: { select: { id: true } }, }, }, }, where: { project: { customer: { config: { path: ['isMerchantMonitoringEnabled'], equals: true, }, }, }, }, + take: 100, // Add pagination + skip: 0, }); const unifiedApiClient = new UnifiedApiClient(); return businesses.map(business => { return unifiedApiClient.formatBusiness({ ...business, customerId: business.project.customer.id, }); }); + } catch (error) { + throw new common.InternalServerErrorException('Failed to sync businesses'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(0 hunks)services/workflows-service/src/business/business.controller.external.ts(1 hunks)services/workflows-service/src/business/business.controller.internal.ts(2 hunks)services/workflows-service/src/business/business.repository.ts(4 hunks)services/workflows-service/src/business/business.service.ts(3 hunks)services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts(2 hunks)services/workflows-service/src/customer/types.ts(1 hunks)services/workflows-service/src/workflow/workflow.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- services/workflows-service/src/business-report/business-report.controller.external.ts
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (5)
services/workflows-service/src/business/business.controller.internal.ts (1)
25-29: LGTM! Constructor dependencies properly injected.The constructor has been updated to include BusinessRepository and PrismaService, which are required for the new sync functionality.
services/workflows-service/src/business/business.repository.ts (3)
42-46: LGTM! New unscoped query method added.The findManyUnscoped method is correctly implemented to bypass project scope filters, which is necessary for the sync functionality.
64-72: LGTM! Transaction support added to findByIdUnscoped.The method now accepts an optional transaction parameter, maintaining consistency with other repository methods.
109-109: LGTM! Return type updated for better type safety.The updateById method now explicitly returns Promise instead of the implicit return type.
services/workflows-service/src/business/business.controller.external.ts (1)
170-174: LGTM! Raw SQL replaced with service method.The change from raw SQL to businessService.updateById improves maintainability and type safety.
| @@ -258,23 +258,6 @@ export class BusinessReportControllerExternal { | |||
| }); | |||
| } | |||
|
|
|||
| @common.Get('/sync') | |||
There was a problem hiding this comment.
This endpoint is no longer necessary. We completed the data migration.
| WHERE "id" = ${businessId} | ||
| AND "projectId" = ${currentProjectId}; | ||
| `; | ||
| await this.businessService.updateById(businessId, { |
There was a problem hiding this comment.
Updating has to go through the BusinessService to sync the new data with the Unified API.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
services/workflows-service/src/business/business.service.ts (2)
30-30: 🛠️ Refactor suggestionInject
UnifiedApiClientvia the constructor for better testability.Currently,
UnifiedApiClientis instantiated directly within the class. It's recommended to inject it via the constructor to adhere to the dependency inversion principle and improve testability by allowing for easier mocking.
41-75: 🛠️ Refactor suggestionMove HTTP calls outside the transaction.
Making external HTTP calls within a transaction can lead to prolonged transaction times and potential deadlocks. Move the HTTP calls outside the transaction.
Also applies to: 114-148
🧹 Nitpick comments (2)
services/workflows-service/src/business/business.controller.internal.ts (1)
54-87: Consider adding error handling and pagination.While the implementation is correct, consider these improvements:
- Add error handling for potential database errors
- Add pagination to handle large datasets efficiently
- Add logging for monitoring and debugging
Apply this diff:
@common.Get('sync') @common.UseGuards(AdminAuthGuard) @ApiExcludeEndpoint() async getAllBusinesses() { + this.logger.log('Starting business sync'); + try { const businesses = (await this.repository.findManyUnscoped({ + take: 100, // Add pagination select: { id: true, createdAt: true, updatedAt: true, correlationId: true, companyName: true, metadata: true, project: { select: { customer: { select: { id: true, config: true } }, }, }, }, where: { project: { customer: { config: { path: ['isMerchantMonitoringEnabled'], equals: true, }, }, }, }, })) as BusinessPayload[]; const unifiedApiClient = new UnifiedApiClient(); + this.logger.log(`Syncing ${businesses.length} businesses`); return businesses.map(business => unifiedApiClient.formatBusiness(business)); + } catch (error) { + this.logger.error('Failed to sync businesses', error); + throw new common.InternalServerErrorException('Failed to sync businesses'); + } }services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts (1)
96-118: Consider adding input validation.The formatBusiness method should validate the input payload to ensure all required fields are present and have the correct types.
Apply this diff:
public formatBusiness(business: BusinessPayload) { + if (!business?.id || !business?.project?.customer?.id) { + throw new Error('Invalid business payload: missing required fields'); + } const metadata = business.metadata as unknown as { featureConfig: TCustomerWithFeatures['features']; lastOngoingReportInvokedAt: number; } | null; const unsubscribedMonitoringAt = metadata?.featureConfig?.[FEATURE_LIST.ONGOING_MERCHANT_REPORT] ?.disabledAt ? new Date(metadata.featureConfig[FEATURE_LIST.ONGOING_MERCHANT_REPORT]!.disabledAt!) : metadata?.featureConfig?.[FEATURE_LIST.ONGOING_MERCHANT_REPORT]?.enabled === false ? new Date() : null; return { id: business.id, correlationId: business.correlationId, companyName: business.companyName, customerId: business.project.customer.id, unsubscribedMonitoringAt: unsubscribedMonitoringAt?.toISOString() ?? null, createdAt: business.createdAt.toISOString(), updatedAt: business.updatedAt.toISOString(), }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/backoffice-v2/src/pages/MerchantMonitoringUploadMultiple/hooks/useMerchantMonitoringUploadMultiplePageLogic/batch-report-template.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/business/business.controller.external.ts(1 hunks)services/workflows-service/src/business/business.controller.internal.ts(2 hunks)services/workflows-service/src/business/business.service.ts(3 hunks)services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/business/business.controller.external.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: build (windows-latest)
- GitHub Check: test_linux
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
services/workflows-service/src/business/business.controller.internal.ts (1)
28-32: LGTM! Dependencies are properly injected.The constructor properly injects the required dependencies, following NestJS best practices.
services/workflows-service/src/common/utils/unified-api-client/unified-api-client.ts (1)
9-14: LGTM! Type definition is well-structured.The BusinessPayload type properly defines the required fields and nested customer configuration.
services/workflows-service/src/business/business.service.ts (1)
290-299: LGTM! Retry mechanism is well implemented.The retry mechanism is properly implemented with:
- Reasonable retry count (5 attempts)
- Randomized delays
- Proper timeout bounds (100ms - 10s)
Note: The dynamic import of
p-retryis necessary due to ESM/CJS compatibility, as explained in the previous review comments.
| public async createOrUpdateBusiness(payload: BusinessPayload) { | ||
| if (!this.shouldUpdateBusiness(payload)) { | ||
| return; | ||
| } | ||
|
|
||
| const formattedPayload = this.formatBusiness(payload); | ||
|
|
||
| return await this.axiosInstance.put( | ||
| `/customers/${payload.project.customer.id}/merchants/${payload.id}`, | ||
| formattedPayload, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for API calls.
The createOrUpdateBusiness method should handle potential API errors and provide proper error context.
Apply this diff:
public async createOrUpdateBusiness(payload: BusinessPayload) {
if (!this.shouldUpdateBusiness(payload)) {
return;
}
const formattedPayload = this.formatBusiness(payload);
+ try {
return await this.axiosInstance.put(
`/customers/${payload.project.customer.id}/merchants/${payload.id}`,
formattedPayload,
);
+ } catch (error) {
+ this.logger.error('Failed to create/update business in Unified API', {
+ error,
+ businessId: payload.id,
+ customerId: payload.project.customer.id,
+ });
+ throw error;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Refactor