Skip to content

Commit

Permalink
Bug 796527 - invalid currency on scheduled transactions
Browse files Browse the repository at this point in the history 
1. Don't even check for price/exchange rate on template transactions,
there's no point.

2. Extract function get_transaction_currency:
a. Check all split commodities are valid, abort transaction creation if
not.
b. If the template transaction's currency isn't used by any of the
splits set the new transaction's currency to the first-found currency if
there is one, otherwise to the first-found commodity.

3. Fix a minor typo in a comment.
  • Loading branch information
@jralls
jralls committed Jun 11, 2018
1 parent 5f53e29 commit 9e6760f
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 39 deletions.
7 changes: 7 additions & 0 deletions gnucash/register/ledger-core/split-register-control.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1322,6 +1322,13 @@ gnc_split_register_handle_exchange (SplitRegister *reg, gboolean force_dialog)

ENTER("reg=%p, force_dialog=%s", reg, force_dialog ? "TRUE" : "FALSE" );

/* No point in setting a rate on a template transaction. */
if (reg->is_template)
{
LEAVE("Template transaction, rate makes no sense.");
return FALSE;
}

/* Make sure we NEED this for this type of register */
if (!gnc_split_reg_has_rate_cell (reg->type))
{
Expand Down
121 changes: 82 additions & 39 deletions libgnucash/app-utils/gnc-sx-instance-model.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1140,20 +1140,86 @@ split_apply_exchange_rate (Split *split, GHashTable *bindings,
xaccSplitSetAmount(split, amt); /* marks split dirty */

}
/* If the template_txn was created from the SX Editor then it has the default
* currency even if none of its splits do; if the template_txn was created from
* a non-currency register then it will be requesting backwards prices. Check
* that the template_txn currency is in at least one split; if it's not a
* currency and one of the splits is, use that currency. If there are no
* currencies at all assume that the user knew what they were doing and return
* the template_transaction's commodity.
*
* Since we're going through the split commodities anyway, check that they all
* have useable values. If we find an error return NULL as a signal to
* create_each_transaction_helper to bail out.
*/

static gnc_commodity*
get_transaction_currency(SxTxnCreationData *creation_data,
SchedXaction *sx, Transaction *template_txn)
{
gnc_commodity *first_currency = NULL, *first_cmdty = NULL;
gboolean err_flag = FALSE, txn_cmdty_in_splits = FALSE;
gnc_commodity *txn_cmdty = xaccTransGetCurrency (template_txn);
GList* txn_splits = xaccTransGetSplitList (template_txn);

if (txn_cmdty)
g_debug("Template txn currency is %s.",
gnc_commodity_get_mnemonic (txn_cmdty));
else
g_debug("No template txn currency.");

for (;txn_splits; txn_splits = txn_splits->next)
{
Split* t_split = (Split*)txn_splits->data;
Account* split_account = NULL;
gnc_commodity *split_cmdty = NULL;
if (!_get_template_split_account(sx, t_split, &split_account,
creation_data->creation_errors))
{
err_flag = TRUE;
break;
}
split_cmdty = xaccAccountGetCommodity (split_account);
if (!txn_cmdty)
txn_cmdty = split_cmdty;
if (!first_cmdty)
first_cmdty = split_cmdty;
if (gnc_commodity_equal (split_cmdty, txn_cmdty))
txn_cmdty_in_splits = TRUE;
if (!first_currency && gnc_commodity_is_currency (split_cmdty))
first_currency = split_cmdty;
}
if (err_flag)
{
g_critical("Error in SX transaction [%s], split missing account: "
"Creation aborted.", xaccSchedXactionGetName(sx));
return NULL;
}
if (first_currency &&
(!txn_cmdty_in_splits || !gnc_commodity_is_currency (txn_cmdty)))
return first_currency;
if (!txn_cmdty_in_splits)
return first_cmdty;
return txn_cmdty;
}

static gboolean
create_each_transaction_helper(Transaction *template_txn, void *user_data)
{
Transaction *new_txn;
GList *txn_splits, *template_splits;
GList *txn_splits, *template_splits, *node;
Split *copying_split;
gnc_commodity *first_cmdty = NULL;
gboolean err_flag = FALSE;
SxTxnCreationData *creation_data = (SxTxnCreationData*)user_data;
SchedXaction *sx = creation_data->instance->parent->sx;
gnc_commodity *txn_cmdty = get_transaction_currency (creation_data,
sx, template_txn);

/* No txn_cmdty means there was a defective split. Bail. */
if (txn_cmdty == NULL)
return FALSE;

/* FIXME: In general, this should [correctly] deal with errors such
as not finding the approrpiate Accounts and not being able to
as not finding the appropriate Accounts and not being able to
parse the formula|credit/debit strings. */

new_txn = xaccTransCloneNoKvp(template_txn);
Expand All @@ -1163,8 +1229,6 @@ create_each_transaction_helper(Transaction *template_txn, void *user_data)
xaccTransGetDescription(new_txn),
xaccSchedXactionGetName(sx));

g_debug("template txn currency is %s",
gnc_commodity_get_mnemonic(xaccTransGetCurrency (template_txn)));

/* Bug#500427: copy the notes, if any */
if (xaccTransGetNotes(template_txn) != NULL)
Expand All @@ -1188,6 +1252,14 @@ create_each_transaction_helper(Transaction *template_txn, void *user_data)
return FALSE;
}

if (txn_cmdty == NULL)
{
xaccTransDestroy(new_txn);
xaccTransCommitEdit(new_txn);
return FALSE;
}
xaccTransSetCurrency(new_txn, txn_cmdty);

for (;
txn_splits && template_splits;
txn_splits = txn_splits->next, template_splits = template_splits->next)
Expand All @@ -1202,30 +1274,10 @@ create_each_transaction_helper(Transaction *template_txn, void *user_data)
template_split = (Split*)template_splits->data;
copying_split = (Split*)txn_splits->data;

if (!_get_template_split_account(sx, template_split, &split_acct,
creation_data->creation_errors))
{
err_flag = TRUE;
break;
}
_get_template_split_account(sx, template_split, &split_acct,
creation_data->creation_errors);

split_cmdty = xaccAccountGetCommodity(split_acct);
if (first_cmdty == NULL)
{
/* Set new_txn currency to template_txn if we have one, else first
* split
*/
if (xaccTransGetCurrency(template_txn))
xaccTransSetCurrency(new_txn,
xaccTransGetCurrency(template_txn));
else /* FIXME check for marker split */
xaccTransSetCurrency(new_txn, split_cmdty);

first_cmdty = xaccTransGetCurrency(new_txn);
}
g_debug("new txn currency is %s",
gnc_commodity_get_mnemonic(first_cmdty));

xaccSplitSetAccount(copying_split, split_acct);

{
Expand All @@ -1235,26 +1287,17 @@ create_each_transaction_helper(Transaction *template_txn, void *user_data)
g_debug("value is %s for memo split '%s'",
gnc_numeric_to_string (final),
xaccSplitGetMemo (copying_split));
if (! gnc_commodity_equal(split_cmdty,
xaccTransGetCurrency (new_txn)))
if (! gnc_commodity_equal(split_cmdty, txn_cmdty))
{
split_apply_exchange_rate(copying_split,
creation_data->instance->variable_bindings,
first_cmdty, split_cmdty, &final);
txn_cmdty, split_cmdty, &final);
}

xaccSplitScrub(copying_split);
}
}

if (err_flag)
{
g_critical("Error in SX transaction [%s], creation aborted.",
xaccSchedXactionGetName(sx));
xaccTransDestroy(new_txn);
xaccTransCommitEdit(new_txn);
return FALSE;
}

{
qof_instance_set (QOF_INSTANCE (new_txn),
Expand Down

0 comments on commit 9e6760f

Please sign in to comment.