bal 1880 ongoing moitoring alerts #2303
Conversation
|
WalkthroughThe recent updates focus on enhancing the alert management system in the workflows service. New types for different monitoring scenarios were introduced, along with specific methods to handle transaction and business report alerts. These changes aim to refine data handling and response structuring for better clarity and functionality in alert processing. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (2)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
PR Description updated to latest commit (6fe5693) |
PR Review(Review updated until commit 6fe5693)
Code feedback:
✨ Review tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
See the improve usage page for a comprehensive guide on using this tool. |
|
PR Description updated to latest commit (6fe5693) |
|
PR Description updated to latest commit (6fe5693) |
github-actions
bot
added
Review effort [1-5]: 3
and removed
Review effort [1-5]: 4
labels
|
Persistent review updated to latest commit 6fe5693 |
github-actions
bot
added
Review effort [1-5]: 4
and removed
Review effort [1-5]: 3
labels
|
Persistent review updated to latest commit 6fe5693 |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
See the improve usage page for a comprehensive guide on using this tool. |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
See the improve usage page for a comprehensive guide on using this tool. |
services/workflows-service/src/business-report/business-report.controller.spec.ts
Outdated
Show resolved
Hide resolved
| | { | ||
| // since we don't know the other options, we can use never | ||
| fnName: unknown; | ||
| options: never; | ||
| } |
There was a problem hiding this comment.
Why? I think it's better to remove it, so we'll have an exhaustive list for now.
There was a problem hiding this comment.
same response:
#2303 (comment)
| // Used for exhaustive check | ||
| inlineRule satisfies never; |
There was a problem hiding this comment.
This line is important to verify we didn't miss any function. I think it's better you'll revert this line removal.
There was a problem hiding this comment.
It just allow TS show the rest of the code as relevant instead of such a gray which it wont get this section
There was a problem hiding this comment.
with my type changes its looks like that...
…-alerts-merge Bal 1880 ongoing moitoring alerts merge
![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
Out of diff range and nitpick comments (11)
services/workflows-service/src/alert/dtos/create-alert-definition.dto.ts (2)
Line range hint
9-9: The example values provided in@ApiPropertyshould be replaced with actual values before deployment.
Line range hint
25-25: Consider providing a specific type foroptionsin theevaluateRiskScorecase to ensure type safety.- options?: undefined; + options: RiskScoreOptions;services/workflows-service/src/transaction/transaction.service.ts (1)
Line range hint
38-59: Ensure that the error handling increateBulkmethod logs all errors, not just unique constraint errors.- this.sentry.captureException(errorToLog); + this.sentry.captureException(error);services/workflows-service/src/transaction/transaction.mapper.ts (3)
Line range hint
14-14: Consider improving type safety foradditionalInfocasting.- additionalInfo: (dto.additionalInfo ?? null) as unknown as InputJsonValue, + additionalInfo: dto.additionalInfo ? JSON.parse(dto.additionalInfo) : null,
Line range hint
14-14: Refine error messages for clarity ingetCounterpartyCreateInput.- throw new HttpException('Counterparty must have either business or end user data.', 400); + throw new HttpException('Counterparty must have business data.', 400); - throw new HttpException('Counterparty must have either business or end user data.', 400); + throw new HttpException('Counterparty must not have both business and end user data.', 400);
Line range hint
14-14: Add error handling for required fields inaltDtoToOriginalDto.+ if (!altDto.tx_id || !altDto.tx_amount) { + throw new Error('Required fields tx_id and tx_amount are missing.'); + }services/workflows-service/scripts/alerts/generate-alerts.ts (2)
Line range hint
14-14: Consider externalizing configuration values inTRANSACTIONS_ALERT_DEFINITIONS.
Line range hint
14-14: Add error handling for Prisma operations ingenerateAlertDefinitions.services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts (3)
Line range hint
14-14: Add more detailed assertions to verify transaction data in the test casecreates a transaction with business originator and end user beneficiary.
Line range hint
14-14: Expand the test case to cover more types of validation errors inreturns 400 when validation fails.
Line range hint
14-14: Refine the error message for clarity in the test casereturns 400 when counterparty is missing from the body.- message: 'Originator and beneficiary are required.', + message: 'Transaction request must include both originator and beneficiary data.',
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
services/workflows-service/scripts/alerts/generate-alerts.ts (1)
433-445: Add a meaningful description to theMRCNT_RISKalert definition inMERCHANT_MONITORING_ALERT_DEFINITIONSto provide better context and information for users and developers.
| const alertDefinitions = await this.getAlertDefinitions({ | ||
| type: MonitoringType.transaction_monitoring, | ||
| }); |
There was a problem hiding this comment.
Consider refactoring the checkAllAlerts method to allow dynamic passing of the monitoringType. Currently, it is hardcoded to MonitoringType.transaction_monitoring, which may limit flexibility given the PR's objectives to handle multiple monitoring types.
| private async checkAlert(alertDefinition: AlertDefinition, ...args: any[]) { | ||
| const unknownData: unknown = alertDefinition.inlineRule; | ||
|
|
||
| const inlineRule: InlineRule = unknownData as InlineRule; | ||
|
|
||
| const ruleExecutionResults = await this.dataAnalyticsService.runInlineRule( | ||
| alertDefinition.projectId, | ||
| inlineRule, | ||
| args, |
There was a problem hiding this comment.
Add type checks before casting unknownData to InlineRule in the checkAlert method to prevent potential runtime errors if the data does not match the expected structure.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (5)
services/workflows-service/src/data-analytics/data-analytics.service.ts (5)
Line range hint
77-177: Consider using parameterized queries instead of raw SQL to enhance security and prevent potential SQL injection vulnerabilities.
Line range hint
178-204: Ensure the use of parameterized queries to mitigate the risk of SQL injection in theevaluateHighTransactionTypePercentagemethod.
Line range hint
205-238: Adopt parameterized queries in theevaluateTransactionLimitHistoricAverageInboundmethod to prevent SQL injection risks.
Line range hint
239-255: Implement parameterized queries in theevaluatePaymentUnexpectedmethod to enhance security against SQL injection.
Line range hint
256-290: Use parameterized queries in theevaluateDormantAccountmethod to safeguard against SQL injection vulnerabilities.
| @@ -4,6 +4,7 @@ export type TExecutionDetails = { | |||
| checkpoint: { | |||
| hash: string; | |||
| }; | |||
| subject: Array<Record<string, unknown>>; | |||
There was a problem hiding this comment.
Consider specifying more detailed types for subject if possible to enhance type safety.
User description
Curl command:
Type
Enhancement, Tests
Description
generate-alerts.ts.alert.controller.external.tsfor fetching transaction and merchant monitoring alerts.alert.service.tsto handle alert queries based on monitoring type.Changes walkthrough
generate-alerts.ts
Enhance Alert Definitions and Seeding Logicservices/workflows-service/scripts/alerts/generate-alerts.ts
merchant monitoring.
on the new definitions.
seed.ts
Update Seeding Script with New Alert Functionsservices/workflows-service/scripts/seed.ts
calls for transaction and merchant monitoring alerts.
alert.controller.external.ts
Implement API Endpoints for Different Types of Alertsservices/workflows-service/src/alert/alert.controller.external.ts
and response structures.
monitoring type.
alert.service.ts
Refactor Alert Service to Support Monitoring Typesservices/workflows-service/src/alert/alert.service.ts
getAlertsmethod to accept a monitoring type parameter todifferentiate alert queries.
additional details.
business-report.controller.ts
Add Business Report Controller and Fetch Endpointservices/workflows-service/src/business-report/business-report.controller.ts
filters.
business-report.service.ts
Implement Business Report Fetching Logic in Serviceservices/workflows-service/src/business-report/business-report.service.ts
capabilities.
Summary by CodeRabbit
New Features
Enhancements
Documentation