Bug 362139 - CSV Importer asks the same question twice during profile deletion
Summary: CSV Importer asks the same question twice during profile deletion
Status: RESOLVED FIXED
Alias: None
Product: kmymoney
Classification: Applications
Component: importer (show other bugs)
Version: git (master)
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: KMyMoney Devel Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-23 13:41 UTC by wojnilowicz
Modified: 2017-07-01 11:11 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.8.0
Sentry Crash Report:


Attachments
Delete or Keep (14.63 KB, image/png)
2016-04-23 13:41 UTC, wojnilowicz
Details

Note You need to log in before you can comment on or make changes to this bug.
Description wojnilowicz 2016-04-23 13:41:17 UTC
When I want to delete importing profile, CSV Importer asks me twice whether I want to keep or delete that profile.

Reproducible: Always

Steps to Reproduce:
1. File->Import->CSV
2. Select Investment
3. Select existing profile
4. Click on clear text button

Actual Results:  
Question from attachment is asked twice.

Expected Results:  
Question from attachment should be asked once.
Comment 1 wojnilowicz 2016-04-23 13:41:56 UTC
Created attachment 98535 [details]
Delete or Keep
Comment 2 wojnilowicz 2016-04-23 13:43:17 UTC
Actual Results: 
Question from attachment is asked twice if I answer "Keep".
Comment 3 wojnilowicz 2016-05-08 07:12:02 UTC
Git commit f610c254a72ca5fda20759b2bd39501f75b14f54 by Łukasz Wojniłowicz.
Committed on 08/05/2016 at 07:07.
Pushed by wojnilowicz into branch 'master'.

Remember transaction type identified by user

Transaction type identified by user isn't remembered, so in case of
consecutive transaction of the same type user is asked again about
identification. For KMM it is better to ask only once for unknown
transaction and use that answer to identify further transactions.
REVIEW: 127718

M  +18   -0    kmymoney/plugins/csvimport/investprocessing.cpp

http://commits.kde.org/kmymoney/f610c254a72ca5fda20759b2bd39501f75b14f54
Comment 4 wojnilowicz 2016-05-08 07:12:02 UTC
Git commit 58d9b27f84f9cdc43ea981006ebfd04df15e934f by Łukasz Wojniłowicz.
Committed on 08/05/2016 at 07:04.
Pushed by wojnilowicz into branch 'master'.

Check if CSV profile has been removed in UI before its deletion
REVIEW: 127722

M  +1    -1    kmymoney/plugins/csvimport/csvwizard.cpp

http://commits.kde.org/kmymoney/58d9b27f84f9cdc43ea981006ebfd04df15e934f
Comment 5 wojnilowicz 2016-05-08 07:15:13 UTC
Git commit bcd0cd05110264d5104b20c53e3bcf4a6cf506be by Łukasz Wojniłowicz.
Committed on 08/05/2016 at 07:14.
Pushed by wojnilowicz into branch 'frameworks'.

Check if CSV profile has been removed in UI before its deletion
REVIEW: 127722

M  +1    -1    kmymoney/plugins/csvimport/csvwizard.cpp

http://commits.kde.org/kmymoney/bcd0cd05110264d5104b20c53e3bcf4a6cf506be
Comment 6 wojnilowicz 2016-05-08 07:15:13 UTC
Git commit b73a474fdd43f1eecf2dd97253db883f3fc1439e by Łukasz Wojniłowicz.
Committed on 08/05/2016 at 07:14.
Pushed by wojnilowicz into branch 'frameworks'.

Remember transaction type identified by user

Transaction type identified by user isn't remembered, so in case of
consecutive transaction of the same type user is asked again about
identification. For KMM it is better to ask only once for unknown
transaction and use that answer to identify further transactions.
REVIEW: 127718

M  +18   -0    kmymoney/plugins/csvimport/investprocessing.cpp

http://commits.kde.org/kmymoney/b73a474fdd43f1eecf2dd97253db883f3fc1439e
Comment 7 allan 2016-05-15 21:14:08 UTC
Today, and the first for a while, I updated from HEAD and to ensure all was OK, I did a CSV import of an investment file, which contained two similar entries.  They were both CashDividends, with the main difference being that one had an identical but negative amount.  On import, the negative sign had been dropped.
I think I've traced it, possibly,  to the recent fee patch.
In investprocessing.cpp(), at circa line 1723 -
tr.m_amount = tr.m_amount.abs() - m_trInvestData.fee.abs();
appears to drop the sign.
Comment 8 allan 2016-05-15 21:24:28 UTC
(In reply to allan from comment #7)
> Today, and the first for a while, I updated from HEAD and to ensure all was
> OK, I did a CSV import of an investment file, which contained two similar
> entries.  They were both CashDividends, with the main difference being that
> one had an identical but negative amount.  On import, the negative sign had
> been dropped.
> I think I've traced it, possibly,  to the recent fee patch.
> In investprocessing.cpp(), at circa line 1723 -
> tr.m_amount = tr.m_amount.abs() - m_trInvestData.fee.abs();
> appears to drop the sign.

This comment of mine, #7, was added to this bug when I first discovered this problem, but on investigating, it seems that it doesn't really belong here, but probably to BUG: 361021.  I can add this entry to that bug report, but that might cause confusion about which bug to follow.  So, do I leave as is?
Comment 9 wojnilowicz 2016-05-16 18:40:18 UTC
(In reply to allan from comment #7)
> Today, and the first for a while, I updated from HEAD and to ensure all was
> OK, I did a CSV import of an investment file, which contained two similar
> entries.  They were both CashDividends, with the main difference being that
> one had an identical but negative amount.  On import, the negative sign had
> been dropped.
> I think I've traced it, possibly,  to the recent fee patch.
> In investprocessing.cpp(), at circa line 1723 -
> tr.m_amount = tr.m_amount.abs() - m_trInvestData.fee.abs();
> appears to drop the sign.

I'm glad you did it and gave feedback. I apologize if something is broken for you lately with CSV imports. I would like to help fixing that. Could you send your problematic, anonymized CSV test file and explain me what result you expect after importing?
As I understand, you've got two CashDividends: one negative and one positive. Frankly I don't get it, shouldn't CashDividends be always positive, as it its you who gets the cash?

>This comment of mine, #7, was added to this bug when I first discovered this problem, but on >investigating, it seems that it doesn't really belong here, but probably to BUG: 361021. I can >add this entry to that bug report, but that might cause confusion about which bug to follow. >So, do I leave as is?
I would say don't bother. For the first time I used an feature, that Thomas Baumgart explained to me, and I wasn't too careful with that, so you confused right patch with wrong bug.
Comment 10 allan 2016-05-16 23:46:41 UTC
(In reply to NSLW from comment #9)
> (In reply to allan from comment #7)

> > I think I've traced it, possibly,  to the recent fee patch.
> > In investprocessing.cpp(), at circa line 1723 -
> > tr.m_amount = tr.m_amount.abs() - m_trInvestData.fee.abs();
> > appears to drop the sign.
> 
> I'm glad you did it and gave feedback. I apologize if something is broken
> for you lately with CSV imports. 

I don't have any problem, as I was using a test file, not live data.

> I would like to help fixing that. Could you
> send your problematic, anonymized CSV test file and explain me what result
> you expect after importing?
> As I understand, you've got two CashDividends: one negative and one
> positive. Frankly I don't get it, shouldn't CashDividends be always
> positive, as it its you who gets the cash?

The test file is one I've used for several years, and originally I obtained it from another user.  At that time, I was developing the CSV investment handling and I found the file quite useful, as the data was not the usual straight-forward simple investment transactions.  I cannot now remember if the CashDividend with the negative amount was an original entry, or whether I modified it for the purpose.  My "justification/rationale" for the entry was that it was documenting a refund of an erroneous earlier transaction.  I suppose a "miscexp" would be similar (or perhaps not).  The file was from a US broker, who did produce some odd methods in his files.  As it's quite small, here it is below.

"Trade Date","Settlement Date","Type","Description 1 ","Description 2","Symbol/CUSIP","Quantity","Price ($)","Amount ($)"
"","2/24/2010","DividendAndInterest","Div","description","NECZX","","","504.72"

"","3/28/2010","Other","Div","description",""NECZX"","","","-504.72"
Comment 11 wojnilowicz 2016-05-17 18:04:21 UTC
(In reply to allan from comment #10)
> (In reply to NSLW from comment #9)
> > (In reply to allan from comment #7)
> 
> > > I think I've traced it, possibly,  to the recent fee patch.
> > > In investprocessing.cpp(), at circa line 1723 -
> > > tr.m_amount = tr.m_amount.abs() - m_trInvestData.fee.abs();
> > > appears to drop the sign.
> > 
> > I'm glad you did it and gave feedback. I apologize if something is broken
> > for you lately with CSV imports. 
> 
> I don't have any problem, as I was using a test file, not live data.
> 
> > I would like to help fixing that. Could you
> > send your problematic, anonymized CSV test file and explain me what result
> > you expect after importing?
> > As I understand, you've got two CashDividends: one negative and one
> > positive. Frankly I don't get it, shouldn't CashDividends be always
> > positive, as it its you who gets the cash?
> 
> The test file is one I've used for several years, and originally I obtained
> it from another user.  At that time, I was developing the CSV investment
> handling and I found the file quite useful, as the data was not the usual
> straight-forward simple investment transactions.  I cannot now remember if
> the CashDividend with the negative amount was an original entry, or whether
> I modified it for the purpose.  My "justification/rationale" for the entry
> was that it was documenting a refund of an erroneous earlier transaction.  I
> suppose a "miscexp" would be similar (or perhaps not).  The file was from a
> US broker, who did produce some odd methods in his files.  As it's quite
> small, here it is below.
> 
> "Trade Date","Settlement Date","Type","Description 1 ","Description
> 2","Symbol/CUSIP","Quantity","Price ($)","Amount ($)"
> "","2/24/2010","DividendAndInterest","Div","description","NECZX","","","504.
> 72"
> 
> "","3/28/2010","Other","Div","description",""NECZX"","","","-504.72"

If it's some kind of "refund of an erroneous earlier transaction" then in my opinion it's not CashDividend. I read more on dividends on folowing websites:
http://www.investopedia.com/terms/c/cashdividend.asp?o=40186&l=dir&qsrc=999&qo=investopediaSiteSearch&ap=investopedia.com
http://www.investopedia.com/ask/answers/011215/how-are-dividends-usually-paid-out.asp
http://www.investopedia.com/walkthrough/corporate-finance/5/dividends/cash-payment.aspx?o=40186&l=dir&qsrc=999&qo=investopediaSiteSearch&ap=investopedia.com

and I cannot find any real life situation where stockholder would pay money to corporation under term dividend. Are you sure this case is real world case?

BTW. Do you've got more test files for CSV imports?
Comment 12 Jeff 2016-05-19 09:25:50 UTC
> I cannot find any real life situation where stockholder would pay money to corporation under term dividend

When you are short the stock and the dividend is due, it is paid out of your account. (You don't pay it to the corporation, you pay it to the stockholder from whom you borrowed the stock.)  I'm not sure how this would be tagged in the CSV / OFX data, but it could well be a "cash dividend" with a negative value (i.e. a payment = deducted from your account). You can google "short stock dividend payment" for an explanation of the process.
Comment 13 wojnilowicz 2016-05-21 13:21:28 UTC
Thank you both for valuable comments. I posted patch for review which should allow importing negative CashDividend through CSV importer.