feat: templating on payee_name #3619
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. |
WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/rules/Value.tsx (1)
89-89: LGTM! Consider grouping similar cases for improved readability.The addition of the
payee_namecase is consistent with the handling of similar fields likenotesandimported_payee. This change aligns well with the PR objective of introducing templating capabilities for thepayee_namefield.For improved code readability, consider grouping similar cases together. You could refactor the switch statement like this:
switch (field) { case 'notes': case 'imported_payee': case 'payee_name': return value; // ... other cases }This grouping makes it easier to see at a glance which fields are treated similarly.
packages/loot-core/src/shared/rules.ts (1)
64-64: Overall impact:payee_namefield support added successfullyThe changes successfully implement the templating capability for the
payee_namefield as described in the PR objectives. The implementation is consistent with the existing code structure and patterns.Consider the following follow-up actions:
- Ensure that other parts of the codebase (e.g., UI components, API endpoints) are updated to utilize this new field.
- Update relevant documentation to reflect the addition of the
payee_namefield.- Add or update unit tests to cover the new
payee_namefield in rule processing.Also applies to: 116-117
packages/loot-core/src/server/accounts/sync.ts (1)
Ensure all
runRulescalls are awaited in test filesThe shell script results indicate that several instances of
runRulesintransaction-rules.test.tsare not using theawaitkeyword. To maintain consistency and proper asynchronous handling across the codebase, please update these calls as follows:
- File:
packages/loot-core/src/server/accounts/transaction-rules.test.ts
- Replace
const transaction = runRules({withconst transaction = await runRules({- Replace
let transaction = runRules({withlet transaction = await runRules({- Replace
transaction = runRules({withtransaction = await runRules({- Ensure all other instances of
runRulesin this file are preceded byawait.🔗 Analysis chain
Line range hint
1-824: Summary of changes and follow-up suggestionThe modifications in this file consistently update the
runRulesfunction to be asynchronous, aligning with the PR objectives for introducing templating capabilities for thepayee_namefield. While the changes are approved, there are a few improvements suggested:
- Update the
matchTransactionsfunction signature to be explicitly async.- Add error handling for the asynchronous
runRulescalls in bothmatchTransactionsandaddTransactionsfunctions.To ensure the changes are comprehensive and consistent, it's recommended to:
This will help identify any other instances of
runRulesthat might need to be updated to maintain consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of runRules that might need to be updated rg "runRules\(" --type tsLength of output: 1252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3619.mdis excluded by!**/*.md
📒 Files selected for processing (7)
- packages/desktop-client/src/components/modals/EditRuleModal.jsx (3 hunks)
- packages/desktop-client/src/components/rules/Value.tsx (1 hunks)
- packages/loot-core/src/server/accounts/rules.ts (1 hunks)
- packages/loot-core/src/server/accounts/sync.ts (2 hunks)
- packages/loot-core/src/server/accounts/transaction-rules.ts (5 hunks)
- packages/loot-core/src/shared/rules.ts (2 hunks)
- packages/loot-core/src/types/models/rule.d.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/accounts/transaction-rules.ts
[error] 804-804: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (7)
packages/loot-core/src/shared/rules.ts (2)
64-64: LGTM: Addition ofpayee_nametoFIELD_INFOThe addition of
payee_nameas a string type in theFIELD_INFOobject is consistent with the existing structure and aligns with the PR objectives. This change enables the use ofpayee_namein rule processing and validation.
116-117: LGTM: Update tomapFieldfunctionThe addition of the
payee_namecase in themapFieldfunction is consistent with the PR objectives. The returned string 'payee (name)' provides a clear and user-friendly representation of the field.packages/loot-core/src/server/accounts/rules.ts (1)
47-50: LGTM! Improved null handling in regex helper.This change enhances the robustness of the
regexhelper function by adding a null check for thevalueparameter. It prevents potential runtime errors when the function is called with null values, returning null in such cases. This is a good defensive programming practice.packages/loot-core/src/server/accounts/transaction-rules.ts (1)
Line range hint
277-291: Ensure all callers ofrunRuleshandle its new async natureThe
runRulesfunction has been updated to be asynchronous. Verify that all functions callingrunRuleshave been updated to handle the Promise it now returns, to prevent unhandled Promise rejections or unexpected behavior.Run the following script to find all usages of
runRulesand check if they handle the async function properly:packages/desktop-client/src/components/modals/EditRuleModal.jsx (3)
350-350: 'payee_name' field successfully added to actionFieldsThe inclusion of
'payee_name'inactionFieldsexpands the action options available to users, allowing them to modify the payee name through templating as intended.
376-381: Verify the filtering logic for fields including 'payee_name'Please ensure that the filtering logic in the
fieldsvariable correctly includes the'payee_name'field when theactionTemplatingfeature flag is enabled or when the currentfieldis'payee_name'. This verification is crucial to confirm that users have access to thepayee_namefield as expected when templating is active.
394-394: FieldSelect now utilizes dynamic fieldsThe
FieldSelectcomponent has been updated to use the dynamicfieldsvariable, enhancing flexibility by displaying the appropriate fields based on the feature flag and current context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/server/accounts/transaction-rules.test.ts (3)
Line range hint
129-134: LGTM! Consider using async/await syntax consistently.The addition of
awaittorunRulesis correct and necessary if the function has been changed to return a Promise. This ensures that the test waits for the rules to be applied before making assertions.For consistency, consider updating the entire test case to use async/await syntax:
- test('insert a rule into the database', async () => { + test('insert a rule into the database', async () => { await loadRules(); await insertRule({ stage: 'pre', conditionsOp: 'and', conditions: [], actions: [], }); expect((await db.all('SELECT * FROM rules')).length).toBe(1); // Make sure it was projected expect(getRules().length).toBe(1); // ... (other test code) - const transaction = await runRules({ + const transaction = await runRules({ date: '2019-05-10', notes: '', category: null, }); expect(transaction.date).toBe('2019-05-10'); expect(transaction.notes).toBe('Sarah'); expect(transaction.category).toBe('food'); });
Line range hint
204-209: LGTM! Consistent use of await for runRules in delete rule test.The addition of
awaitto bothrunRulescalls in this test case is correct and consistent with the previous changes. It ensures that the test waits for the rules to be applied before and after deleting a rule, allowing for accurate assertions on the transaction properties.For consistency with the rest of the file, consider updating the entire test case to use
constinstead ofletfor thetransactionvariable:- let transaction = await runRules({ + const transaction = await runRules({ payee: 'Kroger', notes: '', category: null, }); expect(transaction.payee).toBe('Kroger'); expect(transaction.category).toBe('food'); await deleteRule(id); expect(getRules().length).toBe(0); - transaction = await runRules({ + const updatedTransaction = await runRules({ payee: 'Kroger', notes: '', category: null, }); - expect(transaction.payee).toBe('Kroger'); - expect(transaction.category).toBe(null); + expect(updatedTransaction.payee).toBe('Kroger'); + expect(updatedTransaction.category).toBe(null);Also applies to: 214-219
Line range hint
1-1024: Overall, excellent updates to handle asynchronous operations.The changes made to this test file are consistent and improve the reliability of the tests by properly handling the asynchronous nature of the
runRulesfunction. All instances ofrunRulesare now correctly awaited, ensuring that the tests accurately reflect the behavior of the transaction rules system.A few minor suggestions for improvement have been made in previous comments, but these are not critical and are mainly for consistency and readability.
Consider adding a test case that specifically checks the asynchronous behavior of
runRules, such as testing concurrent rule applications or checking the order of rule execution in an asynchronous context. This would further validate the robustness of the transaction rules system in real-world scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/loot-core/src/server/accounts/transaction-rules.test.ts (9 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/loot-core/src/server/accounts/transaction-rules.test.ts (3)
Line range hint
152-157: LGTM! Correct usage of await for asynchronous operation.The addition of
awaittorunRulesis correct and consistent with the asynchronous nature of the function. This ensures that the test waits for the rules to be applied before making assertions on the transaction properties.
Line range hint
168-173: LGTM! Consistent use of await for runRules.The addition of
awaitto thisrunRulescall is correct and consistent with the previous changes in this test case. It ensures that the test waits for the updated rules to be applied before making assertions on the transaction properties.
Line range hint
182-187: LGTM! Consistent use of await for runRules after condition change.The addition of
awaitto thisrunRulescall is correct and consistent with the previous changes in this test case. It ensures that the test waits for the rules with updated conditions to be applied before making assertions on the transaction properties.
UnderKoen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/accounts/transaction-rules.ts (1)
791-810: LGTM! Transaction finalization function added with a minor optimization opportunity.The
finalizeTransactionForRulesfunction ensures proper handling of payee information, creating or retrieving payee IDs as needed. This maintains data integrity and consistency in the database.A minor optimization could be made:
Consider replacing the
deleteoperator with an assignment toundefined:- delete trans.payee_name; + trans.payee_name = undefined;This change addresses the static analysis hint and may provide a slight performance benefit, although the impact is likely minimal in this context.
🧰 Tools
🪛 Biome
[error] 806-806: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/loot-core/src/server/accounts/transaction-rules.ts (5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/accounts/transaction-rules.ts (2)
Learnt from: UnderKoen PR: actualbudget/actual#3619 File: packages/loot-core/src/server/accounts/transaction-rules.ts:551-0 Timestamp: 2024-10-09T20:17:46.493Z Learning: When finalizing transactions that involve inserting or retrieving payees, avoid using `Promise.all` as it may result in duplicate payees due to concurrent operations. Sequential processing ensures payees are correctly handled without duplication.Learnt from: UnderKoen PR: actualbudget/actual#3619 File: packages/loot-core/src/server/accounts/transaction-rules.ts:795-801 Timestamp: 2024-10-09T20:30:39.127Z Learning: In the `finalizeTransactionForRules` function within `packages/loot-core/src/server/accounts/transaction-rules.ts`, potential race conditions when inserting payees are handled in the method that calls this function, so additional safeguards within `finalizeTransactionForRules` are unnecessary.
🪛 Biome
packages/loot-core/src/server/accounts/transaction-rules.ts
[error] 806-806: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
packages/loot-core/src/server/accounts/transaction-rules.ts (4)
Line range hint
277-291: LGTM! New transaction preparation workflow implemented.The
runRulesfunction has been updated to include a new workflow for handling transactions:
- Transactions are prepared using
prepareTransactionForRules.- Rules are applied as before.
- Transactions are finalized using
finalizeTransactionForRules.This change ensures consistent handling of transaction data, particularly for payee information, throughout the rule application process.
543-556: LGTM! Efficient transaction preparation with safe finalization.The
applyActionsfunction has been updated to usePromise.allfor preparing transactions, which is an efficient approach. The finalization of transactions is done sequentially in a for loop, which aligns with the previous learning about avoiding duplicate payees when usingPromise.allfor operations involving payee insertions or retrievals.This implementation strikes a good balance between efficiency and data integrity.
773-775: LGTM! New type for rule processing introduced.The
TransactionForRulestype extendsTransactionEntitywith an optionalpayee_nameproperty. This addition allows for better type safety and clarity when dealing with transactions during the rule processing phase, where both payee ID and name might be needed.
777-789: LGTM! Transaction preparation function added.The
prepareTransactionForRulesfunction enriches the transaction data by fetching the payee name when a payee ID is present. This allows for more flexible rule conditions based on payee names and provides a clear separation of concerns in the rule processing workflow.
|
what the difference between |
|
|
In that case its probably not useful to be able to set just payee, and only let people set payee_name |
|
Expect that |
Adds the field
payee_nameas possiblity for templating.Makes it possible to change
payee_nameto whatever template you want.Currently there is another field in the selector for setting the name but maybe this should not be visible and be auto selected when enabling templating on
payee#3606