Bug 302181

Summary: Problems with libalkimia - was Problems importing CSV files
Product: [Applications] kmymoney Reporter: allan <agander93>
Component: generalAssignee: KMyMoney Devel Mailing List <kmymoney-devel>
Status: RESOLVED FIXED    
Severity: normal CC: onet.cristian, richier7
Priority: NOR    
Version: 4.6.1   
Target Milestone: ---   
Platform: Ubuntu   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description allan 2012-06-19 10:54:14 UTC
I am running kmymoney on Kubuntu 12.04, and I am trying to migrate from GnuCash to KmyMoney. Now I've run into a problem:

I need to import lists of transactions, that I download as xls-files from my bank, then edit them in gnumeric/libreoffice calc, and save as csv. Only 3 coumns: date, description, and amount (+ and - in one column, NOT separate debet and credit). Like this:

2012/06/04 	VA-SYD 	-512
2012/06/04 	FRIHAMNSVIADUK 	-2478,25

When importing into KmyMoney, the decimals are not identified. So 130,50 is recognized as 13050. I've tried using a decimal comma (saved the file with commas AND set it to comma in the configuration tab of the imprt window) and dots, same error produced.

ANy ideas on what I might do wrong? Any more information that I should supply?


Reproducible: Always

Steps to Reproduce:
1.Read the above lines into the csv importer plugin
2.Select tab as separator and comma as decimal.
3.Import

Actual Results:  
The decimal sign is not recognised.  -512 is imported as -51200,00 and -2478,25 as -247825,00.

Expected Results:  
Should import as -512,00 and -2478,25.
Comment 1 allan 2012-06-19 10:55:45 UTC
Using tab as separator and comma as decimal both lines are handled correctly initially, the '-512' correctly showing as '-512,00'.  KMM version 4.6.2 now imports the values correctly, However, 4.6.1 does not.

Can you build 4.6.2 from source, or get it from Claydoh's repository?
Comment 2 allan 2012-06-19 11:09:15 UTC
Within the csv plugin, all gets handled correctly in csvprocessing.cpp until line 469.
tr.m_amount = m_trData.amount;
a) m_trData.amount is "-512,00" but tr.m_amount produces "-51200/1"
b) m_trData.amount is "-2478,25" but tr.m_amount produces "-247825/1".

4.6.2 handles this correctly.
Comment 3 allan 2012-06-19 23:09:17 UTC
Correction.

4.6.2 has the same problem, but git definitely has the fix.

Am I right in thinking that 4.6.2 is the current stable release and that this problem needs to fixed in it?  Or, as it is fixed in git, no action is required here?  I'm not sure if in general distros will update, but perhaps Claydoh might?
Comment 4 Thomas Baumgart 2012-06-20 06:06:11 UTC
I am lost: can you please elaborate what causes the problem as I don't see why it is a problem with libalkimia but fixed in KMyMoney git master. I am not saying that this can't be, but a pointer in the right direction to understand what's going on is certainly welcome. And if it is fixed in git master the question arises if that fix has been backported to the 4.6 branch.
Comment 5 allan 2012-06-20 10:13:31 UTC
Sorry for causing the confusion.  During debugging, I had traced the cause to the 
tr.m_amount = m_trData.amount statement and had found that tr.m_amount.decimalSeparator() produced a '.' when it should have been a ','.
I tried a couple of 'fixes' which didn't work, and monitoring the libAlkimia ctor, I found it too was producing the wrong separator,  decided I needed help, so changed the heading to include libalkimia, as I thought that was the problem area.  Before submitting the revised bug entry, the original poster responded that he had now switched to 4.6.2 and still had the problem, and I then realised that the problem had been fixed in git rather than 4.6.2.  I had been thinking the problem might lie with the change to the libAlkimia version, but now realised that wasn't the case.

With further monitoring of the libalkimia ctor, I found the separator was correct during KMM startup, but changed as the csvplugin started, before it manipulated the decimal separator.  I traced the problem ctors to the initialisation of three mymoneymoney instances.  Casting these to libalkimia fixed them, but not the failing csv file.  It was then I found that in git I had changed the 
tr.m_amount = m_trData.amount statements to 
tr.m_amount = MyMoneyMoney(m_trData.amount).

I then submitted the bug entry with the confusing heading.  To be fair to myself, it does say 'Problem with...', not 'Problem in...', and I did need to get some help.

The change in git was introduced last November, related to the change to wizard structure, one of several changes.  It was not back-ported.
Comment 6 allan 2012-06-21 09:51:13 UTC
Git is definitely able to import the problem file, and the same change made to my local 4.6.1 also allows it to do the import successfully.

So, what next?  Getting back to my query in comment 3, should this be backported to 4.6.2?  If so, I would welcome guidance on exactly how to do this.  I would intend to change just this particular piece of code, which is very small, not to add the whole of the commit of which it was part, as the commit was for application to the wizard version of the plugin.

It would also be helpful to know what  must, or should, or should not be packported.  Presumably, it would go back no further than the current stable release?  That is, 4.6.2 and not also to 4.6.1?
Comment 7 Thomas Baumgart 2012-06-21 20:31:39 UTC
Can you provide the SHA1 of the fix in git master? If uncertain, a date/time of the commit is also welcome.
Comment 8 allan 2012-06-21 21:29:45 UTC
(In reply to comment #7)
> Can you provide the SHA1 of the fix in git master? If uncertain, a date/time
> of the commit is also welcome.

It's ff75edf07655f814de489e336d7f5c501a7540c0 ( 21 Nov.), part of several fixes as follow up to the first csvplugin wizard.  Just a two-line change resolves the present issue, but I've noticed a similar need elsewhere, which appears not (yet) to be causing trouble.  Presumably, I should attend to this myself in my next commit to git?
Comment 9 Thomas Baumgart 2012-06-22 08:59:15 UTC
I see that this fix has not been backported to the 4.6 branch (the file csvdialog.cpp is missing all together). So it's a matter of checking what is going on in the 4.6 series and fix it there.

All you need to do is to switch to the 4.6 branch in your environment. Do that as follows:

git checkout 4.6
git pull -r

That should leave you with the latest that is available for 4.6. You know best what has been added to the importer since then. If you need further help, please ask on the devel mailinig list.
Comment 10 Cristian Oneț 2012-06-22 10:12:21 UTC
(In reply to comment #9)
> I see that this fix has not been backported to the 4.6 branch (the file
> csvdialog.cpp is missing all together). So it's a matter of checking what is
> going on in the 4.6 series and fix it there.

That is because the 4.6 branch contains the first implementation of the csvimporter plugin while the master branch contains a re-written (wizard based) implementation. I guess since we are talking about a plugin we could backport the whole version from the master branch if it has been thoroughly tested but I'm not sure about that.
Comment 11 allan 2012-06-22 10:37:25 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > I see that this fix has not been backported to the 4.6 branch (the file
> > csvdialog.cpp is missing all together). So it's a matter of checking what is
> > going on in the 4.6 series and fix it there.
> 
> That is because the 4.6 branch contains the first implementation of the
> csvimporter plugin while the master branch contains a re-written (wizard
> based) implementation.

Yes, that is exactly the case.  The fix for the current problem is very minor and I have implemented that now in 4.6.2.  Going beyond that, I need to go through quite a few commits to see what might be relevant or important enough to backport, many being specific only to the wizard version.

> I guess since we are talking about a plugin we could
> backport the whole version from the master branch if it has been thoroughly
> tested but I'm not sure about that.

That's an interesting idea, but it is a biggish change, from the testing point of view, and I'm not sure it would be justified.  It would certainly delay the fix to the current problem.

Just let me know how to proceed.
Comment 12 Cristian Oneț 2012-06-22 11:00:57 UTC
If you have only the fix against 4.6.2 for this problem then commit and push it into the 4.6 branch.
Comment 13 allan 2012-07-23 21:04:41 UTC
*** Bug 303969 has been marked as a duplicate of this bug. ***
Comment 14 icare34 2012-07-24 08:03:54 UTC
Sorry to have duplicated a bug!!

I have installed the 4.6.2 version on my computer (by : deb http://ppa.launchpad.net/claydoh/kmymoney2-kde4/ubuntu precise main ), but I have still the problem.
Comment 15 allan 2012-07-24 09:35:16 UTC
(In reply to comment #14)
> Sorry to have duplicated a bug!!

No worries.

> I have installed the 4.6.2 version on my computer (by : deb
> http://ppa.launchpad.net/claydoh/kmymoney2-kde4/ubuntu precise main ), but I
> have still the problem.

The original 4.6.2 did have the bug, but anumber of fixes were back-ported from Git on 30 Jun.

It might be worth contacting Claydoh and asking if he can possibly update from Git.  Or, looking at Debian testing to see if they have updated their 4.6.2.  Otherwise, I'm afraid it's a case of compiling from source, or waiting until...?

@Thomas.
Having commited the backported fixes, how do the distros find out about them/it?
Comment 16 Thomas Baumgart 2012-07-24 10:35:42 UTC
@Allen: we would have to release 4.6.3 so that they pick it up..
Comment 17 allan 2012-07-24 10:41:50 UTC
(In reply to comment #16)
> @Allen: we would have to release 4.6.3 so that they pick it up..

Yes, I had a feeling we would need to do that.  Well, having gone to the trouble of back-porting, I guess that needs to be done.  By whom?  I know you're busy.  Is it something I could/should do?  If so, how?
Comment 18 icare34 2012-07-24 21:36:08 UTC
I have compiled, now I have Version 4.6.90-1701953db7.
The importation is OK now.
Thanks for your help.
Comment 19 allan 2012-07-24 22:06:23 UTC
(In reply to comment #18)
> I have compiled, now I have Version 4.6.90-1701953db7.
> The importation is OK now.
> Thanks for your help.

Glad you're OK now.
Comment 20 Thomas Baumgart 2012-07-25 05:42:38 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > @Allen: we would have to release 4.6.3 so that they pick it up..
> 
> Yes, I had a feeling we would need to do that.  Well, having gone to the
> trouble of back-porting, I guess that needs to be done.  By whom?  I know
> you're busy.  Is it something I could/should do?  If so, how?

Releasing 4.6.3 would not stop the back-porting issue.as 4.6.3 will live on the 4.6 branch not in master. I will start the discussion of a release among those who have done it in the past: it always was a collaborative thing.
Comment 21 Cristian Oneț 2014-07-29 10:12:08 UTC
As I understood this was fixed in alkimia.
Comment 22 allan 2014-07-29 10:27:55 UTC
On 29/07/14 11:12, Cristian Oneț  wrote:
> https://bugs.kde.org/show_bug.cgi?id=302181
>
> Cristian Oneț <onet.cristian@gmail.com> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>           Resolution|---                         |FIXED
>               Status|CONFIRMED                   |RESOLVED
>
> --- Comment #21 from Cristian Oneț <onet.cristian@gmail.com> ---
> As I understood this was fixed in alkimia.
>

Not quite.  It's a pretty rambling bug report, but the problem, which is 
resolved, was fixed by me in the CSV plugin.  I added the  "Problems 
*with* libalkimia" as I was seeking help with it, but isolated the 
problem to the plugin.

Allan
Comment 23 Cristian Oneț 2014-07-29 10:37:46 UTC
OK, but it was fixed so the status is correct.