[bal-3193] - backward compatbility for previous alert's subject #2898
Conversation
|
WalkthroughThis pull request introduces modifications to several services within the workflows service: Changes
Possibly Related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/workflows-service/src/data-analytics/data-investigation.service.ts (1)
94-105: Track temporary backward compatibility fixWhile the implementation correctly handles the mapping of counterparty IDs, it's marked as temporary. Consider creating a tracking issue to ensure this temporary fix is properly addressed in the future.
Would you like me to create a GitHub issue to track the removal of this temporary fix once all rules are updated?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/workflows-service/src/alert/alert.repository.ts(1 hunks)services/workflows-service/src/alert/alert.service.ts(2 hunks)services/workflows-service/src/data-analytics/data-investigation.service.ts(2 hunks)
🔇 Additional comments (3)
services/workflows-service/src/alert/alert.repository.ts (1)
21-21: LGTM: Type extension enables related entity inclusion
The addition of 'include' to the type definition enhances query flexibility, allowing for related entities to be included when needed.
services/workflows-service/src/data-analytics/data-investigation.service.ts (1)
23-23: LGTM: Optional subject parameter aligns with backward compatibility needs
Making the subject parameter optional is a good approach for maintaining backward compatibility.
services/workflows-service/src/alert/alert.service.ts (1)
67-77: LGTM: Enhanced alert filtering with monitoring type
The addition of monitoringType filtering improves the precision of alert queries. The implementation correctly uses the equals operator for exact matching.
Let's verify the monitoring type usage across the codebase:
✅ Verification successful
Let me gather more information about the monitoringType usage to ensure it's consistently applied.
Based on the gathered information, I can now provide the final response:
Verified: Monitoring type filtering is correctly implemented
The implementation is consistent across the codebase:
getAlertWithDefinitionandgetAlertsmethods properly filter by monitoringType- The monitoringType parameter is correctly passed from controllers for both transaction_monitoring and ongoing_merchant_monitoring
- Alert definitions are consistently filtered by monitoringType in related queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of monitoringType in alert queries
# Search for alert queries with monitoringType
ast-grep --pattern 'monitoringType: {
equals: $_
}'
# Check for any potential missed monitoringType filters
rg -A 3 'getAlertWithDefinition|getAlerts'
Length of output: 5820
Script:
#!/bin/bash
# Check for all alert-related service calls to verify monitoringType parameter usage
rg -A 2 "alertService\."
# Check the AlertService class implementation for other methods that might need monitoringType
ast-grep --pattern 'class AlertService {
$$$
}'
# Check for MonitoringType enum usage
rg "MonitoringType\." -A 2
Length of output: 63081
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
services/workflows-service/src/transaction/transaction.controller.external.ts (1)
Type safety concern confirmed - Runtime type checking needed for executionDetails
The investigation reveals that
executionDetailsis typed as a unionTCustomerSubscription | TExecutionDetails | anyin the Alert type definition, making the direct type assertion toTExecutionDetailspotentially unsafe. The suggested refactoring is valid since:
- No runtime type checking exists for
executionDetails- The type includes
anywhich bypasses type safety- The property is accessed immediately after casting without validation
The original refactoring suggestion is appropriate:
-if (isEmpty((alert.executionDetails as TExecutionDetails).filters)) { +const executionDetails = alert.executionDetails as TExecutionDetails; +if (!executionDetails || isEmpty(executionDetails.filters)) {🔗 Analysis chain
Line range hint
371-374: Consider type safety for executionDetails castingThe type assertion
(alert.executionDetails as TExecutionDetails)might be unsafe. Consider adding runtime type checking.-if (isEmpty((alert.executionDetails as TExecutionDetails).filters)) { +const executionDetails = alert.executionDetails as TExecutionDetails; +if (!executionDetails || isEmpty(executionDetails.filters)) {
Let me gather more information about the usage of TExecutionDetails to understand the context better.
Let me check one more thing to understand the Alert type and its usage better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TExecutionDetails type definition and its usage rg -A 5 "type TExecutionDetails"Length of output: 456
Script:
#!/bin/bash # Search for files where TExecutionDetails is used rg -B 2 -A 2 "executionDetails.*TExecutionDetails" # Search for the specific casting pattern ast-grep --pattern 'alert.executionDetails as TExecutionDetails'Length of output: 1228
Script:
#!/bin/bash # Search for Alert type definition and its usage rg -B 2 -A 5 "type Alert" # Check if there are any existing type guards or validation for executionDetails rg -B 2 -A 5 "executionDetails.*instanceof|isExecutionDetails"Length of output: 493
🧹 Nitpick comments (2)
services/workflows-service/src/transaction/transaction.controller.external.ts (2)
Line range hint
371-371: Improve backward compatibility commentThe comment "Backward compatibility will be remove soon" contains a typo and lacks context about when and why it will be removed.
-// Backward compatibility will be remove soon, +// TODO: Remove backward compatibility in next major version. +// This handles legacy alerts that don't have executionDetails.filters
Line range hint
382-447: Document V1/V2 version differencesThe code maintains two versions of transaction retrieval (V1 and V2) but lacks documentation explaining their differences and migration path.
Add JSDoc comments to both methods explaining:
/** * @deprecated Use getTransactionsByAlertV2 for new implementations * Legacy method that handles alerts without executionDetails.filters * Uses inlineRule and subject for filtering */ private getTransactionsByAlertV1... /** * Current version that uses executionDetails.filters * Provides more flexible filtering capabilities */ private getTransactionsByAlertV2...Also applies to: 448-507
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/workflows-service/src/transaction/transaction.controller.external.ts(2 hunks)
🔇 Additional comments (1)
services/workflows-service/src/transaction/transaction.controller.external.ts (1)
40-40: LGTM: Import statement is correctly placed
The MonitoringType import is properly added and used in the code.
services/workflows-service/src/transaction/transaction.controller.external.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts (4)
639-639: Consider concurrency management with Promise.all().
Creating multiple transaction records in parallel is okay, but if there's any concurrency-limiting requirement or rate-limiting constraints, you may need to adopt a throttling mechanism like p-limit.
678-679: Follow through with the TODO comment.
Comment suggests this scenario “shouldn’t happen.” If that’s truly the case, consider removing or updating the test to reflect realistic usage.
682-686: Enforce consistent patterns for returning only the first element.
Here you explicitly retrieve the first transaction record from the array. If you need multiple records, consider returning them as a list.
733-737: Skim for potential usage of all created items.
As before, you directly pick index 0 from the returned array. If multiple records exist, confirm that you only need the first.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
services/workflows-service/src/alert/alert.repository.ts(1 hunks)services/workflows-service/src/alert/alert.service.ts(4 hunks)services/workflows-service/src/test/helpers/create-alert-definition.ts(1 hunks)services/workflows-service/src/test/helpers/create-alert.ts(1 hunks)services/workflows-service/src/test/helpers/create-transaction-record.ts(3 hunks)services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts(6 hunks)services/workflows-service/src/transaction/transaction.controller.external.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/workflows-service/src/alert/alert.repository.ts
- services/workflows-service/src/alert/alert.service.ts
- services/workflows-service/src/transaction/transaction.controller.external.ts
🔇 Additional comments (14)
services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts (5)
654-655: Verify passing only the first array element as 'transactions'.
Here, “result[0]” is presumably an array of transaction records. If multiple transactions are relevant to this alert, you may want to flatten them or pass them all.
661-661: Check test assertion alignment.
The test now expects only one transaction returned. Confirm that the business requirement is indeed limiting the result to a single record.
717-717: Alert creation with empty transactions array.
Like the earlier test, you’re passing an empty array. Make sure the alert creation flow supports having zero transactions, or clarify if it’s a corner case.
745-745: Confirm single-item array usage.
You are creating an alert with only [tx1]. Validate if that’s consistent with the expectation for multiple relevant transactions.
752-752: Check final expectation for correct coverage.
The test checks for exactly one transaction. Ensure that the logic truly restricts or filters the transactions to one in the given timeframe.
services/workflows-service/src/test/helpers/create-alert.ts (3)
3-4: Confirm necessity of new imports.
The import from create-transaction-record is used to type the “transactions” parameter. This is valid, but be sure this file truly needs the direct import (rather than relying on an interface or shared type).
10-10: Updated function signature.
The new “transactions” parameter adds helpful context for alert creation. Make sure any callers are updated accordingly.
24-24: Validate subject correctness.
If the alert definition’s subjects array has multiple entries or none, consider how that affects the data passed here. Be sure subjects[0] is always set.
services/workflows-service/src/test/helpers/create-alert-definition.ts (1)
61-63: Revised subject set.
Switching to only “counterpartyBeneficiaryId” or “counterpartyOriginatorId” might exclude prior subjects (e.g. “businessId”). Confirm that’s intentional.
services/workflows-service/src/test/helpers/create-transaction-record.ts (5)
25-25: Ensure the new DTO import is used throughout.
This line imports TransactionCreatedDto. Confirm that any references to it are correct and you aren’t importing unused or mismatched types.
126-126: holderName generation changed.
Using faker.name.fullName() is an improvement over the deprecated faker.name.findName(). This is good for consistency.
135-145: Improved type clarity.
Using “satisfies” clarifies the shape of created transactions. This helps ensure type checks for error messages vs. valid transaction data.
146-157: Return full transaction record from DB.
You now retrieve the actual records, including references to counterparties. This aligns with your new requirement to use these records elsewhere (e.g., alert creation).
160-160: Good practice: returning the transaction records.
Returning the full records ensures the next steps have the necessary details.
services/workflows-service/src/transaction/transaction.controller.external.intg.test.ts
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Bug Fixes
Documentation