[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

NSLW  changed:

   What|Removed |Added

 Status|CONFIRMED   |RESOLVED
 Resolution|--- |FIXED
  Latest Commit||http://commits.kde.org/kmym
   ||oney/84eaeecc2e48a9d9391d77
   ||154243d6ea052a0c6f
   Version Fixed In||4.8.0

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #18 from NSLW  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[allan via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #17 from allan  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #16 from NSLW  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[allan via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #15 from 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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[allan via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #14 from allan  ---
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

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[allan via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #13 from allan  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #12 from NSLW  ---
(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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[allan via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #11 from allan  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

NSLW  changed:

   What|Removed |Added

  Attachment #97972|0   |1
is obsolete||

--- Comment #10 from NSLW  ---
Created attachment 98111
  --> https://bugs.kde.org/attachment.cgi?id=98111=edit
[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::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?

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[allan via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #9 from allan  ---
(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

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #8 from NSLW  ---
(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?

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[allan via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

allan  changed:

   What|Removed |Added

 Status|UNCONFIRMED |CONFIRMED
 Ever confirmed|0   |1

--- Comment #7 from allan  ---
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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[Thomas Baumgart via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

Thomas Baumgart  changed:

   What|Removed |Added

 CC||agande...@gmail.com

--- Comment #6 from Thomas Baumgart  ---
I have not tested it, but the patch looks good to me as well. Let's wait for
Allen to take care of it.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[Cristian Oneț via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

Cristian Oneț  changed:

   What|Removed |Added

 CC||onet.crist...@gmail.com

--- Comment #5 from Cristian Oneț  ---
@Allan please review this, it looks good to me.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #4 from NSLW  ---
Created attachment 97972
  --> https://bugs.kde.org/attachment.cgi?id=97972=edit
[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.

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #3 from NSLW  ---
Created attachment 97857
  --> https://bugs.kde.org/attachment.cgi?id=97857=edit
Column Assignment for CSV file

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #1 from NSLW  ---
Created attachment 97855
  --> https://bugs.kde.org/attachment.cgi?id=97855=edit
Recoginition of security is case sensitive

-- 
You are receiving this mail because:
You are watching all bug changes.

[kmymoney4] [Bug 360435] CSV Importer doesn't recognize security if its symbol isn't lower case

[NSLW via KDE Bugzilla]

https://bugs.kde.org/show_bug.cgi?id=360435

--- Comment #2 from NSLW  ---
Created attachment 97856
  --> https://bugs.kde.org/attachment.cgi?id=97856=edit
CSV Test File

-- 
You are receiving this mail because:
You are watching all bug changes.