-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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) |
Persistent review updated to latest commit 6fe5693 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same response:
#2303 (comment)
// Used for exhaustive check | ||
inlineRule satisfies never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…-alerts-merge Bal 1880 ongoing moitoring alerts merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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@ApiProperty
should be replaced with actual values before deployment.
Line range hint
25-25
: Consider providing a specific type foroptions
in theevaluateRiskScore
case 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 increateBulk
method 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 foradditionalInfo
casting.- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_RISK
alert definition inMERCHANT_MONITORING_ALERT_DEFINITIONS
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theevaluateHighTransactionTypePercentage
method.
Line range hint
205-238
: Adopt parameterized queries in theevaluateTransactionLimitHistoricAverageInbound
method to prevent SQL injection risks.
Line range hint
239-255
: Implement parameterized queries in theevaluatePaymentUnexpected
method to enhance security against SQL injection.
Line range hint
256-290
: Use parameterized queries in theevaluateDormantAccount
method to safeguard against SQL injection vulnerabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
@@ -4,6 +4,7 @@ export type TExecutionDetails = { | |||
checkpoint: { | |||
hash: string; | |||
}; | |||
subject: Array<Record<string, unknown>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.ts
for fetching transaction and merchant monitoring alerts.alert.service.ts
to handle alert queries based on monitoring type.Changes walkthrough
generate-alerts.ts
Enhance Alert Definitions and Seeding Logic
services/workflows-service/scripts/alerts/generate-alerts.ts
merchant monitoring.
on the new definitions.
seed.ts
Update Seeding Script with New Alert Functions
services/workflows-service/scripts/seed.ts
calls for transaction and merchant monitoring alerts.
alert.controller.external.ts
Implement API Endpoints for Different Types of Alerts
services/workflows-service/src/alert/alert.controller.external.ts
and response structures.
monitoring type.
alert.service.ts
Refactor Alert Service to Support Monitoring Types
services/workflows-service/src/alert/alert.service.ts
getAlerts
method to accept a monitoring type parameter todifferentiate alert queries.
additional details.
business-report.controller.ts
Add Business Report Controller and Fetch Endpoint
services/workflows-service/src/business-report/business-report.controller.ts
filters.
business-report.service.ts
Implement Business Report Fetching Logic in Service
services/workflows-service/src/business-report/business-report.service.ts
capabilities.
Summary by CodeRabbit
New Features
Enhancements
Documentation