Only pull single account data from SimpleFIN if syncing one account #483
Conversation
actual-github-bot
bot
changed the title
psybers
changed the title
WalkthroughThe changes in the pull request primarily focus on the In the Error handling has been improved across both endpoints, with specific error messages logged for mismatched array lengths in 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 (
|
![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 ignored due to path filters (1)
upcoming-release-notes/483.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- src/app-simplefin/app-simplefin.js (5 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/app-simplefin/app-simplefin.js (1)
98-102: Usage ofgetTransactionsupdated correctly.The function
getTransactionsis called with the updated parameters, correctly passing theaccountIdas an array. The changes integrate well with the new function signature.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/app-simplefin/app-simplefin.js (3)
304-309: Consider adding JSDoc documentationThe function signature has changed significantly. Adding JSDoc would help document the new parameter and its usage.
+/** + * Fetches transactions for specified accounts within a date range + * @param {string} accessKey - The SimpleFIN access key + * @param {string[]} accounts - Array of account IDs to fetch + * @param {Date} [startDate] - Start date for transaction range + * @param {Date} [endDate] - End date for transaction range + * @returns {Promise<Object>} Transaction data for specified accounts + */ async function getTransactions(accessKey, accounts, startDate, endDate) {
Line range hint
320-345: Add input validation and JSDoc documentationThe function would benefit from proper documentation and input validation.
+/** + * Fetches account data from SimpleFIN API + * @param {string} accessKey - The SimpleFIN access key + * @param {string[]|null} accounts - Optional array of account IDs to fetch + * @param {Date} [startDate] - Start date for transaction range + * @param {Date} [endDate] - End date for transaction range + * @returns {Promise<Object>} Account and transaction data + * @throws {Error} When access key is invalid or API request fails + */ async function getAccounts(accessKey, accounts, startDate, endDate) { + if (!accessKey) { + throw new Error('Access key is required'); + } + if (accounts && !Array.isArray(accounts)) { + throw new Error('accounts parameter must be an array or null'); + }
Line range hint
304-345: Implementation aligns well with PR objectivesThe changes effectively implement the SimpleFIN API's new account filtering capability while maintaining backward compatibility. The implementation:
- Properly handles both single and multiple account scenarios
- Maintains existing error handling and date range functionality
- Uses URL encoding for safety
- Integrates seamlessly with the existing architecture
Consider adding integration tests to verify the behavior with the SimpleFIN API.
Would you like help creating integration tests for these changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/app-simplefin/app-simplefin.js (5 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/app-simplefin/app-simplefin.js (2)
55-55: LGTM: Appropriate use of null accounts parameterThe null parameter correctly maintains the existing behavior of fetching all accounts while supporting the new filtering capability.
98-102: Verify error handling for empty accountIdThe code assumes accountId is always provided. Consider adding validation to handle cases where accountId is null, undefined, or an empty array.
|
I have tested this PR and it works (with my data at least). |
|
Sorry if I merged these in the wrong order, looks like there's a conflict now |
|
@matt-fidd No I knew they would conflict, either way you merged them. Conflict is resolved now. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/app-simplefin/app-simplefin.js (1)
51-51: Consider using explicit parameter names for clarity.The getAccounts call with multiple null parameters makes the code harder to understand. Consider using object destructuring for named parameters.
-const accounts = await getAccounts(accessKey, null, null, null, true); +const accounts = await getAccounts(accessKey, { + accounts: null, + startDate: null, + endDate: null, + noTransactions: true +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/app-simplefin/app-simplefin.js (5 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/app-simplefin/app-simplefin.js (1)
345-349: Consider URL length limitations when adding multiple account parameters.While the code correctly handles account parameters, adding too many accounts could potentially exceed URL length limits. Consider:
- Adding a reasonable limit to the number of accounts
- Using POST with a request body for large numbers of accounts if the API supports it
| async function getTransactions(accessKey, accounts, startDate, endDate) { | ||
| const now = new Date(); | ||
| startDate = startDate || new Date(now.getFullYear(), now.getMonth(), 1); | ||
| endDate = endDate || new Date(now.getFullYear(), now.getMonth() + 1, 1); | ||
| console.log(`${getDate(startDate)} - ${getDate(endDate)}`); | ||
| return await getAccounts(accessKey, startDate, endDate); | ||
| return await getAccounts(accessKey, accounts, startDate, endDate); |
There was a problem hiding this comment.
Add input validation for accounts parameter.
The function should validate that the accounts parameter is either null or an array of non-empty strings to prevent potential API errors.
async function getTransactions(accessKey, accounts, startDate, endDate) {
+ if (accounts !== null && (!Array.isArray(accounts) || accounts.some(id => !id || typeof id !== 'string'))) {
+ throw new Error('accounts parameter must be null or an array of non-empty strings');
+ }
const now = new Date();
startDate = startDate || new Date(now.getFullYear(), now.getMonth(), 1);
endDate = endDate || new Date(now.getFullYear(), now.getMonth() + 1, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function getTransactions(accessKey, accounts, startDate, endDate) { | |
| const now = new Date(); | |
| startDate = startDate || new Date(now.getFullYear(), now.getMonth(), 1); | |
| endDate = endDate || new Date(now.getFullYear(), now.getMonth() + 1, 1); | |
| console.log(`${getDate(startDate)} - ${getDate(endDate)}`); | |
| return await getAccounts(accessKey, startDate, endDate); | |
| return await getAccounts(accessKey, accounts, startDate, endDate); | |
| async function getTransactions(accessKey, accounts, startDate, endDate) { | |
| if (accounts !== null && (!Array.isArray(accounts) || accounts.some(id => !id || typeof id !== 'string'))) { | |
| throw new Error('accounts parameter must be null or an array of non-empty strings'); | |
| } | |
| const now = new Date(); | |
| startDate = startDate || new Date(now.getFullYear(), now.getMonth(), 1); | |
| endDate = endDate || new Date(now.getFullYear(), now.getMonth() + 1, 1); | |
| console.log(`${getDate(startDate)} - ${getDate(endDate)}`); | |
| return await getAccounts(accessKey, accounts, startDate, endDate); |
This utilizes the new support for
account=in the SimpleFIN API so that when requesting transactions you can specify which account(s). When Actual syncs a single account, this uses that parameter to limit the data sent back.See: simplefin/simplefin.github.com#26 for details on the new API being used.