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

Composite type cast does not respect locales #11

Closed
olivermt opened this issue Jan 28, 2021 · 5 comments
Closed

Composite type cast does not respect locales #11

olivermt opened this issue Jan 28, 2021 · 5 comments

Comments

@olivermt
Copy link

iex(5)> Money.Ecto.Composite.Type.cast(%{currency: :NOK, amount: "218,75"})
** (WithClauseError) no with clause matching: {:error, "218,75"}
    (ex_money_sql 1.3.1) lib/money/ecto/money_ecto_composite_type.ex:62: Money.Ecto.Composite.Type.cast/1
iex(5)> Money.Ecto.Composite.Type.cast(%{currency: :NOK, amount: "218.75"})
{:ok, #Money<:NOK, 218.75>}
iex(6)> Money.new(:NOK, "218,72")
#Money<:NOK, 218.72>

The perpetrator is here:
https://github.com/kipcole9/money_sql/blob/master/lib/money/ecto/money_ecto_composite_type.ex#L52

The step trying to parse Decimal is unneccessary as Money does its own validation (which is also Locale aware).
So it will error out on invalid decimals (unknown strings or float source) and as such should not require this extra step with the with clause.

It's a pretty trivial change, do you just wanna plop the code change in and release new version or do you want me to send a PR?

@kipcole9
Copy link
Owner

Thanks for the report. And apologies for the slow response - I'm not getting issue notifications from GitHub for this repo it seems.

As of 3cfa502..324c691 I believe the issue is fixed, basically in the way you suggested by letting Money.new/3 take care of parsing.

@olivermt
Copy link
Author

Awesome! Whats your release schedule for this? Is it possible to backport into a tag and get a new minor version out?

@kipcole9
Copy link
Owner

I've published ex_money version 5.5.0 and ex_money_sql version 1.4.0.

Does that work for you? If you have version constraint and are looking for a back port, just let me know and I'll see what I can do. It's midnight my timezone now so I may not get to it for another few hours.

Apologies again for the slow response, I think I've fixed GitHub notifications now.

@olivermt
Copy link
Author

No not at all, I for some reason thought you didnt release a new version.

@kipcole9
Copy link
Owner

There is only one test case currently that uses exactly your example but I think covers the issue.

I was waiting to see if there was any feedback on a new feature added in ex_money but given how late I have been on the two issues here I just published anyway :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants