prevent hidden or placeholder accounts in CSV transaction import#2183
prevent hidden or placeholder accounts in CSV transaction import#2183alonsoac wants to merge 2 commits intoGnucash:stablefrom
Conversation
…-import CSV import: prevent auto-assignment of hidden and placeholder accounts
jralls
left a comment
jralls
left a comment
There was a problem hiding this comment.
Blocking splits from being imported into placeholder accounts seems reasonable, but not so much for hidden accounts. What's the rationale for the latter and what problem are you trying to solve?
Please get rid of the merge commit. Since the change is a single commit you can just cherry-pick it into stable, though it's generally better practice to make the PR from your feature branch instead.
| static bool | ||
| csv_tximp_account_is_eligible (Account *account) | ||
| { | ||
| return account && !xaccAccountGetPlaceholder (account) && !xaccAccountIsHidden (account); | ||
| } | ||
|
|
There was a problem hiding this comment.
This would be better done as a lambda in csv_tximp_acct_match_load_mappings since that's the only place it's used.
There was a problem hiding this comment.
Hi. I am not experienced in github or changes to GnuCash code. I just wanted to make you aware of this bug.
For hidden accounts I just think it is unhelpful that it would suggest accounts that the user took the time to hide.
I probably cannot be of any further help and hope you can take it from here.
There was a problem hiding this comment.
So far you haven't revealed any bug, and the code you've written impacts what can be imported as well as (or maybe instead of, I haven't tested it yet) what's displayed in an account selector.
So again, what is the problem that you're trying to address?
There was a problem hiding this comment.
The specific problem I had is that when importing transactions from CSV some of the automatically detected accounts were "unavailable". By unavailable I mean that after import, when the account is selected in the register the field is cleared because the account that was selected is not part of the list of selectable accounts. I just assumed that this was because the account is a placeholder or hidden. I then inspected the code and could not find code to prevent those accounts from being suggested, so I though that it was a bug.
There was a problem hiding this comment.
The specific problem I had is that when importing transactions from CSV some of the automatically detected accounts were "unavailable". By unavailable I mean that after import, when the account is selected in the register the field is cleared because the account that was selected is not part of the list of selectable accounts. I just assumed that this was because the account is a placeholder or hidden. I then inspected the code and could not find code to prevent those accounts from being suggested, so I though that it was a bug.
So you have found an issue in the account register code, not in the importer code.
I can reproduce this without importing at all:
- choose any two accounts
- make a transaction between these two accounts
- mark one of the account placeholder and invisible
- go to the other account and in the transaction, click on the hidden/placeholder account
=> The account will disappear as you describe.
It has odd behaviour though. It will not always disappear. For example, if I first click in the account field of an adjacent transaction and then in the account field of the transaction with the hidden account, it will not disappear. If the previously active transaction is not adjacent, it will disappear. I don't see this behaviour for accounts that are marked invisible only (and not placeholder). Go figure.
In any case, more related to this PR, I'm not sure this is a real fix of the issue, more like a band-aid. There is a use case of exporting and importing accounts from one gnucash file to another. If the export had splits in a placeholder account, it may be desirable to import them in the same placeholder account in the target file for consistency. Personally I would limit the interference in the importer to warning the user about the selection of placeholder/invisible accounts. Slightly more nuanced would be to allow manual assignment to a placeholder account, but not automatic assignment to a placeholder account. It would be user friendly if in both cases the user is notified in some way about what is about to happen.
The other part is the visual glitches in the register code. The register should work properly regardless of whether a transfer account is marked placeholder/hidden or not. I'm deliberately a bit vague as I'm not really sure what "working properly" should be defined as without some discussion. It's clear it should just display whatever account was previously selected. But should it be marked in some way, should it remain selectable in the drop-down list because it was selected before (only for the current transaction of course). I don't know for sure.
| gnc_csv_account_is_eligible (acct))) | ||
| m_account = acct; | ||
| else | ||
| throw std::invalid_argument (_("Account value can't be mapped back to an account.")); |
There was a problem hiding this comment.
The exception string should be updated, maybe "Account value can't be mapped to a non-hidden, non-placeholder account.".
3 participants
prevent hidden or placeholder accounts to be suggested in automatic mapping in CSV transaction import