Bug 498903 - Import fails for numbers with more than 3 decimal places
Summary: Import fails for numbers with more than 3 decimal places
Status: RESOLVED FIXED
Alias: None
Product: skrooge
Classification: Applications
Component: general (show other bugs)
Version: 25.1.0
Platform: Arch Linux Linux
: NOR normal
Target Milestone: 25.1.0
Assignee: Stephane MANKOWSKI
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-19 22:53 UTC by Christoph Vogtländer
Modified: 2025-01-26 20:31 UTC (History)
1 user (show)

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


Attachments
Stooq AYEM.DE imported values (162.69 KB, image/png)
2025-01-19 22:53 UTC, Christoph Vogtländer
Details
Example test unit (68.35 KB, image/png)
2025-01-19 22:54 UTC, Christoph Vogtländer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Vogtländer 2025-01-19 22:53:26 UTC
Created attachment 177535 [details]
Stooq AYEM.DE imported values

SUMMARY
Adding values for a unit produces wrong results when there are more than two decimal numbers. For example, adding "1,234" or "1.234" will result in a value of "1234.000". This also happens when downloading values from a configured source. To reproduce, use "Stooq" with the code "AYEM.DE" (see also attached screenshot).

Adding numbers with only two decimal places, e.g. "1.23" or "1,23" works as expected.

This also applies when manually adding transactions in an account using this unit.

STEPS TO REPRODUCE
1. add a custom unit, e.g. "Test", configure 3 decimal numbers
2. use "values" to manually add a value
3. enter "1,234" as the amount and click "add"

OBSERVED RESULT
the value is added as "1234,00"

EXPECTED RESULT
the value should be "1,234"

SOFTWARE/OS VERSIONS
Operating System: Arch Linux 
KDE Plasma Version: 6.2.5
KDE Frameworks Version: 6.10.0
Qt Version: 6.8.1
Kernel Version: 6.12.9-arch1-1 (64-bit)
Graphics Platform: Wayland
Processors: 4 × Intel® Core™ i5-4460 CPU @ 3.20GHz
Memory: 14.6 GiB of RAM
Graphics Processor: Mesa Intel® HD Graphics 4600
Comment 1 Christoph Vogtländer 2025-01-19 22:54:05 UTC
Created attachment 177536 [details]
Example test unit
Comment 2 Stephane MANKOWSKI 2025-01-21 17:03:32 UTC
Hi,

This change of behavior is due to this request: https://bugs.kde.org/show_bug.cgi?id=494159.
In Germany, the dot is the thousand separator. So 1.234 is understood as 1234. But, 1,234 should be understood as 1.234.

So, I don't know how to do yet. If you have an idea let me know.
Comment 3 Christoph Vogtländer 2025-01-22 01:10:49 UTC
A quick test on my system shows that

output = QLocale().toDouble("1,234", &ok);

results in

output == 1.234
ok == true

as expected. I'm not sure why this does not work in skrooge. For me, it would be perfectly fine if I could manually enter "1,234" and have it recognized as double "1.234". Would a test with QLocale::German in skrooge like

SKGTEST(QStringLiteral("CONV:stringToDouble"), SKGServices::stringToDouble(QStringLiteral("1,234")), 1.234)

succeed? I'm not able to test this atm. But, I can download the skrooge source and try to compile it myself. What do you think?
Comment 4 pat_h 2025-01-22 09:39:37 UTC
I wonder whether the `...replace(',', '.');` in the [relevant change from Stephane's link](https://invent.kde.org/office/skrooge/-/commit/459a500b74fd03b315ffef2d3d86e3192b888b60#c1a20a2bfbced2220df9e5c56d88875af42dd052_389_380) might be problematic. Because of that, I think, both '.' and ',' are effectively used as decimal separator while only ' ' is valid (which probably no-one uses, yet, to be fair) and other than that only ',' should be used.

The downside of changing this would obviously be that with a German locale, an import from a non-German bank is likely to be incorrect. I am not sure how far heuristics can go here with everyone violating the standard, but probably everyone in a different way. Maybe everything internally should just use the locale and everything import-related should use the locale as default and allow overriding with a configuration knob.
Comment 5 Stephane MANKOWSKI 2025-01-23 20:49:23 UTC
Hi,

Here is my analysis:

    QLocale::setDefault(QLocale::German);
    SKGTEST(QStringLiteral("CONV:stringToDouble"), SKGServices::stringToDouble(QStringLiteral("1,234")), 1.234) => this test is OK
    SKGTEST(QStringLiteral("CONV:stringToDouble"), SKGServices::stringToDouble(QStringLiteral("1.234")), 1234) => this test is OK
    SKGTEST(QStringLiteral("CONV:stringToDouble"), SKGServices::stringToDouble(SKGServices::doubleToString(1.234)), 1.234) => this test is KO because doubleToString(1.234) return "1.234" and stringToDouble("1.234") return 1234.

This is why you have the issue in transaction edition, because the value 1,234 is converted into 1.234  when you type on enter or when the focus changes. I don't know how to fix that yet.
Comment 6 Stephane MANKOWSKI 2025-01-25 21:00:47 UTC
Git commit 33a51633fdc4af061d8251453eb439ac9d988bdb by Stéphane MANKOWSKI.
Committed on 25/01/2025 at 21:00.
Pushed by smankowski into branch 'master'.

Import fails for numbers with more than 3 decimal places

M  +1    -0    CHANGELOG
M  +8    -8    plugins/import/skrooge_import_csv/skgimportplugincsv.cpp
M  +2    -2    skgbasemodeler/skgservices.cpp
M  +2    -1    skgbasemodeler/skgservices.h
M  +5    -1    tests/skgbasemodelertest/skgtestbase.cpp

https://invent.kde.org/office/skrooge/-/commit/33a51633fdc4af061d8251453eb439ac9d988bdb
Comment 7 pat_h 2025-01-26 10:56:47 UTC
Thanks a lot! That looks like a sane implementation for an insane problem. Now let's wait until people start to import data from a foreign bank … :-)
Comment 8 Stephane MANKOWSKI 2025-01-26 12:11:03 UTC
Git commit 06d6edb7bc74412a31469a6bd916ac03f34dc1f1 by Stéphane MANKOWSKI.
Committed on 26/01/2025 at 12:10.
Pushed by smankowski into branch 'master'.

Import fails for numbers with more than 3 decimal places

M  +1    -0    tests/skgbasemodelertest/skgtestbase.cpp

https://invent.kde.org/office/skrooge/-/commit/06d6edb7bc74412a31469a6bd916ac03f34dc1f1
Comment 9 Christoph Vogtländer 2025-01-26 20:31:11 UTC
I just compiled the current master and can confirm that
1. downloading unit values with more than 3 decimal places via Stooq
2. manually entering numbers with more than two decimal places (e.g. 1,234) in "units" 
3. manually entering numbers with more than two decimal places (e.g. 1,234) in a transaction for an account using that unit

works as expected when using German localization. Thank you Stephane!