Skip to content

prevent hidden or placeholder accounts in CSV transaction import #2183

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.

Sign up for GitHub

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

Closed
alonsoac wants to merge 2 commits into from
+25 −7
Closed

prevent hidden or placeholder accounts in CSV transaction import #2183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ae61fb
csv import: ignore hidden/placeholder accounts in auto-mapping
alonsoac Feb 24, 2026
d7f279c
Merge pull request #1 from alonsoac/codex/find-bug-in-csv-transaction…
alonsoac Feb 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions gnucash/import-export/csv-imp/assistant-csv-trans-import.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ namespace bl = boost::locale;
/* This static indicates the debugging module that this .o belongs to. */
static QofLogModule log_module = GNC_MOD_ASSISTANT;

static bool
csv_tximp_account_is_eligible (Account *account)
{
return account && !xaccAccountGetPlaceholder (account) && !xaccAccountIsHidden (account);
}

Comment on lines +86 to +91
Copy link
Copy Markdown
Member

@jralls jralls Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better done as a lambda in csv_tximp_acct_match_load_mappings since that's the only place it's used.

Copy link
Copy Markdown
Author

@alonsoac alonsoac Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@jralls jralls Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

@alonsoac alonsoac Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@gjanssens gjanssens Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

enum GncImportColumn {
MAPPING_STRING,
MAPPING_FULLPATH,
Expand Down Expand Up @@ -1804,9 +1810,11 @@ csv_tximp_acct_match_load_mappings (GtkTreeModel *mappings_store)
// It may already be set in the tree model. If not we try to match the map_string with
// - an entry in our saved account maps
// - a full name of any of our existing accounts
if (account ||
(account = gnc_account_imap_find_any (gnc_get_current_book(), IMAP_CAT_CSV, map_string)) ||
(account = gnc_account_lookup_by_full_name (gnc_get_current_root_account(), map_string)))
if ((account && csv_tximp_account_is_eligible (account)) ||
((account = gnc_account_imap_find_any (gnc_get_current_book(), IMAP_CAT_CSV, map_string)) &&
csv_tximp_account_is_eligible (account)) ||
((account = gnc_account_lookup_by_full_name (gnc_get_current_root_account(), map_string)) &&
csv_tximp_account_is_eligible (account)))
{
auto fullpath = gnc_account_get_full_name (account);
gtk_list_store_set (GTK_LIST_STORE(mappings_store), &iter, MAPPING_FULLPATH, fullpath, -1);
Expand Down
18 changes: 14 additions & 4 deletions gnucash/import-export/csv-imp/gnc-imp-props-tx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ namespace bl = boost::locale;

G_GNUC_UNUSED static QofLogModule log_module = GNC_MOD_IMPORT;

static bool
gnc_csv_account_is_eligible (Account *account)
{
return account && !xaccAccountGetPlaceholder (account) && !xaccAccountIsHidden (account);
}

/* This map contains a set of strings representing the different column types. */
std::map<GncTransPropType, const char*> gnc_csv_col_type_strs = {
{ GncTransPropType::NONE, N_("None") },
Expand Down Expand Up @@ -446,8 +452,10 @@ void GncPreSplit::set (GncTransPropType prop_type, const std::string& value)
m_account.reset();
if (value.empty())
throw std::invalid_argument (_("Account value can't be empty."));
if ((acct = gnc_account_imap_find_any (gnc_get_current_book(), IMAP_CAT_CSV, value.c_str())) ||
(acct = gnc_account_lookup_by_full_name (gnc_get_current_root_account(), value.c_str())))
if (((acct = gnc_account_imap_find_any (gnc_get_current_book(), IMAP_CAT_CSV, value.c_str())) &&
gnc_csv_account_is_eligible (acct)) ||
((acct = gnc_account_lookup_by_full_name (gnc_get_current_root_account(), value.c_str())) &&
gnc_csv_account_is_eligible (acct)))
m_account = acct;
else
throw std::invalid_argument (_("Account value can't be mapped back to an account."));
Copy link
Copy Markdown
Member

@jralls jralls Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception string should be updated, maybe "Account value can't be mapped to a non-hidden, non-placeholder account.".

Expand All @@ -458,8 +466,10 @@ void GncPreSplit::set (GncTransPropType prop_type, const std::string& value)
if (value.empty())
throw std::invalid_argument (_("Transfer account value can't be empty."));

if ((acct = gnc_account_imap_find_any (gnc_get_current_book(), IMAP_CAT_CSV,value.c_str())) ||
(acct = gnc_account_lookup_by_full_name (gnc_get_current_root_account(), value.c_str())))
if (((acct = gnc_account_imap_find_any (gnc_get_current_book(), IMAP_CAT_CSV,value.c_str())) &&
gnc_csv_account_is_eligible (acct)) ||
((acct = gnc_account_lookup_by_full_name (gnc_get_current_root_account(), value.c_str())) &&
gnc_csv_account_is_eligible (acct)))
m_taccount = acct;
else
throw std::invalid_argument (_("Transfer account value can't be mapped back to an account."));
Expand Down
Loading