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.

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

Jump to bottom

New txn warnings #1883

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

New txn warnings #1883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e1a392
[utest-Account.cpp] test bool from xaccAccountGetReconcileLastDate
christopherlam Mar 30, 2024
27318d3
[Account.cpp] cache account->last_recn_date getter
christopherlam Mar 30, 2024
0bde786
[import-main-matcher.cpp] warn if trans_date < recn_date
christopherlam Mar 30, 2024
1eedc78
[gnc-ofx-import.cpp] use <string>
christopherlam Apr 8, 2024
5f604a4
[gnc-ofx-import.cpp] if OFX corrects a transaction, show correction
christopherlam Apr 8, 2024
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
64 changes: 63 additions & 1 deletion gnucash/import-export/import-main-matcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@

#include <memory>
#include <algorithm>
#include <numeric>
#include <vector>
#include <string>

#include "import-main-matcher.h"

Expand All @@ -59,6 +61,7 @@
#include "guid.h"
#include "gnc-session.h"
#include "Query.h"
#include <optional>

#define GNC_PREFS_GROUP "dialogs.import.generic.transaction-list"
#define IMPORT_MAIN_MATCHER_CM_CLASS "transaction-matcher-dialog"
Expand Down Expand Up @@ -116,6 +119,7 @@ enum downloaded_cols
DOWNLOADED_COL_ACTION_UPDATE,
DOWNLOADED_COL_ACTION_INFO,
DOWNLOADED_COL_ACTION_PIXBUF,
DOWNLOADED_COL_ACTION_PIXBUF_TOOLTIP,
DOWNLOADED_COL_DATA,
DOWNLOADED_COL_COLOR,
DOWNLOADED_COL_ENABLE,
Expand Down Expand Up @@ -1258,6 +1262,11 @@ gnc_gen_trans_row_activated_cb (GtkTreeView *treeview,
&first, is_selection, path,
&assigned_account, info);

auto model = gtk_tree_view_get_model (info->view);
GtkTreeIter iter;
gtk_tree_model_get_iter (model, &iter, path);
refresh_model_row (info, model, &iter, get_trans_info (model, &iter));

gtk_tree_selection_select_path (gtk_tree_view_get_selection (treeview), path);

gchar *namestr = gnc_account_get_full_name (assigned_account);
Expand Down Expand Up @@ -1559,7 +1568,7 @@ gnc_gen_trans_init_view (GNCImportMainMatcher *info,
G_TYPE_STRING, G_TYPE_STRING, G_TYPE_INT, //memo stuff
G_TYPE_STRING, G_TYPE_BOOLEAN,
G_TYPE_BOOLEAN, G_TYPE_BOOLEAN, G_TYPE_STRING,
GDK_TYPE_PIXBUF, G_TYPE_POINTER, G_TYPE_STRING,
GDK_TYPE_PIXBUF, G_TYPE_STRING, G_TYPE_POINTER, G_TYPE_STRING,
G_TYPE_BOOLEAN);
gtk_tree_view_set_model (view, GTK_TREE_MODEL(store));
g_object_unref (store);
Expand Down Expand Up @@ -1909,6 +1918,16 @@ get_peer_acct_names (Split *split)
return retval;
}

static inline std::optional<time64>
acct_recndate_warning (time64 trans_date, const Account* acc)
{
time64 recn_date;
if (xaccAccountGetReconcileLastDate (acc, &recn_date) && trans_date < recn_date)
return recn_date;
else
return {};
}

static void
refresh_model_row (GNCImportMainMatcher *gui,
GtkTreeModel *model,
Expand Down Expand Up @@ -1976,6 +1995,7 @@ refresh_model_row (GNCImportMainMatcher *gui,
ro_text = text = NULL;
const gchar *color = NULL;
bool show_pixbuf = true;
std::vector<std::string> warnings;
switch (gnc_import_TransInfo_get_action (info))
{
case GNCImport_ADD:
Expand Down Expand Up @@ -2028,6 +2048,22 @@ refresh_model_row (GNCImportMainMatcher *gui,
imbalance, acct_full_name);

}
if (auto recn_date = acct_recndate_warning (date, dest_acc))
{
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.");
Comment on lines +2053 to +2055
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.

auto trans_date_str = qof_print_date (date);
auto recn_date_str = qof_print_date (*recn_date);
auto acc_full_name = gnc_account_get_full_name (dest_acc);
auto s = g_strdup_printf (_(recn_date_warning), trans_date_str,
acc_full_name, recn_date_str);
warnings.push_back (s);
g_free (s);
g_free (acc_full_name);
g_free (recn_date_str);
g_free (trans_date_str);
}

g_free (acct_full_name);
}
Expand Down Expand Up @@ -2136,6 +2172,28 @@ refresh_model_row (GNCImportMainMatcher *gui,
DOWNLOADED_COL_ACTION_ADD,
gnc_import_TransInfo_get_action (info) == GNCImport_ADD,
-1);

if (gnc_import_TransInfo_get_action (info) == GNCImport_ADD && !warnings.empty())
{
auto icon_theme = gtk_icon_theme_get_default();
auto pixbuf = gtk_icon_theme_load_icon (icon_theme, "dialog-warning", 16,
GTK_ICON_LOOKUP_FORCE_SIZE, nullptr);
// in case theme changes, we copy and release old icon
auto warnings_str = std::accumulate (warnings.begin(), warnings.end(), std::string{},
[](const std::string& a, const std::string& b)
{ return a.empty() ? b : a + "\n" + b; });
gtk_tree_store_set (store, iter,
DOWNLOADED_COL_ACTION_PIXBUF, pixbuf,
DOWNLOADED_COL_ACTION_PIXBUF_TOOLTIP, warnings_str.c_str(),
-1);
g_object_unref (pixbuf);
}
else
gtk_tree_store_set (store, iter,
DOWNLOADED_COL_ACTION_PIXBUF, nullptr,
DOWNLOADED_COL_ACTION_PIXBUF_TOOLTIP, nullptr,
-1);

if (gnc_import_TransInfo_get_action (info) == GNCImport_SKIP)
{
/*If skipping the row, there is no best match's confidence pixmap*/
Expand Down Expand Up @@ -2475,6 +2533,10 @@ query_tooltip_tree_view_cb (GtkWidget *widget, gint x, gint y,
-1);
break;
default:
if (!g_strcmp0 (gtk_tree_view_column_get_title (column), _("Info")))
gtk_tree_model_get (model, &iter,
DOWNLOADED_COL_ACTION_PIXBUF_TOOLTIP, &tooltip_text,
-1);
break;
}

Expand Down
78 changes: 35 additions & 43 deletions gnucash/import-export/ofx/gnc-ofx-import.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
#include "dialog-utils.h"
#include "window-reconcile.h"

#include <string>

#define GNC_PREFS_GROUP "dialogs.import.ofx"
#define GNC_PREF_AUTO_COMMODITY "auto-create-commodity"

Expand Down Expand Up @@ -464,80 +466,70 @@ static void
fill_transaction_notes(Transaction *transaction, OfxTransactionData *data)
{
/* Put everything else in the Notes field */
char *notes = g_strdup_printf("OFX ext. info: ");
std::string notes = "OFX ext. info: ";

if (data->transactiontype_valid)
{
char *tmp = notes;
notes = g_strdup_printf("%s%s%s", tmp, "|Trans type:",
gnc_ofx_ttype_to_string(data->transactiontype));
g_free(tmp);
notes += "|Trans type:";
notes += gnc_ofx_ttype_to_string(data->transactiontype);
}

if (data->invtransactiontype_valid)
{
char *tmp = notes;
notes = g_strdup_printf("%s%s%s", tmp, "|Investment Trans type:",
gnc_ofx_invttype_to_str(data->invtransactiontype));
g_free(tmp);
notes += "|Investment Trans type:";
notes += gnc_ofx_invttype_to_str(data->invtransactiontype);
}
if (data->memo_valid && data->name_valid) /* Copy only if memo wasn't put in Description */
{
char *tmp = notes;
notes = g_strdup_printf("%s%s%s", tmp, "|Memo:", data->memo);
g_free(tmp);
notes += "|Memo:";
notes += data->memo;
}
if (data->date_funds_available_valid)
{
char dest_string[MAX_DATE_LENGTH];
time64 time = data->date_funds_available;
char *tmp = notes;

gnc_time64_to_iso8601_buff (time, dest_string);
notes = g_strdup_printf("%s%s%s", tmp,
"|Date funds available:", dest_string);
g_free(tmp);
gnc_time64_to_iso8601_buff (data->date_funds_available, dest_string);
notes += "|Date funds available:";
notes += dest_string;
}
if (data->server_transaction_id_valid)
{
char *tmp = notes;
notes = g_strdup_printf("%s%s%s", tmp,
"|Server trans ID (conf. number):",
sanitize_string (data->server_transaction_id));
g_free(tmp);
notes += "|Server trans ID (conf. number):";
notes += sanitize_string (data->server_transaction_id);
}
if (data->standard_industrial_code_valid)
{
char *tmp = notes;
notes = g_strdup_printf("%s%s%ld", tmp,
"|Standard Industrial Code:",
data->standard_industrial_code);
g_free(tmp);

notes += "|Standard Industrial Code:";
notes += data->standard_industrial_code;
}
if (data->payee_id_valid)
{
char *tmp = notes;
notes = g_strdup_printf("%s%s%s", tmp, "|Payee ID:",
sanitize_string (data->payee_id));
g_free(tmp);
notes += "|Payee ID:";
notes += sanitize_string (data->payee_id);
}
//PERR("WRITEME: GnuCash ofx_proc_transaction():Add PAYEE and ADDRESS here once supported by libofx! Notes=%s\n", notes);

/* Ideally, gnucash should process the corrected transactions */
if (data->fi_id_corrected_valid)
{
char *tmp = notes;
PERR("WRITEME: GnuCash ofx_proc_transaction(): WARNING: This transaction corrected a previous transaction, but we created a new one instead!\n");
notes = g_strdup_printf("%s%s%s%s", tmp,
"|This corrects transaction #",
sanitize_string (data->fi_id_corrected),
"but GnuCash didn't process the correction!");
g_free(tmp);
notes += "|This corrects transaction #";
notes += sanitize_string (data->fi_id_corrected);
notes += " via a ";
switch (data->fi_id_correction_action)
{
case FiIdCorrectionAction::REPLACE:
notes += "REPLACE";
break;
case FiIdCorrectionAction::DELETE:
notes += "DELETE";
break;
default:
notes += "UKNOWN";
break;
}
notes += " but GnuCash didn't process the correction!";
}
xaccTransSetNotes(transaction, notes);
g_free(notes);

xaccTransSetNotes (transaction, notes.c_str());
}

static void
Expand Down
26 changes: 12 additions & 14 deletions libgnucash/engine/Account.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ gnc_account_init(Account* acc)
priv->starting_cleared_balance = gnc_numeric_zero();
priv->starting_reconciled_balance = gnc_numeric_zero();
priv->balance_dirty = FALSE;
priv->cached_recn_date = INT64_MAX;

priv->higher_balance_limit = gnc_numeric_create (1,0);
priv->higher_balance_cached = false;
Expand Down Expand Up @@ -1416,6 +1417,7 @@ xaccFreeAccount (Account *acc)
priv->noclosing_balance = gnc_numeric_zero();
priv->cleared_balance = gnc_numeric_zero();
priv->reconciled_balance = gnc_numeric_zero();
priv->cached_recn_date = INT64_MAX;

priv->type = ACCT_TYPE_NONE;
gnc_commodity_decrement_usage_count(priv->commodity);
Expand Down Expand Up @@ -4731,23 +4733,18 @@ xaccAccountIsPriced(const Account *acc)
gboolean
xaccAccountGetReconcileLastDate (const Account *acc, time64 *last_date)
{
gint64 date = 0;
GValue v = G_VALUE_INIT;
gboolean retval = FALSE;
g_return_val_if_fail(GNC_IS_ACCOUNT(acc), FALSE);
qof_instance_get_path_kvp (QOF_INSTANCE(acc), &v, {KEY_RECONCILE_INFO, "last-date"});
if (G_VALUE_HOLDS_INT64 (&v))
date = g_value_get_int64 (&v);

g_value_unset (&v);
if (date)
auto priv = GET_PRIVATE(acc);
if (priv->cached_recn_date == INT64_MAX)
{
if (last_date)
*last_date = date;
retval = TRUE;
GValue v = G_VALUE_INIT;
qof_instance_get_path_kvp (QOF_INSTANCE(acc), &v, {KEY_RECONCILE_INFO, "last-date"});
priv->cached_recn_date = G_VALUE_HOLDS_INT64 (&v) ? g_value_get_int64 (&v) : 0;
g_value_unset (&v);
}
g_value_unset (&v);
return retval;
if (priv->cached_recn_date && last_date)
*last_date = priv->cached_recn_date;
return (priv->cached_recn_date != 0);
}

/********************************************************************\
Expand All @@ -4766,6 +4763,7 @@ xaccAccountSetReconcileLastDate (Account *acc, time64 last_date)
mark_account (acc);
xaccAccountCommitEdit (acc);
g_value_unset (&v);
GET_PRIVATE(acc)->cached_recn_date = last_date;
}

/********************************************************************\
Expand Down
1 change: 1 addition & 0 deletions libgnucash/engine/AccountP.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ typedef struct AccountPrivate
gnc_numeric lower_balance_limit;
gboolean lower_balance_cached;
TriState include_sub_account_balances;
time64 cached_recn_date;

gboolean balance_dirty; /* balances in splits incorrect */

Expand Down
10 changes: 9 additions & 1 deletion libgnucash/engine/test/utest-Account.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,16 +1254,24 @@ test_gnc_account_kvp_setters_getters (Fixture *fixture, gconstpointer pData)
g_assert_true (xaccAccountGetIncludeSubAccountBalances (account) == false);

// Reconcile getter/setter
g_assert_false (xaccAccountGetReconcileLastDate (account, &returned_date));
date = date - (60*60*24*7); // -7 days
xaccAccountSetReconcileLastDate (account, date);
xaccAccountGetReconcileLastDate (account, &returned_date);
g_assert_true (xaccAccountGetReconcileLastDate (account, &returned_date));
g_assert_true (date == returned_date);

date = date + (60*60*24*2); // +2 days
xaccAccountSetReconcilePostponeDate (account, date);
xaccAccountGetReconcilePostponeDate (account, &returned_date);
g_assert_true (date == returned_date);

// if last recn_date exists and is set to 0, getter returns false,
// and returned_date is unchanged
returned_date = 123;
xaccAccountSetReconcileLastDate (account, 0);
g_assert_false (xaccAccountGetReconcileLastDate (account, &returned_date));
g_assert_cmpint (returned_date, ==, 123);

g_assert_true (xaccAccountGetReconcilePostponeBalance (account, &returned_post_balance) == false);
xaccAccountSetReconcilePostponeBalance (account, post_balance);
g_assert_true (xaccAccountGetReconcilePostponeBalance (account, &returned_post_balance) == true);
Expand Down
Loading