Skip to content
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

New txn warnings #1883

Open
wants to merge 5 commits into
base: stable
Choose a base branch
from

Conversation

christopherlam
Copy link
Contributor

adds mechanism to show easily verifiable warnings into register. in this example, the fuel and groceries accounts were reconciled 11/03/24 and we tried to set posting date 08/03/24.

image

@jralls
Copy link
Member

jralls commented Mar 11, 2024

I'm pretty sure that gnc_split_register_save fires on edits of existing txns as well as creation of new ones, never mind that gnc_transaction_get_warnings will operate on a transaction any time you call it.

This strikes me as something many users will find more annoying than helpful.

@christopherlam
Copy link
Contributor Author

I'm pretty sure that gnc_split_register_save fires on edits of existing txns as well as creation of new ones, never mind that gnc_transaction_get_warnings will operate on a transaction any time you call it.

This shows how little I know of the register. The primary purpose of this is to prevent import errors. I was caught by an errant $20 matched to an incorrect account. The import matcher would verify the new transactions prior to committing. In any case this is a Proof of Concept.

@christopherlam christopherlam force-pushed the new-txn-warnings branch 3 times, most recently from a33acc3 to 9bf0f5f Compare March 30, 2024 00:59
Copy link
Member

@jralls jralls left a comment

Choose a reason for hiding this comment

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

Please rebase away the reverted commits.

gnucash/import-export/import-main-matcher.cpp Outdated Show resolved Hide resolved
gnucash/import-export/import-main-matcher.cpp Outdated Show resolved Hide resolved
@christopherlam christopherlam force-pushed the new-txn-warnings branch 6 times, most recently from 5a726b5 to c628dd9 Compare March 30, 2024 07:06
@christopherlam
Copy link
Contributor Author

xaccAccountGetReconcileLastDate faster and tested 100% including an unusual behaviour confirmed in test.

@christopherlam christopherlam marked this pull request as ready for review April 2, 2024 13:09
@christopherlam
Copy link
Contributor Author

christopherlam commented Apr 2, 2024

Last two commits to have a warning icon with tooltip, instead of adding into the additional comments. Identifying the GtkPixBuf column was not straightforward; its gtk_tree_view_column_get_sort_column_id returns -1...

Screenshot from 2024-04-04 06-28-19

@christopherlam christopherlam force-pushed the new-txn-warnings branch 2 times, most recently from e2d570f to 4c4ed30 Compare April 3, 2024 16:09
Comment on lines +2048 to +2055
static const char* recn_date_warning = N_("The import date %s is \
earlier than destination account %s whose last reconciled date is %s. Further \
reconciliation of the destination account may be difficult.");
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to say `"The import includes at least one transaction whose posted date %s is before the last reconciliation date %s of account %s for which it has a split. This may make reconciliation of that account difficult."

But that's utter nonsense. GnuCash's reconciliation mechanism handles that situation very nicely. It's a pretty common occurence and raising a warning about it will only be annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd offer an alternative view... it's common to have multiple accounts, eg checking, savings, spouse bank accounts tracked.

If I have reconciled my checking account with the bank statement dated 31-dec-2023 successfully, it means all bank's transactions have been matched with my book account. It would be unusual that I'd want to add another split in my checking account before 31-dec-2023. In my view, any import which automatically or manually matches to the checking account before 31-dec-2023 is a mismatch, and it would be nice to detect this.

Copy link
Member

Choose a reason for hiding this comment

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

In my usage it's quite common for a payment I posted not to clear the bank for a few days, sometimes even weeks and in that case the transaction won't get reconciled until a later statement. It would be pretty annoying for GnuCash to pop a warning dialog every time that happens. It certainly doesn't make reconciling more difficult, the older transactions keep showing up in the reconcile window until they're reconciled, and you don't reconcile them until they show up on the bank statement.

Of course that doesn't work if you only enter transactions by importing them from the bank, but if you do that you're keeping the bank's books not your own and you have no way of noticing when a transaction doesn't clear. There's not really any reconciliation going on either: You're just downloading transactions from the bank and marking them reconciled. Since both the downloaded transactions and the statement come from the same database if not the same computer it would be pretty strange if they didn't agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This warning isn't about handling unreconciled splits in an account already reconciled. It's about popping up a soft warning that we're adding a new split in a reconciled account via import. The warning will only pop up as a yellow triangle instead of the red/yellow/green bars, which is not intrusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my scenario I will exercise much care in manually entering data in my main checking account: it will receive all paychecks, it is regularly used to pay various operating expenses. It is also often reconciled with the bank statement.

I also hold other accounts, eg savings accounts. I'd rely on the bank imports.

This work implements a soft warning only, ensuring the main checking account doesn't receive new transactions before the reconciliation date.

Copy link
Member

Choose a reason for hiding this comment

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

The warning will only pop up as a yellow triangle instead of the red/yellow/green bars, which is not intrusive.

Not exactly, at least according to your screenshot. You add a new column that displays an icon if there's a warning on the transaction. (Oddly the example is warning about an expense account. Equity accounts normally don't represent an external account to reconcile with.) It's not as intrusive as the dialog box in your first pass but it's still intrusive.

This work implements a soft warning only, ensuring the main checking account doesn't receive new transactions before the reconciliation date.

Ensuring nothing. It adds a flag with a misleading tooltip attached when an import adds a transaction to the import account when the matched account has a reconciled date after the transaction's post date.

How might that happen? Either the matcher selected the wrong "other" account or the transaction exists but the matcher failed to match it or the transaction exists in the "other" account but with a different other account than the one you're importing now. Relying on the "other" account's last reconciled date to catch any of those is dicey: It depends on the reconcile periods of the various accounts being the same and on the order that the accounts are reconciled. It's more reliable to review carefully the import in the matcher window before clicking OK. In your case any added transaction with the checking account as a match is a problem regardless of reconciliation date. If you've already done savings account A's import and it turns up as a matched account in an add in savings account B's import that's a problem too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning will only pop up as a yellow triangle instead of the red/yellow/green bars, which is not intrusive.

Not exactly, at least according to your screenshot. You add a new column that displays an icon if there's a warning on the transaction. (Oddly the example is warning about an expense account. Equity accounts normally don't represent an external account to reconcile with.) It's not as intrusive as the dialog box in your first pass but it's still intrusive.

This was a contrived example. There's no additional column. It reuses the existing automatch pixbuf column.

A better example with standard icon:

image

This work implements a soft warning only, ensuring the main checking account doesn't receive new transactions before the reconciliation date.

Ensuring nothing. It adds a flag with a misleading tooltip attached when an import adds a transaction to the import account when the matched account has a reconciled date after the transaction's post date.

As above it won't prevent the user from adding the transaction, but will flag that the account is probably an incorrect auto-match or manual-match.

How might that happen? Either the matcher selected the wrong "other" account or the transaction exists but the matcher failed to match it or the transaction exists in the "other" account but with a different other account than the one you're importing now. Relying on the "other" account's last reconciled date to catch any of those is dicey: It depends on the reconcile periods of the various accounts being the same and on the order that the accounts are reconciled.

It's more reliable to review carefully the import in the matcher window before clicking OK.

Exactly^ it would be nice to flag automatically these imports whereby an algorithm can warn of obvious warnings that the user may miss. See similar in assistant-stock-transaction.cpp -- it flags, but doesn't prevent adding a new transaction dated before the stock account latest split, which could affect capgains calculations.

@christopherlam christopherlam force-pushed the new-txn-warnings branch 3 times, most recently from 96d841e to 263495f Compare April 6, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants