See first attachment. Reproducible: Always Steps to Reproduce: 1. create your investment account 2. go to investment tab and create new investment 3. type in the security in upper case details (see first attachment) and press next 4. choose GPW as quotes source and press finish 5. import CSV (see attachment) Actual Results: Security "NET" isn't recognized (see first attachment) Expected Results: Security "NET" should be recognized If the security symbol from point 3 of steps to reproduce is typed in lower case then the security is recognized correctly (see first attachment)
Created attachment 97855 [details] Recoginition of security is case sensitive
Created attachment 97856 [details] CSV Test File
Created attachment 97857 [details] Column Assignment for CSV file
Created attachment 97972 [details] [PATCH] Make matching securities by their symbols case insensitive Here is patch to make matching securities by trading symbol case insensitive. Please revise it and apply to master branch.
@Allan please review this, it looks good to me.
I have not tested it, but the patch looks good to me as well. Let's wait for Allen to take care of it.
I did fix this a while ago, following https://bugs.kde.org/show_bug.cgi?id=352789, but somehow things got side-tracked, and it was not committed. The proposed patch looks, good, but I would make one change, in " securityName = m_wizDlg->m_csvDialog->ui->tableWidget->item(row, detail)->text().toUpper().trimmed();" I would propose removing the '.toUpper()', allowing the user's value/preference to be maintained.
(In reply to allan from comment #7) > I did fix this a while ago, following > https://bugs.kde.org/show_bug.cgi?id=352789, but somehow things got > side-tracked, and it was not committed. > > The proposed patch looks, good, but I would make one change, in > " > securityName = m_wizDlg->m_csvDialog->ui->tableWidget->item(row, > detail)->text().toUpper().trimmed();" > > I would propose removing the '.toUpper()', allowing the user's > value/preference to be maintained. That seems reasonable. Do you need me to send you another patch?
(In reply to NSLW from comment #8) > (In reply to allan from comment #7) > > I did fix this a while ago, following > > https://bugs.kde.org/show_bug.cgi?id=352789, but somehow things got > > side-tracked, and it was not committed. > > > > The proposed patch looks, good, but I would make one change, in > > " > > securityName = m_wizDlg->m_csvDialog->ui->tableWidget->item(row, > > detail)->text().toUpper().trimmed();" > > > > I would propose removing the '.toUpper()', allowing the user's > > value/preference to be maintained. > > That seems reasonable. Do you need me to send you another patch? No, thanks. I think we have enough. Allan
Created attachment 98111 [details] [PATCH] Make matching securities by their symbols case insensitive v2 (In reply to allan from comment #9) > (In reply to NSLW from comment #8) > > That seems reasonable. Do you need me to send you another patch? > > No, thanks. I think we have enough. > > Allan It seems that it will be necessary. During search for another bug I found out that QMap is sadly case sensitive and occasionally my security names were wrong. Attached patch addresses this issue, so please revise it again and apply to master. In patch I also modified following code from csvwizard.cpp >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> bool exists; QString name; QList<MyMoneySecurity>::ConstIterator it = list.constBegin(); while (it != list.constEnd()) { exists = false; if (!symbl.isEmpty()) { // symbol already exists sec = *it; name.clear(); if (sec.tradingSymbol() == symbl) { exists = true; name = sec.name(); break; } } ++it; } if (!exists) { name = securityName; } >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I put "exists = false;" out of while loop, otherwise condition "if (!exists)" will newer be met and thus if security was not in "securities tab" from outset, then it couldn't be created in "equities tab" automatically during import. Do you want me to open another bug report for that?
Are you sure about the change to csvwizard.cpp? As far as I can see, with "exists = false;" in the while loop, it works correctly.
(In reply to allan from comment #11) > Are you sure about the change to csvwizard.cpp? As far as I can see, with > "exists = false;" in the while loop, it works correctly. As long as list variable is not empty. If it's empty you wont even enter while loop (thus wont even define exists variable) and it is empty if you have no securities on "securities tab". It's corner case, I'm sure of.
(In reply to NSLW from comment #12) > (In reply to allan from comment #11) > > Are you sure about the change to csvwizard.cpp? As far as I can see, with > > "exists = false;" in the while loop, it works correctly. > > As long as list variable is not empty. If it's empty you wont even enter > while loop (thus wont even define exists variable) and it is empty if you > have no securities on "securities tab". It's corner case, I'm sure of. Yes, of course. You are right. I think we're all happy with this now, so I'll be going ahead to commit.
Very sadly, I find I can not continue my involvement. Thomas is aware of the situation. I think it might be as well for this complete topic to be submitted to Reviewboard. Allan
Unfortunately, I am unable to follow the detail of the latest proposed patch, but I would like to urge some caution. It may be that the proposal does not affect the config file - csvimporterrc, but in its current implementation, it is possible for the user to edit the entries in the file to suit his needs. If instead the entries are moved into the coding, then recompilation would be required for any changes, which most users would not wish to face. If this is not the case, then that's fine.
(In reply to allan from comment #15) > Unfortunately, I am unable to follow the detail of the latest proposed > patch, but I would like to urge some caution. It may be that the proposal > does not affect the config file - csvimporterrc, but in its current > implementation, it is possible for the user to edit the entries in the file > to suit his needs. If instead the entries are moved into the coding, then > recompilation would be required for any changes, which most users would not > wish to face. If this is not the case, then that's fine. The second version of the patch is different from the first you've committed only in asking m_map for security name using its symbol in uppercase instead of lowercase, because m_map is from now on filled by default with symbols in uppercase and m_map won't return correct name if asked in lowercase. AFAIK no part of csvimporterrc shouldn't impact the behavior changed here. In fact we make the behavior lax, so user can in any time change his symbol from uppercase to lowercase or mixed case and still get it detected as the same symbol. If that's fine I think I can apply second version of patch just like you've applied the first one.
(In reply to NSLW from comment #16) > (In reply to allan from comment #15) > > Unfortunately, I am unable to follow the detail of the latest proposed > > patch, but I would like to urge some caution. It may be that the proposal > > does not affect the config file - csvimporterrc, but in its current > > implementation, it is possible for the user to edit the entries in the file > > to suit his needs. If instead the entries are moved into the coding, then > > recompilation would be required for any changes, which most users would not > > wish to face. If this is not the case, then that's fine. > > The second version of the patch is different from the first you've committed > only in asking m_map for security name using its symbol in uppercase instead > of lowercase, because m_map is from now on filled by default with symbols in > uppercase and m_map won't return correct name if asked in lowercase. > > AFAIK no part of csvimporterrc shouldn't impact the behavior changed here. > In fact we make the behavior lax, so user can in any time change his symbol > from uppercase to lowercase or mixed case and still get it detected as the > same symbol. > > If that's fine I think I can apply second version of patch just like you've > applied the first one. That sounds good to me. Would you check on the Frameworks side please, as Christian committed only my 'partial' patch.
(In reply to allan from comment #17) > That sounds good to me. > Would you check on the Frameworks side please, as Christian committed only > my 'partial' patch. Yeah, I'll check on both branches: frameworks and master.