Fix YNAB batch payee updates, add YNAB integration tests#57
Conversation
This commit fixes several issues with YNAB payee/merchant batch updates: 1. Skip no-op renames: When renaming a payee to itself (A→A), skip the API call entirely to avoid creating duplicates. 2. Batch reassign for existing payees: When the target payee already exists, use YNAB's batch transaction update API to reassign all transactions to the existing payee (2 API calls) instead of falling back to individual updates (N+1 API calls). 3. Bulk local cache update: Changed CommitOrchestrator.apply_merchant_edit to update ALL transactions with the old merchant name, not just the specific transaction ID. This matches YNAB's batch behavior and ensures the local cache stays in sync after commits. Performance: - New payee name: 1 API call (unchanged) - Existing payee: 2 API calls (was N+1) - No-op rename: 0 API calls (was 1+) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This adds backend-specific behavior for local cache updates: - Monarch Money: Updates only the specific transaction (by ID) - YNAB: Updates ALL transactions with the old merchant name (bulk) The behavior is controlled by the `bulk_merchant_renames` parameter: - `commit_pending_edits` now returns a set of (old, new) tuples for merchant renames that were batch-updated on the backend - `CommitOrchestrator.apply_edits_to_dataframe` accepts this set and uses bulk updates only for matching renames - When `bulk_merchant_renames` is None or empty, single-transaction updates are used (backwards compatible) New methods: - `apply_merchant_edit(df, transaction_id, new_merchant)` - single txn - `apply_bulk_merchant_edit(df, old_merchant, new_merchant)` - all matching 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When renaming merchants in YNAB, if more transactions exist than selected, prompt the user to choose between renaming all transactions or just the selected ones. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When renaming a merchant in YNAB, prompt the user if the batch update would affect more transactions than they have selected: - "Rename all N" - Use batch payee update (affects all transactions) - "Rename selected N only" - Use individual transaction updates Implementation: - Add get_transaction_count_by_payee() to ynab_client.py - Add get_transaction_count_by_merchant() to backend base class - Create BatchScopeScreen modal for user choice - Add check_batch_scope() to detect scope mismatches - Add skip_batch_for parameter to commit_pending_edits() - Integrate prompt flow into app.py commit workflow 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The feature has been implemented; the plan document is no longer needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| old_merchant_name: str, | ||
| new_merchant_name: str, | ||
| ) -> Dict[str, Any]: | ||
| """ |
There was a problem hiding this comment.
The _batch_reassign_transactions method fetches transactions and performs a batch update without transaction atomicity guarantees. If the batch update partially fails, some transactions may be reassigned while others remain with the old payee, creating inconsistent state. Consider implementing validation to check update success and providing rollback capability or at minimum detailed failure reporting.
Automated security review by Claude 4.5 Sonnet - Human review still required
There was a problem hiding this comment.
In my testing, I had the bulk transaction reassignment fail.
From Claude's interpretation of the logs (enter cyborg voice):
Key Findings
1. The batch operation failed entirely (not partially):
PATCH /v1/budgets/.../transactions HTTP/1.1" 400
detail='date must not be in the future or over 5 years ago (index: 0), date must not be in the future or over 5 years ago (index: 1)'
The YNAB API rejected the entire batch with a 400 error, indicating all-or-nothing behavior - it validated all transactions before applying any. The error even includes indices (0, 1) showing which
transactions in the batch had issues.
2. Fallback to individual updates worked correctly:
- Batch failed → fell back to individual updates (as designed)
- 4 of 6 transactions succeeded individually (HTTP 200)
- 2 failed with the same date error (transactions older than 5 years)
3. The root cause is a YNAB constraint, not our code:
date must not be in the future or over 5 years ago
You have 2 transactions from "The Westchester Mall Park" that are older than 5 years. YNAB won't allow updating them at all.
Regarding the Review Concern
This actually addresses the atomicity concern - the logs show YNAB validates the entire batch before applying changes. The batch operation appears to be atomic (all-or-nothing), which is good. Our fallback
mechanism then correctly handles partial success at the individual transaction level.
The Real Bug
The code should filter out transactions older than 5 years before attempting updates, or at least warn the user that some transactions can't be modified due to YNAB's date restrictions.
Would you like me to add filtering for >5-year-old transactions?
(Exit cyborg voice) - we'll address the >5 year old transactions separately.
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
|
|
||
| assert updated["merchant"][0] == "" | ||
|
|
||
| def test_large_dataset_performance(self): |
There was a problem hiding this comment.
I'm not sure if this change is ok.
Document batch renaming, payee merging, and duplicate payee handling features for YNAB integration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| if not payee_result["payee"]: | ||
| return 0 | ||
|
|
||
| transactions_api = ynab.TransactionsApi(self.api_client) |
There was a problem hiding this comment.
The _batch_reassign_transactions method reassigns all transactions from one payee to another without explicit user confirmation in the method itself. While the caller may handle confirmation, this operation is destructive and affects all matching transactions. Ensure the caller (app.py) properly validates user intent before calling this method, and consider adding a docstring warning about the destructive nature of this operation.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 1 High/Medium Issue FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
|
@SamEdwardes would you be able to help with reviewing or testing? I might try to have Claude write some integration tests against my test account |
Add a framework for true e2e testing of YNAB backend operations against a live test account. Tests verify the batch payee update functionality added in this PR: - Payee renaming with cascade to transactions - Batch transaction reassignment (payee merging) - Duplicate payee detection - Transaction count by payee - Cache invalidation after updates Tests are skipped when YNAB_TEST_API_KEY is not set, so they don't run in regular CI. Run with: export YNAB_TEST_API_KEY="your-token" uv run pytest integration_tests/ -v 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The integration tests were failing when the test budget had no accounts. This commit improves the test setup process: - Auto-create a test checking account if none exists in test budget - Add better error messages for missing accounts - Update README with clearer setup instructions - Add troubleshooting section for common setup issues This ensures integration tests run smoothly without manual account setup.
Added defensive check to prevent unsafe merges when the target payee has duplicates. Previously, the code only checked for duplicates in the source payee, but not the target. Changes: - Check target_payee_result["duplicates_found"] before merging - Return error with method="duplicate_target_payees_found" if found - Add integration test with mocking to verify the check works This prevents transactions from being assigned to the wrong payee when duplicate target payees exist (rare but possible via imports). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
| """ | ||
|
|
||
| import os | ||
| import uuid |
There was a problem hiding this comment.
🚨 Hardcoded credentials from environment without validation (high severity)
The get_test_credentials() function loads YNAB_TEST_API_KEY from environment variables without any validation. If this integration test suite runs in CI/CD with credentials stored in secrets, a compromised secret or misconfigured environment could expose real API keys. Add validation to ensure test credentials are explicitly marked (e.g., require a prefix like 'test_') and never allow production keys.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
I added some integration tests with Claude. I'll merge this once the build is green, thank you! |
| ) | ||
| wrapper = ynab.PostAccountWrapper(account=save_account) | ||
|
|
||
| response = accounts_api.create_account(budget_id=client.budget_id, data=wrapper) |
There was a problem hiding this comment.
The ynab_client fixture automatically creates a checking account (test_checking_account) if none exists, without explicit user confirmation. For integration tests against a live budget, automatically creating accounts could be unexpected. Consider requiring explicit setup or at minimum logging a prominent warning that an account will be created.
Automated security review by Claude 4.5 Sonnet - Human review still required
There was a problem hiding this comment.
No, that's ok, this is the whole point of the integration tests to operate against a test account where it's ok to make destructive changes
| old_merchant_name: Name of old payee (for logging) | ||
| new_merchant_name: Name of target payee (for logging) | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
The _batch_reassign_transactions method reassigns all transactions from one payee to another in a single batch operation. If the payee lookup logic has a bug or returns unexpected results, this could bulk-reassign many transactions incorrectly. The code lacks validation that old_payee_id and target_payee_id are different, potentially causing data corruption if they're the same.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 3 High/Medium Issues FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
wesm
changed the title
|
|
||
| - Each test creates transactions with a unique prefix (e.g., `__test_a1b2c3d4_`) | ||
| - Transactions created during tests are automatically deleted in cleanup | ||
| - Payees cannot be deleted via YNAB API, but test payees are easily identifiable |
There was a problem hiding this comment.
I've emailed YNAB suggesting to add delete functionality to their payees endpoint, fwiw.
|
|
||
| # Refresh account cache to include the newly created account | ||
| client._fetch_and_cache_accounts() | ||
|
|
There was a problem hiding this comment.
🚨 API key logged without redaction in test fixtures (high severity)
The print statement at line 95 logs the account ID directly without sanitization. While this is test code, API keys and identifiers should be redacted in logs to prevent accidental exposure. Use print(f"Created test account: {response.data.account.name[:10]}... (id={response.data.account.id[:8]}...)") to limit exposure in logs.
Automated security review by Claude 4.5 Sonnet - Human review still required
|
|
||
| transactions_api = ynab.TransactionsApi(self.api_client) | ||
| response = transactions_api.get_transactions_by_payee( | ||
| budget_id=self.budget_id, payee_id=payee_result["payee"].id |
There was a problem hiding this comment.
The get_transaction_count_by_payee method calls get_transactions_by_payee without pagination limits. For payees with thousands of transactions, this could cause memory exhaustion or API timeouts. Add pagination or use a dedicated count endpoint if available from YNAB API.
Automated security review by Claude 4.5 Sonnet - Human review still required
🔒 Security Review: 2 High/Medium Issues FoundClaude's automated security review identified potential security concerns. Please review the inline comments. Additionally:
Note: This is an automated review. False positives are possible. Please review each issue carefully and use your judgment. Powered by Claude 4.5 Sonnet |
|
Thank you Wes and Thomas - I will report back if I have any issues! Apologies, I wasn't able to provide any help earlier. |
3 participants
More from the Claude cyborg department, this PR fixes bugs (raised in #56) with YNAB merchant/payee batch updates and adds a user prompt when batch renames would affect more transactions than selected. I felt like introducing a prompt gives some transparency to how YNAB behaves when bulk updating, and gives user control over whether or not they want to perform a bulk operation or edit just the transactions that are queued. Orthogonal to this, I have some $0 YNAB and Amazon transactions (voided checks - what are those?! - and some cancelled Amazon items), so I added a fill_nan to aggregate_by_merchant() so the top category calc works and doesn't raise.
Problems Solved
Changes
Manual Test Plan