search

kipcole9 / money_sql

Money functions for the serialization of a money data type in Elixir
Other
28 stars 18 forks source link

Composite type cast does not respect locales #11

Closed olivermt closed 3 years ago

olivermt commented 3 years ago 
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 commented 3 years ago

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 commented 3 years ago

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

kipcole9 commented 3 years ago

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 commented 3 years ago

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

kipcole9 commented 3 years ago

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 :-)