-
Notifications
You must be signed in to change notification settings - Fork 171
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
blokh/feat/business report alerts #2311
Conversation
|
WalkthroughThe recent updates introduce several key enhancements to the workflows service. Notably, the Changes
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 (fa0dd21) |
1 similar comment
PR Description updated to latest commit (fa0dd21) |
PR Review(Review updated until commit dc05878)
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. |
Persistent review updated to latest commit fa0dd21 |
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. |
…:ballerine-io/ballerine into blokh/feat/add-business-report-report-id
services/workflows-service/src/workflow/hook-callback-handler.service.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/workflow/hook-callback-handler.service.ts
Show resolved
Hide resolved
PR Description updated to latest commit (dc05878) |
Persistent review updated to latest commit dc05878 |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
See the improve usage page for a comprehensive guide on using this tool. |
# Conflicts: # services/workflows-service/prisma/data-migrations # services/workflows-service/src/business-report/business-report.repository.ts
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 (1)
services/workflows-service/src/workflow/cron/ongoing-monitoring.cron.ts (1)
74-74
: Logging level change approved; consider enhancing the log message.The change in logging level from error to debug is appropriate if the scenario is non-critical. However, consider adding more details to the log message to aid in debugging, such as the business name or other identifiers.
# Conflicts: # services/workflows-service/prisma/data-migrations
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: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!pnpm-lock.yaml
Files selected for processing (10)
- services/workflows-service/prisma/migrations/20240516085653_add_unique_report_id/migration.sql (1 hunks)
- services/workflows-service/prisma/schema.prisma (5 hunks)
- services/workflows-service/scripts/alerts/generate-alerts.ts (14 hunks)
- services/workflows-service/src/alert/alert.controller.external.ts (3 hunks)
- services/workflows-service/src/alert/alert.service.intg.test.ts (34 hunks)
- services/workflows-service/src/alert/alert.service.ts (8 hunks)
- services/workflows-service/src/alert/consts.ts (1 hunks)
- services/workflows-service/src/alert/types.ts (2 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
- services/workflows-service/src/data-analytics/types.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/migrations/20240516085653_add_unique_report_id/migration.sql
Files skipped from review as they are similar to previous changes (4)
- services/workflows-service/src/alert/consts.ts
- services/workflows-service/src/alert/types.ts
- services/workflows-service/src/data-analytics/data-analytics.service.ts
- services/workflows-service/src/data-analytics/types.ts
Additional comments not posted (23)
services/workflows-service/src/alert/alert.controller.external.ts (3)
13-13
: New imports forAlert
,AlertDefinition
,MonitoringType
, and types are correctly added.Also applies to: 18-23
136-203
: ThelistBusinessReportAlerts
method is well-implemented, with appropriate validation, response types, and data retrieval logic.
24-24
: The import forexpress
is correctly added for handling responses.services/workflows-service/src/alert/alert.service.ts (3)
6-16
: New imports forInputJsonValue
,ObjectValues
,TProjectId
,Alert
,AlertDefinition
,AlertSeverity
,AlertState
,AlertStatus
,BusinessReport
,MonitoringType
,CheckRiskScoreOptions
, andInlineRule
are correctly added.Also applies to: 24-25
Line range hint
82-101
: ThegetAlerts
method is well-implemented, with appropriate filtering, ordering, and pagination logic.
155-202
: ThecheckOngoingMonitoringAlert
method is well-implemented, with appropriate logic for checking alerts and creating new alerts if necessary.services/workflows-service/scripts/alerts/generate-alerts.ts (4)
Line range hint
557-594
: LGTM! The functiongetAlertDefinitionCreateData
is well-defined.
Line range hint
596-648
: LGTM! The functiongenerateAlertDefinitions
is well-defined.
Line range hint
649-697
: LGTM! The functiongenerateFakeAlert
is well-defined.
Line range hint
701-747
: LGTM! The functionseedTransactionsAlerts
is well-defined.services/workflows-service/prisma/schema.prisma (5)
711-714
: The addition of theMonitoringType
enum looks good and will help categorize different types of monitoring.
719-720
: The addition of themonitoringType
field to theAlertDefinition
model is appropriate and aligns with the newMonitoringType
enum.
783-785
: The addition of thebusinessId
andbusiness
fields to theAlert
model is appropriate and enhances the model by allowing alerts to be associated with a specific business.
844-844
: The addition of thereportId
field to theBusinessReport
model is appropriate and ensures that each report has a unique identifier.
847-860
: The addition of theriskScore
field to theBusinessReport
model is appropriate and enhances the model by allowing the storage and efficient querying of risk scores.services/workflows-service/src/alert/alert.service.intg.test.ts (8)
27-28
: Necessary imports for new tests.
75-76
: Necessary setup for new tests.
1091-1122
: Test case for "When no previousReport" is well-written.
1125-1162
: Test case for "When previous report not ongoing report" is well-written.
1166-1203
: Test case for "When previous report has higher risk score" is well-written.
1206-1243
: Test case for "When previous report does not hit required changes" is well-written.
1246-1366
: Test case for "When previous report hits necessary changes to invoke alert" is well-written.
Line range hint
1589-1611
: Necessary function for creating counterparty in tests.
# Conflicts: # services/workflows-service/package.json # services/workflows-service/prisma/data-migrations # services/workflows-service/src/data-analytics/data-analytics.service.ts # services/workflows-service/src/data-analytics/types.ts
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
Out of diff range and nitpick comments (1)
services/workflows-service/scripts/alerts/generate-alerts.ts (1)
Line range hint
38-499
: TheTRANSACTIONS_ALERT_DEFINITIONS
object is well-structured. Consider improving type safety by using TypeScript's advanced types features, such as utility types (Partial
,Pick
) or custom types, to make the code more robust and maintainable. This could help prevent bugs related to incorrect or missing properties and make the code easier to understand and modify.- } as const satisfies Record< - Partial<keyof typeof TransactionAlertLabel>, - { - inlineRule: InlineRule & InputJsonValue; - defaultSeverity: AlertSeverity; - dedupeStrategy?: TDedupeStrategy; - monitoringType?: MonitoringType; - enabled?: boolean; - description?: string; - } - >; + } as const satisfies Record<keyof typeof TransactionAlertLabel, AlertDefinition>;
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!pnpm-lock.yaml
Files selected for processing (5)
- services/workflows-service/package.json (2 hunks)
- services/workflows-service/scripts/alerts/generate-alerts.ts (14 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
- services/workflows-service/src/data-analytics/types.ts (4 hunks)
- services/workflows-service/src/workflow/hook-callback-handler.service.ts (6 hunks)
Files skipped from review as they are similar to previous changes (3)
- services/workflows-service/package.json
- services/workflows-service/src/data-analytics/types.ts
- services/workflows-service/src/workflow/hook-callback-handler.service.ts
Additional comments not posted (11)
services/workflows-service/src/data-analytics/data-analytics.service.ts (7)
4-5
: New imports look good and are necessary for the added functionality.Also applies to: 10-10, 14-20, 23-23
30-30
: The addition ofBusinessReportService
as a dependency is appropriate for the new functionality.
74-84
: The function signature and initial variable extraction look correct.
85-95
: The check for the previous report is necessary and correctly implemented.
97-127
: The extraction and validation of risk scores are correctly implemented.
129-178
: The generation of rule results and execution details is correctly implemented.
383-383
: The added condition to check for non-nullcounterpartyBeneficiaryId
is appropriate for the functionality.Also applies to: 395-395
services/workflows-service/scripts/alerts/generate-alerts.ts (4)
6-6
: Import statement forMonitoringType
looks good.
20-20
: Import statement fordaysToMinutesConverter
looks good.
21-21
: Import statement forSEVEN_DAYS
looks good.
23-23
: Import statement forTWENTY_ONE_DAYS
looks good.
# Conflicts: # services/workflows-service/prisma/data-migrations # services/workflows-service/scripts/alerts/generate-alerts.ts # services/workflows-service/src/alert/alert.service.intg.test.ts # services/workflows-service/src/data-analytics/data-analytics.service.ts # services/workflows-service/src/data-analytics/types.ts
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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!pnpm-lock.yaml
Files selected for processing (4)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/alert/alert.service.intg.test.ts (5 hunks)
- services/workflows-service/src/alert/consts.ts (1 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/alert/alert.service.intg.test.ts
- services/workflows-service/src/alert/consts.ts
- services/workflows-service/src/data-analytics/data-analytics.service.ts
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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- services/workflows-service/src/alert/alert.service.ts (8 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
- services/workflows-service/src/data-analytics/types.ts (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- services/workflows-service/src/alert/alert.service.ts
- services/workflows-service/src/data-analytics/data-analytics.service.ts
- services/workflows-service/src/data-analytics/types.ts
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: 4
Outside diff range and nitpick comments (1)
services/workflows-service/src/alert/alert.service.ts (1)
82-82
: Consider adding type annotations toargs
for better type safety and code readability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!pnpm-lock.yaml
Files selected for processing (3)
- services/workflows-service/scripts/alerts/generate-alerts.ts (11 hunks)
- services/workflows-service/src/alert/alert.service.ts (8 hunks)
- services/workflows-service/src/alert/consts.ts (1 hunks)
Files not reviewed due to errors (1)
- services/workflows-service/scripts/alerts/generate-alerts.ts (no review received)
Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/alert/consts.ts
Additional comments not posted (1)
services/workflows-service/src/alert/alert.service.ts (1)
Line range hint
292-309
: Ensure thatcreateAlert
method properly handles the case wheresubject
is empty.Verification successful
The
createAlert
method usesObject.assign({}, ...(subject || []))
to handle thesubject
parameter. This approach ensures that ifsubject
is empty or undefined, it will default to an empty object, thus preventing any errors.
subject || []
ensures that ifsubject
is falsy (e.g.,null
orundefined
), it will be replaced with an empty array.Object.assign({}, ...[])
will result in an empty object if the array is empty.This implementation correctly handles the case where
subject
is empty.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify handling of empty `subject` in `createAlert`. # Test: Search for error handling related to empty `subject`. rg --type typescript 'createAlert' 'services/workflows-service/src/alert/alert.service.ts'Length of output: 123
Script:
#!/bin/bash # Description: Verify handling of empty `subject` in `createAlert`. # Correct the file type and search for the `createAlert` method. rg 'createAlert' 'services/workflows-service/src/alert/alert.service.ts'Length of output: 376
Script:
#!/bin/bash # Description: Extract the implementation of the `createAlert` method to verify handling of empty `subject`. # Extract the `createAlert` method implementation. ast-grep --lang typescript --pattern $'class $_ { $$$ private createAlert($_, $_, $_) { $$$ } $$$ }' 'services/workflows-service/src/alert/alert.service.ts'Length of output: 169
Script:
#!/bin/bash # Description: Extract the implementation of the `createAlert` method to verify handling of empty `subject`. # Search for the `createAlert` method definition and extract surrounding lines. rg -A 20 'private createAlert' 'services/workflows-service/src/alert/alert.service.ts'Length of output: 798
Co-authored-by: Lior Zamir <liorz@ballerine.com>
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: 2
Outside diff range and nitpick comments (2)
services/workflows-service/src/alert-definition/alert-definition.repository.ts (1)
14-17
: Consider adding a comment explaining the use ofprojectIds
for clarity.services/workflows-service/src/alert/alert.service.ts (1)
Line range hint
82-100
: The inclusion ofmonitoringType
directly in the query enhances specificity and efficiency. Consider adding a comment to explain its usage for future maintainers.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- services/workflows-service/src/alert-definition/alert-definition.repository.ts (3 hunks)
- services/workflows-service/src/alert/alert.service.ts (9 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/data-analytics/data-analytics.service.ts
Additional comments not posted (5)
services/workflows-service/src/alert-definition/alert-definition.repository.ts (4)
14-17
: The updated method signature and the use of scoped query are consistent with multi-tenant application requirements.
52-59
: Dynamic ordering via theorderBy
parameter enhances flexibility in data retrieval.
62-64
: The method signature update is consistent and well-implemented.
90-92
: Enhanced flexibility in thedeleteById
method through the inclusion of additional arguments is a positive change.services/workflows-service/src/alert/alert.service.ts (1)
127-129
: Filtering alert definitions bymonitoringType
is an effective and relevant addition.
User description
feat: updated callback to ongoing to format response for each report
Type
enhancement
Description
findFirst
methods in bothBusinessReportRepository
andBusinessReportService
to enhance data retrieval.HookCallbackHandlerService
with Zod validation and additional logic for handling comparison reports.reportId
field and index in theBusinessReport
table.Changes walkthrough
business-report.repository.ts
Add findFirst Method in BusinessReportRepository
services/workflows-service/src/business-report/business-report.repository.ts
findFirst
to fetch the first business report withenhanced scoping.
business-report.service.ts
Implement findFirst Method in BusinessReportService
services/workflows-service/src/business-report/business-report.service.ts
findFirst
to retrieve the first businessreport.
hook-callback-handler.service.ts
Enhance Data Handling and Validation in HookCallbackHandlerService
services/workflows-service/src/workflow/hook-callback-handler.service.ts
prepareMerchantAuditReportContext
.prepareMerchantAuditReportContext
.prepareMerchantAuditReportContext
toinclude
reportId
.migration.sql
SQL Migration to Add reportId to BusinessReport
services/workflows-service/prisma/migrations/20240421105728_add_business_report_report_id/migration.sql
reportId
to theBusinessReport
table.reportId
column.schema.prisma
Update Prisma Schema with reportId in BusinessReport
services/workflows-service/prisma/schema.prisma
reportId
field to theBusinessReport
model.reportId
field.data-migrations
Update Data Migrations Submodule Reference
services/workflows-service/prisma/data-migrations
Summary by CodeRabbit
New Features
riskScore
field to business reports for enhanced risk assessment.MonitoringType
for better alert categorization.Improvements
Bug Fixes
Dependencies