✨ Bank sync avoid reimporting deleted transactions #4644
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 |
|
This is an interesting proposal, but I can see it being somewhat controversial as it will impact existing workflows folks already have. Perhaps we allow users to configure this vis the banksync page? Similar to how "should pending transactions be imported?". |
alecbakholdin
changed the title
alecbakholdin
changed the title
|
There is also a PR that allows rules to delete transactions |
alecbakholdin
changed the title
|
This would also make it really difficult to fix unintentional deletions without some way to manually, undelete |
WalkthroughThis pull request introduces enhancements to the transaction reconciliation process by adding two new test cases to the Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 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 (
|
This probably functions as workaround in each specific case but not in the general case, and can potentially catch more transactions that I didn't intend to be deleted. @MatissJanis To me this looks like a bug and not how I would expect Actual to behave; if I delete a transaction, it should stay deleted. However, after a hair more thinking I can understand people relying on this behavior so I'll make it configurable. Similar to importing pending transactions, I suppose I would call it "Reimport deleted transactions" and default it to true. If you have an alternate name that you would like me to name it let me know! |
alecbakholdin
changed the title
I would argue that if you manually deleted a transaction, then realized you actually want it, you can just manually enter that transaction. I don't see this as being much of an issue unless you're doing this en masse. Also, with this being an opt-in config option now, no longer really an issue. |
|
Image of config [on by default]: |
This is generally where my concern is. People getting started with bank sync, or trying to make new rules, delete transactions all the time and then expect them to show up again on the import. I don't think this is a bad feature, but it should be opt in. |
Ah, that's something I hadn't considered. Sorry about that! |
alecbakholdin
changed the title
alecbakholdin
changed the title
![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/desktop-client/src/components/banksync/EditSyncAccount.tsx (1)
236-242: Consider updating the checkbox ID.The ID
form_deleted_notesseems inconsistent with the functionality it represents. Consider renaming it to better reflect that it's for reimporting deleted transactions.- id="form_deleted_notes" + id="form_reimport_deleted"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/loot-core/src/server/accounts/__snapshots__/sync.test.ts.snapis excluded by!**/*.snapupcoming-release-notes/4644.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/banksync/EditSyncAccount.tsx(4 hunks)packages/loot-core/src/server/accounts/sync.test.ts(2 hunks)packages/loot-core/src/server/accounts/sync.ts(2 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/accounts/sync.ts
🧰 Additional context used
🧠 Learnings (1)
packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/desktop-client/src/components/banksync/EditSyncAccount.tsx:154-160
Timestamp: 2025-03-12T13:10:05.044Z
Learning: In EditSyncAccount.tsx, mappings state is initialized with defaultMappings (containing both 'payment' and 'deposit' directions) if no saved mappings exist, so direction maps are guaranteed to exist.
🔇 Additional comments (7)
packages/loot-core/src/types/prefs.d.ts (1)
35-35: New preference type looks good.The added preference type for reimporting deleted transactions follows the established naming pattern and is appropriately positioned with other sync-related preferences.
packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (3)
125-127: New preference declaration looks good.This follows the established pattern for synced preferences in this component, setting the default value to
truewhich aligns with the current system behavior.
137-139: State initialization is consistent with other preferences.The state variable is properly initialized and transformed from string to boolean, following the pattern used for other similar preferences.
177-177: Preference saving implementation looks good.The implementation correctly saves the preference value when the user clicks the Save button.
packages/loot-core/src/server/accounts/sync.test.ts (3)
3-3: Added import for SyncedPrefs type.Good addition for type safety in the tests.
127-148: Well-structured test for disabled reimporting.This test case effectively verifies that when the reimport preference is disabled, deleted transactions are not reimported a second time. The implementation includes:
- Proper setup of the test database and preference
- Good use of TypeScript's
satisfiesoperator for type safety- Clear assertions that validate the expected behavior
150-168: Good test coverage for default reimporting behavior.This test effectively verifies the default behavior where deleted transactions are reimported. The structure is consistent with the first test but verifies the opposite behavior.
The tests together provide complete coverage of the feature's behavior under different preference settings.
| checked={reimportDeleted} | ||
| onChange={() => setReimportDeleted(!reimportDeleted)} | ||
| > | ||
| <Trans>Reimport deleted transactions</Trans> |
There was a problem hiding this comment.
💬 suggestion: would you mind also adding a <Tooltip> here explaining what this option does?
Something along the lines of..
By default imported transactions that you delete will be re-imported with the next bank sync operation. To disable this behaviour - untick this box.
alecbakholdin
force-pushed
the
bank-sync-reimporting-deleted-transactions
branch
from
dfce669 to
e78e476
Compare
* matchTransactions imported_id query checks for deleted transactions * added release notes * removed stray import * Added configuration option to control reimporting deleted transactions * Updated release notes * Unused import * Typo * Linting errors * Fixed Checkbox id to match what it's for * Added tooltip for the checkbox --------- Co-authored-by: Alec Bakholdin <abakho@icims.com>
When you import a transaction using bank-sync, delete it, then re-import it, the transaction gets reimported instead of getting ignored. The reason for this is the v_transactions view filters out deleted transactions. I've updated the matchTransactions method to use the v_transactions_internal "view" (js-defined, looks like?), which includes deleted transactions.