feat(rules): templating actions #3305
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 No assets were 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 |
UnderKoen
changed the title
|
Could you put in some more examples of what is possible with this? I see that you can pull in field values, can you pull in things like account balance? |
|
You can currently only pull in information about the transaction. This is limited to anything what is currently possible with data available to rules. So not the name of the payee or name of the category. I saw an todo The main use case for this would currently be cleaning imported transactions. Or altering existing transactions. Or changing the date of new income to the first of the month. Is this now very verbose but we could add another helper for adding duration to dates. The helpers I currently implemented are:
Values available:
This list can be extended by adding more values at |
|
There probably needs to be a syntax check in the rule edit window since its so easy to have bad syntax with something like this. Are these supposed to run when manually creating a transaction? My testing doesn't seem to work in that case. I think its working for file imports though. Im worried that there could be some issues with how the csv import checks for duplicates if someone has a templated rule. That will require more testing. edit: So the csv importer can't see the matching transaction if there is a big change (change the amount sign for example), but the rule gets run before the real deduplication goes. Maybe we should look at moving the rules inside of the check that the csv import does for duplication matching. |
Will add
I just tested this and it has been working but not consistently, I think the rules are not always run?
Seems smart, I wouldn't have a clue how to achieve this. Could this be fixxed in an separatem PR and making this feature expiremental? |
|
Making this experimental would probably be good for now. |
Can this be set? or is it just available to use? |
I'm not sure, if it's possible to change it with current
I will look how to do this! |
Out of scope of this PR I think but wouldn't this be best solved with field like |
I don't think that will fix matching if the amount gets changed |
I don't think I understand what the intented behaviour is and how to check the behaviour? |
If you import a csv and then import the same csv again you should see that all the "new" transactions get matched with the existing transaction and shown to you in the import window to decide what to do. So compare that to if you have a rule that modifies the amount using a template rule and there wont be a match even though its the same transaction and will get deduplicated after the rules run. |
|
Does this not work with the exported CSV's? I have no rules active and use the demo file |
|
Opened tracking issue #3606 for experimental feedback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/accounts/rules.ts (2)
Line range hint
553-575: Enhance error handling and type safety in the Action class.The addition of Handlebars templating to the
Actionclass is a good improvement. However, there are a couple of areas that could be enhanced:
- The error handling for invalid templates could be more informative.
- The
optionsparameter lacks type specificity.Consider the following improvements:
- Enhance error handling:
if (options?.template) { try { this.handlebarsTemplate = Handlebars.compile(options.template); this.handlebarsTemplate({}); } catch (e) { throw new Error(`Invalid Handlebars template: ${e.message}`); } }
- Improve type safety by defining an interface for the options:
interface ActionOptions { template?: string; // Add other option properties as needed } constructor(op: ActionOperator, field: string, value: any, options?: ActionOptions) { // ... existing code ... }These changes will provide more detailed error messages and improve type safety in the
Actionclass.
1-1: Summary: Handlebars templating enhances rule flexibility, but consider improving error handling and type safety.The introduction of Handlebars templating to the rules system is a significant enhancement that provides greater flexibility in defining actions. However, there are several areas where the implementation could be improved:
- Error handling in Handlebars helpers, particularly for math operations and regex usage.
- Type safety in the
Actionclass constructor and options.- Robustness in template execution and type conversion in the
execmethod.Addressing these points will lead to a more robust and maintainable codebase. Consider implementing the suggested improvements in error handling, type safety, and data validation throughout the file.
As this change introduces more complexity to the rule system, consider the following architectural improvements:
- Create a separate module for Handlebars helpers to improve modularity.
- Implement a robust error handling and logging strategy throughout the rule execution process.
- Consider adding unit tests for the new templating functionality to ensure reliability.
These changes will help maintain the integrity of the rule system as it grows in complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/src/components/settings/Experimental.tsx (1 hunks)
- packages/loot-core/src/server/accounts/rules.test.ts (1 hunks)
- packages/loot-core/src/server/accounts/rules.ts (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/accounts/rules.test.ts
🧰 Additional context used
🔇 Additional comments (2)
packages/desktop-client/src/components/settings/Experimental.tsx (2)
Line range hint
1-138: Summary: New feature toggle successfully integratedThe changes in this file are focused and well-implemented. The new feature toggle for "Rule action templating" has been seamlessly integrated into the
ExperimentalFeaturescomponent, following the established patterns in the codebase. This addition aligns perfectly with the PR objectives and enhances the application's functionality by allowing users to enable or disable the templating feature for rule actions.The implementation includes:
- Proper use of the
FeatureTogglecomponent- Correct flag naming ("actionTemplating")
- An appropriate feedback link for user input
These changes contribute to the overall goal of introducing templating actions within rules, as described in the PR objectives. The code is clean, consistent, and ready for integration.
113-118: LGTM! The new feature toggle is correctly implemented.The addition of the
FeatureTogglefor "Rule action templating" aligns well with the PR objectives. The implementation follows the established pattern in the file, and the feedback link correctly points to the relevant GitHub issue.To ensure consistency across the codebase, let's verify the usage of this new feature flag:
This script will help us verify that the feature flag is properly defined, used consistently, and not hardcoded elsewhere in the codebase.
✅ Verification successful
Verification Successful!
The
actionTemplatingfeature flag is correctly defined and utilized across the codebase. No unintended or duplicated references were found, ensuring consistent usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the 'actionTemplating' feature flag across the codebase. # Test 1: Check for the feature flag definition echo "Checking for feature flag definition:" rg --type typescript --type javascript "actionTemplating.*FeatureFlag" # Test 2: Check for usage of the feature flag echo "Checking for usage of the feature flag:" rg --type typescript --type javascript "useFeatureFlag.*actionTemplating" # Test 3: Check for any hardcoded references to the feature echo "Checking for hardcoded references:" rg --type typescript --type javascript "actionTemplating" -g '!**/Experimental.tsx'Length of output: 590
Script:
#!/bin/bash # Description: Verify the usage of the 'actionTemplating' feature flag across the codebase. # Test 1: Check for the feature flag definition echo "Checking for feature flag definition:" rg "actionTemplating.*FeatureFlag" -g "*.ts" -g "*.tsx" # Test 2: Check for usage of the feature flag echo "Checking for usage of the feature flag:" rg "useFeatureFlag.*actionTemplating" -g "*.ts" -g "*.tsx" # Test 3: Check for any hardcoded references to the feature echo "Checking for hardcoded references:" rg "actionTemplating" -g "*.ts" -g "*.tsx" -g "!Experimental.tsx"Length of output: 580
| @@ -1,5 +1,6 @@ | |||
| // @ts-strict-ignore | |||
| import * as dateFns from 'date-fns'; | |||
| import * as Handlebars from 'handlebars'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and security in Handlebars helpers.
The addition of Handlebars templating and helper functions is a good improvement for flexibility. However, there are a few points to consider:
- The
mathHelperfunction doesn't handle potentialNaNvalues, which could lead to unexpected results. - The
regexhelper might be vulnerable to ReDoS (Regular Expression Denial of Service) attacks if user input is directly used. - The
debughelper logs to the console, which might not be ideal for production environments.
Consider the following improvements:
- Add
NaNchecking in themathHelper:
function mathHelper(fn: (a: number, b: number) => number) {
return (a: unknown, ...b: unknown[]) => {
b.splice(-1, 1);
const numbers = [a, ...b].map(Number);
if (numbers.some(isNaN)) {
throw new Error('Invalid number input');
}
return numbers.reduce(fn);
};
}- Implement a safeguard against complex regex patterns in the
regexhelper:
regex: (value: unknown, regex: unknown, replace: unknown) => {
// ... existing checks ...
if (regex.length > 100) { // Adjust the limit as needed
throw new Error('Regex pattern too complex');
}
// ... rest of the function ...
}- Consider replacing the
debughelper with a more production-friendly logging mechanism or remove it entirely if not needed in production.
Also applies to: 34-88
| if (this.handlebarsTemplate) { | ||
| object[this.field] = this.handlebarsTemplate({ | ||
| ...object, | ||
| today: currentDay(), | ||
| }); | ||
|
|
||
| // Handlebars always returns a string, so we need to convert | ||
| switch (this.type) { | ||
| case 'number': | ||
| object[this.field] = parseFloat(object[this.field]); | ||
| break; | ||
| case 'date': | ||
| object[this.field] = parseDate(object[this.field]); | ||
| break; | ||
| case 'boolean': | ||
| object[this.field] = object[this.field] === 'true'; | ||
| break; | ||
| } | ||
| } else { | ||
| object[this.field] = this.value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness in template execution and type conversion.
The modifications to the exec method to handle Handlebars templates are a good addition. However, there are a few areas that could be improved for better error handling and robustness:
- There's no error handling for failed type conversions.
- The type conversion switch statement could be more comprehensive.
Consider the following improvements:
- Add error handling for type conversions:
case 'number':
const num = parseFloat(object[this.field]);
if (isNaN(num)) {
throw new Error(`Invalid number conversion for field ${this.field}`);
}
object[this.field] = num;
break;
case 'date':
const date = parseDate(object[this.field]);
if (isNaN(date.getTime())) {
throw new Error(`Invalid date conversion for field ${this.field}`);
}
object[this.field] = date;
break;- Make the type conversion more robust:
switch (this.type) {
case 'number':
// ... (as above)
case 'date':
// ... (as above)
case 'boolean':
object[this.field] = object[this.field].toLowerCase() === 'true';
break;
case 'string':
// Ensure it's a string
object[this.field] = String(object[this.field]);
break;
default:
throw new Error(`Unsupported type ${this.type} for field ${this.field}`);
}These changes will make the exec method more robust and less prone to runtime errors.
Relates to #500 and #3378
chrome_jgqSYZmJON.mp4