-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Explicitly ask when reconciling transactions on manual import #2717
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
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 |
3a60724
to
8b0c22d
Compare
Could you add a test budget and file for us to try this with? |
8b0c22d
to
e91bece
Compare
@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. (GitHub does not like OFX files, I appended those with a .txt) |
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 |
@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:
|
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 |
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.
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. |
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. |
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. |
e91bece
to
02f00f0
Compare
@youngcw I updated the checkboxes and icons to hopefully better differentiate the transactions. I also plugged the "Reconcile transactions" checkbox with the transactions checkboxes. |
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. |
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. |
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) |
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. |
02f00f0
to
df2aed1
Compare
@youngcw It should be working now. Sorry for the issue, I made a mess of React state management while cleaning up my code. |
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?
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? |
Thanks for the feedback!
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.
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.
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. |
df2aed1
to
aa36260
Compare
@MatissJanis I updated with 3-states checkboxes, the boxes now have a more coherent behaviour:
I also updated the Reconcile checkbox with another wording, based on a suggestion on #2564 |
I like these changes. It seems much clearer than before |
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.
Ideally we could even make all the 3x options always available (for all transactions). And let the user decide how he wants to proceed. |
aa36260
to
07fe953
Compare
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.
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). |
07fe953
to
94e5c63
Compare
This is much more clearer! Great! I see you already added 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 -- I'll try to do code review shortly. |
Ok, did a first pass review. Here's the things we should change:
|
94e5c63
to
a20bc56
Compare
@MatissJanis I push an update with
I'm not sure I follow you about that.
I'm not sure that having two lists 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 |
a20bc56
to
6867fe8
Compare
Sorry for taking so long to get back to this. The core issue here right now is code duplication. We should not have
TBH I think the button isn't a good idea. Most times clicking it will yield no results. Leading to users getting confused about it. Instead we should automatically detect that the configuration has changed and thus recalculate the list (whilst still keeping the checkbox states unchanged). |
6867fe8
to
e6a924f
Compare
Ok, I merged the code for previewing and running imports in the same function.
I removed the button and instead refreshed the preview when the options fields change. By the way VRT is failing due to an issue with uploading the results on GitHub. Dunno why, I'm not an expert with GitHub actions. |
: transaction.ignored | ||
? 'Already imported transaction. You can skip it, or import it again.' | ||
: transaction.existing | ||
? 'Updated transaction. You can update it, or import it again instead, or skip it.' |
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.
nit. Can this be reworded to "You can update it, import it again, or skip it."
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.
Done, I updated the tooltip.
@MatissJanis Do you have any other thoughts on this or is this ready for a full review? |
e6a924f
to
7b8bcfa
Compare
- Added import preview in transaction import list - Added checkboxes to selectively prevent merging transactions
7b8bcfa
to
5c54e19
Compare
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.
Seems good to me. @MatissJanis Ill merge this in a few days unless there is something else you would like changed.
Explicitly ask when reconciling transactions on manual import
Resolve #2679