Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a few number parsing issues #3044

Merged
merged 7 commits into from
Aug 4, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions packages/loot-core/src/server/accounts/parse-file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,18 +84,6 @@ describe('File import', () => {
expect(await getTransactions('one')).toMatchSnapshot();
}, 45000);

test('ofx import works with multiple decimals in amount', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't mean anything anymore since the file parser was returning a string for the value which would get converted later

const ofxFile = __dirname + '/../../mocks/files/data-multi-decimal.ofx';

const { transactions } = (await parseFile(ofxFile)) as {
transactions: { amount: number }[];
};

expect(transactions).toHaveLength(2);
expect(transactions[0].amount).toBe(-30.0);
expect(transactions[1].amount).toBe(-3.77);
}, 45000);

test('ofx import works (credit card)', async () => {
prefs.loadPrefs();
await db.insertAccount({ id: 'one', name: 'one' });
Expand Down
2 changes: 1 addition & 1 deletion packages/loot-core/src/server/accounts/parse-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ async function parseOFX(
errors,
transactions: data.transactions.map(trans => {
return {
amount: Number(trans.amount),
amount: trans.amount,
Copy link
Member Author

@youngcw youngcw Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont use the built in conversion, which cant handle other format types other than decimal comma.

imported_id: trans.fitId,
date: trans.date,
payee_name: trans.name || (useMemoFallback ? trans.memo : null),
Expand Down
12 changes: 12 additions & 0 deletions packages/loot-core/src/shared/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
expect(looselyParseAmount('3')).toBe(3);
expect(looselyParseAmount('3.4')).toBe(3.4);
expect(looselyParseAmount('3.45')).toBe(3.45);
// cant tell if this next case should be decimal or different format
// so we set as full numbers
expect(looselyParseAmount('3.456')).toBe(3456);
expect(looselyParseAmount('3.45000')).toBe(3.45);
expect(looselyParseAmount('3.450000')).toBe(3.45);
Expand All @@ -24,6 +26,15 @@
expect(looselyParseAmount('3,4500000')).toBe(3.45);
expect(looselyParseAmount('3,45000000')).toBe(3.45);
expect(looselyParseAmount('3,450000000')).toBe(3.45);
expect(looselyParseAmount("3'456.78")).toBe(3456.78);

Check warning on line 29 in packages/loot-core/src/shared/util.test.ts

View workflow job for this annotation

GitHub Actions / lint

Avoid using straight quotes (' or ") in user-visible text. Use curly quotes (‘ ’ or “ ”) instead
jfdoming marked this conversation as resolved.
Show resolved Hide resolved
expect(looselyParseAmount("3'456.78000")).toBe(3456.78);

Check warning on line 30 in packages/loot-core/src/shared/util.test.ts

View workflow job for this annotation

GitHub Actions / lint

Avoid using straight quotes (' or ") in user-visible text. Use curly quotes (‘ ’ or “ ”) instead
expect(looselyParseAmount('1,00,000.99')).toBe(100000.99);
expect(looselyParseAmount('1,00,000.99000')).toBe(100000.99);
});

test('looseParseAmount works with leading decimal characters', () => {
expect(looselyParseAmount('.45')).toBe(0.45);
expect(looselyParseAmount(',45')).toBe(0.45);
});

test('looseParseAmount works with negative numbers', () => {
Expand All @@ -42,6 +53,7 @@
// `3_45_23` (it needs a decimal amount). This function should be
// thought through more.
expect(looselyParseAmount('3_45_23.10')).toBe(34523.1);
expect(looselyParseAmount('(1 500.99)')).toBe(-1500.99);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for fun to be sure that spaces parse right

});

test('number formatting works with comma-dot format', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/loot-core/src/shared/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ export function looselyParseAmount(amount: string) {
// Look for a decimal marker, then look for either 1-2 or 5-9 decimal places.
// This avoids matching against 3 places which may not actually be decimal
const m = amount.match(/[.,]([^.,]{5,9}|[^.,]{1,2})$/);
if (!m || m.index === undefined || m.index === 0) {
if (!m || m.index === undefined) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may need some vetting. The reason for this is that the check I removed made it so a value like '.50' would return as $50. This will return the correct amount now even if there is no non-decimal digits.

return safeNumber(parseFloat(extractNumbers(amount)));
}

Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/3044.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Bugfix
authors: [youngcw,wdpk]
---

Fix decimal comma parsing for ofx files
Loading