Skip to content

Commit

Permalink
Bug 796955 - Import CSV - Single-line two-currency transactions can't…
Browse files Browse the repository at this point in the history 
… be imported

Behaviour is as follows:
If
  single line provides a price
And
  at some point in the import process we get into a
  situation where the base account and transfer
  account have a different commodity
Then
  transfer_acct amount = base_acct amount * price

If on the other hand base_acct and transfer_acct turn
out to have the same commodity, price is ignored.
  • Loading branch information
@gjanssens
gjanssens committed Feb 4, 2023
1 parent 56d16f4 commit 2d8bb6f
Show file tree
Hide file tree
Showing 15 changed files with 368 additions and 194 deletions.
11 changes: 10 additions & 1 deletion gnucash/import-export/csv-imp/assistant-csv-trans-import.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@

#include "import-account-matcher.h"
#include "import-main-matcher.h"
#include "import-backend.h"
#include "gnc-csv-account-map.h"
#include "gnc-account-sel.h"

Expand Down Expand Up @@ -2096,7 +2097,15 @@ CsvImpTransAssist::assist_match_page_prepare ()
auto draft_trans = trans_it.second;
if (draft_trans->trans)
{
gnc_gen_trans_list_add_trans (gnc_csv_importer_gui, draft_trans->trans);
auto lsplit = GNCImportLastSplitInfo {
draft_trans->m_price ? static_cast<gnc_numeric>(*draft_trans->m_price) : gnc_numeric{0, 0},
draft_trans->m_taction ? draft_trans->m_taction->c_str() : nullptr,
draft_trans->m_tmemo ? draft_trans->m_tmemo->c_str() : nullptr,
draft_trans->m_trec_state ? *draft_trans->m_trec_state : '\0',
draft_trans->m_trec_date ? static_cast<time64>(GncDateTime(*draft_trans->m_trec_date, DayPart::neutral)) : 0,
};

gnc_gen_trans_list_add_trans_with_split_data (gnc_csv_importer_gui, std::move (draft_trans->trans), &lsplit);
draft_trans->trans = nullptr;
}
}
Expand Down
18 changes: 13 additions & 5 deletions gnucash/import-export/csv-imp/gnc-imp-props-tx.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ std::string GncPreTrans::verify_essentials (void)
return std::string();
}

Transaction* GncPreTrans::create_trans (QofBook* book, gnc_commodity* currency)
std::shared_ptr<DraftTransaction> GncPreTrans::create_trans (QofBook* book, gnc_commodity* currency)
{
if (created)
return nullptr;
Expand Down Expand Up @@ -341,7 +341,7 @@ Transaction* GncPreTrans::create_trans (QofBook* book, gnc_commodity* currency)


created = true;
return trans;
return std::make_shared<DraftTransaction>(trans);
}

bool GncPreTrans::is_part_of (std::shared_ptr<GncPreTrans> parent)
Expand Down Expand Up @@ -662,7 +662,7 @@ static void trans_add_split (Transaction* trans, Account* account, GncNumeric am

}

void GncPreSplit::create_split (Transaction* trans)
void GncPreSplit::create_split (std::shared_ptr<DraftTransaction> draft_trans)
{
if (created)
return;
Expand Down Expand Up @@ -691,7 +691,7 @@ void GncPreSplit::create_split (Transaction* trans)
amount -= *m_amount_neg;

/* Add a split with the cumulative amount value. */
trans_add_split (trans, account, amount, m_action, m_memo, m_rec_state, m_rec_date, m_price);
trans_add_split (draft_trans->trans, account, amount, m_action, m_memo, m_rec_state, m_rec_date, m_price);

if (taccount)
{
Expand All @@ -701,7 +701,15 @@ void GncPreSplit::create_split (Transaction* trans)
auto inv_price = m_price;
if (m_price)
inv_price = m_price->inv();
trans_add_split (trans, taccount, -amount, m_taction, m_tmemo, m_trec_state, m_trec_date, inv_price);
trans_add_split (draft_trans->trans, taccount, -amount, m_taction, m_tmemo, m_trec_state, m_trec_date, inv_price);
}
else
{
draft_trans->m_price = m_price;
draft_trans->m_taction = m_taction;
draft_trans->m_tmemo = m_tmemo;
draft_trans->m_trec_state = m_trec_state;
draft_trans->m_trec_date = m_trec_date;
}

created = true;
Expand Down
33 changes: 31 additions & 2 deletions gnucash/import-export/csv-imp/gnc-imp-props-tx.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,35 @@ GncTransPropType sanitize_trans_prop (GncTransPropType prop, bool multi_split);
gnc_commodity* parse_commodity (const std::string& comm_str);
GncNumeric parse_monetary (const std::string &str, int currency_format);


/** The final form of a transaction to import before it is passed on to the
* generic importer.
*
* @param trans a possibly incomplete transaction created based on the data
* collected from the PreTrans and PreSplit records
*
* @param m_price... values harvested from the import data in single
* line mode and for which the transfer split could not yet
* be created (due to a missing transfer account value). These
* parameters will be passed on to the generic importer
* which can use this to complete information on the balancing
* split for an incomplete transaction
*/
struct DraftTransaction
{
DraftTransaction (Transaction* tx) : trans(tx) {}
~DraftTransaction () { if (trans) { xaccTransDestroy (trans); trans = nullptr; } }
Transaction* trans;

boost::optional<GncNumeric> m_price;
boost::optional<std::string> m_taction;
boost::optional<std::string> m_tmemo;
boost::optional<char> m_trec_state;
boost::optional<GncDate> m_trec_date;

boost::optional<std::string> void_reason;
};

struct GncPreTrans
{
public:
Expand All @@ -116,7 +145,7 @@ struct GncPreTrans
void set_multi_split (bool multi_split) { m_multi_split = multi_split ;}
void reset (GncTransPropType prop_type);
std::string verify_essentials (void);
Transaction *create_trans (QofBook* book, gnc_commodity* currency);
std::shared_ptr<DraftTransaction> create_trans (QofBook* book, gnc_commodity* currency);

/** Check whether the harvested transaction properties for this instance
* match those of another one (the "parent"). Note this function is *not*
Expand Down Expand Up @@ -162,7 +191,7 @@ struct GncPreSplit
void set_date_format (int date_format) { m_date_format = date_format ;}
void set_currency_format (int currency_format) { m_currency_format = currency_format; }
std::string verify_essentials (void);
void create_split(Transaction* trans);
void create_split(std::shared_ptr<DraftTransaction> draft_trans);

Account* get_account () { if (m_account) return *m_account; else return nullptr; }
void set_account (Account* acct) { if (acct) m_account = acct; else m_account = boost::none; }
Expand Down
15 changes: 9 additions & 6 deletions gnucash/import-export/csv-imp/gnc-import-tx.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -604,9 +604,9 @@ std::shared_ptr<DraftTransaction> GncTxImport::trans_properties_to_trans (std::v
QofBook* book = gnc_account_get_book (account);
gnc_commodity* currency = xaccAccountGetCommodity (account);

auto trans = trans_props->create_trans (book, currency);
auto draft_trans = trans_props->create_trans (book, currency);

if (trans)
if (draft_trans)
{
/* We're about to continue with a new transaction
* Time to do some closing actions on the previous one
Expand All @@ -621,19 +621,22 @@ std::shared_ptr<DraftTransaction> GncTxImport::trans_properties_to_trans (std::v
xaccTransCommitEdit (m_current_draft->trans);
xaccTransVoid (m_current_draft->trans, m_current_draft->void_reason->c_str());
}
m_current_draft = std::make_shared<DraftTransaction>(trans);
m_current_draft = draft_trans;
m_current_draft->void_reason = trans_props->get_void_reason();
created_trans = true;
}
else if (m_settings.m_multi_split) // in multi_split mode create_trans will return a nullptr for all but the first split
trans = m_current_draft->trans;
draft_trans = m_current_draft;
else // in non-multi-split mode each line should be a transaction, so not having one here is an error
throw std::invalid_argument ("Failed to create transaction from selected columns.");

if (!trans)
if (!draft_trans)
return nullptr;

split_props->create_split(trans);
split_props->create_split (draft_trans);

// With the added split information, we may have to revisit the transaction's commodity here
// TBD

/* Only return the draft transaction if we really created a new one
* The return value will be added to a list for further processing,
Expand Down
13 changes: 0 additions & 13 deletions gnucash/import-export/csv-imp/gnc-import-tx.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,6 @@
#include <boost/optional.hpp>


/** This struct stores a possibly incomplete transaction
* optionally together with its intended balance in case
* the user had selected a balance column. */
struct DraftTransaction
{
DraftTransaction (Transaction* tx) : trans(tx), balance(gnc_numeric_zero()), balance_set(false) {}
~DraftTransaction () { if (trans) { xaccTransDestroy (trans); trans = nullptr; } }
Transaction* trans;
gnc_numeric balance; /**< The expected balance after this transaction takes place */
bool balance_set; /**< true if balance has been set from user data, false otherwise */
boost::optional<std::string> void_reason;
};

/* A set of currency formats that the user sees. */
extern const int num_currency_formats;
extern const gchar* currency_format_user[];
Expand Down
49 changes: 4 additions & 45 deletions gnucash/import-export/import-account-matcher.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,24 +256,6 @@ show_placeholder_warning (AccountPickerDialog *picker, const gchar *name)
}


/***********************************************************
* show_commodity_warning
*
* show the warning when account is a different commodity to that
* required
************************************************************/
static void
show_commodity_warning (AccountPickerDialog *picker, const gchar *name)
{
const gchar *com_name = gnc_commodity_get_fullname (picker->new_account_default_commodity);
gchar *text = g_strdup_printf (_("The account '%s' has a different commodity to the "
"one required, '%s'. Please choose a different account."),
name, com_name);

show_warning (picker, text);
}


/*******************************************************
* account_tree_row_changed_cb
*
Expand All @@ -285,31 +267,16 @@ account_tree_row_changed_cb (GtkTreeSelection *selection,
{
Account *sel_account = gnc_tree_view_account_get_selected_account (picker->account_tree);

if (!sel_account)
{
gtk_widget_hide (GTK_WIDGET(picker->whbox)); // hide the warning
gtk_widget_set_sensitive (picker->ok_button, FALSE); // disable OK button
return;
}

gtk_widget_set_sensitive (picker->ok_button, TRUE); // enable OK button
/* Reset buttons and warnings */
gtk_widget_hide (GTK_WIDGET(picker->whbox));
gtk_widget_set_sensitive (picker->ok_button, (sel_account != NULL));

/* See if the selected account is a placeholder. */
if (sel_account && xaccAccountGetPlaceholder (sel_account))
{
const gchar *retval_name = xaccAccountGetName (sel_account);

show_placeholder_warning (picker, retval_name);
}
else if (picker->new_account_default_commodity &&
(!gnc_commodity_equal (xaccAccountGetCommodity (sel_account),
picker->new_account_default_commodity))) // check commodity
{
const gchar *retval_name = xaccAccountGetName (sel_account);
show_commodity_warning (picker, retval_name);
}
else
gtk_widget_hide (GTK_WIDGET(picker->whbox)); // hide the warning
}

static gboolean
Expand Down Expand Up @@ -409,7 +376,7 @@ Account * gnc_import_select_account(GtkWidget *parent,
picker->new_account_default_type = new_account_default_type;

/*DEBUG("Looking for account with online_id: \"%s\"", account_online_id_value);*/
if (account_online_id_value != NULL)
if (account_online_id_value)
{
AccountOnlineMatch match = {NULL, 0, account_online_id_value};
retval =
Expand Down Expand Up @@ -511,14 +478,6 @@ Account * gnc_import_select_account(GtkWidget *parent,
response = GNC_RESPONSE_NEW;
break;
}
else if (picker->new_account_default_commodity &&
(!gnc_commodity_equal (xaccAccountGetCommodity (retval),
picker->new_account_default_commodity))) // check commodity
{
show_commodity_warning (picker, retval_name);
response = GNC_RESPONSE_NEW;
break;
}

if (account_online_id_value)
{
Expand Down

0 comments on commit 2d8bb6f

Please sign in to comment.