mirumee / django-prices

Django fields for the prices module
158 stars 53 forks source link

Update prices to 1.0.0 beta 3 #53

Closed rafalp closed 6 years ago

rafalp commented 6 years ago

This is work in progress PR that updates our lib to beta 3, thus making it work with latest changes in our prices library.

TODO

patrys commented 6 years ago

Could we provide any useful validators? Some examples:

codecov-io commented 6 years ago

Codecov Report

Merging #53 into master will increase coverage by 1.41%. The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   94.73%   96.15%   +1.41%     
==========================================
  Files           3        4       +1     
  Lines          76      104      +28     
  Branches        5       10       +5     
==========================================
+ Hits           72      100      +28     
  Misses          2        2              
  Partials        2        2
Impacted Files Coverage Δ
django_prices/templatetags/prices_i18n.py 95.83% <100%> (-0.09%) :arrow_down:
django_prices/templatetags/prices.py 100% <100%> (ø) :arrow_up:
django_prices/validators.py 100% <100%> (ø)
django_prices/widgets.py 88.23% <71.42%> (-0.66%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 67bd17b...707aece. Read the comment docs.

patrys commented 6 years ago

Let's remove Django 2.0 from allowed failures, it's a stable release now.

rafalp commented 6 years ago

I've added MinMoneyValidator, MaxMoneyValidator and MoneyPrecisionValidator that check if money is at least, at most or precise enough. Each of those validators is just layer of abstraction above the Django's validator: MinValueValidator, MaxValueValidator and the DecimalValidator.

Min/Max only change to original is that they display formatted money in error message instead of Money(123, 'USD') and the MoneyPrecisionValidator will raise if you've got number of sub-units for currency wrong, eg 100.999 EUR. All of validators also throw with ValueErrors if you use them with different currencies, in spirit of the contract from prices.

rafalp commented 6 years ago

Following explicit better than implicit rule I've changed the MoneyPrecisionValidator to rely on passed decimal_places instead of resolving that on its own via asking babel how many decimal_places to use. This validator now closely follows DecimalValidator that it extends, except it works with Money and currencies instead of Decimals.

patrys commented 6 years ago

Doesn't that mean that the common use case will require the user to call babel manually?

I think a typical case would be to check that a value fits the minimum of:

rafalp commented 6 years ago

Doesn't that mean that the common use case will require the user to call babel manually?

Yeah. I've landed there after looking under the hood of Django's DecimalField handling. Here, they have nice Model -> Field -> Validator, passing decimal_places at all times. In my code it was Model -> Field, and then validator just disregards what's passed from Field it and goes for its own from currency. This felt like breaking the pattern and I've redid it to play by same rules everywhere.

I think a typical case would be to check that a value fits the minimum of:

This is good idea. Picking from there I'm wondering if there would be any downsides if we've simply madedecimal_places optional, only required when dealing with custom currency that babel fails for, eg. like dogecoins, otherwise being resolved automatically from your currency's name. This could be done in our model and form fields as well as in the validator.

patrys commented 6 years ago

I think the problem with decimal_places being optional is that it will be provided by Django by default. If that means ignoring the currency precision then I'm afraid in most common cases the currency will be ignored.

rafalp commented 6 years ago

All-right. Validator raises if either of those two are true:

rafalp commented 6 years ago

Another pass: changed validation on model field to pass Money to validators instead of Money.amount and also create MoneyPrecisionValidator for itself just like how DecimalField creates DecimalValidator for itself. I've also iterated on our tests suite a little.