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.

Sign up for GitHub

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

Jump to bottom

Bug: CSV number formats are not sanitized/parsed correctly for non-USD currencies #1713

Open
zachgoll opened this issue Jan 27, 2025 · 6 comments · May be fixed by #1819 or #1820
Open

Bug: CSV number formats are not sanitized/parsed correctly for non-USD currencies #1713

zachgoll opened this issue Jan 27, 2025 · 6 comments · May be fixed by #1819 or #1820
Labels
💎 Bounty Community This is a great issue for community members to work on 2️⃣ Medium Priority Community contributions accepted, Maybe team only works on if there are no high priority items open

Comments

@zachgoll
Copy link
Collaborator

zachgoll commented Jan 27, 2025 •
edited
Loading

Currently, we're using a naive sanitization function to deal with CSV import amount string values that have non-numeric characters in them.

maybe/app/models/import.rb

Lines 146 to 149 in d2a7aef

def sanitize_number(value)
return "" if value.nil?
value.gsub(/[^\d.\-]/, "")
end

This causes problems with a CSV where the currency is say, EUR, and the delimiter/separator are reverse of our assumed USD case:

Date,Name,Amount
2023-05-01,Grocery Store,1.254,54

The CSV above will NOT be imported correctly since the , will be stripped away and it will become 125454

Instead, we should be asking the user for:

  • Currency of transactions in the import
  • Separator/Delimiter for each amount value

Based on these options, the CSV import should default to the selected currency and sanitize inputs based on the separator/delimiter option, where the possible currencies are found in config/currencies.yml and the possible separator/delimiter options are:

NUMBER_FORMATS = {
  "1,234.56" => { separator: ".", delimiter: "," },  # US/UK/Asia
  "1.234,56" => { separator: ",", delimiter: "." },  # Most of Europe
  "1 234,56" => { separator: ",", delimiter: " " },  # French/Scandinavian
  "1,234"    => { separator: "",  delimiter: "," }   # Zero-decimal currencies like JPY
}

Fields

We need to add these two fields (currency, format) to the configuration screen for a CSV import:

Image
@zachgoll zachgoll mentioned this issue Jan 27, 2025
Open
2 tasks
@zachgoll zachgoll added 2️⃣ Medium Priority Community contributions accepted, Maybe team only works on if there are no high priority items open Community Community This is a great issue for community members to work on and removed Community labels Jan 27, 2025
@javiermartingonzalez
Copy link

Same here, I created a custom script to format the amount field from my different banks. It gets the CSV as input, removes thousand "." separator or space, and then changes from "," to "." for the decimal part. Finally the output CSV is the one I'm uploading to Maybe,.

But as you said, like it's in the date, will be good to have a format selector for amount

@Shpigford
Copy link
Member

/bounty $300

@algora-pbc Algora PBC
Copy link

algora-pbc bot commented Feb 6, 2025 •
edited
Loading

💎 $300 bounty • Maybe

Steps to solve:

  1. Start working: Comment /attempt #1713 with your implementation plan
  2. Submit work: Create a pull request including /claim #1713 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to maybe-finance/maybe!

Add a bounty • Share on socials

Attempt Started (GMT+0) Solution
🟢 @danestves Feb 7, 2025, 12:27:59 AM #1819
🟢 @onyedikachi-david Feb 7, 2025, 5:57:09 AM #1820

@hunxjunedo
Copy link

@zachgoll that's understandable but looks like when the comma is stripped away, it doesn't consider the numbers after the comma to be a part of the amount. Do we already know about this ?Its pretty obvious,

@danestves
Copy link

danestves commented Feb 7, 2025 •
edited by algora-pbc bot
Loading

/attempt #1713 will start looking at this tomorrow and Saturday I already deal with this kind of things when implementing VISA payments so will drop a PR in a couple of days

Options

@danestves danestves linked a pull request Feb 7, 2025 that will close this issue
Open
@onyedikachi-david
Copy link

onyedikachi-david commented Feb 7, 2025 •
edited by algora-pbc bot
Loading

/attempt #1713

Algora profile Completed bounties Tech Active attempts Options
@onyedikachi-david    1 maybe-finance bounty
+ 16 bounties from 6 projects
TypeScript, Python,
Rust & more
Cancel attempt

@onyedikachi-david onyedikachi-david linked a pull request Feb 7, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
💎 Bounty Community This is a great issue for community members to work on 2️⃣ Medium Priority Community contributions accepted, Maybe team only works on if there are no high priority items open
Projects
None yet
Milestone
No milestone
6 participants
@Shpigford @zachgoll @javiermartingonzalez @danestves @onyedikachi-david @hunxjunedo