fix: corrected home page merchants metrics source of truth (BAL-3396, BAL-3397) #2968
Conversation
|
|
Warning Rate limit exceeded@r4zendev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces modifications to the Changes
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/workflows-service/src/business-report/business-report.controller.external.ts (1)
172-182: Consider adjusting date range comparison operators.The current implementation might miss edge cases. Consider using:
gte(greater than or equal) for thefromdatelt(less than) for thetodateThis ensures consistent date range boundaries and follows the common practice of inclusive start and exclusive end dates.
metadata: { path: ['featureConfig', FEATURE_LIST.ONGOING_MERCHANT_REPORT, 'disabledAt'], not: 'null', - ...(from && { gt: dayjs(from).toDate().getTime() }), - ...(to && { lte: dayjs(to).toDate().getTime() }), + ...(from && { gte: dayjs(from).toDate().getTime() }), + ...(to && { lt: dayjs(to).toDate().getTime() }), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/workflows-service/src/business-report/business-report.controller.external.ts(2 hunks)services/workflows-service/src/business/business.controller.external.ts(1 hunks)
⏰ 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 (4)
services/workflows-service/src/business/business.controller.external.ts (1)
158-165: LGTM! Improved state management for merchant monitoring.The changes enhance tracking of merchant monitoring state by:
- Introducing a clear
isEnabledflag- Adding a
disabledAttimestamp when monitoring is disabledservices/workflows-service/src/business-report/business-report.controller.external.ts (3)
47-47: LGTM! Added dayjs for consistent date handling.The import of dayjs ensures consistent date handling across the application.
184-193: LGTM! Correctly combines metrics with removed merchants count.The implementation properly:
- Fetches metrics from the merchant monitoring client
- Adds the count of removed merchants
- Returns a combined response
172-193: Verify single source of truth for removed merchants metric.Let's ensure there are no conflicting implementations of the "removed merchants" metric.
✅ Verification successful
Implementation verified as single source of truth
The "removed merchants" metric is consistently implemented and has a clear data flow from the backend calculation through to the UI display, with no conflicting implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of the disabledAt field or removed merchants metric echo "Checking for other uses of disabledAt field..." rg -A 5 "disabledAt.*ONGOING_MERCHANT_REPORT" echo "Checking for other implementations of removed merchants metric..." rg -A 5 "removedMerchants"Length of output: 5076
r4zendev
changed the title
services/workflows-service/src/business-report/business-report.controller.external.ts
Outdated
Show resolved
Hide resolved
r4zendev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/workflows-service/src/business/business.service.ts (1)
97-117: Simplify complex metadata parsing with a helper function.The metadata parsing logic is complex and could be moved to a separate helper function for better readability and reusability.
+ private parseFeatureDisabledAt(metadata: unknown): number | null { + return z + .number() + .nullable() + .catch(() => null) + .parse( + ( + metadata as { + featureConfig: Record< + (typeof FEATURE_LIST)[keyof typeof FEATURE_LIST], + TCustomerFeaturesConfig & { disabledAt: number | null | undefined } + >; + } + )?.featureConfig?.[FEATURE_LIST.ONGOING_MERCHANT_REPORT]?.disabledAt, + ); + } async getMerchantMonitoringMetrics({ // ... existing parameters }) { // ... existing date handling const totalActiveMerchants = allProjectMerchants.filter(b => { - const disabledAt = z - .number() - .nullable() - .catch(() => null) - .parse( - ( - b.metadata as { - featureConfig: Record< - (typeof FEATURE_LIST)[keyof typeof FEATURE_LIST], - TCustomerFeaturesConfig & { disabledAt: number | null | undefined } - >; - } - )?.featureConfig?.[FEATURE_LIST.ONGOING_MERCHANT_REPORT]?.disabledAt, - ); + const disabledAt = this.parseFeatureDisabledAt(b.metadata); return ( disabledAt === null || (b.metadata === null && features?.ONGOING_MERCHANT_REPORT?.options?.runByDefault) ); }).length;services/workflows-service/src/business-report/business-report.controller.external.ts (1)
169-192: Consider handling potential errors when combining metrics.The implementation looks good but could benefit from error handling when combining metrics from different sources. If either call fails, the entire metrics endpoint will fail.
async getMetrics( @CurrentProject() currentProjectId: TProjectId, @Query() { from, to }: BusinessReportMetricsRequestQueryDto, ) { const { id: customerId, features } = await this.customerService.getByProjectId( currentProjectId, ); + try { const { totalActiveMerchants, addedMerchantsCount, unmonitoredMerchants } = await this.businessService.getMerchantMonitoringMetrics({ projectIds: [currentProjectId], features, from, to, }); const merchantMonitoringMetrics = await this.merchantMonitoringClient.getMetrics({ customerId, from, to, }); return { ...merchantMonitoringMetrics, totalActiveMerchants, addedMerchantsCount, removedMerchantsCount: unmonitoredMerchants, }; + } catch (error) { + this.logger.error('Failed to retrieve metrics', { error }); + throw new common.InternalServerErrorException('Failed to retrieve metrics'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/business/business.controller.external.ts(1 hunks)services/workflows-service/src/business/business.repository.ts(1 hunks)services/workflows-service/src/business/business.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/business/business.controller.external.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: test_windows
- GitHub Check: build (windows-latest)
- GitHub Check: test_linux
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (3)
services/workflows-service/src/business/business.repository.ts (1)
110-117: LGTM! Well-implemented count method.The implementation correctly uses project scoping and follows the repository pattern. The method signature aligns with Prisma's patterns, making it type-safe and consistent with the codebase.
services/workflows-service/src/business/business.service.ts (2)
163-168: LGTM! Clear and consistent return structure.The return object provides a clean interface with well-named metrics.
142-161: Verify the date comparison logic for unmonitored merchants.The date comparison in the unmonitored merchants query uses
getTime()which might not match the format of stored timestamps. Also, consider extracting the common query conditions into a shared variable.✅ Verification successful
The timestamp comparison logic is correct and safe to use.
The
metadatafield is a JSON type in PostgreSQL that can handle numeric timestamp comparisons. The current implementation usinggetTime()correctly converts dates to Unix timestamps in milliseconds, which is a valid format for JSON field comparisons in PostgreSQL.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how timestamps are stored in the database ast-grep --pattern 'metadata: { path: ["featureConfig", $_, "disabledAt"], $$$ }'Length of output: 87
Script:
#!/bin/bash # Check Prisma schema for metadata field definition fd -e prisma | xargs cat # Find similar metadata timestamp comparisons rg -A 2 -B 2 '"featureConfig".+"disabledAt"' --type ts # Look for other timestamp comparisons with metadata rg "metadata.*getTime\(\)" --type tsLength of output: 27551
r4zendev
changed the title
Summary by CodeRabbit
New Features
Improvements