-
Notifications
You must be signed in to change notification settings - Fork 190
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
Transaction Monitoring - Investigation Page - Load transactions that fit rule #2671
base: dev
Are you sure you want to change the base?
Conversation
|
WalkthroughThis update involves significant structural changes to the database schema, specifically the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AlertService
participant DataAnalyticsService
participant Database
User->>AlertService: Create Alert
AlertService->>Database: Save Alert with Beneficiary and Originator IDs
AlertService->>DataAnalyticsService: Get Investigation Filter
DataAnalyticsService->>Database: Query Transaction Records
Database-->>DataAnalyticsService: Return Transaction Records
DataAnalyticsService-->>AlertService: Return Filtered Data
AlertService-->>User: Alert Created
Tip Announcements
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (14)
Files skipped from review as they are similar to previous changes (4)
Additional context usedBiome
Additional comments not posted (24)
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 (
|
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
Outside diff range, codebase verification and nitpick comments (1)
services/workflows-service/prisma/schema.prisma (1)
859-860
: Update Required:The
alerts
field in theCounterparty
model is still being used in the codebase, specifically inservices/workflows-service/src/case-management/controllers/case-management.controller.ts
. Please update this usage to reflect the new fields,alertsBenefiting
andalertsOriginating
, to ensure consistency with the updated Prisma schema.
- File:
services/workflows-service/src/case-management/controllers/case-management.controller.ts
- Line: Usage of
alerts
fieldAnalysis chain
LGTM!
The modifications to the
Counterparty
model, replacing thealerts
field withalertsBenefiting
andalertsOriginating
, enhance the clarity of the data model by clearly delineating the types of alerts related to each counterparty. This improves the schema's expressiveness and maintainability.Please verify that all usages of the
alerts
field in the codebase have been updated to use the newalertsBenefiting
andalertsOriginating
fields as appropriate. Run the following script to check for any remaining usages of the old field:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the `alerts` field in the `Counterparty` model have been updated. # Test: Search for usages of the old `alerts` field. Expect: No occurrences. rg --type typescript $'Counterparty.*alerts'Length of output: 80
Script:
#!/bin/bash # Description: Verify all usages of the `alerts` field in the `Counterparty` model have been updated. # Test: Search for usages of the old `alerts` field. Expect: No occurrences. rg --type ts $'Counterparty.*alerts'Length of output: 177
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- services/workflows-service/prisma/migrations/20240902144459_add_alerts_counterparty_relation_for_advanced_filtering/migration.sql (1 hunks)
- services/workflows-service/prisma/schema.prisma (2 hunks)
- services/workflows-service/scripts/alerts/generate-alerts.ts (3 hunks)
- services/workflows-service/src/alert/alert.service.ts (1 hunks)
- services/workflows-service/src/alert/types.ts (1 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (13 hunks)
- services/workflows-service/src/data-analytics/types.ts (1 hunks)
- services/workflows-service/src/transaction/transaction.controller.external.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- services/workflows-service/src/transaction/transaction.controller.external.ts
Additional comments not posted (27)
services/workflows-service/prisma/migrations/20240902144459_add_alerts_counterparty_relation_for_advanced_filtering/migration.sql (5)
1-2
: LGTM!The code changes are approved.
4-5
: LGTM!The code changes are approved.
7-9
: LGTM!The code changes are approved.
11-15
: LGTM!The code changes are approved.
17-21
: LGTM!The code changes are approved.
services/workflows-service/src/alert/types.ts (1)
1-6
: LGTM! Verify the usage of thefilters
property.The changes to the
TExecutionDetails
type look good:
- The removal of the
checkpoint
property suggests a simplification or a shift in focus away from that aspect of execution details.- The addition of the
filters
property indicates a shift in how execution details are represented, likely to accommodate new filtering capabilities based on the Prisma ORM's transaction record input specifications.- The overall functionality appears to be enhanced by allowing more complex queries or conditions through the
filters
property.Ensure that the
filters
property is being used correctly in the codebase. Run the following script to verify the usage:services/workflows-service/src/data-analytics/types.ts (9)
11-11
: LGTM!The code changes are approved.
16-16
: LGTM!The code changes are approved.
21-21
: LGTM!The code changes are approved.
26-26
: LGTM!The code changes are approved.
31-31
: LGTM!The code changes are approved.
36-36
: LGTM!The code changes are approved.
46-46
: LGTM!The code changes are approved.
51-51
: LGTM!The code changes are approved.
56-56
: LGTM!The code changes are approved.
services/workflows-service/src/alert/alert.service.ts (2)
328-328
: LGTM!The code changes are approved.
330-345
: LGTM!The code changes are approved.
services/workflows-service/prisma/schema.prisma (3)
799-803
: LGTM!The changes to the
Alert
model, replacing thecounterpartyId
field withcounterpartyOriginatorId
andcounterpartyBeneficiaryId
, provide a clearer structure for the relationships between alerts and counterparties by explicitly distinguishing the originator and beneficiary roles. This improves the schema's expressiveness and maintainability.
804-807
: LGTM!The addition of the
counterpartyOriginator
andcounterpartyBeneficiary
relations to theAlert
model establishes explicit connections between alerts and counterparties, clearly delineating the types of counterparties related to each alert. This enhances the clarity of the data model.
815-816
: LGTM!The addition of the
@@index([counterpartyOriginatorId])
and@@index([counterpartyBeneficiaryId])
indexes to theAlert
model reflects the changes in the model's fields and optimizes queries involving these fields.services/workflows-service/src/data-analytics/data-analytics.service.ts (2)
39-53
: LGTM!The new
getInvestigationFilter
function enhances the service's ability to handle different investigation rules by dynamically selecting the appropriate investigation function based on theinlineRule.fnInvestigationName
. This improves modularity and maintainability.The function correctly throws an error if no evaluation function is found for the given rule name.
275-301
: LGTM!The new
investigateTransactionsAgainstDynamicRules
function constructs a filter object for querying transaction records based on various parameters. It enhances type safety by utilizing TypeScript's type assertions to ensure that the constructed filter adheres to the expected Prisma input types.The function correctly handles optional parameters and provides default values where necessary.
services/workflows-service/scripts/alerts/generate-alerts.ts (5)
47-48
: LGTM!The changes are approved:
- Renaming
fnName
tofnInvestigationName
indicates a shift in the function's purpose from evaluation to investigation.- Updating the
subjects
array to focus oncounterpartyBeneficiaryId
reflects the new focus on beneficiary IDs for the alerts.
51-52
: LGTM!The change is approved. Setting
groupBy
to['counterpartyBeneficiaryId']
is consistent with the new focus on beneficiary IDs for the alerts.
54-54
: LGTM!The change is approved. Setting
direction
toTransactionDirection.inbound
specifies the direction of the transactions for the alert.
59-59
: LGTM!The change is approved. Updating the
excludedCounterparty
property to includecounterpartyBeneficiaryIds
andcounterpartyOriginatorIds
allows specifying excluded counterparty IDs for both beneficiary and originator roles.
96-96
: LGTM!The change is approved. Setting
amountThreshold
to1000
standardizes the value across multiple alert definitions.
options: Omit<TDormantAccountOptions, 'projectId'>; | ||
} | ||
| { | ||
fnName: 'checkMerchantOngoingAlert'; | ||
fnInvestigationName: 'checkMerchantOngoingAlert'; |
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.
Inconsistent naming convention for fnInvestigationName
.
The value of the new fnInvestigationName
property is the same as the existing fnName
value, which is inconsistent with the naming convention used in other cases.
Apply this diff to fix the naming:
- fnInvestigationName: 'checkMerchantOngoingAlert';
+ fnInvestigationName: 'investigateMerchantOngoingAlert';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fnInvestigationName: 'checkMerchantOngoingAlert'; | |
fnInvestigationName: 'investigateMerchantOngoingAlert'; |
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- services/workflows-service/scripts/alerts/generate-alerts.ts (6 hunks)
- services/workflows-service/src/alert/alert.service.ts (2 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (12 hunks)
- services/workflows-service/src/data-analytics/types.ts (1 hunks)
- services/workflows-service/src/test/db-setup.ts (1 hunks)
- services/workflows-service/src/transaction/dtos/get-transactions.dto.ts (1 hunks)
- services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts (7 hunks)
- services/workflows-service/src/transaction/transaction.controller.external.ts (2 hunks)
- services/workflows-service/src/transaction/transaction.repository.ts (3 hunks)
- services/workflows-service/src/transaction/transaction.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- 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
- services/workflows-service/src/transaction/transaction.controller.external.ts
Additional context used
Biome
services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts
[error] 673-673: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
Additional comments not posted (14)
services/workflows-service/src/test/db-setup.ts (1)
35-35
: LGTM!The change to the conditional check in the
runPrismaMigrations
function is approved. It enhances the clarity and reliability of the condition by explicitly checking ifprocess.env.SKIP_DB_SETUP_TEARDOWN
is strictly equal to the string 'true'. This ensures that the database setup and teardown process is skipped only when the environment variable is set to the exact string 'true', preventing potential misconfigurations that could arise from other truthy values.services/workflows-service/src/transaction/dtos/get-transactions.dto.ts (1)
Line range hint
48-48
: Verify the impact of removing thecounterpartyId
property.The
counterpartyId
property has been removed from theGetTransactionsDto
class. This change alters the data structure expected when utilizing theGetTransactionsDto
, potentially impacting how transactions are queried or filtered based on the counterparty.Please ensure that:
- The removal of the
counterpartyId
property is intentional and aligns with the PR objectives.- The code that uses the
GetTransactionsDto
class has been updated to handle the removal of thecounterpartyId
property.Run the following script to verify the usage of the
GetTransactionsDto
class:services/workflows-service/src/transaction/transaction.service.ts (1)
75-76
: LGTM!The updated
getTransactions
method signature and implementation look good:
- The use of the spread operator with
Parameters<TransactionRepository['findManyWithFilters']>
enhances the method's flexibility by allowing it to accept a variable number of arguments that match the parameter types of thefindManyWithFilters
method.- The method now directly passes the received arguments to the
findManyWithFilters
method, simplifying the interface and aligning it more closely with the repository's method signature.- The change improves the method's usability by allowing for a more dynamic argument handling approach while reducing the need for explicit parameter definitions.
The code changes are approved.
services/workflows-service/src/transaction/transaction.repository.ts (2)
33-33
: LGTM!The code changes are approved.
Also applies to: 57-59
69-72
: LGTM!The code changes are approved.
Also applies to: 76-86
services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts (4)
1-4
: Imports look good.The newly added imports
getAlertDefinitionCreateData
andALERT_DEFINITIONS
are valid and used in the test cases.
44-44
: Import looks good.The newly added import
TransactionFactory
is valid and used in the test cases.
Line range hint
159-165
: Imports look good.The newly added imports are valid and used in the
beforeAll
setup for the test suite.
655-671
: The beforeAll setup looks good.It correctly initializes the
prismaService
and creates analertDefinition
with the expected options for the test case.services/workflows-service/scripts/alerts/generate-alerts.ts (5)
47-48
: LGTM!The changes are approved:
- Renaming
fnName
tofnInvestigationName
indicates a shift in the function's purpose from evaluation to investigation.- Updating the
subjects
array to focus oncounterpartyBeneficiaryId
instead ofcounterpartyId
is consistent with the new investigation purpose.
51-52
: LGTM!The change is approved:
- Setting the
groupBy
property to['counterpartyBeneficiaryId']
reinforces the new focus on beneficiary IDs and is consistent with the other changes in the alert definition.
54-54
: LGTM!The change is approved:
- Setting the
direction
property toTransactionDirection.inbound
specifies the transaction direction for the alert. It is a valid value.
59-59
: LGTM!The change is approved:
- Updating the
excludedCounterparty
property to includecounterpartyBeneficiaryIds
andcounterpartyOriginatorIds
allows specifying excluded counterparty IDs for both beneficiary and originator roles. It is consistent with the structure of theexcludedCounterparty
object.
Line range hint
636-652
: LGTM!The changes are approved:
- Adding the
fnInvestigationName
property to theMERCHANT_ONGOING_RISK_ALERT_RISK_INCREASE
andMERCHANT_ONGOING_RISK_ALERT_THRESHOLD
alert definitions is consistent with the changes made to thePAY_HCA_CC
alert definition.- It indicates a shift in the function's purpose from evaluation to investigation for these merchant monitoring alerts.
it.only('returns transactions associated with the given alertId', async () => { | ||
// Arrange | ||
await baseTransactionFactory | ||
.withBusinessBeneficiary() | ||
.direction(TransactionDirection.inbound) | ||
.paymentMethod(PaymentMethod.credit_card) | ||
.amount(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold + 1) | ||
.count(1) | ||
.create(); | ||
|
||
// Act | ||
await app.get(AlertService).checkAllAlerts(); | ||
|
||
// Assert | ||
const alerts = await app.get(PrismaService).alert.findMany(); | ||
if (alerts.length === 0 || alerts[0] === undefined) { | ||
throw new Error('No alerts found'); | ||
} | ||
expect(alerts).toHaveLength(1); | ||
expect(alerts[0]?.alertDefinitionId).toEqual(alertDefinition.id); | ||
expect(alerts[0] as any).toMatchObject({ | ||
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } }, | ||
}); | ||
|
||
const response = await request(app.getHttpServer()) | ||
.get(`/external/transactions/by-alert?alertId=${alerts[0].id}`) | ||
.set('authorization', `Bearer ${API_KEY}`); | ||
|
||
expect(response.status).toBe(200); | ||
console.log(response.body); | ||
expect(response.body).toMatchInlineSnapshot(); | ||
}); |
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.
Remove the focus from the test case and remove the console.log statement.
A few suggestions for this test case:
- Remove the focus (
it.only
) from the test case to ensure all test cases are executed. - Remove the
console.log
statement at line 702 as it is used for debugging purposes and should not be present in the final code.
Apply this diff to address the suggestions:
- it.only('returns transactions associated with the given alertId', async () => {
+ it('returns transactions associated with the given alertId', async () => {
// Arrange
await baseTransactionFactory
.withBusinessBeneficiary()
.direction(TransactionDirection.inbound)
.paymentMethod(PaymentMethod.credit_card)
.amount(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold + 1)
.count(1)
.create();
// Act
await app.get(AlertService).checkAllAlerts();
// Assert
const alerts = await app.get(PrismaService).alert.findMany();
if (alerts.length === 0 || alerts[0] === undefined) {
throw new Error('No alerts found');
}
expect(alerts).toHaveLength(1);
expect(alerts[0]?.alertDefinitionId).toEqual(alertDefinition.id);
expect(alerts[0] as any).toMatchObject({
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } },
});
const response = await request(app.getHttpServer())
.get(`/external/transactions/by-alert?alertId=${alerts[0].id}`)
.set('authorization', `Bearer ${API_KEY}`);
expect(response.status).toBe(200);
- console.log(response.body);
expect(response.body).toMatchInlineSnapshot();
});
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
it.only('returns transactions associated with the given alertId', async () => { | |
// Arrange | |
await baseTransactionFactory | |
.withBusinessBeneficiary() | |
.direction(TransactionDirection.inbound) | |
.paymentMethod(PaymentMethod.credit_card) | |
.amount(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold + 1) | |
.count(1) | |
.create(); | |
// Act | |
await app.get(AlertService).checkAllAlerts(); | |
// Assert | |
const alerts = await app.get(PrismaService).alert.findMany(); | |
if (alerts.length === 0 || alerts[0] === undefined) { | |
throw new Error('No alerts found'); | |
} | |
expect(alerts).toHaveLength(1); | |
expect(alerts[0]?.alertDefinitionId).toEqual(alertDefinition.id); | |
expect(alerts[0] as any).toMatchObject({ | |
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } }, | |
}); | |
const response = await request(app.getHttpServer()) | |
.get(`/external/transactions/by-alert?alertId=${alerts[0].id}`) | |
.set('authorization', `Bearer ${API_KEY}`); | |
expect(response.status).toBe(200); | |
console.log(response.body); | |
expect(response.body).toMatchInlineSnapshot(); | |
}); | |
it('returns transactions associated with the given alertId', async () => { | |
// Arrange | |
await baseTransactionFactory | |
.withBusinessBeneficiary() | |
.direction(TransactionDirection.inbound) | |
.paymentMethod(PaymentMethod.credit_card) | |
.amount(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold + 1) | |
.count(1) | |
.create(); | |
// Act | |
await app.get(AlertService).checkAllAlerts(); | |
// Assert | |
const alerts = await app.get(PrismaService).alert.findMany(); | |
if (alerts.length === 0 || alerts[0] === undefined) { | |
throw new Error('No alerts found'); | |
} | |
expect(alerts).toHaveLength(1); | |
expect(alerts[0]?.alertDefinitionId).toEqual(alertDefinition.id); | |
expect(alerts[0] as any).toMatchObject({ | |
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } }, | |
}); | |
const response = await request(app.getHttpServer()) | |
.get(`/external/transactions/by-alert?alertId=${alerts[0].id}`) | |
.set('authorization', `Bearer ${API_KEY}`); | |
expect(response.status).toBe(200); | |
expect(response.body).toMatchInlineSnapshot(); | |
}); |
Tools
Biome
[error] 673-673: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
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: 13
Outside diff range, codebase verification and nitpick comments (4)
services/workflows-service/src/workflow/utils/toPrismaOrderBy.ts (1)
17-17
: References totoPrismaOrderByGeneric
still exist in the codebase. The functiontoPrismaOrderByGeneric
is still being imported and used in the following file, which may lead to runtime errors if not addressed:
services/workflows-service/src/business-report/business-report.service.ts
- Line:
import { toPrismaOrderByGeneric } from '@/workflow/utils/toPrismaOrderBy';
- Line:
args.orderBy = toPrismaOrderByGeneric(getTransactionsParameters.orderBy);
Please review and update or remove these references to ensure the codebase remains functional.
Analysis chain
Consider the implications of removing
toPrismaOrderByGeneric
.The removal of
toPrismaOrderByGeneric
simplifies the API but may affect other parts of the codebase that relied on this function. It's important to ensure that all references to this function have been updated or removed to prevent runtime errors.Run the following script to check for any remaining references to
toPrismaOrderByGeneric
:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to `toPrismaOrderByGeneric`. # Test: Search for the function usage. Expect: No occurrences. rg --type typescript 'toPrismaOrderByGeneric'Length of output: 80
Script:
#!/bin/bash # Description: Check for any remaining references to `toPrismaOrderByGeneric` across all files. # Test: Search for the function usage. Expect: No occurrences. rg 'toPrismaOrderByGeneric'Length of output: 330
package.json (1)
29-38
: Missing Directory:apps/kyb-ap
The directory
apps/kyb-ap
specified in the "workspaces" section does not exist. Please ensure that this directory is created or correctly referenced to avoid potential issues with workspace management.
- Ensure the
apps/kyb-ap
directory is created or update the path if incorrect.Analysis chain
Approved: Addition of workspaces.
The addition of the "workspaces" section enhances project organization and modularization. Ensure that the specified paths are correctly set up and accessible.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of workspace directories. # Test: Check if workspace directories exist. Expect: Directories should exist. for dir in apps packages sdks services websites examples/headless-example examples/report-generation-example apps/kyb-ap; do if [ ! -d "$dir" ]; then echo "Directory $dir does not exist." else echo "Directory $dir exists." fi doneLength of output: 1825
services/workflows-service/src/transaction/transaction.controller.external.ts (1)
37-38
: Unused Dependency: DataAnalyticsServiceThe
DataAnalyticsService
is imported and initialized in theTransactionControllerExternal
class but is not used within any methods. Consider reviewing its necessity in the constructor to ensure it is required for the current implementation.
- File:
services/workflows-service/src/transaction/transaction.controller.external.ts
- Lines: Initialization in the constructor
Analysis chain
Addition of DataAnalyticsService
The addition of
DataAnalyticsService
as a dependency in the constructor is a significant change. It's important to ensure that this service is properly initialized and used within the class to justify its inclusion.Verify the initialization and usage of
DataAnalyticsService
:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of DataAnalyticsService in the TransactionControllerExternal class. # Test: Search for the initialization and usage of DataAnalyticsService. Expect: Proper initialization and usage. ast-grep --lang typescript --pattern $'class TransactionControllerExternal { $$$ constructor($_, $_, $_, $_, $_, $_, $_) { $$$ } $$$ }'Length of output: 148
Script:
#!/bin/bash # Description: Verify the initialization and usage of DataAnalyticsService in the TransactionControllerExternal class. # Test: Search for the class definition and constructor to check for DataAnalyticsService initialization. ast-grep --lang typescript --pattern $'class TransactionControllerExternal { constructor($_, $_, $_, $_, $_, $_, $_, $_) { $$$ } }' # Test: Search for the usage of DataAnalyticsService within the class methods. rg 'DataAnalyticsService' -A 5 --type=tsLength of output: 18513
services/workflows-service/src/data-analytics/data-analytics.service.ts (1)
Line range hint
418-487
: Optimize SQL Query Construction for Performance.The method
evaluateTransactionsAgainstDynamicRules
constructs SQL queries dynamically based on various parameters. While this provides flexibility, it also introduces potential performance issues, especially with complex conditions and large datasets.Consider optimizing the SQL query construction by reducing the number of dynamic parts and using prepared statements or parameterized queries where possible. This can help prevent SQL injection attacks and improve performance by allowing the database to cache and reuse query plans.
Here's a suggestion for using parameterized queries with Prisma:
const query = Prisma.sql`SELECT ${selectClause} FROM "TransactionRecord" WHERE ${whereClause} GROUP BY ${groupByClause}`;Ensure that all dynamic inputs are properly sanitized and validated before being included in the query.
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 (52)
- apps/backoffice-v2/CHANGELOG.md (1 hunks)
- apps/backoffice-v2/package.json (3 hunks)
- apps/kyb-app/CHANGELOG.md (1 hunks)
- apps/kyb-app/package.json (3 hunks)
- apps/workflows-dashboard/CHANGELOG.md (1 hunks)
- apps/workflows-dashboard/package.json (3 hunks)
- examples/headless-example/CHANGELOG.md (1 hunks)
- examples/headless-example/package.json (2 hunks)
- examples/report-generation-example/CHANGELOG.md (1 hunks)
- examples/report-generation-example/package.json (2 hunks)
- package.json (3 hunks)
- packages/blocks/CHANGELOG.md (1 hunks)
- packages/blocks/package.json (3 hunks)
- packages/common/CHANGELOG.md (1 hunks)
- packages/common/package.json (2 hunks)
- packages/react-pdf-toolkit/CHANGELOG.md (1 hunks)
- packages/react-pdf-toolkit/package.json (3 hunks)
- packages/rules-engine/CHANGELOG.md (1 hunks)
- packages/rules-engine/package.json (2 hunks)
- packages/ui/CHANGELOG.md (1 hunks)
- packages/ui/package.json (3 hunks)
- packages/workflow-core/CHANGELOG.md (1 hunks)
- packages/workflow-core/package.json (3 hunks)
- sdks/web-ui-sdk/CHANGELOG.md (1 hunks)
- sdks/web-ui-sdk/package.json (3 hunks)
- sdks/workflow-browser-sdk/CHANGELOG.md (1 hunks)
- sdks/workflow-browser-sdk/package.json (3 hunks)
- sdks/workflow-node-sdk/CHANGELOG.md (1 hunks)
- sdks/workflow-node-sdk/package.json (3 hunks)
- services/websocket-service/CHANGELOG.md (1 hunks)
- services/websocket-service/package.json (2 hunks)
- services/workflows-service/CHANGELOG.md (1 hunks)
- services/workflows-service/jest.config.cjs (1 hunks)
- services/workflows-service/package.json (4 hunks)
- services/workflows-service/src/alert/alert.controller.external.ts (1 hunks)
- services/workflows-service/src/alert/alert.service.ts (3 hunks)
- services/workflows-service/src/alert/alert2.service.intg.test.ts (1 hunks)
- services/workflows-service/src/alert/dtos/get-alerts.dto.ts (3 hunks)
- services/workflows-service/src/common/dto.ts (3 hunks)
- services/workflows-service/src/data-analytics/data-analytics.service.ts (12 hunks)
- services/workflows-service/src/prisma/prisma.util.ts (1 hunks)
- services/workflows-service/src/project/project-scope.service.ts (2 hunks)
- services/workflows-service/src/test/helpers/create-alert.ts (1 hunks)
- services/workflows-service/src/transaction/dtos/get-transactions.dto.ts (2 hunks)
- services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts (11 hunks)
- services/workflows-service/src/transaction/transaction.controller.external.ts (4 hunks)
- services/workflows-service/src/transaction/transaction.controller.external2.intg.test.ts (1 hunks)
- services/workflows-service/src/transaction/transaction.repository.ts (2 hunks)
- services/workflows-service/src/transaction/transaction.service.ts (2 hunks)
- services/workflows-service/src/workflow/dtos/find-workflows-list.dto.ts (1 hunks)
- services/workflows-service/src/workflow/utils/toPrismaOrderBy.ts (3 hunks)
- websites/docs/package.json (1 hunks)
Files skipped from review due to trivial changes (25)
- apps/backoffice-v2/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- apps/workflows-dashboard/CHANGELOG.md
- examples/headless-example/CHANGELOG.md
- examples/headless-example/package.json
- examples/report-generation-example/CHANGELOG.md
- packages/blocks/CHANGELOG.md
- packages/blocks/package.json
- packages/common/CHANGELOG.md
- packages/common/package.json
- packages/react-pdf-toolkit/CHANGELOG.md
- packages/react-pdf-toolkit/package.json
- packages/rules-engine/CHANGELOG.md
- packages/rules-engine/package.json
- packages/ui/CHANGELOG.md
- packages/ui/package.json
- packages/workflow-core/CHANGELOG.md
- sdks/web-ui-sdk/CHANGELOG.md
- sdks/workflow-browser-sdk/CHANGELOG.md
- sdks/workflow-node-sdk/CHANGELOG.md
- sdks/workflow-node-sdk/package.json
- services/websocket-service/CHANGELOG.md
- services/websocket-service/package.json
- services/workflows-service/CHANGELOG.md
- websites/docs/package.json
Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/src/alert/alert.service.ts
- services/workflows-service/src/transaction/transaction.service.ts
Additional context used
Biome
services/workflows-service/src/transaction/transaction.repository.ts
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
services/workflows-service/src/common/dto.ts
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
services/workflows-service/src/alert/alert2.service.intg.test.ts
[error] 38-38: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 162-162: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
services/workflows-service/src/transaction/transaction.controller.external2.intg.test.ts
[error] 233-233: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts
[error] 689-689: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
Additional comments not posted (74)
examples/report-generation-example/package.json (3)
4-4
: Version Increment ApprovedThe increment in the version number from
0.2.10
to0.2.11
is noted and appears appropriate for a minor update or patch. This change is consistent with semantic versioning practices.
13-13
: Dependency Update ApprovedThe update of
@ballerine/react-pdf-toolkit
from^1.2.15
to^1.2.21
is approved. It's important to ensure that this update includes necessary enhancements or bug fixes that are beneficial for the project.
20-20
: Verify Dependency DowngradeThe downgrade of
@typescript-eslint/eslint-plugin
from^6.14.0
to^6.11.0
is unusual. Typically, downgrades might be necessary due to compatibility issues with other packages or specific bugs in newer versions.Please provide a rationale for this downgrade. If it's due to compatibility issues, it might be helpful to document this explicitly for future reference.
services/workflows-service/src/test/helpers/create-alert.ts (1)
11-15
: Refactor and Simplify Testing Helper FunctionThe refactoring of
createAlert
to useINestApplication
for retrievingAlertService
directly aligns well with NestJS's dependency injection principles. This change simplifies the setup by eliminating the need to configure a testing module manually, which enhances clarity and reduces overhead.However, the use of
await
in the return statement is redundant since the function is already returning a promise. Consider simplifying the return statement to improve efficiency and readability.Here's a suggested change to remove the redundant
await
:- return await alertService.createAlert(alertDefinition, [], {}, {}); + return alertService.createAlert(alertDefinition, [], {}, {});Additionally, ensure that all references to the old function signature are updated to reflect these changes across the codebase.
Run the following script to verify the function usage:
Verification successful
Verification Successful: Updated Function Signature
The
createAlert
function has been successfully refactored and integrated across the codebase. The test files using this function are consistent with the new signature, confirming that the changes align with the intended design and best practices.
- The function is used in
transaction.controller.external.intg.test.ts
andtransaction.controller.external2.intg.test.ts
with the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `createAlert` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'createAlert'Length of output: 74
Script:
#!/bin/bash # Description: Verify all function calls to `createAlert` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type ts -A 5 $'createAlert'Length of output: 12246
services/workflows-service/jest.config.cjs (1)
3-3
: Approve addition oftestTimeout
.The addition of the
testTimeout
property set to 30000 milliseconds is a sensible default to prevent tests from hanging indefinitely. This change should help improve the reliability of the test suite by ensuring that tests that take too long can fail gracefully instead of potentially tying up resources indefinitely.services/workflows-service/src/workflow/utils/toPrismaOrderBy.ts (2)
17-17
: Approve the use ofPrisma.SortOrder
for enhanced type safety.The change to use
Prisma.SortOrder
instead of a custom type aligns well with Prisma's conventions and improves type safety. This is a positive change that leverages the Prisma client's capabilities more effectively.
17-17
: Verify the handling oforderBy
parameter split and mapping.The function splits the
orderBy
string to extract the column and direction, which are then used to construct the return object. This approach assumes theorderBy
string is always correctly formatted, which might not always be the case.Consider adding error handling or validation to ensure the
orderBy
string is correctly formatted before splitting:+ if (!orderBy.includes(':')) { + throw new Error('Invalid orderBy format. Expected format: "column:direction"'); + } const [column, direction] = orderBy.split(':') as [TColumn, TDirection];services/workflows-service/src/transaction/transaction.repository.ts (1)
31-40
: Verify the integration ofbuildFilters
withfindManyWithFilters
.The method
buildFilters
has undergone significant changes to its parameters, allowing for more refined filtering capabilities. It is essential to ensure that these changes are correctly integrated with thefindManyWithFilters
method and that the new parameters are being utilized effectively.Please provide the implementation of
buildFilters
or confirm that it has been adapted to work seamlessly with the changes infindManyWithFilters
.Tools
Biome
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
services/workflows-service/src/workflow/dtos/find-workflows-list.dto.ts (3)
Line range hint
5-34
: ClassesFilterDto
andFindWorkflowsListDto
are well-defined.The classes are appropriately structured with necessary API documentation and correct use of optional types. No changes in the logic or structure are noted in the diff.
Line range hint
36-58
: Validation schemas are correctly implemented.The use of the
zod
library for defining constraints and custom validation for sorting columns is correctly implemented and follows best practices.
1-1
: Verify the impact of the import change on other parts of the application.The import of
sortDirections
has been moved from@/common/dto
to@/prisma/prisma.util
. This change could potentially impact other parts of the application that rely onsortDirections
.Run the following script to verify the usage of
sortDirections
across the application:Also applies to: 5-5
Verification successful
Verification successful: Import change for
sortDirections
is consistent across the codebase.The import of
sortDirections
has been updated to@/prisma/prisma.util
in all relevant files, ensuring consistent usage throughout the application. No issues were found with this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `sortDirections` across the application. # Test: Search for the usage of `sortDirections`. Expect: Only occurrences from the new location. rg --type typescript -A 5 $'sortDirections'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of `sortDirections` across the application. # Test: Search for the usage of `sortDirections` without specifying a file type. rg -A 5 'sortDirections'Length of output: 5517
sdks/workflow-browser-sdk/package.json (4)
4-4
: Version update approved.The increment from
0.6.36
to0.6.37
is consistent with semantic versioning practices for minor updates.
36-36
: Dependency update approved for@ballerine/common
.The update from
0.9.25
to0.9.26
is a minor change, likely addressing bug fixes or small improvements.
37-37
: Dependency update approved for@ballerine/workflow-core
.The version update to
0.6.37
aligns with the package version, maintaining consistency.
56-56
: Verify compatibility for the major update of@typescript-eslint/eslint-plugin
.The update from
5.48.1
to6.11.0
is a major version change. It's important to ensure that this update does not introduce any compatibility issues with the existing codebase.Run the following script to verify the compatibility:
services/workflows-service/src/alert/dtos/get-alerts.dto.ts (3)
7-7
: Approved import change.The change in the import source for
sortDirections
likely reflects an improvement in modularity and organization within the project.
41-41
: Type safety enhancement withTAlertOrderBy
.The introduction of
TAlertOrderBy
is a positive change, enhancing type safety by restricting sortable fields, which helps in maintaining consistency and preventing errors in sorting operations.
67-67
: Enhanced type safety inorderBy
property.The change in the
orderBy
property to useTAlertOrderBy
combined withPrisma.SortOrder
significantly enhances type safety and clarity, ensuring that sorting operations are more robust and less prone to errors.services/workflows-service/src/prisma/prisma.util.ts (2)
91-91
: Approved: Addition ofsortDirections
.The addition of
sortDirections
as an array of keys fromPrisma.SortOrder
is correctly implemented and enhances code reusability.
93-93
: Approved: Definition ofDirection
type alias.The
Direction
type alias is correctly defined as a union type of the keys fromPrisma.SortOrder
, enhancing code readability and type safety.packages/workflow-core/package.json (4)
4-4
: Version update approved.The increment from
0.6.36
to0.6.37
is appropriate for minor changes or fixes.
34-34
: Dependency version update approved.Changing from a fixed version
0.9.25
to a caret version^0.9.26
allows automatic updates for minor versions and patches, which is beneficial for maintaining up-to-date dependencies.
64-64
: Linting plugin update approved.Upgrading
@typescript-eslint/eslint-plugin
from^5.48.1
to^6.11.0
likely introduces enhancements or fixes that improve linting capabilities.
59-59
: Removal of duplicate dependency entry approved.Removing the duplicate entry for
@types/json-logic-js
helps clean up the dependency list and prevent potential conflicts during dependency resolution.apps/workflows-dashboard/package.json (4)
4-4
: Version number update approved.The increment from
0.2.11
to0.2.12
is appropriate for minor updates or fixes.
17-17
: Dependency update approved for@ballerine/common
.Updating from
^0.9.22
to^0.9.26
likely includes minor enhancements or bug fixes.
18-18
: Dependency update approved for@ballerine/ui
.Updating from
^0.5.15
to^0.5.21
suggests improvements or bug fixes in the UI components.
79-79
: Major version update approved for@typescript-eslint/eslint-plugin
.Updating from
^5.59.0
to^6.11.0
indicates significant improvements or changes in the linting rules.Please ensure to verify the impact of this update on the existing codebase, particularly if new linting rules introduce breaking changes or require adjustments in the code.
apps/kyb-app/package.json (6)
4-4
: Application version update approved.The update from
0.3.41
to0.3.42
is a minor version increment, typically used for bug fixes or small enhancements.
17-17
: Dependency update approved for@ballerine/blocks
.The update from
0.2.12
to0.2.13
is a patch version increment, typically used to address bugs without introducing breaking changes.
18-18
: Dependency update approved for@ballerine/common
.The update from
^0.9.25
to^0.9.26
is a minor version increment, which should not introduce breaking changes due to the caret (^) versioning.
19-19
: Dependency update approved for@ballerine/ui
.The update from
0.5.20
to0.5.21
is a patch version increment, typically used to address bugs without introducing breaking changes.
20-20
: Dependency update approved for@ballerine/workflow-browser-sdk
.The update from
0.6.36
to0.6.37
is a patch version increment, typically used to address bugs without introducing breaking changes.
82-82
: Verify compatibility for@typescript-eslint/eslint-plugin
update.The update from
^5.61.0
to^6.11.0
is a major version increment. It is important to ensure that this update is compatible with other project dependencies and does not disrupt the existing linting setup.services/workflows-service/src/project/project-scope.service.ts (2)
4-4
: Approved import statement.The import of
InvalidArgumentParser
is correctly placed. However, its usage within this file is not visible.Please verify if this import is used elsewhere in the project or if it's intended for future use.
49-49
: Approved changes to thewhere
clause.The update to the
where
clause in thescopeFindMany
method is correctly implemented, ensuring that theprojectIdsClause
is used effectively.services/workflows-service/src/common/dto.ts (8)
9-11
: InterfaceITransactionOrderDto
is well-defined.The interface is simple and correctly captures the optional ordering parameter for transactions.
14-17
: InterfaceIPageDto
is well-defined.This interface correctly defines the necessary properties for pagination, which are essential for the functionality of the investigation page.
37-37
: TypePagination
is effectively defined.This type simplifies the usage of Prisma arguments for pagination, enhancing code readability and maintainability.
49-58
: ClassOrderBy
is well-structured.The use of decorators enhances the clarity and usability of the class, making it well-suited for defining sorting parameters.
60-68
: FunctionbuildOrderByGenericFilter
is well-implemented.This utility function simplifies the creation of order by filters, enhancing modularity and reusability of the code.
97-101
: FunctionbuildOrderByFilter
maintains API consistency.This function is a simple wrapper around
buildOrderByGenericFilter
, correctly maintaining consistency in the API.
118-123
: ConstantfirstPage
is well-defined.This constant provides a default pagination state, ensuring that the application can reliably start with a known state.
125-131
: FunctionTransformPageDecorator
is well-implemented.This function correctly transforms
PageDto
into a format suitable for pagination, enhancing the usability of pagination data.services/workflows-service/src/transaction/dtos/get-transactions.dto.ts (6)
19-27
: Approved changes to SubjectDto with a suggestion for improvement.The introduction of
counterpartyOriginatorId
andcounterpartyBeneficiaryId
enhances the flexibility in transaction filtering. Consider adding documentation to clarify the roles of these IDs in transaction filtering.
29-31
: Approved the definition of TransactionsOrderBy.This type definition simplifies the specification of order parameters, enhancing the sorting functionality of transaction queries.
33-38
: Approved new sorting type definitions with a suggestion for improvement.The introduction of
TransactionSortableColumn
,SortOrderType
, andTransactionOrderBy
types are crucial for the enhanced sorting functionality. Consider adding documentation to clarify the usage and limitations of these types.
40-44
: Approved the definition of TransactionOrderByOptions.This constant provides useful predefined sorting options, ensuring consistent behavior in transaction sorting.
Line range hint
56-72
: Approved changes to TransactionOrderDto with a suggestion for improvement.The enhancements to the
TransactionOrderDto
class allow for more flexible sorting options. Consider adding more examples in theApiProperty
decorator to cover a wider range of possible sorting scenarios.
111-117
: Approved changes to GetTransactionsByAlertDto with a suggestion for improvement.The
GetTransactionsByAlertDto
class is well-designed for its purpose. Ensure that all necessary validations are in place for thealertId
to prevent potential issues with incorrect IDs.sdks/web-ui-sdk/package.json (3)
24-24
: Version Update ApprovedThe update from
1.5.26
to1.5.27
is a minor version increment, which is appropriate for the changes described.
99-99
: Dependency Update ApprovedThe update of
@ballerine/common
from0.9.25
to0.9.26
is a minor version increment, typically indicating minor improvements or bug fixes. This change is approved.
73-73
: Verify Compatibility of ESLint Plugin UpdateThe update of
@typescript-eslint/eslint-plugin
from5.41.0
to6.11.0
is a major version change. It is crucial to verify that this update does not introduce any breaking changes or require updates to the ESLint configuration.Run the following script to verify the ESLint configuration:
apps/backoffice-v2/package.json (8)
3-3
: Version Update ApprovedThe project version has been incremented from
0.7.35
to0.7.36
. This minor version update is appropriate for the changes described, as they seem to introduce new functionality without breaking existing features.
53-53
: Dependency Update Approved: @ballerine/blocksThe update from
0.2.12
to0.2.13
is a minor version change, which typically includes backward-compatible bug fixes or enhancements.
54-54
: Dependency Update Approved: @ballerine/commonThe update from
0.9.25
to0.9.26
is a minor version change, which typically includes backward-compatible bug fixes or enhancements.
55-55
: Dependency Update Approved: @ballerine/react-pdf-toolkitThe update from
^1.2.20
to^1.2.21
is a patch version change, which typically includes backward-compatible bug fixes or enhancements.
56-56
: Dependency Update Approved: @ballerine/uiThe update from
^0.5.20
to^0.5.21
is a patch version change, which typically includes backward-compatible bug fixes or enhancements.
57-57
: Dependency Update Approved: @ballerine/workflow-browser-sdkThe update from
0.6.36
to0.6.37
is a minor version change, which typically includes backward-compatible bug fixes or enhancements.
58-58
: Dependency Update Approved with Caution: @ballerine/workflow-node-sdkThe update from
0.6.36
to^0.6.37
introduces not only a minor version change but also a policy change to automatically adopt future minor updates. While this can keep the project up-to-date, it may introduce unexpected behavior if not monitored closely.
149-149
: Major Dependency Update: @typescript-eslint/eslint-pluginThe update from
^5.30.0
to^6.11.0
is a major version change. It is crucial to verify that this update does not introduce any breaking changes or compatibility issues with the existing codebase.services/workflows-service/package.json (3)
4-4
: Version update approved.The increment in the version number is standard practice for releasing new features or fixes.
136-136
: Major update to ESLint plugin approved.This update may introduce new linting rules or configurations. Review the ESLint changes to ensure they align with the project's coding standards.
Run the following script to verify the impact of ESLint changes:
49-51
: Dependency updates approved.The minor version updates for dependencies are typically safe, but ensure compatibility with existing code.
Run the following script to verify the compatibility of updated dependencies:
services/workflows-service/src/transaction/transaction.controller.external.ts (4)
204-204
: Refinement in getTransactionsByAlert MethodThe renaming of the parameter from
getTransactionsByAlertParameters
tobyAlertFilters
improves clarity. Additionally, the method now utilizes a new pagination filter and orders results based onbyAlertFilters.orderBy
. This change should enhance the method's functionality by providing more structured and clear data retrieval based on alert filters.The changes in the method are well-aligned with the PR's objectives and improve the clarity and functionality of the API.
Also applies to: 258-258
210-210
: Error Handling in getTransactionsByAlertThe addition of a specific error message when an alert is not found enhances the user's understanding of what went wrong. This is a good practice in API design, providing clear and actionable error messages.
The explicit error handling for missing alerts is a positive change, improving the robustness and user-friendliness of the API.
Line range hint
217-258
: Detailed Transaction Retrieval LogicThe method
getTransactionsByAlert
now includes detailed logic for fetching transactions based on the alert's execution details. The inclusion of specific fields in theinclude
clause ensures that relevant data is fetched efficiently. The use of pagination and ordering is also a good practice to handle large datasets effectively.The detailed and structured approach to fetching transactions based on alert criteria is commendable and aligns with the PR's objectives to enhance the investigation process.
46-46
: Constructor Dependency InjectionThe constructor now includes
DataAnalyticsService
. This change aligns with the PR's objective to enhance data analytics capabilities. Ensure that all instances where theTransactionControllerExternal
class is instantiated are updated to pass this new dependency.Verify that all instances of
TransactionControllerExternal
instantiation include the new dependency:Verification successful
Constructor Dependency Injection Verified
The
DataAnalyticsService
is correctly included in theproviders
array of thetransaction.module.ts
file, ensuring it is injected intoTransactionControllerExternal
by the NestJS framework. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all instances of TransactionControllerExternal instantiation include the new dependency. # Test: Search for the instantiation of TransactionControllerExternal. Expect: Inclusion of DataAnalyticsService. ast-grep --lang typescript --pattern $'new TransactionControllerExternal($_, $_, $_, $_, $_, $_)'Length of output: 98
Script:
#!/bin/bash # Search for all instances of TransactionControllerExternal instantiation, regardless of parameter count. ast-grep --lang typescript --pattern $'new TransactionControllerExternal($$$)'Length of output: 79
Script:
#!/bin/bash # Search for the definition of TransactionControllerExternal to understand its usage. ast-grep --lang typescript --pattern $'class TransactionControllerExternal { $$$ }' # Search for references to TransactionControllerExternal to identify indirect instantiation methods. rg 'TransactionControllerExternal' -A 5Length of output: 31229
Script:
#!/bin/bash # Verify that DataAnalyticsService is included in the providers array or imported in the transaction.module.ts file. rg 'DataAnalyticsService' services/workflows-service/src/transaction/transaction.module.ts -A 5Length of output: 509
services/workflows-service/src/alert/alert.controller.external.ts (1)
79-98
: Review of Changes to Counterparty HandlingThe changes introduced to the
counterpartyOriginator
andcounterpartyBeneficiary
properties in thelist
method are significant. They aim to enhance the clarity and specificity of the data retrieval process by defining more granularselect
objects for these properties.Key Observations:
- Data Structure and Retrieval: The nested
select
structure allows for specific fields to be retrieved, which can improve performance by reducing the amount of data fetched from the database. This is particularly useful in systems where the volume of data is large.- Potential Data Integrity Issues: The renaming from
counterparty
tocounterpartyOriginator
and the addition ofcounterpartyBeneficiary
could lead to potential data integrity issues if not handled correctly in other parts of the application. It's crucial to ensure that all references to these properties are updated accordingly.Recommendations:
- Verify Data Integrity: Ensure that all parts of the application that interact with these properties are updated to reflect the new structure. This includes any data processing or business logic that depends on the old
counterparty
structure.- Performance Testing: Conduct performance tests to ensure that the new data retrieval structure does not adversely affect the application's performance, especially under load.
Potential Improvements:
- Documentation: Update the API documentation to reflect these changes. This will help developers and API consumers understand the new data structures and how to interact with them.
- Error Handling: Improve error handling around these properties to gracefully handle cases where data may not be available or mismatches occur.
Overall, these changes are aligned with the PR objectives of enhancing the investigation page by providing more specific and relevant data for transaction monitoring.
Also applies to: 99-120
services/workflows-service/src/transaction/transaction.controller.external2.intg.test.ts (3)
1-46
: Review of imports and initial setup.The imports and initial setup are generally well-organized and follow best practices for structuring a NestJS integration test file. The use of relative and absolute paths is consistent, and the separation of concerns is evident with specific imports for database helpers, DTOs, and services.
47-143
: Review of utility functions for data generation.The utility functions
getBusinessCounterpartyData
,getEndUserCounterpartyData
, andgetBaseTransactionData
are well-implemented. They effectively use thefaker
library to generate randomized data needed for the tests, which helps in simulating real-world scenarios in a controlled environment.
147-184
: Review of test suite setup and teardown.The setup and teardown logic using
beforeAll
andafterAll
are correctly implemented. They ensure that each test runs in a clean state by cleaning up the database and properly initializing the Nest application. This setup helps prevent tests from affecting each other, which is crucial for reliable integration testing.services/workflows-service/src/data-analytics/data-analytics.service.ts (1)
376-402
: Verify Type Safety and Logic Consistency in Transaction Filtering.The method
investigateTransactionsAgainstDynamicRules
constructs a filter for querying transaction records. It uses TypeScript's type assertions to ensure the filter matches the expected Prisma input types. However, the use of type assertions (as const
) should be verified to ensure that it does not mask potential type mismatches.Additionally, the logic for including or excluding payment methods based on
excludePaymentMethods
should be thoroughly tested to ensure it behaves as expected under various scenarios.Consider adding unit tests to cover these cases:
describe('investigateTransactionsAgainstDynamicRules', () => { it('should correctly handle payment method inclusion', () => { // Test setup and assertions... }); it('should correctly handle payment method exclusion', () => { // Test setup and assertions... }); });This will help ensure the robustness and correctness of the transaction filtering logic.
services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts (1)
732-732
: Approve the test setup for handling multiple transactions.The setup for creating and testing multiple transactions is well-implemented, ensuring comprehensive coverage of the functionality.
Also applies to: 805-805
@@ -1,6 +1,6 @@ | |||
import { Direction } from '@/prisma/prisma.util'; |
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.
Review import statements for relevance and correctness.
The import of Direction
from @/prisma/prisma.util
appears to be unused since the custom Direction
type has been replaced by Prisma.SortOrder
. This could lead to unnecessary code and potential confusion.
Consider removing the unused import to clean up the code:
- import { Direction } from '@/prisma/prisma.util';
import { Prisma } from '@prisma/client';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
import { Direction } from '@/prisma/prisma.util'; | |
import { Prisma } from '@prisma/client'; |
// getTransactionsParameters: Parameters<typeof this.buildFilters>[0], | ||
projectId: string, | ||
options?: Prisma.TransactionRecordFindManyArgs, | ||
): Promise<TransactionRecord[]> { | ||
const args: Prisma.TransactionRecordFindManyArgs = {}; | ||
|
||
if (getTransactionsParameters.page?.number && getTransactionsParameters.page?.size) { | ||
// Temporary fix for pagination (class transformer issue) | ||
const size = parseInt(getTransactionsParameters.page.size as unknown as string, 10); | ||
const number = parseInt(getTransactionsParameters.page.number as unknown as string, 10); | ||
|
||
args.take = size; | ||
args.skip = size * (number - 1); | ||
} | ||
|
||
if (getTransactionsParameters.orderBy) { | ||
args.orderBy = toPrismaOrderByGeneric(getTransactionsParameters.orderBy); | ||
} | ||
|
||
return this.prisma.transactionRecord.findMany( | ||
this.scopeService.scopeFindMany( | ||
{ | ||
...options, | ||
where: { | ||
...this.buildFilters(getTransactionsParameters), | ||
...((options || {}).where || {}), |
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.
Missing parameter and potential issue with spread operator usage.
The method signature for findManyWithFilters
seems to be missing the getTransactionsParameters
parameter. This is crucial for the functionality described in the PR summary and needs to be corrected.
Additionally, the use of the spread operator in line 40 could lead to unintended consequences if not handled properly. Consider using optional chaining to ensure that the operation is safe and does not result in runtime errors.
Proposed fix for the method signature and spread operator usage:
-async findManyWithFilters(
+async findManyWithFilters(getTransactionsParameters: Parameters<typeof this.buildFilters>[0],
projectId: string,
options?: Prisma.TransactionRecordFindManyArgs,
): Promise<TransactionRecord[]> {
return this.prisma.transactionRecord.findMany(
this.scopeService.scopeFindMany(
{
...options,
where: {
- ...((options || {}).where || {}),
+ ...((options?.where) || {}),
},
},
[projectId],
),
);
}
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// getTransactionsParameters: Parameters<typeof this.buildFilters>[0], | |
projectId: string, | |
options?: Prisma.TransactionRecordFindManyArgs, | |
): Promise<TransactionRecord[]> { | |
const args: Prisma.TransactionRecordFindManyArgs = {}; | |
if (getTransactionsParameters.page?.number && getTransactionsParameters.page?.size) { | |
// Temporary fix for pagination (class transformer issue) | |
const size = parseInt(getTransactionsParameters.page.size as unknown as string, 10); | |
const number = parseInt(getTransactionsParameters.page.number as unknown as string, 10); | |
args.take = size; | |
args.skip = size * (number - 1); | |
} | |
if (getTransactionsParameters.orderBy) { | |
args.orderBy = toPrismaOrderByGeneric(getTransactionsParameters.orderBy); | |
} | |
return this.prisma.transactionRecord.findMany( | |
this.scopeService.scopeFindMany( | |
{ | |
...options, | |
where: { | |
...this.buildFilters(getTransactionsParameters), | |
...((options || {}).where || {}), | |
async findManyWithFilters(getTransactionsParameters: Parameters<typeof this.buildFilters>[0], | |
projectId: string, | |
options?: Prisma.TransactionRecordFindManyArgs, | |
): Promise<TransactionRecord[]> { | |
return this.prisma.transactionRecord.findMany( | |
this.scopeService.scopeFindMany( | |
{ | |
...options, | |
where: { | |
...((options?.where) || {}), |
Tools
Biome
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
const projectIdsClause = projectIds?.length || 0 > 1 ? { in: projectIds } : projectIds?.[0]; | ||
|
||
if (!projectIdsClause) { | ||
throw new Error( | ||
`Missing or empty 'projectId': A valid 'projectId' is required to proceed with the query.`, | ||
); | ||
} |
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.
Fix logical error in projectIdsClause
construction.
The current implementation of projectIdsClause
might not work as intended due to operator precedence. The expression projectIds?.length || 0 > 1
should be corrected to ensure proper evaluation.
Consider revising the condition to:
- const projectIdsClause = projectIds?.length || 0 > 1 ? { in: projectIds } : projectIds?.[0];
+ const projectIdsClause = (projectIds?.length > 1) ? { in: projectIds } : projectIds?.[0];
This change ensures that the length check is correctly evaluated before applying the logical OR.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const projectIdsClause = projectIds?.length || 0 > 1 ? { in: projectIds } : projectIds?.[0]; | |
if (!projectIdsClause) { | |
throw new Error( | |
`Missing or empty 'projectId': A valid 'projectId' is required to proceed with the query.`, | |
); | |
} | |
const projectIdsClause = (projectIds?.length > 1) ? { in: projectIds } : projectIds?.[0]; | |
if (!projectIdsClause) { | |
throw new Error( | |
`Missing or empty 'projectId': A valid 'projectId' is required to proceed with the query.`, | |
); | |
} |
export const buildPaginationFilter = ( | ||
filters: | ||
| { | ||
page: PageDto | undefined; | ||
} | ||
| undefined, | ||
) => { | ||
const args: Pagination = {}; | ||
|
||
if (filters && filters.page) { | ||
// Temporary fix for pagination (class transformer issue) | ||
if (!filters?.page.number || !filters?.page.size) { | ||
args.take = 10; | ||
} else { | ||
const size = parseInt(filters.page.size as unknown as string, 10); | ||
const number = parseInt(filters.page.number as unknown as string, 10); | ||
|
||
args.take = size; | ||
args.skip = size * (number - 1); | ||
} | ||
} else { | ||
args.take = 10; | ||
} | ||
|
||
return 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.
Suggest using an optional chain in buildPaginationFilter
.
The function correctly handles pagination logic. However, using an optional chain can simplify the code and enhance safety, as suggested by static analysis.
- if (!filters?.page.number || !filters?.page.size) {
+ if (!filters?.page?.number || !filters?.page?.size) {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const buildPaginationFilter = ( | |
filters: | |
| { | |
page: PageDto | undefined; | |
} | |
| undefined, | |
) => { | |
const args: Pagination = {}; | |
if (filters && filters.page) { | |
// Temporary fix for pagination (class transformer issue) | |
if (!filters?.page.number || !filters?.page.size) { | |
args.take = 10; | |
} else { | |
const size = parseInt(filters.page.size as unknown as string, 10); | |
const number = parseInt(filters.page.number as unknown as string, 10); | |
args.take = size; | |
args.skip = size * (number - 1); | |
} | |
} else { | |
args.take = 10; | |
} | |
return args; | |
}; | |
export const buildPaginationFilter = ( | |
filters: | |
| { | |
page: PageDto | undefined; | |
} | |
| undefined, | |
) => { | |
const args: Pagination = {}; | |
if (filters && filters.page) { | |
// Temporary fix for pagination (class transformer issue) | |
if (!filters?.page?.number || !filters?.page?.size) { | |
args.take = 10; | |
} else { | |
const size = parseInt(filters.page.size as unknown as string, 10); | |
const number = parseInt(filters.page.number as unknown as string, 10); | |
args.take = size; | |
args.skip = size * (number - 1); | |
} | |
} else { | |
args.take = 10; | |
} | |
return args; | |
}; |
Tools
Biome
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
const createCounterparty = async ( | ||
prismaService: PrismaService, | ||
proj?: Pick<Project, 'id'>, | ||
{ | ||
correlationIdFn, | ||
}: { | ||
correlationIdFn?: () => string; | ||
} = {}, | ||
) => { | ||
const correlationId = correlationIdFn ? correlationIdFn() : faker.datatype.uuid(); | ||
|
||
if (!proj) { | ||
const customer = await createCustomer( | ||
prismaService, | ||
faker.datatype.uuid(), | ||
faker.datatype.uuid(), | ||
'', | ||
'', | ||
'webhook-shared-secret', | ||
); | ||
|
||
proj = await createProject(prismaService, customer, faker.datatype.uuid()); | ||
} | ||
|
||
return await prismaService.counterparty.create({ | ||
data: { | ||
project: { connect: { id: proj.id } }, | ||
correlationId, | ||
business: { | ||
create: { | ||
correlationId, | ||
companyName: faker.company.name(), | ||
registrationNumber: faker.datatype.uuid(), | ||
mccCode: faker.datatype.number({ min: 1000, max: 9999 }), | ||
businessType: faker.lorem.word(), | ||
project: { connect: { id: proj.id } }, | ||
}, | ||
}, | ||
}, | ||
}); | ||
}; |
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.
Refactor the createCounterparty
function for better readability and maintainability.
The function createCounterparty
is quite complex and does multiple things, which can make it hard to understand and maintain. Consider breaking it down into smaller, more focused functions. Additionally, the function handles creating a customer and a project if none is provided, which could be separated out for clarity.
Consider refactoring the function to separate concerns and improve clarity. For example, extracting the customer and project creation logic into separate functions would make the code cleaner and easier to maintain.
describe('GET /external/transactions/by-alert', () => { | ||
let customer: Customer; | ||
let project: Project; | ||
let alertDefinition: AlertDefinition; | ||
let baseTransactionFactory: TransactionFactory; | ||
|
||
beforeAll(async () => { | ||
customer = await createCustomer( | ||
app.get(PrismaService), | ||
faker.datatype.uuid(), | ||
API_KEY, | ||
'', | ||
'', | ||
'webhook-shared-secret', | ||
); | ||
project = await createProject(app.get(PrismaService), customer, faker.datatype.uuid()); | ||
|
||
baseTransactionFactory = new TransactionFactory({ | ||
prisma: app.get(PrismaService), | ||
projectId: project.id, | ||
}) | ||
.paymentMethod(PaymentMethod.credit_card) | ||
.transactionDate(faker.date.recent(6)); | ||
}); | ||
|
||
describe('investigation transactions by alert', () => { | ||
let alertDefinition: AlertDefinition; | ||
let prismaService: PrismaService; | ||
|
||
beforeAll(async () => { | ||
prismaService = app.get(PrismaService); | ||
|
||
alertDefinition = await prismaService.alertDefinition.create({ | ||
data: getAlertDefinitionCreateData(ALERT_DEFINITIONS.PAY_HCA_CC, project, undefined, { | ||
crossEnvKey: faker.datatype.uuid(), | ||
}), | ||
}); | ||
|
||
expect( | ||
ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold, | ||
).toBeGreaterThanOrEqual(1000); | ||
|
||
expect(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.direction).toBe( | ||
TransactionDirection.inbound, | ||
); | ||
}); | ||
|
||
it.only('returns transactions associated with the given alertId', async () => { | ||
// Arrange | ||
await baseTransactionFactory | ||
.withBusinessBeneficiary() | ||
.direction(TransactionDirection.inbound) | ||
.paymentMethod(PaymentMethod.credit_card) | ||
.amount(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold + 1) | ||
.count(1) | ||
.create(); | ||
|
||
// Act | ||
await app.get(AlertService).checkAllAlerts(); | ||
|
||
// Assert | ||
const alerts = await app.get(PrismaService).alert.findMany({ | ||
where: { alertDefinitionId: alertDefinition.id, projectId: project.id }, | ||
}); | ||
if (alerts.length === 0 || alerts[0] === undefined) { | ||
throw new Error('No alerts found'); | ||
} | ||
expect(alerts).toHaveLength(1); | ||
expect(alerts[0]?.alertDefinitionId).toEqual(alertDefinition.id); | ||
expect(alerts[0] as any).toMatchObject({ | ||
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } }, | ||
}); | ||
|
||
console.log('Alert log: ', alerts[0].id); | ||
|
||
const response = await request(app.getHttpServer()) | ||
.get(`/api/v1/external/transactions/by-alert?alertId=${alerts[0].id}`) | ||
.set('Authorization', `Bearer ${API_KEY}`); | ||
|
||
expect(response.status).toBe(200); | ||
console.log(response.body); | ||
expect(response.body).toMatchInlineSnapshot(); | ||
}); | ||
}); | ||
}); |
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.
Review of main test block for transaction loading.
The test block investigation transactions by alert
is well-structured and effectively tests the integration of services like AlertService
and PrismaService
. The use of mock data and assertions are appropriate for verifying that the system behaves as expected when loading transactions based on alert rules.
However, the use of .only
on the test suite at line 233 is problematic as it prevents other tests from running. This should be removed to ensure that all tests are executed during integration testing.
Consider removing .only
to ensure all tests are executed. Here's the suggested change:
- it.only('returns transactions associated with the given alertId', async () => {
+ it('returns transactions associated with the given alertId', async () => {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
describe('GET /external/transactions/by-alert', () => { | |
let customer: Customer; | |
let project: Project; | |
let alertDefinition: AlertDefinition; | |
let baseTransactionFactory: TransactionFactory; | |
beforeAll(async () => { | |
customer = await createCustomer( | |
app.get(PrismaService), | |
faker.datatype.uuid(), | |
API_KEY, | |
'', | |
'', | |
'webhook-shared-secret', | |
); | |
project = await createProject(app.get(PrismaService), customer, faker.datatype.uuid()); | |
baseTransactionFactory = new TransactionFactory({ | |
prisma: app.get(PrismaService), | |
projectId: project.id, | |
}) | |
.paymentMethod(PaymentMethod.credit_card) | |
.transactionDate(faker.date.recent(6)); | |
}); | |
describe('investigation transactions by alert', () => { | |
let alertDefinition: AlertDefinition; | |
let prismaService: PrismaService; | |
beforeAll(async () => { | |
prismaService = app.get(PrismaService); | |
alertDefinition = await prismaService.alertDefinition.create({ | |
data: getAlertDefinitionCreateData(ALERT_DEFINITIONS.PAY_HCA_CC, project, undefined, { | |
crossEnvKey: faker.datatype.uuid(), | |
}), | |
}); | |
expect( | |
ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold, | |
).toBeGreaterThanOrEqual(1000); | |
expect(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.direction).toBe( | |
TransactionDirection.inbound, | |
); | |
}); | |
it.only('returns transactions associated with the given alertId', async () => { | |
// Arrange | |
await baseTransactionFactory | |
.withBusinessBeneficiary() | |
.direction(TransactionDirection.inbound) | |
.paymentMethod(PaymentMethod.credit_card) | |
.amount(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold + 1) | |
.count(1) | |
.create(); | |
// Act | |
await app.get(AlertService).checkAllAlerts(); | |
// Assert | |
const alerts = await app.get(PrismaService).alert.findMany({ | |
where: { alertDefinitionId: alertDefinition.id, projectId: project.id }, | |
}); | |
if (alerts.length === 0 || alerts[0] === undefined) { | |
throw new Error('No alerts found'); | |
} | |
expect(alerts).toHaveLength(1); | |
expect(alerts[0]?.alertDefinitionId).toEqual(alertDefinition.id); | |
expect(alerts[0] as any).toMatchObject({ | |
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } }, | |
}); | |
console.log('Alert log: ', alerts[0].id); | |
const response = await request(app.getHttpServer()) | |
.get(`/api/v1/external/transactions/by-alert?alertId=${alerts[0].id}`) | |
.set('Authorization', `Bearer ${API_KEY}`); | |
expect(response.status).toBe(200); | |
console.log(response.body); | |
expect(response.body).toMatchInlineSnapshot(); | |
}); | |
}); | |
}); | |
it('returns transactions associated with the given alertId', async () => { | |
// Arrange | |
await baseTransactionFactory | |
.withBusinessBeneficiary() | |
.direction(TransactionDirection.inbound) | |
.paymentMethod(PaymentMethod.credit_card) | |
.amount(ALERT_DEFINITIONS.PAY_HCA_CC.inlineRule.options.amountThreshold + 1) | |
.count(1) | |
.create(); | |
// Act | |
await app.get(AlertService).checkAllAlerts(); | |
// Assert | |
const alerts = await app.get(PrismaService).alert.findMany({ | |
where: { alertDefinitionId: alertDefinition.id, projectId: project.id }, | |
}); | |
if (alerts.length === 0 || alerts[0] === undefined) { | |
throw new Error('No alerts found'); | |
} | |
expect(alerts).toHaveLength(1); | |
expect(alerts[0]?.alertDefinitionId).toEqual(alertDefinition.id); | |
expect(alerts[0] as any).toMatchObject({ | |
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } }, | |
}); | |
console.log('Alert log: ', alerts[0].id); | |
const response = await request(app.getHttpServer()) | |
.get(`/api/v1/external/transactions/by-alert?alertId=${alerts[0].id}`) | |
.set('Authorization', `Bearer ${API_KEY}`); | |
expect(response.status).toBe(200); | |
console.log(response.body); | |
expect(response.body).toMatchInlineSnapshot(); | |
}); |
Tools
Biome
[error] 233-233: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
private buildTransactionsFiltersByAlert(inlineRule: InlineRule, alert?: Alert) { | ||
let whereClause: Prisma.TransactionRecordWhereInput = {}; | ||
|
||
const filters: { | ||
endDate: Date | undefined; | ||
startDate: Date | undefined; | ||
} = { | ||
endDate: undefined, | ||
startDate: undefined, | ||
}; | ||
|
||
if (alert) { | ||
const endDate = alert.updatedAt || alert.createdAt; | ||
endDate.setHours(23, 59, 59, 999); | ||
filters.endDate = endDate; | ||
} | ||
|
||
// @ts-ignore - TODO: Replace logic with proper implementation for each rule | ||
// eslint-disable-next-line | ||
let { timeAmount, timeUnit } = inlineRule.options; | ||
|
||
if (!timeAmount || !timeUnit) { | ||
if ( | ||
inlineRule.fnName === 'evaluateHighVelocityHistoricAverage' && | ||
inlineRule.options.lastDaysPeriod && | ||
timeUnit | ||
) { | ||
timeAmount = inlineRule.options.lastDaysPeriod.timeAmount; | ||
} else { | ||
return filters; | ||
} | ||
} | ||
|
||
let startDate = new Date(); | ||
|
||
let subtractValue = 0; | ||
|
||
const baseSubstractByMin = timeAmount * 60 * 1000; | ||
|
||
switch (timeUnit) { | ||
case TIME_UNITS.minutes: | ||
subtractValue = baseSubstractByMin; | ||
break; | ||
case TIME_UNITS.hours: | ||
subtractValue = 60 * baseSubstractByMin; | ||
break; | ||
case TIME_UNITS.days: | ||
subtractValue = 24 * 60 * baseSubstractByMin; | ||
break; | ||
case TIME_UNITS.months: | ||
startDate.setMonth(startDate.getMonth() - timeAmount); | ||
break; | ||
case TIME_UNITS.years: | ||
startDate.setFullYear(startDate.getFullYear() - timeAmount); | ||
break; | ||
} | ||
|
||
startDate.setHours(0, 0, 0, 0); | ||
startDate = new Date(startDate.getTime() - subtractValue); | ||
|
||
if (filters.endDate) { | ||
startDate = new Date(Math.min(startDate.getTime(), filters.endDate.getTime())); | ||
} | ||
|
||
filters.startDate = startDate; | ||
|
||
if (filters.startDate) { | ||
whereClause.transactionDate = { | ||
gte: filters.startDate, | ||
}; | ||
} | ||
|
||
if (filters.endDate) { | ||
whereClause.transactionDate = { | ||
lte: filters.endDate, | ||
}; | ||
} | ||
|
||
return whereClause; | ||
} |
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.
Refactor: Simplify date manipulation and improve readability.
The method buildTransactionsFiltersByAlert
constructs a date range filter based on the provided inlineRule
and alert
. The logic for adjusting the date based on timeUnit
and timeAmount
is overly complex and can be simplified using a date manipulation library like date-fns
or moment.js
. This would improve readability and reduce the potential for bugs related to date calculations.
Consider refactoring the date manipulation to use date-fns
, which provides straightforward utilities for adding and subtracting time units:
import { sub } from 'date-fns';
let startDate = new Date();
if (timeUnit && timeAmount) {
startDate = sub(startDate, { [timeUnit]: timeAmount });
}
This change would make the code cleaner and easier to maintain.
getInvestigationFilter( | ||
projectId: string, | ||
inlineRule: InlineRule, | ||
subject: Record<string, string>, | ||
) { | ||
let investigationFilter; | ||
switch (inlineRule.fnInvestigationName) { | ||
case 'investigateTransactionsAgainstDynamicRules': | ||
investigationFilter = this[inlineRule.fnInvestigationName]({ | ||
...inlineRule.options, | ||
projectId, | ||
}); | ||
} | ||
|
||
if (!investigationFilter) { | ||
this.logger.error(`No evaluation function found`, { | ||
inlineRule, | ||
}); | ||
|
||
throw new Error( | ||
`No evaluation function found for rule name: ${(inlineRule as InlineRule).id}`, | ||
); | ||
} | ||
|
||
return { | ||
counterpartyBeneficiaryId: subject.counterpartyBeneficiaryId, | ||
counterpartyOriginatorId: subject.counterpartyOriginatorId, | ||
...investigationFilter, | ||
...this.buildTransactionsFiltersByAlert(inlineRule), | ||
projectId, | ||
}; | ||
} |
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.
Enhance Error Handling: Add specific error messages and conditions.
The method getInvestigationFilter
dynamically selects an investigation function based on inlineRule.fnInvestigationName
. However, the error handling could be improved by providing more specific error messages and handling potential null values more gracefully.
Consider adding checks for null or undefined values and providing more detailed error messages to help with debugging:
if (!inlineRule || !inlineRule.fnInvestigationName) {
throw new Error('Invalid rule configuration: Missing function name.');
}
Additionally, ensure that the method handles cases where the function name does not correspond to a valid method, possibly logging these cases or notifying an administrator.
executionDetails: { executionRow: { transactionCount: '1', totalAmount: 1001 } }, | ||
}); | ||
|
||
console.log('Alert log: ', alerts[0].id); |
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.
Remove the debugging statement.
The console.log
statement should be removed from the test case to clean up the test output and adhere to best practices in production code.
Apply this diff to address the issue:
- console.log('Alert log: ', alerts[0].id);
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
console.log('Alert log: ', alerts[0].id); |
); | ||
}); | ||
|
||
it.only('returns transactions associated with the given alertId', async () => { |
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.
Remove the focus from the test case.
The use of it.only
should be removed to ensure that all test cases are executed during integration testing. This is a common practice to avoid during the final stages of development as it prevents other tests from running.
Apply this diff to address the issue:
- it.only('returns transactions associated with the given alertId', async () => {
+ it('returns transactions associated with the given alertId', async () => {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
it.only('returns transactions associated with the given alertId', async () => { | |
it('returns transactions associated with the given alertId', async () => { |
Tools
Biome
[error] 689-689: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- apps/workflows-dashboard/src/pages/Filters/hooks/useFiltersPagePagination/useFiltersPagePagination.ts (1 hunks)
- apps/workflows-dashboard/src/pages/WorkflowDefinitions/hooks/useWorkflowDefinitionsPagination/useWorkflowDefinitionsPagination.ts (1 hunks)
- apps/workflows-dashboard/src/pages/Workflows/Workflows.tsx (1 hunks)
- services/workflows-service/src/alert/alert.service.ts (4 hunks)
- services/workflows-service/src/alert/dtos/get-alerts.dto.ts (4 hunks)
- services/workflows-service/src/business-report/business-report.service.ts (2 hunks)
- services/workflows-service/src/business-report/dto/get-business-report.dto.ts (2 hunks)
- services/workflows-service/src/business-report/list-business-reports.dto.ts (2 hunks)
- services/workflows-service/src/case-management/controllers/case-management.controller.ts (1 hunks)
- services/workflows-service/src/common/dto.ts (2 hunks)
- services/workflows-service/src/project/project-scope.service.ts (2 hunks)
- services/workflows-service/src/transaction/dtos/get-transactions.dto.ts (1 hunks)
- services/workflows-service/src/transaction/transaction.controller.external.ts (4 hunks)
- services/workflows-service/src/transaction/transaction.repository.ts (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- services/workflows-service/src/alert/alert.service.ts
- services/workflows-service/src/alert/dtos/get-alerts.dto.ts
- services/workflows-service/src/project/project-scope.service.ts
- services/workflows-service/src/transaction/transaction.controller.external.ts
Additional context used
Biome
services/workflows-service/src/transaction/transaction.repository.ts
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (24)
services/workflows-service/src/business-report/dto/get-business-report.dto.ts (1)
1-8
: LGTM!The changes to the
orderBy
property improve the handling of sorting parameters by using a more dynamic approach with thebuildOrderByGenericFilter
function. The property is now validated using theIsEnum
decorator to ensure only valid sorting criteria can be specified. The changes also align the DTO with the underlying Prisma model by usingPrisma.Enumerable<Prisma.BusinessReportOrderByWithRelationInput>
type.Also applies to: 28-35
services/workflows-service/src/transaction/transaction.repository.ts (2)
30-30
: MissinggetTransactionsParameters
parameter in the method signature.The method signature for
findManyWithFilters
is still missing thegetTransactionsParameters
parameter. This is crucial for the functionality described in the PR summary and needs to be corrected.Proposed fix for the method signature:
async findManyWithFilters( + getTransactionsParameters: Parameters<typeof this.buildFilters>[0], projectId: string, options?: Prisma.TransactionRecordFindManyArgs, ): Promise<TransactionRecord[]> {
39-39
: Unsafe spread operator usage.The use of the spread operator at line 39 could lead to unintended consequences if not handled properly. Consider using optional chaining to ensure that the operation is safe and does not result in runtime errors.
Proposed fix for the spread operator usage:
where: { - ...((options || {}).where || {}), + ...((options?.where) || {}), },Tools
Biome
[error] 39-39: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
services/workflows-service/src/business-report/business-report.service.ts (1)
Line range hint
1-1
: Verify the removal of thefindManyWithFilters
method.The
findManyWithFilters
method has been entirely removed from theBusinessReportService
. This is a breaking change that alters the service's functionality and impacts any dependent components or services that relied on this functionality.Please ensure that this removal is intended and that all the dependent components have been updated accordingly. If this change is unintended, please restore the method.
To verify the impact of this change, run the following script:
Verification successful
No direct dependencies found for
findManyWithFilters
method.The
findManyWithFilters
method is not directly referenced in the codebase, suggesting that its removal might not impact other components. However, please ensure that this change is intentional and verify any indirect dependencies or documentation that might reference this method.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the removed `findManyWithFilters` method. # Test: Search for the method usage. Expect: No occurrences of the method call. rg --type typescript -A 5 $'BusinessReportService.*findManyWithFilters'Length of output: 107
Script:
#!/bin/bash # Description: Verify the usage of the removed `findManyWithFilters` method. # Test: Search for the method usage across all files. Expect: No occurrences of the method call. rg -A 5 'BusinessReportService.*findManyWithFilters'Length of output: 54
services/workflows-service/src/business-report/list-business-reports.dto.ts (2)
9-9
: LGTM!The addition of the
TBusinessOrderBy
type alias is a good change as it enhances type safety by restricting the possible values fororderBy
to a predefined set, rather than allowing any string.
40-40
: LGTM!The update to the
orderBy
property is a good change as it improves the clarity and robustness of the data transfer object by enforcing stricter validation rules on the ordering of business reports.services/workflows-service/src/transaction/dtos/get-transactions.dto.ts (8)
21-29
: LGTM!The code changes are approved.
31-33
: LGTM!The code changes are approved.
35-35
: LGTM!The code changes are approved.
36-36
: LGTM!The code changes are approved.
38-40
: LGTM!The code changes are approved.
42-46
: LGTM!The code changes are approved.
50-57
: LGTM!The code changes are approved.
59-75
: LGTM!The code changes are approved.
services/workflows-service/src/common/dto.ts (9)
10-12
: LGTM!The code changes are approved.
15-18
: LGTM!The code changes are approved.
56-56
: LGTM!The code changes are approved.
65-79
: LGTM!The code changes are approved.
105-114
: LGTM!The code changes are approved.
116-128
: LGTM!The code changes are approved.
130-154
: LGTM!The code changes are approved.
156-160
: LGTM!The code changes are approved.
180-185
: LGTM!The code changes are approved.
services/workflows-service/src/case-management/controllers/case-management.controller.ts (1)
121-122
: LGTM!The code changes are approved. The introduction of
alertsBenefiting
andalertsOriginating
properties in theCounterparty
object aligns with the objective of providing a more granular approach to categorizing alerts related to counterparties.
@@ -17,6 +17,6 @@ export const useFiltersPagePagination = () => { | |||
return { | |||
handlePageChange, | |||
total: data?.meta.pages || 1, | |||
page: filters.page || 1, | |||
page: filters || 1, |
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.
Fix the incorrect page
property in the return statement.
The change is incorrect as it returns the entire filters
object instead of the page
property. This will lead to incorrect pagination behavior as the page
property is expected to be a number.
Apply this diff to fix the issue:
- page: filters || 1,
+ page: filters.page || 1,
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
page: filters || 1, | |
page: filters.page || 1, |
@@ -17,6 +17,6 @@ export const useWorkflowDefinitionsPagination = () => { | |||
return { | |||
handlePageChange, | |||
total: data?.meta.pages || 1, | |||
page: filters.page || 1, | |||
page: filters || 1, |
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.
Fix the incorrect page
property in the return statement.
The change is incorrect as it returns the entire filters
object instead of the page
property. This will lead to incorrect pagination behavior as the page
property is expected to be a number.
Apply this diff to fix the issue:
- page: filters || 1,
+ page: filters.page || 1,
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
page: filters || 1, | |
page: filters.page || 1, |
import { BusinessReportType, Prisma } from '@prisma/client'; | ||
import { Transform } from 'class-transformer'; | ||
import { IsEnum, IsIn, IsOptional, IsString } from 'class-validator'; | ||
import { orderBy } from 'lodash'; |
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.
Remove the unused orderBy
import from lodash
.
The orderBy
import from lodash
is unused and should be removed to keep the code clean.
Apply this diff to fix the issue:
-import { orderBy } from 'lodash';
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
import { orderBy } from 'lodash'; |
@@ -71,7 +71,7 @@ export const Workflows = withFilters<FiltersProps<WorkflowsFiltersValues>, Workf | |||
<WorkflowsLayout.Footer> | |||
<Pagination | |||
totalPages={data.meta.pages || 1} | |||
page={filters.page || 1} | |||
page={filters || 1} |
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.
Fix the page
prop assignment.
The page
prop is incorrectly assigned the entire filters
object instead of the page
value from the filters
object. This will break the Pagination
component.
Apply this diff to fix the issue:
-<Pagination
- totalPages={data.meta.pages || 1}
- page={filters || 1}
- onChange={handlePageChange}
-/>
+<Pagination
+ totalPages={data.meta.pages || 1}
+ page={filters.page || 1}
+ onChange={handlePageChange}
+/>
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores