feat(import): add currency and number format support for CSV imports #1819
Reference in New Issue
Block a user
Delete Branch "main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #1713
/claim #1713
Before this change, importing a CSV with European-style numbers (like
1.234,56) would result in incorrect values because the parser would strip the decimal comma. Now, users can specify their number format and get accurate imports regardless of their region.Before: Would parse as
125454After: Correctly parses as
1254.54when European format is selected💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe.
Hey @danestves thanks for the contribution!
Overall, I think the code looks good.
Could we add a few tests cases to
import_interface_test.rbthat verifies:We may have to slightly alter the
import.rbmethods for better testability. We don't currently have any tests written forgenerate_rows_from_csv(or any of the methods that translate the config to the import rows). It would be great to have something that covers this area of the import process.I'm thinking we might want to rename this to
default_currency(and remove the method that's already in the model) because a user is allowed to add acurrencycolumn in their CSV and assign a mapping to that.If they explicitly provide us a currency column, we'll want to use that value first. If not, we use this default value.
Coming back to this after looking at #1814 and I'm realizing that we might need to entirely remove this
currencyconfiguration column and just stick with thenumber_format.Since the user can directly map one of their CSV columns to "Currency" and we're defaulting to the family currency anyways, I'm thinking this "Default currency" configuration option might cause more confusion than its worth.
So in summary, I think we need to:
currencycolumn and the associated configuration field in the formsnumber_formatconfig field side by side with the existing currency mapping columns added in #1814For sure! Let me work on that!
Changes have been made @zachgoll also I made a little change to
generate_rows_from_csv(and also added tests) we were usinginsert_all!which doesn't trigger validations or callbacks, and more importantly, doesn't return the created records, so:create!to properly create and return row recordsmap+ bulk insert to aneachloop with individual createsThat kept all the same field mappings, but now they'll be created with proper
ActiveRecordobjectsLooks great! Just left one small comment about method naming, but otherwise once tests are passing I think we'll be good to merge this one.
@@ -177,6 +185,39 @@ class Import < ApplicationRecordCan we set this to
set_default_number_formatnow that we've removed currency?@@ -177,6 +185,39 @@ class Import < ApplicationRecordThis is done, changed the method name, now we just need to wait for the tests to pass
@@ -177,6 +185,39 @@ class Import < ApplicationRecordAdded another commit since I forgot to change it in
before_validation@danestves make sure to create an Algora account to receive the bounty payout if you haven't already. Let me know if you run into any issues.
Hey, thanks! Yes is now been processed by Stripe to my bank account right now, thanks
This is a very promising update! However it does not work for me. For some reason, it just jumps back to the default of
1,234.56and ignores my selection. (at least i think so as it is on that value when i go back to "configure" after discovering the wrongly parsed numbers)30eur are formatted
30,00(– becoming3000for example)1100eur are formatted
1100,00@verymilan fix going in here:
https://github.com/maybe-finance/maybe/pull/1876