-
-
Notifications
You must be signed in to change notification settings - Fork 930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(#2562): Prevent transaction deduplication for imported transactions #2618
base: master
Are you sure you want to change the base?
Conversation
✅ 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 No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were 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 |
@@ -492,7 +492,7 @@ export async function reconcileTransactions( | |||
// fields. | |||
fuzzyDataset = await db.all( | |||
`SELECT id, is_parent, date, imported_id, payee, category, notes, reconciled FROM v_transactions | |||
WHERE date >= ? AND date <= ? AND amount = ? AND account = ?`, | |||
WHERE date >= ? AND date <= ? AND amount = ? AND account = ? AND imported_id IS NULL`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 thought: this is an interesting proposal, but I think it would break an existing use-case.
There are folks that do both bank-sync + manual imports (don't ask me why.. I couldn't explain that. It's just my observation from the questions on discord). So for these folks the bank-sync imports would work fine with your patch. However, then the subsequent manual csv imports would break as they would create duplicates.
(also semi-related note - lately I've been considering enabling the "import" button even for accounts that have bank-sync enabled)
There is a new addition to the import modal to skip the reconciliation logic entirely. It's a checkbox you can turn on. Would using that solve your problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be able to shed some light on the reasons:
- not everyone will be interacting with actual every single month & several banks heavily restrict how far syncs go back, so there might be gaps with just sync (me, sometimes :) )
- some people might not trust the syncing (from either Actual or their bank's side) enough and want to manually import their year overview (f.e.)
(the checkbox would work perfect for my use-case, as I rely on it to avoid duplicates)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a new addition to the import modal to skip the reconciliation logic entirely. It's a checkbox you can turn on.
Disabling all reconciliation / dedup is too aggressive of a fix for #2562 .
In cases where people trust the data they're importing, and they trust that the imported IDs will correctly match, they just want to avoid magic heuristics from silently deleting their transactions.
Hi, I took a look at this PR (context: I'm the author of #2562). I understand MatissJanis' point, and I believe the issue could be approached in a different way to avoid breaking existing use cases. Do not completely exclude transactions that have a (On a side note, I believe reconciled transactions should be excluded from deduplication altogether, instead.) I think this would both solve the issue I reported and avoid breaking the other use case. Hopefully it makes sense. |
I think this would work. Can't come up with any counter arguments for this at the moment. |
This sounds good to me. |
@ttlgeek Just wondering if you were going to continue this work as suggested by the comments above. If not I can give it a go. It would be my first contribution so would probably take longer, but got to start somewhere. Let me know what your plans are. |
Looks like for whatever reason he's not coming back to it anytime soon. Possibly he's deployed his forked branch to his own instance and is happy with its' behaviour.
This PR / branch is probably still good basis for your work btw - Take the work here and then apply @MatissJanis + @jacopo-j 's comments (Should mainly be a change to the SQL query that Matiss commented on) and you shouldn't have to do too much to get approved. |
In fact, I may also have a go at this over my lunch break now |
@jacopo-j re-reading your comment
I think @ttlgeek 's PR already does what you're suggesting, it's just not obvious from the default diff. actual/packages/loot-core/src/server/accounts/sync.ts Lines 380 to 402 in e5933ad
To rephrase your specification:
This behaviour is consistent with @ttlgeek 's PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to poke holes in the logic, but couldn't find any issues. Thanks for correcting me!
I'm happy to merge this, but I'd like another maintainer to review it since it's a risky change to the reconciliation logic.
This morning I thought about it and had a feeling I may be overlooking something. I think possibly there's an issue with this being the skip case:
Because that includes I think the change is small, however, we can just add another condition to ttlgeek's original SQL modification. In terms of style, I think we should have linebreaks that delineate the logic more clearly in the SQL statements, and comments, `SELECT id, is_parent, date, imported_id, payee, category, notes, reconciled FROM v_transactions
WHERE date >= ? AND date <= ? AND amount = ? AND account = ?
-- Only perform it if either ID is null
AND ( imported_id IS NULL OR ? IS NULL)`, // tx_in.imported_id should be last parameter |
Would it be clearer if the I'm not sure what the conventions around dynamic SQL predicates are in this project though. |
Different branching logic overall might be preferable and more readable, however my assumption is that dynamic SQL should be avoided. It's true that this statement slightly violates DRY with the code branch that checks that AND ( imported_id IS NULL OR ? IS NULL) |
Hi @MatissJanis @pmoon00 I've taken a stab at this in my own PR #2770 which bases off of @ttlgeek 's branch (though without all the master -> feature merge commits because I think those are ugly). The only difference is the logic of the SQL statement, which now allows the dedup to occur if an existing transaction OR the input transaction are null. |
Hello,
After looking into the issue, it seems that the transaction deduplication is due to the two transactions having the same amount and account and the second transaction happened within 7 days of the first one.
After reading the import transaction code, the fuzzy matching matching uses the 7 days method to try to match similar transactions and merge them.
Proposed solution: prevent the fuzzy matching from matching imported transactions (where import_id is not null) to prevent this behavior.
@MatissJanis what do you think ?