In case insensitive risk rule engine operator #2790
Conversation
|
|
Warning Rate limit exceeded@tomer-shvadron has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces version updates across multiple packages in the Changes
Possibly related PRs
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: 4
🧹 Outside diff range and nitpick comments (12)
packages/common/src/rule-engine/operators/constants.ts (1)
30-30: LGTM: Addition of IN_CASE_INSENSITIVE to OperationHelpersThe new entry for
IN_CASE_INSENSITIVEis correctly added to theOperationHelpersobject, following the existing pattern and maintaining type safety with theas constassertion. This addition completes the implementation of the new case-insensitive operator in the constants file.Consider grouping related operations together for better code organization. For example, you could move this new entry next to the
INoperation:export const OperationHelpers = { // ... other operations [OPERATION.IN]: IN, [OPERATION.IN_CASE_INSENSITIVE]: IN_CASE_INSENSITIVE, [OPERATION.NOT_IN]: NOT_IN, // ... remaining operations } as const;This grouping would make it easier to locate and maintain related operations in the future.
packages/common/src/rule-engine/operators/enums.ts (1)
12-12: LGTM! Consider adding a brief comment for clarity.The addition of
IN_CASE_INSENSITIVEto theOPERATIONobject is consistent with the PR objective and follows the existing naming conventions. Well done!To improve maintainability, consider adding a brief comment explaining the purpose of this new operation. For example:
// Performs a case-insensitive 'IN' operation IN_CASE_INSENSITIVE: 'IN_CASE_INSENSITIVE',packages/common/src/rule-engine/operators/types.ts (1)
9-9: LGTM! Consider grouping related imports.The addition of
OPERATIONandOPERATORimports aligns with the PR objective. To improve code organization, consider grouping related imports together.You could group the imports like this:
import { z, ZodSchema } from 'zod'; import { AmlCheckSchema, BetweenSchema, LastYearsSchema, PrimitiveSchema, } from '@/rule-engine/operators/schemas'; import { OPERATION, OPERATOR } from './enums';websites/docs/package.json (1)
20-20: LGTM: Dependency version update looks good.The update of @ballerine/common from ^0.9.43 to ^0.9.44 is consistent with the broader changes across the project. This minor version bump likely includes backward-compatible new features or bug fixes.
Don't forget to update the changelog if there are any notable changes or features introduced in this new version of @ballerine/common.
services/workflows-service/src/rule-engine/rule-engine.service.intg.test.ts (3)
16-31: LGTM: Well-structured test case for IN_CASE_INSENSITIVE rule.The test case setup correctly defines rules to test the new IN_CASE_INSENSITIVE operator. The structure and combination of rules are appropriate.
Consider adding a descriptive comment above the
rulesconstant to explain the purpose of these specific rules, enhancing the test's readability.
38-43: LGTM with suggestions: Test execution is correct, but assertions could be more robust.The test correctly executes the rule engine and checks for 'PASSED' status. However, consider enhancing the assertions for more comprehensive testing:
- Verify the number of rules in the result:
expect(result.length).toBe(2);
- Check individual rule results:
expect(result[0].status).toBe('PASSED'); expect(result[0].key).toBe('single'); expect(result[1].status).toBe('PASSED'); expect(result[1].key).toBe('array');These additions will make the test more robust and informative.
1-44: Overall: Good test implementation with room for expansion.This integration test effectively covers the new IN_CASE_INSENSITIVE rule, aligning well with the PR objectives. The test structure is sound and follows good practices.
To further improve the test suite:
- Consider adding more test cases to cover edge cases, such as:
- Empty strings or arrays
- Mixed case input
- Special characters or numbers in the input
- Add a negative test case where the rule should fail.
- Test the behavior when combining IN_CASE_INSENSITIVE with other operators.
These additions would provide more comprehensive coverage of the new feature and ensure its robustness across various scenarios.
packages/common/src/rule-engine/rules/schemas.ts (1)
87-91: LGTM! New case-insensitive operator added.The addition of the
IN_CASE_INSENSITIVEoperator aligns with the PR objective and expands the rule engine's functionality. This allows for more flexible rule definitions, which is a valuable improvement.Consider updating the documentation to include information about this new operator and its usage. This will help developers understand and utilize this new feature effectively.
services/workflows-service/src/notion/notion.service.ts (1)
95-97: LGTM! Consider adding a comment for consistency.The addition of handling for the 'unique_id' field type is correct and consistent with the existing pattern in the method. It successfully extends the functionality to support this new Notion field type.
For consistency with other conditions, consider adding a brief comment explaining the 'unique_id' field type, similar to how other conditions are documented. This would improve code readability and maintainability. For example:
+ // Handle Notion's unique identifier field if (notionField.type === 'unique_id') { return notionField.unique_id.number; }packages/common/CHANGELOG.md (1)
3-7: Improve the change description and fix grammatical issue.The changelog entry is mostly correct, but there are a couple of minor improvements we can make:
- The description "In case insensitive risk rule engine operator" could be more informative. Consider expanding it to clarify what exactly changed.
- There's a minor grammatical issue: "case insensitive" should be hyphenated as "case-insensitive".
Consider updating the changelog entry as follows:
## 0.9.44 ### Patch Changes -- In case insensitive risk rule engine operator ++ Added case-insensitive operator to the risk rule engineThis change provides more context about what was added and fixes the hyphenation issue.
🧰 Tools
🪛 LanguageTool
[misspelling] ~7-~7: This word is normally spelled with a hyphen.
Context: ...mon ## 0.9.44 ### Patch Changes - In case insensitive risk rule engine operator ## 0.9.43 #...(EN_COMPOUNDS_CASE_INSENSITIVE)
examples/headless-example/CHANGELOG.md (1)
3-10: LGTM! Consider adding a brief description of changes.The changelog entry for version 0.3.55 correctly documents the updated dependencies. This is consistent with the overall changes in the pull request.
Consider adding a brief description of any notable changes or improvements introduced by these dependency updates. This can help users understand the impact of the new versions.
services/workflows-service/CHANGELOG.md (1)
7-7: Consider hyphenating "case-insensitive" for better readability.The new feature description currently reads "In case insensitive risk rule engine operator". For improved clarity and adherence to common English conventions, consider hyphenating "case-insensitive":
- In case insensitive risk rule engine operator + In case-insensitive risk rule engine operatorThis minor change enhances readability and follows standard spelling conventions for compound adjectives.
🧰 Tools
🪛 LanguageTool
[misspelling] ~7-~7: This word is normally spelled with a hyphen.
Context: ...ice ## 0.7.59 ### Patch Changes - In case insensitive risk rule engine operator - Updated dep...(EN_COMPOUNDS_CASE_INSENSITIVE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
- apps/backoffice-v2/CHANGELOG.md (1 hunks)
- apps/backoffice-v2/package.json (2 hunks)
- apps/kyb-app/CHANGELOG.md (1 hunks)
- apps/kyb-app/package.json (2 hunks)
- examples/headless-example/CHANGELOG.md (1 hunks)
- examples/headless-example/package.json (2 hunks)
- packages/common/CHANGELOG.md (1 hunks)
- packages/common/package.json (1 hunks)
- packages/common/src/rule-engine/operators/constants.ts (2 hunks)
- packages/common/src/rule-engine/operators/enums.ts (2 hunks)
- packages/common/src/rule-engine/operators/helpers.ts (18 hunks)
- packages/common/src/rule-engine/operators/types.ts (2 hunks)
- packages/common/src/rule-engine/rules/schemas.ts (2 hunks)
- packages/workflow-core/CHANGELOG.md (1 hunks)
- packages/workflow-core/package.json (2 hunks)
- sdks/web-ui-sdk/CHANGELOG.md (1 hunks)
- sdks/web-ui-sdk/package.json (2 hunks)
- sdks/workflow-browser-sdk/CHANGELOG.md (1 hunks)
- sdks/workflow-browser-sdk/package.json (2 hunks)
- sdks/workflow-node-sdk/CHANGELOG.md (1 hunks)
- sdks/workflow-node-sdk/package.json (2 hunks)
- services/workflows-service/CHANGELOG.md (1 hunks)
- services/workflows-service/package.json (2 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/notion/notion.service.ts (1 hunks)
- services/workflows-service/src/rule-engine/rule-engine.service.intg.test.ts (1 hunks)
- websites/docs/package.json (1 hunks)
✅ Files skipped from review due to trivial changes (9)
- apps/backoffice-v2/CHANGELOG.md
- apps/kyb-app/CHANGELOG.md
- packages/common/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/workflows-service/prisma/data-migrations
🧰 Additional context used
🪛 LanguageTool
packages/common/CHANGELOG.md
[misspelling] ~7-~7: This word is normally spelled with a hyphen.
Context: ...mon ## 0.9.44 ### Patch Changes - In case insensitive risk rule engine operator ## 0.9.43 #...(EN_COMPOUNDS_CASE_INSENSITIVE)
services/workflows-service/CHANGELOG.md
[misspelling] ~7-~7: This word is normally spelled with a hyphen.
Context: ...ice ## 0.7.59 ### Patch Changes - In case insensitive risk rule engine operator - Updated dep...(EN_COMPOUNDS_CASE_INSENSITIVE)
🔇 Additional comments (29)
packages/common/src/rule-engine/operators/constants.ts (2)
14-14: LGTM: New import for IN_CASE_INSENSITIVE constantThe import of the
IN_CASE_INSENSITIVEconstant is consistent with the existing import style and naming conventions. This addition aligns with the PR objective of introducing a case-insensitive risk rule engine operator.
14-14: Summary: Successful implementation of IN_CASE_INSENSITIVE operatorThe changes in this file successfully implement the new case-insensitive risk rule engine operator as described in the PR objectives. The additions are minimal, focused, and consistent with the existing code structure and conventions. These changes lay the groundwork for using the new operator in the rule engine, enhancing its flexibility as intended.
Also applies to: 30-30
packages/common/src/rule-engine/operators/enums.ts (1)
28-28: LGTM! Verify usage across the codebase.The addition of
OPERATION.IN_CASE_INSENSITIVEto theOPERATIONSarray is correct and maintains consistency with theOPERATIONobject.To ensure this change is properly integrated, let's verify its usage across the codebase:
✅ Verification successful
Verified:
OPERATION.IN_CASE_INSENSITIVEis correctly integrated and used across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new IN_CASE_INSENSITIVE operation # Test 1: Search for direct usage of IN_CASE_INSENSITIVE echo "Searching for direct usage of IN_CASE_INSENSITIVE:" rg "IN_CASE_INSENSITIVE" --type ts # Test 2: Search for any switch statements or if-else chains that might need updating echo "Searching for switch statements or if-else chains involving OPERATION:" rg "switch.*OPERATION|if.*OPERATION" --type ts # Test 3: Check for any documentation or comments that might need updating echo "Checking for documentation or comments mentioning operations:" rg "Available operations|Supported operations" --type tsLength of output: 1600
packages/common/src/rule-engine/operators/types.ts (4)
11-13: LGTM! Good addition of type definitions.The new
TOperationandTOperatortypes provide improved type safety for the rule engine, aligning with the PR's objectives. This change enhances the overall type checking in the codebase.
Line range hint
1-31: Overall, the changes enhance the rule engine's flexibility.The modifications to this file, including the new imports, type definitions, and the update to
ConditionFn, align well with the PR's objective of introducing a new case-insensitive risk rule engine operator. These changes should improve the type safety and flexibility of the rule engine.However, it's crucial to verify the impact of these changes, particularly the removal of
OperationHelperand the update toConditionFn, across the entire codebase to ensure no unintended side effects.
Line range hint
1-31: Verify impact of OperationHelper type removal.The removal of the
OperationHelpertype and its related import suggests a change in how operations are handled. This might be related to the introduction of the new case-insensitive operator.Please run the following script to check for any remaining usage of
OperationHelperorOperationHelpersacross the codebase:#!/bin/bash # Description: Check for usage of OperationHelper or OperationHelpers echo "Searching for OperationHelper usage:" rg "OperationHelper" --type ts echo "\nSearching for OperationHelpers usage:" rg "OperationHelpers" --type tsIf any usages are found, they may need to be updated or removed to align with this change.
28-31: LGTM! Verify usage across the codebase.The update to
ConditionFntype enhances its flexibility by allowing more specific type definitions. This aligns well with the PR's objective of improving the risk rule engine.To ensure this change doesn't break existing code, please verify its usage across the codebase. Run the following script to check for potential issues:
services/workflows-service/src/rule-engine/rule-engine.service.intg.test.ts (2)
1-14: LGTM: Proper test setup and imports.The test setup follows NestJS testing best practices. The import of
RuleSetfrom@ballerine/commonaligns with the PR objectives mentioning updates to this package.
33-36: LGTM: Appropriate test data for case insensitivity.The
formDataobject is well-structured to test the IN_CASE_INSENSITIVE operator. It correctly includes both single string and array scenarios with intentional case differences.packages/common/src/rule-engine/rules/schemas.ts (2)
15-16: LGTM! Stylistic improvement in function definition.The change from a traditional function declaration to an arrow function is a good stylistic choice that aligns with modern JavaScript practices. The functionality remains unchanged, maintaining the code's behavior while improving readability.
5-6: LGTM! Verify consistency of import changes across the project.The consolidation of imports from
@/rule-engineimproves maintainability. However, ensure this change is consistent across the entire project to maintain architectural integrity.Run the following script to verify the import changes:
Also applies to: 13-13
✅ Verification successful
Imports Consolidated Successfully
All old import patterns from
'../operators/enums'and'../operators/schemas'have been removed. The new import pattern from'@/rule-engine'is consistently applied across the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of import changes for OPERATION and OPERATOR # Test 1: Check for any remaining imports from '../operators/enums' or '../operators/schemas' echo "Checking for old import patterns:" rg -g '*.ts' -e "from '../operators/enums'" -e "from '../operators/schemas'" # Test 2: Check for new import pattern echo "Checking for new import pattern:" rg -g '*.ts' -e "from '@/rule-engine'"Length of output: 453
sdks/workflow-browser-sdk/package.json (1)
4-4: LGTM! Version and dependency updates are consistent.The changes in this file are part of a coordinated update across the @ballerine ecosystem:
- Package version bumped from 0.6.55 to 0.6.56
- @ballerine/common dependency updated from 0.9.43 to 0.9.44
- @ballerine/workflow-core dependency updated from 0.6.55 to 0.6.56
These updates align with the overall changes mentioned in the pull request summary.
Let's verify if these version updates are consistent across the entire pull request:
Also applies to: 36-37
✅ Verification successful
Consistency Verified! All version and dependency updates are uniformly applied across the repository.
- Verified "@ballerine/workflow-browser-sdk": "0.6.56" in multiple package.json files.
- Verified "@ballerine/common": "0.9.44" in multiple package.json files.
- Verified "@ballerine/workflow-core": "0.6.56" in multiple package.json files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of version updates across the repository # Test 1: Check for @ballerine/workflow-browser-sdk version echo "Checking @ballerine/workflow-browser-sdk version:" rg '"@ballerine/workflow-browser-sdk": "0\.6\.56"' -g '*.json' # Test 2: Check for @ballerine/common version echo "Checking @ballerine/common version:" rg '"@ballerine/common": "0\.9\.44"' -g '*.json' # Test 3: Check for @ballerine/workflow-core version echo "Checking @ballerine/workflow-core version:" rg '"@ballerine/workflow-core": "0\.6\.56"' -g '*.json' # Note: If any of these searches return no results, it might indicate inconsistent updatesLength of output: 1368
sdks/web-ui-sdk/package.json (2)
24-24: LGTM: Version update is appropriate.The package version has been incremented from 1.5.44 to 1.5.45, which is a patch version update. This aligns with semantic versioning principles and is consistent with the PR objectives mentioning updates across multiple packages.
99-99: LGTM: Dependency update looks good. Verify compatibility.The @ballerine/common dependency has been updated from 0.9.43 to 0.9.44, which is consistent with the updates mentioned in the PR summary. This change aligns with the PR objectives of updating dependencies across multiple packages.
To ensure this update doesn't introduce any breaking changes, please run the following verification script:
apps/backoffice-v2/package.json (5)
3-3: LGTM: Version update is appropriate.The minor version bump from 0.7.58 to 0.7.59 is consistent with the changes made in this update and follows semantic versioning principles.
54-54: LGTM: @ballerine/common dependency update is appropriate.The patch version update of @ballerine/common from 0.9.43 to 0.9.44 is consistent with the other dependency updates in this PR and likely includes bug fixes or minor improvements.
57-57: LGTM: @ballerine/workflow-browser-sdk dependency update is appropriate.The patch version update of @ballerine/workflow-browser-sdk from 0.6.55 to 0.6.56 is consistent with the other dependency updates in this PR and likely includes bug fixes or minor improvements.
58-58: LGTM: @ballerine/workflow-node-sdk dependency update is appropriate.The patch version update of @ballerine/workflow-node-sdk from 0.6.55 to 0.6.56 is consistent with the other dependency updates in this PR and likely includes bug fixes or minor improvements.
3-3: Summary: All dependency updates and version bump are appropriate.The changes in this file include:
- A minor version bump of the package itself (0.7.58 to 0.7.59)
- Patch updates to three @ballerine dependencies:
- @ballerine/common (0.9.43 to 0.9.44)
- @ballerine/workflow-browser-sdk (0.6.55 to 0.6.56)
- @ballerine/workflow-node-sdk (0.6.55 to 0.6.56)
These updates are consistent with the PR objectives and the AI-generated summary. They reflect proper version management and are likely to include bug fixes or minor improvements across the @ballerine ecosystem.
Also applies to: 54-54, 57-58
services/workflows-service/package.json (5)
4-4: Version bump looks good.The package version has been incremented from 0.7.58 to 0.7.59, which aligns with the addition of the new case-insensitive risk rule engine operator mentioned in the PR objectives.
52-52: Dependency update looks good. Verify consistency across the monorepo.The @ballerine/workflow-node-sdk dependency has been updated from 0.6.55 to 0.6.56. This patch update is likely to contain bug fixes or minor improvements.
Please run the following script to ensure this update is consistent across the monorepo:
#!/bin/bash # Description: Check for consistency of @ballerine/workflow-node-sdk version across the monorepo echo "Checking @ballerine/workflow-node-sdk version consistency:" grep -R '"@ballerine/workflow-node-sdk": "' . --include=package.json | grep -v node_modules
4-4: Summary of changes and final verification step.All changes in this file look good:
- The package version has been bumped to 0.7.59.
- The following dependencies have been updated to their latest patch versions:
- @ballerine/common
- @ballerine/workflow-core
- @ballerine/workflow-node-sdk
These updates are consistent with the PR objectives and the AI-generated summary.
As a final step, please run the following script to ensure all dependency versions are consistent and to verify that the package-lock.json file has been updated:
#!/bin/bash # Description: Final verification of dependency versions and package-lock.json echo "Checking all updated dependency versions:" grep -R '"@ballerine/common": "' . --include=package.json | grep -v node_modules grep -R '"@ballerine/workflow-core": "' . --include=package.json | grep -v node_modules grep -R '"@ballerine/workflow-node-sdk": "' . --include=package.json | grep -v node_modules echo "Verifying package-lock.json:" grep -A 3 '"@ballerine/workflows-service":' package-lock.jsonIf all versions are consistent and the package-lock.json has been updated correctly, the changes are ready to be merged.
Also applies to: 50-52
51-51: Dependency update looks good. Verify consistency across the monorepo.The @ballerine/workflow-core dependency has been updated from 0.6.55 to 0.6.56. This patch update is likely to contain bug fixes or minor improvements.
Please run the following script to ensure this update is consistent across the monorepo:
50-50: Dependency update looks good. Verify consistency across the monorepo.The @ballerine/common dependency has been updated from 0.9.43 to 0.9.44. This patch update is likely to contain bug fixes or minor improvements.
Please run the following script to ensure this update is consistent across the monorepo:
services/workflows-service/CHANGELOG.md (1)
3-11: LGTM! New feature and dependency updates look good.The changelog entry for version 0.7.59 includes a new feature and dependency updates:
New feature: "In case insensitive risk rule engine operator"
This feature enhances the risk rule engine by adding case-insensitive operations, which should improve flexibility in rule matching.Dependency updates:
- @ballerine/common updated to 0.9.44
- @ballerine/workflow-core updated to 0.6.56
- @ballerine/workflow-node-sdk updated to 0.6.56
These changes are well-documented and follow the expected format for a changelog entry.
🧰 Tools
🪛 LanguageTool
[misspelling] ~7-~7: This word is normally spelled with a hyphen.
Context: ...ice ## 0.7.59 ### Patch Changes - In case insensitive risk rule engine operator - Updated dep...(EN_COMPOUNDS_CASE_INSENSITIVE)
packages/common/src/rule-engine/operators/helpers.ts (4)
Line range hint
21-38: Update toBaseOperatorGenerics Enhances Type SafetyThe change to
BaseOperatornow accepting separateTDataValueandTConditionValuegeneric parameters improves type specificity and flexibility across operator implementations. This enhances the clarity of data and condition value types in derived classes.
121-157:InCaseInsensitiveOperator Implementation Looks GoodThe new
InCaseInsensitiveoperator correctly implements case-insensitive comparison for both single values and arrays. The approach of normalizing strings to lowercase ensures accurate comparisons regardless of case.
391-391: Export ofIN_CASE_INSENSITIVEOperator AddedThe export statement
export const IN_CASE_INSENSITIVE = new InCaseInsensitive();correctly exposes the new operator for use throughout the project.
Line range hint
107-118: Ensure Consistent Generic Parameters in Derived ClassesThe
Inoperator has been updated to extendBaseOperator<Primitive, Primitive[]>. Please verify that all instances where operators extendBaseOperatorhave been updated to reflect the new generic parameter structure to prevent potential type mismatches.You can run the following script to identify any derived classes that may need updating:
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1)
585-637: LGTM: Comprehensive tests added forIN_CASE_INSENSITIVEoperatorThe new test suite for the
IN_CASE_INSENSITIVEoperator is well-structured and covers both string and array properties, including positive and negative cases. This ensures the operator functions correctly for different data types and scenarios.However, there's a minor issue in the test description:
On line 612, the test description "should correctly evaluate when using a string property" should be changed to "should correctly evaluate when using an array property" to accurately reflect the test case.
Consider applying this change:
- it('should correctly evaluate when using a string property', () => { + it('should correctly evaluate when using an array property', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/common/src/rule-engine/operators/constants.ts (2 hunks)
- services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/common/src/rule-engine/operators/constants.ts
🧰 Additional context used
🔇 Additional comments (1)
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1)
122-123: LGTM: Error message updated to include new operatorThe error message has been correctly updated to include the new
IN_CASE_INSENSITIVEoperator in the list of expected operators. This change is consistent with the addition of the new operator to the system.Also applies to: 128-128
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1)
369-369: Clarify the purpose of "not in use" in the describe block title.The describe block title includes "- not in use" which is confusing. If the operator is not in use, why are we testing it? Consider either removing the "not in use" suffix or adding a comment explaining why these tests exist for an unused operator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (2)
Line range hint
471-522: LGTM!The NOT_EQUALS operator tests are well-structured and cover both positive and negative cases effectively.
Line range hint
523-584: LGTM!The IN operator tests are comprehensive and cover both positive and negative cases effectively.
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (2)
585-637: Enhance test coverage for IN_CASE_INSENSITIVE operator.While the current tests cover basic functionality, consider adding the following test cases:
- Edge cases:
- Empty strings
- Null values
- Mixed case arrays (e.g.,
['US', 'ca', 'Gb'])- Error handling:
- Validation of error messages
- Validation of error types for invalid inputs
Example additional test:
it('should handle edge cases correctly', () => { const ruleSetExample: RuleSet = { operator: OPERATOR.AND, rules: [ { key: 'country', operator: OPERATION.IN_CASE_INSENSITIVE, value: ['US', 'ca', ''], }, ], }; // Test null value const data = { country: null }; let validationResults = runRuleSet(ruleSetExample, data); expect(validationResults[0]!.status).toBe('FAILED'); expect(validationResults[0]!.error).toBeInstanceOf(DataValueNotFoundError); // Test empty string data.country = ''; validationResults = runRuleSet(ruleSetExample, data); expect(validationResults[0]!.status).toBe('PASSED'); // Test mixed case array data.country = 'Ca'; validationResults = runRuleSet(ruleSetExample, { country: ['us', 'CA', 'Gb'] }); expect(validationResults[0]!.status).toBe('PASSED'); });
Line range hint
471-637: Maintain consistent assertion styles across test suites.The test suites use a mix of assertion styles:
- Snapshot assertions (e.g., in NOT_EQUALS tests)
- Direct assertions (e.g., in IN_CASE_INSENSITIVE tests)
- Varying levels of error checking
Consider adopting a consistent approach across all operator tests for better maintainability. For example, either:
- Use snapshot testing consistently for all operator results
- Or use direct assertions with consistent error checking patterns
Example consistent pattern:
// Template for operator tests it('should evaluate <operator> correctly', () => { const result = runRuleSet(ruleSet, data); expect(result).toHaveLength(1); expect(result[0]).toMatchObject({ status: expect.any(String), error: expect.any(Error), message: expect.any(String), rule: expect.any(Object), }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (4 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🔇 Additional comments (2)
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (2)
Line range hint
1-1000: Implementation looks good! 👍The test implementation provides good coverage of the rule engine operators, particularly the new IN_CASE_INSENSITIVE operator. The test organization is clear, and the descriptions effectively communicate the test purposes.
369-369: Clarify or remove unused test suite.The test suite description indicates "EXISTS operator - not in use". If this operator is truly not in use, consider removing these tests to maintain a clean and relevant test suite. Otherwise, update the description to reflect its actual status.
Summary by CodeRabbit
@ballerine/commonto version0.9.44across multiple projects.@ballerine/workflow-coreto version0.6.56across multiple projects.@ballerine/workflow-node-sdkto version0.6.56in the workflows service.@ballerine/workflow-browser-sdkto version0.6.56.@ballerine/web-ui-sdkto version1.5.45.unique_id, in the Notion service.IN_CASE_INSENSITIVEoperator.