Add ability to provide defaults for and skip updating cleared status in API #4129
Conversation
- Add ability to provide default for cleared field - Add ability to skip updating cleared field
✅ 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 |
WalkthroughThis pull request introduces a new Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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)
packages/loot-core/src/server/main.ts (1)
1259-1259: Consider adding JSDoc documentation for the new parameter.The implementation correctly uses optional chaining to handle the new parameter. To improve maintainability, consider adding JSDoc documentation to describe the purpose and expected values of the
defaultClearedoption.Example documentation:
/** * @param opts.defaultCleared - Optional parameter to specify the default cleared status for imported transactions */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4129.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/api/methods.ts(1 hunks)packages/loot-core/src/server/accounts/sync.ts(3 hunks)packages/loot-core/src/server/main.ts(2 hunks)packages/loot-core/src/types/api-handlers.d.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(2 hunks)
🔇 Additional comments (11)
packages/api/methods.ts (2)
88-90: LGTM! Clean interface design.The
ImportTransactionsOptsinterface is well-defined and follows TypeScript best practices.
92-102: LGTM! Good backward compatibility.The function signature change maintains backward compatibility by providing a default value for
opts. The implementation correctly passes the options to the API endpoint.packages/loot-core/src/types/api-handlers.d.ts (2)
1-1: LGTM! Proper import.The import statement is correctly placed and imports the new interface from the API package.
85-85: LGTM! Consistent type usage.The method signature is properly updated to use the imported
ImportTransactionsOptstype.packages/loot-core/src/types/server-handlers.d.ts (1)
244-244: LGTM! Consistent type propagation.The server handler interface is properly updated to use the imported
ImportTransactionsOptstype.packages/loot-core/src/server/accounts/sync.ts (3)
396-396: LGTM! Good backward compatibility.The new
defaultClearedparameter maintains backward compatibility with its default value oftrue.
440-440: LGTM! Proper preservation of existing values.The update logic correctly preserves the existing cleared status when not explicitly set in the incoming transaction.
472-472: LGTM! Consistent default handling.The new transaction creation logic consistently applies the
defaultClearedvalue when no explicit cleared status is provided.packages/loot-core/src/server/main.ts (3)
1245-1245: LGTM: Optional parameter addition aligns with PR objectives.The addition of the optional
optsparameter to thetransactions-importhandler function provides a clean way to extend the API's functionality without breaking existing clients.
Line range hint
1245-1272: LGTM: Error handling remains robust with the new parameter.The existing error handling structure adequately covers the new parameter:
- TransactionError catching remains appropriate
- Type safety is maintained through TypeScript
- The underlying reconcileTransactions function handles parameter validation
Line range hint
1245-1259: Well-structured implementation of the new feature.The changes effectively implement the ability to control transaction cleared status during imports while maintaining backward compatibility. The implementation is:
- Minimal and focused
- Non-breaking for existing clients
- Type-safe through TypeScript
| @@ -468,7 +469,7 @@ export async function reconcileTransactions( | |||
| ...newTrans, | |||
| id: uuidv4(), | |||
| category: trans.category || null, | |||
| cleared: trans.cleared != null ? trans.cleared : true, | |||
| cleared: trans.cleared != null ? trans.cleared : defaultCleared, | |||
There was a problem hiding this comment.
| cleared: trans.cleared != null ? trans.cleared : defaultCleared, | |
| cleared: trans.cleared ?? defaultCleared, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/loot-core/src/server/accounts/sync.ts (1)
Line range hint
751-751: Align cleared status handling inaddTransactions.For consistency, consider updating the cleared status handling in
addTransactionsto match the new pattern:- cleared: trans.cleared != null ? trans.cleared : true, + cleared: trans.cleared ?? true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loot-core/src/server/accounts/sync.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: Wait for Netlify build to finish
🔇 Additional comments (3)
packages/loot-core/src/server/accounts/sync.ts (3)
396-396: LGTM! Parameter addition maintains backward compatibility.The new
defaultClearedparameter with a default value oftrueis well-named and maintains backward compatibility with existing callers.
440-440: LGTM! Preserves existing cleared status.Using the nullish coalescing operator to preserve the existing cleared status when not explicitly set is the correct approach.
Line range hint
472-478: Verify cleared status propagation in split transactions.The cleared status is correctly set using the
defaultClearedparameter. However, we should verify that this status is properly propagated to child transactions in themakeSplitTransactionfunction.Let's verify the split transaction handling:
✅ Verification successful
Cleared status is properly propagated to split transactions
The implementation correctly propagates the cleared status from parent to child transactions through the spread operator in the
makeChildfunction. ThedefaultClearedvalue set on the parent transaction is automatically inherited by all child transactions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if makeSplitTransaction preserves cleared status for child transactions ast-grep --pattern 'function makeSplitTransaction($parent, $subtransactions) { $$$ return [$parent, $$$]; }'Length of output: 114
Script:
#!/bin/bash # Try different approaches to find makeSplitTransaction implementation # 1. Use ripgrep to find any occurrence with context rg "makeSplitTransaction" -A 10 -B 2 # 2. Try ast-grep with a more flexible pattern ast-grep --pattern 'makeSplitTransaction = $_' # 3. Look for any function containing "Split" as a fallback rg "function.*Split.*Transaction" -A 10Length of output: 17890
Script:
#!/bin/bash # Search for makeChildTransaction implementation rg "makeChildTransaction" -A 5 -B 2 # Also try with ast-grep to catch any variable declarations ast-grep --pattern 'makeChildTransaction = $_'Length of output: 4212
Script:
#!/bin/bash # Search for makeChild function implementation in shared transactions rg "makeChild = |function makeChild" -A 10 shared/transactions # Also try with ast-grep as fallback ast-grep --pattern 'function makeChild($_) { $$$ }'Length of output: 180
Script:
#!/bin/bash # Search for makeChild implementation with correct path rg "export (function makeChild|const makeChild)" -A 10 packages/loot-core/src/shared/transactions.ts # Also search for any occurrence of makeChild rg "makeChild" -B 2 -A 10 packages/loot-core/src/shared/transactions.tsLength of output: 1772
|
Caution An unexpected error occurred while opening a pull request: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
Motivation
The reason for these changes is explained in NikxDa/actual-moneymoney#20.
Basically, there is currently no way for an API import to leave the cleared status as set by the user, and neither is there a way to import transactions with the status defaulting to false. This addresses both issues, enabling importing and updating transactions without modifying their cleared status (a request I have so far received twice in the importer repo above).