♻️ (bank-sync) unify the sync/reconciliation logic for internal & external sync #2534

MatissJanis · 2024-03-31T12:57:34Z

Unifying the reconciliation and syncing logic to reduce duplication.

Previously we had reconcileTransactions, reconcileExternalTransactions, syncAccount and syncExternalAccount. Now we have only one of each.

The logic there is still not exactly clean. But at least now we have a single place for it instead of copy&paste duplication.

…rnal sync

netlify · 2024-03-31T12:57:53Z

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	`b5fba06`
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/660eff96b867370008fa5bca
😎 Deploy Preview	https://deploy-preview-2534.demo.actualbudget.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

MatissJanis · 2024-03-31T12:58:38Z

packages/loot-core/src/server/accounts/sync.ts

@@ -575,6 +580,7 @@ export async function reconcileExternalTransactions(acctId, transactions) {

      // Update the transaction
      const updates = {
+        date: trans.date,


I think that it's fine to add this to the bank-sync transactions too. It will update the transaction date if it changes after the initial sync. But LMK if you think this is a bad idea. Worst case we can make this conditional.

Also to note: this should be the only functional change. Everything else should be exactly the same as before.

This would revert #1559, and personally I think it's a bad idea since it removes agency from the user to correct the dates. (I for example semi-regularly change the dates after shopping at places that batch-submit transactions multiple days after they occur (sometimes over a week >.>), because to me it matters more when I made the transaction than when my bank actually booked it.)

That's an excellent point!

I've made this operation conditional: 0edcd09

github-actions · 2024-03-31T12:58:44Z

Bundle Stats — desktop-client

Hey 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

Files count	Total bundle size	% Changed
11	4.45 MB → 4.45 MB (-333 B)	-0.01%

Changeset

File	Δ	Size
`home/runner/work/actual/actual/packages/loot-core/src/client/actions/account.ts`	📉 -333 B (-7.60%)	4.28 kB → 3.95 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

Asset	File Size	% Changed
static/js/index.js	2.54 MB → 2.54 MB (-333 B)	-0.01%

Unchanged

Asset	File Size	% Changed
static/js/indexeddb-main-thread-worker-e59fee74.js	13.5 kB	0%
static/js/resize-observer.js	18.37 kB	0%
static/js/ButtonLink.js	379 B	0%
static/js/BackgroundImage.js	122.29 kB	0%
static/js/narrow.js	215.48 kB	0%
static/js/BalanceTooltip.js	7.31 kB	0%
static/js/AppliedFilters.js	20.35 kB	0%
static/js/import.js	105.26 kB	0%
static/js/wide.js	250.13 kB	0%
static/js/ReportRouter.js	1.18 MB	0%

github-actions · 2024-03-31T12:58:45Z

Bundle Stats — loot-core

Hey 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

Files count	Total bundle size	% Changed
1	1.2 MB → 1.2 MB (-3.88 kB)	-0.32%

Changeset

File	Δ	Size
`packages/loot-core/src/server/main.ts`	📉 -1.41 kB (-2.15%)	65.8 kB → 64.39 kB
`packages/loot-core/src/server/accounts/sync.ts`	📉 -9.24 kB (-27.08%)	34.1 kB → 24.87 kB
`packages/loot-core/src/server/accounts/link.ts`	📉 -1.22 kB (-30.46%)	4.02 kB → 2.79 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

Asset	File Size	% Changed
kcab.worker.js	1.2 MB → 1.2 MB (-3.88 kB)	-0.32%

Unchanged

No assets were unchanged

kyrias · 2024-04-02T18:43:38Z

But uh wait, is what you're calling 'internal' sync even used for anything?

I see that it's what was the GoCardless functions that were renamed to External in f5258e6 for some reason, though it's unclear to me why this was done at all, but the functions without GoCardless in the name are from the original partial Plaid integration and are to my knowledge both completely unused, are in no way any more internal than what's now called 'external', and should if anything simply be deleted.

MatissJanis · 2024-04-02T20:44:27Z

But uh wait, is what you're calling 'internal' sync even used for anything?

I see that it's what was the GoCardless functions that were renamed to External in f5258e6 for some reason, though it's unclear to me why this was done at all, but the functions without GoCardless in the name are from the original partial Plaid integration and are to my knowledge both completely unused, are in no way any more internal than what's now called 'external', and should if anything simply be deleted.

The original was used for both plaid, but also for csv/ofx imports. So these file imports are what I would call "internal".

Good shout on Plaid. It's been over a year now without any activity on it, so I think it's finally time to remove it from the codebase (it doesn't work either way, so I don't think we're losing much there..). But that - in a separate PR.

kyrias · 2024-04-02T21:04:37Z

Ah, I'd missed that the CSV import ends up using the same reconciliation logic, makes sense of course.

I still find it rather confusing to use internal/external for these, since there doesn't seem to be anything more internal or external about either. If anything it feels to me like CSV imports would be more external since you need to manually import them, rather than the application itself performing the import, heh. Either way I think this terminology is something that really needs to be documented.

And well, there is certainly no such thing as a syncAccount that is 'internal` in this sense either way since that at the very least was only ever used for the Plaid integration. So since it's currently unused/nonfunctional, rather than unifying them it feels like it might be reasonable to leave the sync functions alone and then just remove the non-'external' one when dropping Plaid support?

MatissJanis · 2024-04-02T21:10:20Z

I'm open to suggestions for a better method/variables names here.

The ultimate goal is to unify this and this duplicated logic (and also syncAccount and syncExternalAccount while I'm at it). If you have other concrete ideas how to reduce this duplication - I'm all ears.

kyrias · 2024-04-02T21:27:57Z

I feel like using Synced instead of External everywhere would be a lot more self-descriptive at least, especially since functionally speaking the thing that makes an account 'External' is the presence of a sync source, and then maybe naming the non-syncing related things File? (You could argue that it might be a bit misleading given that the Plaid support is technically partially there but given that it's non-functional I feel like it's a bit of a moot point.)

For the reconciliation functions I have no real ideas for how to do it better right now, for the sync functions my only concrete suggestion was to not even bother doing it at all since one of them is literally never used, but on the other hand I just reviewed the changes in more detail and since they're pretty small and the work is already done it probably doesn't matter so why bother with more churn.

In all, I think what you currently have looks good, and then we could maybe replace External with Synced when ripping out the Plaid integration if you agree that naming things that would be a good option.

My one question is, does the new reconcileTransactions function's isExternalAccount argument having a default value give us anything worthwhile compared to the higher risk of the function accidentally being misused?

MatissJanis · 2024-04-03T17:13:14Z

Ok, so a few updates:

renamed external to bankSync
stripped out the old syncAccount which was used for Plaid
the above removal had a knock-on effect on tests that needed to be removed and a bunch of other utilities for Plaid

I will follow up with another PR for the full Plaid removal later on. Don't want this PR to balloon even more than it already has.

kyrias · 2024-04-03T17:53:19Z

Looks good to me, though I still wonder about the defaulted function argument. :)

joel-jeremy

LGTM!

♻️ (bank-sync) unify the sync/reconciliation logic for interal & exte…

a9039e8

…rnal sync

trafico-bot bot added the 🔍 Ready for Review label Mar 31, 2024

github-actions bot changed the title ~~♻️ (bank-sync) unify the sync/reconciliation logic for interal & external sync~~ [WIP] ♻️ (bank-sync) unify the sync/reconciliation logic for interal & external sync Mar 31, 2024

trafico-bot bot added the 🚧 WIP label Mar 31, 2024

trafico-bot bot removed the 🔍 Ready for Review label Mar 31, 2024

MatissJanis commented Mar 31, 2024

View reviewed changes

MatissJanis added 2 commits March 31, 2024 14:00

Release notes

54d2ee8

Remove unused TransactionEntity import

967c837

MatissJanis changed the title ~~[WIP] ♻️ (bank-sync) unify the sync/reconciliation logic for interal & external sync~~ ♻️ (bank-sync) unify the sync/reconciliation logic for interal & external sync Mar 31, 2024

trafico-bot bot added 🔍 Ready for Review and removed 🚧 WIP labels Mar 31, 2024

MatissJanis changed the title ~~♻️ (bank-sync) unify the sync/reconciliation logic for interal & external sync~~ ♻️ (bank-sync) unify the sync/reconciliation logic for internal & external sync Mar 31, 2024

MatissJanis added 4 commits April 2, 2024 18:29

Merge branch 'master' into matiss/gocardless-unify

28fbe78

Merge branch 'master' into matiss/gocardless-unify

698ed1f

Feedback: update date only for internal accounts

0edcd09

Patch linter

df35270

MatissJanis added 4 commits April 3, 2024 17:39

Rename external -> bankSync and a bit of cleanup

c8a741a

Remove plaid sync tests

25d278d

Remove more dead code

d0f0b92

Strip even more Plaid code

7095f1c

Merge branch 'master' into matiss/gocardless-unify

b5fba06

MatissJanis requested a review from joel-jeremy April 7, 2024 15:56

youngcw approved these changes Apr 9, 2024

View reviewed changes

trafico-bot bot added ✅ Approved and removed 🔍 Ready for Review labels Apr 9, 2024

joel-jeremy approved these changes Apr 9, 2024

View reviewed changes

MatissJanis merged commit 310cc04 into master Apr 9, 2024
19 checks passed

MatissJanis deleted the matiss/gocardless-unify branch April 9, 2024 06:06

trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Apr 9, 2024

MatissJanis mentioned this pull request Apr 16, 2024

🔥 stripping out plaid integration #2616

Merged

Wizmaster mentioned this pull request Apr 20, 2024

Don't update transaction date when importing manually #2648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ (bank-sync) unify the sync/reconciliation logic for internal & external sync #2534

♻️ (bank-sync) unify the sync/reconciliation logic for internal & external sync #2534

MatissJanis commented Mar 31, 2024

netlify bot commented Mar 31, 2024 •

edited

Loading

MatissJanis Mar 31, 2024

MatissJanis Mar 31, 2024

kyrias Apr 2, 2024

MatissJanis Apr 2, 2024

github-actions bot commented Mar 31, 2024 •

edited

Loading

github-actions bot commented Mar 31, 2024 •

edited

Loading

kyrias commented Apr 2, 2024

MatissJanis commented Apr 2, 2024

kyrias commented Apr 2, 2024 •

edited

Loading

MatissJanis commented Apr 2, 2024 •

edited

Loading

kyrias commented Apr 2, 2024 •

edited

Loading

MatissJanis commented Apr 3, 2024

kyrias commented Apr 3, 2024

joel-jeremy left a comment

♻️ (bank-sync) unify the sync/reconciliation logic for internal & external sync #2534

♻️ (bank-sync) unify the sync/reconciliation logic for internal & external sync #2534

Conversation

MatissJanis commented Mar 31, 2024

netlify bot commented Mar 31, 2024 • edited Loading

✅ Deploy Preview for actualbudget ready!

MatissJanis Mar 31, 2024

Choose a reason for hiding this comment

MatissJanis Mar 31, 2024

Choose a reason for hiding this comment

kyrias Apr 2, 2024

Choose a reason for hiding this comment

MatissJanis Apr 2, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 31, 2024 • edited Loading

Bundle Stats — desktop-client

github-actions bot commented Mar 31, 2024 • edited Loading

Bundle Stats — loot-core

kyrias commented Apr 2, 2024

MatissJanis commented Apr 2, 2024

kyrias commented Apr 2, 2024 • edited Loading

MatissJanis commented Apr 2, 2024 • edited Loading

kyrias commented Apr 2, 2024 • edited Loading

MatissJanis commented Apr 3, 2024

kyrias commented Apr 3, 2024

joel-jeremy left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 31, 2024 •

edited

Loading

github-actions bot commented Mar 31, 2024 •

edited

Loading

github-actions bot commented Mar 31, 2024 •

edited

Loading

kyrias commented Apr 2, 2024 •

edited

Loading

MatissJanis commented Apr 2, 2024 •

edited

Loading

kyrias commented Apr 2, 2024 •

edited

Loading