Fix regression in case sensitivity for is/matches operator
#3399
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jfdoming
changed the title
is/matches
jfdoming
changed the title
is/matchesis and matches
jfdoming
changed the title
is and matchesis/matches
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
jfdoming
force-pushed
the
jfdoming/fix-case-sensitive-regexp-match
branch
from
535f82d
to
6b44dd5
Compare
jfdoming
changed the title
is/matchesmatches
jfdoming
changed the title
matchesmatches operator
jfdoming
force-pushed
the
jfdoming/fix-case-sensitive-regexp-match
branch
2 times, most recently
from
a37b5c7
to
c16a12a
Compare
jfdoming
changed the title
matches operatoris/matches operator
jfdoming
force-pushed
the
jfdoming/fix-case-sensitive-regexp-match
branch
from
c16a12a
to
f846b0f
Compare
jfdoming
changed the title
is/matches operatoris/matches operator
jfdoming
force-pushed
the
jfdoming/fix-case-sensitive-regexp-match
branch
from
f846b0f
to
5f6a356
Compare
WalkthroughThe pull request introduces modifications across multiple files, primarily focusing on enhancing the dynamic handling of operations related to filters and conditions. Key changes include the transition from static operation references to a more flexible system using the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- packages/desktop-client/src/components/filters/FiltersMenu.jsx (3 hunks)
- packages/desktop-client/src/components/filters/updateFilterReducer.ts (1 hunks)
- packages/desktop-client/src/components/modals/EditRuleModal.jsx (3 hunks)
- packages/loot-core/src/server/accounts/rules.test.ts (14 hunks)
- packages/loot-core/src/server/accounts/rules.ts (6 hunks)
- packages/loot-core/src/server/accounts/transaction-rules.ts (4 hunks)
- packages/loot-core/src/server/rules/app.ts (1 hunks)
- packages/loot-core/src/server/schedules/app.ts (1 hunks)
- packages/loot-core/src/shared/rules.ts (3 hunks)
- packages/loot-core/src/types/models/rule.d.ts (1 hunks)
- upcoming-release-notes/3399.md (1 hunks)
Files skipped from review due to trivial changes (3)
- packages/loot-core/src/server/accounts/transaction-rules.ts
- packages/loot-core/src/server/rules/app.ts
- packages/loot-core/src/server/schedules/app.ts
Additional comments not posted (34)
upcoming-release-notes/3399.md (1)
1-6: The release notes look good!The release notes accurately reflect the bug fix described in the PR objectives and the AI summary. The categorization as a "Bugfix" is appropriate, the author is clearly identified, and the title provides a concise description of the fix.
packages/desktop-client/src/components/filters/updateFilterReducer.ts (1)
14-14: Improved type safety.The explicit type annotation
RuleConditionEntity['value'] | nullfor thevaluevariable enhances type safety by ensuring thatvaluecan either be of the specified type ornull. This may help prevent potential runtime errors related to type mismatches.The change is a type-level improvement and does not affect the runtime behavior of the function. It makes the code more maintainable by clearly communicating the expected type of
value.packages/loot-core/src/types/models/rule.d.ts (1)
40-41: LGTM! The new properties enhance the rule condition capabilities.The addition of the
clearedandreconciledboolean properties to theFieldValueTypestype definition is a positive change that expands the rule condition capabilities. These properties likely represent additional states related to financial transactions or records, allowing for more nuanced handling of transaction states.This change may facilitate improvements in features like reconciliation and reporting. However, it's important to ensure that existing code relying on the
FieldValueTypestype is updated to handle these new properties correctly.To verify the impact of this change, run the following script:
Verification successful
The addition of 'cleared' and 'reconciled' properties is well-integrated and consistent with the codebase.
The verification process confirms that the new 'cleared' and 'reconciled' boolean properties in the FieldValueTypes are properly integrated into the existing rule system and used consistently across the codebase. Key observations:
- The properties are correctly defined in 'packages/loot-core/src/types/models/rule.d.ts'.
- They are incorporated into the FIELDS object in 'packages/loot-core/src/shared/rules.ts'.
- The new properties are utilized in various parts of the application, including account syncing, transaction handling, and rule processing.
No inconsistencies or potential issues were identified as a result of these additions. The changes appear to enhance the rule condition capabilities as intended, without causing any apparent problems in the existing codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the new properties are handled correctly in the codebase. # Test 1: Search for usages of the `FieldValueTypes` type. # Expect: Relevant code segments that may need to be updated. rg --type typescript -A 5 $'FieldValueTypes' # Test 2: Search for rule condition checks on the new properties. # Expect: Code segments that utilize the new properties in rule conditions. rg --type typescript -A 5 $'(cleared|reconciled)\s*:\s*boolean'Length of output: 178
Script:
#!/bin/bash # Description: Verify the impact of adding 'cleared' and 'reconciled' properties to FieldValueTypes # Test 1: Search for usages of the `FieldValueTypes` type in .ts and .js files echo "Searching for FieldValueTypes usages:" fd -e ts -e js | xargs grep -n "FieldValueTypes" || echo "No matches found for FieldValueTypes" # Test 2: Search for 'cleared' and 'reconciled' properties in .ts and .js files echo -e "\nSearching for 'cleared' and 'reconciled' properties:" fd -e ts -e js | xargs grep -n -E "(cleared|reconciled)\s*:\s*(boolean|true|false)" || echo "No matches found for cleared or reconciled properties" # Test 3: Look for rule-related files and their contents echo -e "\nSearching for rule-related files:" fd -e ts -e js | grep -i "rule" | while read -r file; do echo "File: $file" cat "$file" | grep -n -E "(FieldValueTypes|cleared|reconciled)" echo -e "---\n" doneLength of output: 8390
packages/loot-core/src/shared/rules.ts (5)
Line range hint
8-51: LGTM!The removal of the
imported_payeefield fromTYPE_INFOaligns with the PR objective of transitioning to a more flexible system for handling field types and their associated operations.
53-72: Great addition!The introduction of the
FieldInfoConstrainttype andFIELD_INFOobject enhances the structure and type safety of the code. It provides a centralized way to define field constraints, including the specific rules for theimported_payeefield. This change improves maintainability and ensures consistent enforcement of field-specific rules.
76-80: Nice improvement!Dynamically generating the
FIELD_TYPESmap from theFIELD_INFOobject is a great way to ensure consistency and reduce duplication. This change centralizes the field type information and improves maintainability by keeping the field-to-type mapping in sync with the defined field constraints.
83-101: Excellent addition!The
isValidOpandgetValidOpsfunctions are a great addition to the codebase. They centralize the logic for operation validation and retrieval, making it more robust and maintainable. By considering the field-specific constraints defined inFIELD_INFO, these functions ensure that disallowed operations are properly excluded. This change enhances the flexibility and reliability of the code when working with field operations.
214-217: Good improvement!Expanding the
getFieldErrorfunction to include new cases for string and boolean types is a positive change. It enhances the error handling and feedback for invalid input values, providing users with more informative error messages. This improvement contributes to a better overall user experience when working with the application.packages/desktop-client/src/components/filters/FiltersMenu.jsx (3)
22-22: LGTM!Importing the
getValidOpsfunction is a good change that allows for dynamically determining the valid operations based on the field type.
80-80: Verify the exclusion of the 'isbetween' operation.Using
getValidOps(field)to dynamically determine the valid operations based on the field type is a good approach. However, please verify that excluding the 'isbetween' operation is intentional and document the reason for its exclusion.
262-262: LGTM!Using
getValidOps(field)to dynamically determine the valid operations based on the selected field type is a good change that ensures the available operations are contextually appropriate.packages/loot-core/src/server/accounts/rules.test.ts (15)
Line range hint
12-31: Test looks good!The test covers parsing of various valid and invalid date formats using the
parseDateStringfunction. The test cases are comprehensive.
34-49: Test changes look good!The test has been correctly updated to use the
notesfield instead ofname. The logic of testing condition operators against null field values remains intact.
55-64: Test changes look good!The test has been correctly updated to use the
notesfield instead ofname. The logic of testing condition operators against undefined field values remains intact.
68-84: Test looks good!The test verifies that the date conditions correctly restrict the allowed operators based on the date format. It checks for the expected errors when using invalid operators with a 'YYYY-MM' date format.
Line range hint
87-108: Test looks good!The test comprehensively verifies the behavior of the
isoperator with date conditions. It checks the operator against different date formats and ensures correct matching at the corresponding granularity. The test also covers theisapproxoperator with a specific date value.
Line range hint
110-161: Test looks good!The test thoroughly verifies the behavior of the
isoperator with recurring date conditions. It checks the operator against different recurring patterns specified using start date, frequency, and optional patterns or interval. The test ensures correct matching of dates that satisfy the recurring condition. It also covers theisapproxoperator with a recurring date condition.
164-181: Test looks good!The test verifies the behavior of comparison operators (
gt,gte,lt,lte) with date conditions. It checks that the operators correctly compare the dates and return the expected results.
184-193: Test changes look good!The test has been updated to use the
payeefield as astringtype instead ofimported_payee. The logic of testing theisandoneOfoperators with case-insensitive matching remains intact.
197-225: Test changes look good!The test has been correctly updated to use the
notesfield instead ofname. The logic of testing theis,oneOf,contains, andmatchesoperators with case-insensitive matching and different scenarios remains intact.
229-230: Test change looks good!The test has been correctly updated to use the
notesfield instead ofname. The logic of testing thematchesoperator with an invalid regex pattern remains intact.
234-252: Test looks good!The test verifies the value validation for number conditions. It checks that the
isapprox,is, andisbetweenoperators accept valid values and throw errors for invalid ones. The test cases cover the relevant scenarios.
255-294: Test looks good!The test comprehensively verifies the behavior of various operators (
is,isapprox,isbetween,gt,gte,lt,lte) with number conditions. It checks that the operators correctly compare the numbers and return the expected results. The test cases cover all the relevant operators and different scenarios.
300-312: Test changes look good!The test has been correctly updated to use the
notesfield instead ofname. The logic of testing thesetoperator's behavior, including setting the field value, handling invalid fields, and handling invalid action operations, remains intact.
315-318: Test looks good!The test verifies that the
setoperator throws an error when given an empty value for theaccountfield. This is an expected behavior to prevent setting empty account values.
325-353: Test changes look good!The test has been correctly updated to use the
notesfield instead ofname. The logic of testing the execution of rules, including applying actions when conditions are met, returning updated fields, and returning the original object when conditions are not met, remains intact.packages/loot-core/src/server/accounts/rules.ts (7)
257-258: LGTM!The removal of the
fieldTypesparameter and the direct usage ofFIELD_TYPESconstant improves the code by reducing coupling. The validation logic is comprehensive and the parsing of thevalueparameter is delegated to the condition type'sparsefunction, allowing for type-specific parsing.
494-502: LGTM!The removal of the
fieldTypesparameter and the direct usage ofFIELD_TYPESconstant improves the code by reducing coupling. The validation logic covers various scenarios and the setting of thefieldandtypeproperties based on theopvalue is a clear and concise way to handle different action types.
729-732: LGTM!The removal of the
fieldTypesparameter from the mapping ofconditionsandactionsis consistent with the changes made to theConditionandActionconstructors. The constructor provides a clear and concise way to create aRuleinstance with the necessary properties.
177-190: LGTM!The additional validation for string condition values is a good addition. The assertion that the
valueis a string ensures that the value is of the expected type. The assertion that thevalueis a non-empty string for certain operations prevents invalid or empty values from being used. These changes improve the robustness and reliability of the string condition parsing.
Line range hint
563-567: LGTM!The
execNonSplitActionsfunction provides a clear and concise way to execute non-split actions on a transaction. The creation of a copy of thetransactionobject ensures that the original transaction is not modified. The iteration over theactionsarray and the execution of each action on theupdateobject is a straightforward approach. The function is focused on its responsibility and does not introduce any changes or new functionality.
Line range hint
569-687: LGTM!The
execActionsfunction handles the execution of actions on a transaction, taking into account split transactions. The separation ofparentActionsandchildActionsbased on thesplitIndexoption is a clear way to handle actions that apply to the parent transaction and split transactions separately. The calculation of the total split count and the early return for transactions with no splits or child transactions is a good optimization.The handling of fixed split amounts and the distribution of the remaining amount to splits without a fixed amount is a complex process, but it is well-structured and handles various scenarios such as fixed percentage splits and remainder splits. The removal of the split at index 0 (reserved for "Apply to all" actions) from the
subtransactionsarray is a necessary step to ensure that the resulting transaction object is consistent.Overall, the function is well-organized and handles the execution of actions on transactions with splits effectively.
Line range hint
1060-1106: LGTM!The
migrateIdsfunction provides a way to migrate IDs in a rule based on a providedmappingsobject. It handles different condition operations (is,isNot,oneOf,notOneOf) and updates thevalueandunparsedValueproperties accordingly. It also handles actions of typeidwith thesetoperation and updates thevalueproperty.The function keeps the original value in the
rawValueproperty, which is important for handling undo operations and maintaining a consistent mapping. The function is well-structured and handles the migration of IDs in a rule effectively.Overall, the function is a useful utility for migrating IDs in rules and maintains the necessary data for consistent mapping and undo operations.
packages/desktop-client/src/components/modals/EditRuleModal.jsx (1)
581-582: LGTM!The changes to use
isValidOpfor checking if the current operation is valid for the new condition's field look good. This ensures that the operation is appropriate for the context of the field, enhancing the robustness of the condition handling.
| @@ -651,7 +653,7 @@ function ConditionsList({ | |||
| ) : ( | |||
| <Stack spacing={2} data-testid="condition-list"> | |||
| {conditions.map((cond, i) => { | |||
| let ops = TYPE_INFO[cond.type].ops; | |||
| let ops = getValidOps(cond.field); | |||
There was a problem hiding this comment.
Use getValidOps consistently.
Consider using getValidOps consistently to retrieve valid operations for all condition fields instead of having a separate hack for recurring dates and amount inflow/outflow. This would make the code more maintainable and less error-prone.
There was a problem hiding this comment.
This could be done in a separate refactor, but right now there is no way for getValidOps to perform op-specific logic. The change would also need additional testing that I don't want to add to an already-large PR. But thanks for the suggestion! I made a note of it.
There was a problem hiding this comment.
@jfdoming, I understand. Deferring this change to a separate refactor makes sense to keep this PR focused. Thank you for considering the suggestion.
(_/)
( ..)
(")(")
jfdoming
added
regression
Thanks for the review! This has fully broken adding any is/oneOf/matches conditions on imported payee for me and others. IMO we can test it more thoroughly but we should try to get some version of it in, there have been many reports of this on discord as well. If you'd like I can try to make a more minimal version? |
|
Ah.. I didn't realize it's that bad. In that case: lets merge now. We still have a few days to have it in edge and test thoroughly. |
Yeah sounds good. I'll make an effort to test a bunch of rules stuff later this weekend to make it safe. Also sorry, realized I came off as a bit combative 😅 just wanted to convey the breakage, really appreciate the diligence though. |
|
Yea, I hit this issue as a hard roadblock when first testing out Actual, and have completely shelved it pending a fix 😄 |
Fixes #3347.
A few changes here:
imported_payeefield type in favour of reusing the logic forstring. Instead, we do a two-stage mapping, from field to type and then from the type to the type-specific logic.fieldTypesparameter to rule-related classes. The parameter always had the same value except in tests, so I just updated the tests to use the same types instead.Steps to repro the issue on edge:
containsop to start.matchesoriswith the same value and observe that there is now no match.The same steps work as expected with this PR.