Explicitly ask when reconciling transactions on manual import

[Wizmaster]

Explicitly ask when reconciling transactions on manual import

Resolve #2679

• Added import preview in transaction import list
• Added checkboxes to selectively prevent merging transactions

[netlify]

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	a20bc56
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/664ceefdfcbf8e0008c5e8e5
😎 Deploy Preview	https://deploy-preview-2717.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.

[github-actions]

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
9	4.71 MB → 4.73 MB (+11.45 kB)	+0.24%

Changeset

File	Δ	Size
src/icons/v2/DownAndRightArrow.tsx	🆕 +902 B	0 B → 902 B
src/components/modals/ImportTransactions.jsx	📈 +9.78 kB (+25.39%)	38.51 kB → 48.29 kB
src/components/forms.tsx	📈 +428 B (+18.54%)	2.25 kB → 2.67 kB
home/runner/work/actual/actual/packages/loot-core/src/client/actions/account.ts	📈 +380 B (+9.04%)	4.1 kB → 4.47 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
static/js/index.js	3 MB → 3.01 MB (+11.45 kB)	+0.37%

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
static/js/resize-observer.js	18.37 kB	0%
static/js/indexeddb-main-thread-worker-e59fee74.js	13.5 kB	0%
static/js/BackgroundImage.js	122.29 kB	0%
static/js/usePreviewTransactions.js	790 B	0%
static/js/AppliedFilters.js	20.54 kB	0%
static/js/narrow.js	59.97 kB	0%
static/js/wide.js	262.07 kB	0%
static/js/ReportRouter.js	1.23 MB	0%

[github-actions]

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 (+1.56 kB)	+0.13%

Changeset

File	Δ	Size
packages/loot-core/src/server/accounts/sync.ts	📈 +2.94 kB (+13.34%)	22.04 kB → 24.97 kB
packages/loot-core/src/server/main.ts	📈 +640 B (+1.04%)	59.99 kB → 60.62 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
kcab.worker.js	1.2 MB → 1.2 MB (+1.56 kB)	+0.13%

Smaller

No assets were smaller

Unchanged

No assets were unchanged

[youngcw]

Could you add a test budget and file for us to try this with?

[Wizmaster]

@youngcw Here you are.

You can import those on an empty account in the demo budget. First init, then update, both for the OFX and CSV files.

• init.ofx.txt
• update.ofx.txt
• init.csv
• update.csv

(GitHub does not like OFX files, I appended those with a .txt)

[youngcw]

could this be made to still work with the option to reconcile transactions. As is it feels like there is too much overlap in the two features

[Wizmaster]

@youngcw Yes this PR overlaps the "Reconcile transactions" option. It provide a more specific way to disable the deduplication only for some transactions instead of the all-or-nothing approach from the option.

I can remove the option in the PR.

The wording I used on the checkboxes isn't great too, I would welcome remarks on how to reword those. Currently I wrote:

• "This transaction is new, it will be added on import." : For new imported transactions
• "This transaction is already imported, uncheck to add it again." : For already imported transactions where the deduplication process will not modify anything, but potentially could skip creating the new transaction if wrongly matched
• "This transaction will be updated on import, uncheck to add as a new transaction instead." : For already imported transactions where the deduplication where the deduplication process will update the transaction and potentially merge with the wrong one

[youngcw]

If im understanding this all right, I think it would work well to use the checkbox for reconcile to inform the checkboxes in the table. If the user doesn't want any reconciliation the checkboxes could all be unchecked ( I think that is the correct equivalent)

Also, it would be very helpful if you put here what each check box state meant. Im still a little fuzzy on the details there especially where that example file has two duplicate payee C lines

[Wizmaster]

If im understanding this all right, I think it would work well to use the checkbox for reconcile to inform the checkboxes in the table. If the user doesn't want any reconciliation the checkboxes could all be unchecked ( I think that is the correct equivalent)

Yes, I could keep the bottom checkbox in that way. You are right, it being checked would be the same as all the transaction checkboxes checked.

Also, it would be very helpful if you put here what each check box state meant. Im still a little fuzzy on the details there especially where that example file has two duplicate payee C lines

The checkboxes mean that the deduplication logic will be applied on the transaction. Unchecking it will directly add the transaction. I tried to add that explanation in the box tooltip.

For added transactions, I kept a disabled checked checkbox, even if the transaction will be added. I intended that the deduplication logic for added transaction is to add them, and that disabling that had no sense.

About the duplicate payee C lines, it shows the imported transaction in the first line. And the existing one under it in a lighter color. That's why there are no checkbox on the second line, it's not an imported transaction.
I can add an icon instead of an empty space? Don't know which one.

[youngcw]

Hey, I hadn't noticed the hover info. That does help.

I think there should be something there in that line showing that the greyed out stuff is existing and connected to above. I think the confusion was that the purple boxes signified an existing transaction already.

[Wizmaster]

I wondered if I needed different checkbox styles for updated import transactions and existing import ones.

I can try that along with adding some kind of arrow to connect the related lines.

[Wizmaster]

@youngcw I updated the checkboxes and icons to hopefully better differentiate the transactions.

I also plugged the "Reconcile transactions" checkbox with the transactions checkboxes.

[youngcw]

Thats much nicer. For the reconcile checkbox it would be nice if you could uncheck that box and the check boxes in the table get unchecked for you automatically.

[Wizmaster]

For the reconcile checkbox it would be nice if you could uncheck that box and the check boxes in the table get unchecked for you automatically.

I implemented that way, it did not work for you? Unchecking the reconcile box should uncheck all transactions checkboxes, and checking it should check the transactions.

The box itself will become automatically checked (or unchecked) when all the transactions are manually checked or unchecked.

It's working on my side testing on Firefox.

[youngcw]

Im not seeing the reconcile box do anything to the table boxes. Chrome or firefox. If I manually uncheck the purple boxes in the table, I do see the reconcile box go away on its own (it only comes back on if ALL boxes are rechecked, I would think just one would turn that back on)

[Wizmaster]

Yes, I implemented the reconcile box automatic state change only when all transactions boxes are in the same state too. The reasoning being that keeping it unchecked while checking some transactions leave the option to check the box and all transactions. And conversely the box starts checked with all transactions checked, unchecking transactions does not change the box state and keeps the option to uncheck all with it.

I reproduced the issue you encounter with the reconcile box. It's working with OFX import not CSV import.
I'll look into it.

[Wizmaster]

@youngcw It should be working now. Sorry for the issue, I made a mess of React state management while cleaning up my code.

[MatissJanis]

Thanks for working on this! Initially I thought the checkboxes meant "import this transaction / skip this transaction from importing". It took a bit of time to understand what they actually mean. Like: when I uncheck a transaction: intuitively I expected it NOT to be imported at all. I wonder if other users will also run into the same issue.

What if we greyed out or in another way made it more visible that a transaction already exists and thus a new one will not be created? That way a user should be able to quickly scan the table for a) transactions that will get updates and b) transactions that will be created (new).

Also: maybe we can have 3x states for the checkbox?

1. default: run deduplication for this transaction
2. minus (?): import as a new transaction without dedupe/updates
3. unchecked: do NOT import this transaction

Another thing: the global "reconcile transactions" checkbox - I think if it is un-checked - we should just hide all the individual transaction checkboxes instead of trying to juggle the state around. It makes things easier to follow and also code IMO.

What do you think?

[Wizmaster]

Thanks for the feedback!

Thanks for working on this! Initially I thought the checkboxes meant "import this transaction / skip this transaction from importing". It took a bit of time to understand what they actually mean. Like: when I uncheck a transaction: intuitively I expected it NOT to be imported at all. I wonder if other users will also run into the same issue.

I intended the checkbox to be "use the update/merge logic" and "skip the logic, add the transaction", but I understand how you saw it differently. Switching to 3-states checkboxes could be clearer on how to use those, and allow to completely skip importing some transactions.

What if we greyed out or in another way made it more visible that a transaction already exists and thus a new one will not be created? That way a user should be able to quickly scan the table for a) transactions that will get updates and b) transactions that will be created (new).

I tried greying out the existing transactions that add no updates, but I already used greyed out transactions to show the transaction delta. I felt it was confusing.
Maybe with a different checkbox icon then?

Another thing: the global "reconcile transactions" checkbox - I think if it is un-checked - we should just hide all the individual transaction checkboxes instead of trying to juggle the state around. It makes things easier to follow and also code IMO.

Oh, I saw that checkbox more like a "check all" / "uncheck all" kind but having it as a "enable" / "disable" would be better. Maybe with a new wording then.

[Wizmaster]

@MatissJanis I updated with 3-states checkboxes, the boxes now have a more coherent behaviour:

• checked with a check mark: transaction will be merge/update
• checked with a plus mark: transaction will be added
• unchecked: transaction will be ignored

I also updated the Reconcile checkbox with another wording, based on a suggestion on #2564

[youngcw]

I like these changes. It seems much clearer than before

[MatissJanis]

This looks and feels much more natural now! Great job!

Can we change it so all the transactions are "checked" when a file is imported first? And then the user has the option to uncheck them if he wishes NOT to import them. It feels more natural to start with the state of "lets try and import all the things in your file" and then give the user the option to selectively disable some rows.

It's basically what your logic already does, but the lack of checkboxes besides some rows (on initial import) feel daunting. I would expect most people would then go through the list and check them all - which would result in lots of duplicates.

checked with a check mark: transaction will be merge/update

Ideally we could even make all the 3x options always available (for all transactions). And let the user decide how he wants to proceed.

[Wizmaster]

I see what you mean. An unchecked box is just screaming to be checked, and lead the user to create duplicated transactions.

I tried something else with other check glyphs and greying out transactions to convey in a more clear fashion how the import will be performed.
Does it feel better that way?

Ideally we could even make all the 3x options always available (for all transactions). And let the user decide how he wants to proceed.

The 3x options are only useful for imported transactions that are merging with existing ones. The two other cases are newly imported transactions (in this case the third option "Merge/Update" would make little sense), or existing imported transactions without any changes (in this case the third option "Merge/Update" would be a no-op).
I can add the third option as a dummy operation for all cases but I feel it would be confusing.

[MatissJanis]

This is much more clearer! Great!

I see you already added title attribute to the checkboxes with descriptions, but could you move them to Tooltips instead? That way we can get immediate feedback when hovering on them. Or alternatively - a legend somewhere explaining what each state represents.

Tooltip code sample: https://github.com/actualbudget/actual/blob/master/packages/desktop-client/src/components/NotesButton.tsx#L45-L51

(just make sure to use the new common/Tooltip component)

--

I'll try to do code review shortly.

[MatissJanis]

Ok, did a first pass review. Here's the things we should change:

• removal of transactions-preview-import - instead we should use transactions-import handler. We should not have 2x operations doing basically the same thing. To make that work with the new 3x states you would need to:
  • convert the reconciled prop to something like forceCreateTransactions: string[] (i.e. transactions that will be created; will entirely skip the de-dupe logic)
  • when passing transactions prop to the import handler: remove all transactions that have been force-removed via the checkboxes (thus they would be excluded from importing)
  • pass transactions only the transactions that would be de-duped
  • the transactions-import handler should perform both creation & dedupe operations (currently it performs only 1x and returns early)
• the split of reconcileTransactions into two parts is good (i.e. the separation between "preview" and "save" operations) - we can keep that and use it for the "preview" transaction calculation
• shadow variable naming is confusing; can we use something more explicit? For example: isMatchedDedupe, isMatchedTransaction, etc.

[Wizmaster]

@MatissJanis I push an update with Tooltips and the renaming of shadow (I went with the suggestion isMatchedTransaction)

removal of transactions-preview-import - instead we should use transactions-import handler. We should not have 2x operations doing basically the same thing.

I'm not sure I follow you about that. transactions-import commits the added/updated transactions at the end of the handler, whereas transactions-preview-import performs the same logic without committing. I could add a new parameter to transactions-import and call either previewReconcileTransactions or reconcileTransactions. Would that be ok?

To make that work with the new 3x states you would need to:
* convert the reconciled prop to something like forceCreateTransactions: string[] (i.e. transactions that will be created; will entirely skip the de-dupe logic)
* when passing transactions prop to the import handler: remove all transactions that have been force-removed via the checkboxes (thus they would be excluded from importing)
* pass transactions only the transactions that would be de-duped
* the transactions-import handler should perform both creation & dedupe operations (currently it performs only 1x and returns early)

I'm not sure that having two lists transactions (with only de-dup transactions) and forceCreateTransactions (with force added transactions) would be better. I tried to have the preview process and the import process work with the same transactions list to ensure it would behave the same way. I fear that removing transactions from the list could allow other transactions to match differently than during the preview phase.

In fact it could happen if the user force-remove an updated transaction: the existing transaction would not be matched anymore and could then match with another one on importing.

I added a "Refresh import preview" button for CSV import to allow the user to have a new preview after changing CSV parsing options. I could make it available globally, maybe have it activated only when the user changes the checkboxes (or the parsing options) so that the user is incited to update the preview and check if it's ok?

Or I can also see to remove the reconcile parameter from importTransactions() and switch it to use the forceAddTransaction attribute to trigger a transaction import without matching, and keep the current transactions processing?