-
Notifications
You must be signed in to change notification settings - Fork 543
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
money field for licence price #3559
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a migration?
UPD: Sorry — did not notice it somehow.
I added the migration while you were reviewing. Then I changed it again. Now it should be ok. |
name='price_currency', | ||
field=djmoney.models.fields.CurrencyField(max_length=3, default='XXX', editable=False, choices=[('XXX', '---'), ('AED', 'AED'), ('AFN', 'AFN'), ('ALL', 'ALL'), ('AMD', 'AMD'), ('ANG', 'ANG'), ('AOA', 'AOA'), ('ARS', 'ARS'), ('AUD', 'AUD'), ('AWG', 'AWG'), ('AZN', 'AZN'), ('BAM', 'BAM'), ('BBD', 'BBD'), ('BDT', 'BDT'), ('BGN', 'BGN'), ('BHD', 'BHD'), ('BIF', 'BIF'), ('BMD', 'BMD'), ('BND', 'BND'), ('BOB', 'BOB'), ('BOV', 'BOV'), ('BRL', 'BRL'), ('BSD', 'BSD'), ('BTN', 'BTN'), ('BWP', 'BWP'), ('BYN', 'BYN'), ('BYR', 'BYR'), ('BZD', 'BZD'), ('CAD', 'CAD'), ('CDF', 'CDF'), ('CHE', 'CHE'), ('CHF', 'CHF'), ('CHW', 'CHW'), ('CLF', 'CLF'), ('CLP', 'CLP'), ('CNY', 'CNY'), ('COP', 'COP'), ('COU', 'COU'), ('CRC', 'CRC'), ('CUC', 'CUC'), ('CUP', 'CUP'), ('CVE', 'CVE'), ('CZK', 'CZK'), ('DJF', 'DJF'), ('DKK', 'DKK'), ('DOP', 'DOP'), ('DZD', 'DZD'), ('EGP', 'EGP'), ('ERN', 'ERN'), ('ETB', 'ETB'), ('EUR', 'EUR'), ('FJD', 'FJD'), ('FKP', 'FKP'), ('GBP', 'GBP'), ('GEL', 'GEL'), ('GHS', 'GHS'), ('GIP', 'GIP'), ('GMD', 'GMD'), ('GNF', 'GNF'), ('GTQ', 'GTQ'), ('GYD', 'GYD'), ('HKD', 'HKD'), ('HNL', 'HNL'), ('HRK', 'HRK'), ('HTG', 'HTG'), ('HUF', 'HUF'), ('IDR', 'IDR'), ('ILS', 'ILS'), ('IMP', 'IMP'), ('INR', 'INR'), ('IQD', 'IQD'), ('IRR', 'IRR'), ('ISK', 'ISK'), ('JMD', 'JMD'), ('JOD', 'JOD'), ('JPY', 'JPY'), ('KES', 'KES'), ('KGS', 'KGS'), ('KHR', 'KHR'), ('KMF', 'KMF'), ('KPW', 'KPW'), ('KRW', 'KRW'), ('KWD', 'KWD'), ('KYD', 'KYD'), ('KZT', 'KZT'), ('LAK', 'LAK'), ('LBP', 'LBP'), ('LKR', 'LKR'), ('LRD', 'LRD'), ('LSL', 'LSL'), ('LTL', 'LTL'), ('LVL', 'LVL'), ('LYD', 'LYD'), ('MAD', 'MAD'), ('MDL', 'MDL'), ('MGA', 'MGA'), ('MKD', 'MKD'), ('MMK', 'MMK'), ('MNT', 'MNT'), ('MOP', 'MOP'), ('MRO', 'MRO'), ('MUR', 'MUR'), ('MVR', 'MVR'), ('MWK', 'MWK'), ('MXN', 'MXN'), ('MXV', 'MXV'), ('MYR', 'MYR'), ('MZN', 'MZN'), ('NAD', 'NAD'), ('NGN', 'NGN'), ('NIO', 'NIO'), ('NOK', 'NOK'), ('NPR', 'NPR'), ('NZD', 'NZD'), ('OMR', 'OMR'), ('PAB', 'PAB'), ('PEN', 'PEN'), ('PGK', 'PGK'), ('PHP', 'PHP'), ('PKR', 'PKR'), ('PLN', 'PLN'), ('PYG', 'PYG'), ('QAR', 'QAR'), ('RON', 'RON'), ('RSD', 'RSD'), ('RUB', 'RUB'), ('RWF', 'RWF'), ('SAR', 'SAR'), ('SBD', 'SBD'), ('SCR', 'SCR'), ('SDG', 'SDG'), ('SEK', 'SEK'), ('SGD', 'SGD'), ('SHP', 'SHP'), ('SLL', 'SLL'), ('SOS', 'SOS'), ('SRD', 'SRD'), ('SSP', 'SSP'), ('STD', 'STD'), ('SVC', 'SVC'), ('SYP', 'SYP'), ('SZL', 'SZL'), ('THB', 'THB'), ('TJS', 'TJS'), ('TMM', 'TMM'), ('TMT', 'TMT'), ('TND', 'TND'), ('TOP', 'TOP'), ('TRY', 'TRY'), ('TTD', 'TTD'), ('TVD', 'TVD'), ('TWD', 'TWD'), ('TZS', 'TZS'), ('UAH', 'UAH'), ('UGX', 'UGX'), ('USD', 'USD'), ('USN', 'USN'), ('UYI', 'UYI'), ('UYU', 'UYU'), ('UZS', 'UZS'), ('VEF', 'VEF'), ('VND', 'VND'), ('VUV', 'VUV'), ('WST', 'WST'), ('XAF', 'XAF'), ('XAG', 'XAG'), ('XAU', 'XAU'), ('XBA', 'XBA'), ('XBB', 'XBB'), ('XBC', 'XBC'), ('XBD', 'XBD'), ('XCD', 'XCD'), ('XDR', 'XDR'), ('XFO', 'XFO'), ('XFU', 'XFU'), ('XOF', 'XOF'), ('XPD', 'XPD'), ('XPF', 'XPF'), ('XPT', 'XPT'), ('XSU', 'XSU'), ('XTS', 'XTS'), ('XUA', 'XUA'), ('XYZ', 'XYZ'), ('YER', 'YER'), ('ZAR', 'ZAR'), ('ZMK', 'ZMK'), ('ZMW', 'ZMW'), ('ZWD', 'ZWD'), ('ZWL', 'ZWL'), ('ZWN', 'ZWN')]), | ||
), | ||
migrations.AlterField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this convert existing values with no data loss?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. MoneyField
extends DecimalField
. Precision and default value stay the same; field name (price
) also remains the same. In the database, currency is stored in a separate field (price_currency
). Also, in the REST API field price
looks the same as before; currency is displayed in a separate price_currency
fideld.
src/ralph/licences/models.py
Outdated
@@ -191,8 +192,8 @@ class Licence(Regionalizable, AdminAbsoluteUrlMixin, BaseObject): | |||
help_text="Leave blank if this licence is perpetual", | |||
) | |||
order_no = models.CharField(max_length=50, null=True, blank=True) | |||
price = models.DecimalField( | |||
max_digits=10, decimal_places=2, default=0, null=True, blank=True, | |||
price = MoneyField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the price_currency
field be also reflected on the model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field price_currency
is automatically created by the library. MoneyField
combines the two fields (price
and price_currency
) into one model.
@szymi- I've commented in a few places I'm concerned about since I'm not aware how this django-money works. If those concerns are invalid and the CI is happy I'll be happy to approve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szymi- I've commented in a few places I'm concerned about since I'm not aware how this django-money works. If those concerns are invalid and the CI is happy I'll be happy to approve.
Thanks. I considered creating a separate field for currency while keeping the original price
field, however this library does exactly the same in the background with some additional bonuses (most importantly, admin form and REST API presentation are dealt with by the library). I verified the library with exactly the issues you rose in mind - and came to the conclusion that it does the job well.
name='price_currency', | ||
field=djmoney.models.fields.CurrencyField(max_length=3, default='XXX', editable=False, choices=[('XXX', '---'), ('AED', 'AED'), ('AFN', 'AFN'), ('ALL', 'ALL'), ('AMD', 'AMD'), ('ANG', 'ANG'), ('AOA', 'AOA'), ('ARS', 'ARS'), ('AUD', 'AUD'), ('AWG', 'AWG'), ('AZN', 'AZN'), ('BAM', 'BAM'), ('BBD', 'BBD'), ('BDT', 'BDT'), ('BGN', 'BGN'), ('BHD', 'BHD'), ('BIF', 'BIF'), ('BMD', 'BMD'), ('BND', 'BND'), ('BOB', 'BOB'), ('BOV', 'BOV'), ('BRL', 'BRL'), ('BSD', 'BSD'), ('BTN', 'BTN'), ('BWP', 'BWP'), ('BYN', 'BYN'), ('BYR', 'BYR'), ('BZD', 'BZD'), ('CAD', 'CAD'), ('CDF', 'CDF'), ('CHE', 'CHE'), ('CHF', 'CHF'), ('CHW', 'CHW'), ('CLF', 'CLF'), ('CLP', 'CLP'), ('CNY', 'CNY'), ('COP', 'COP'), ('COU', 'COU'), ('CRC', 'CRC'), ('CUC', 'CUC'), ('CUP', 'CUP'), ('CVE', 'CVE'), ('CZK', 'CZK'), ('DJF', 'DJF'), ('DKK', 'DKK'), ('DOP', 'DOP'), ('DZD', 'DZD'), ('EGP', 'EGP'), ('ERN', 'ERN'), ('ETB', 'ETB'), ('EUR', 'EUR'), ('FJD', 'FJD'), ('FKP', 'FKP'), ('GBP', 'GBP'), ('GEL', 'GEL'), ('GHS', 'GHS'), ('GIP', 'GIP'), ('GMD', 'GMD'), ('GNF', 'GNF'), ('GTQ', 'GTQ'), ('GYD', 'GYD'), ('HKD', 'HKD'), ('HNL', 'HNL'), ('HRK', 'HRK'), ('HTG', 'HTG'), ('HUF', 'HUF'), ('IDR', 'IDR'), ('ILS', 'ILS'), ('IMP', 'IMP'), ('INR', 'INR'), ('IQD', 'IQD'), ('IRR', 'IRR'), ('ISK', 'ISK'), ('JMD', 'JMD'), ('JOD', 'JOD'), ('JPY', 'JPY'), ('KES', 'KES'), ('KGS', 'KGS'), ('KHR', 'KHR'), ('KMF', 'KMF'), ('KPW', 'KPW'), ('KRW', 'KRW'), ('KWD', 'KWD'), ('KYD', 'KYD'), ('KZT', 'KZT'), ('LAK', 'LAK'), ('LBP', 'LBP'), ('LKR', 'LKR'), ('LRD', 'LRD'), ('LSL', 'LSL'), ('LTL', 'LTL'), ('LVL', 'LVL'), ('LYD', 'LYD'), ('MAD', 'MAD'), ('MDL', 'MDL'), ('MGA', 'MGA'), ('MKD', 'MKD'), ('MMK', 'MMK'), ('MNT', 'MNT'), ('MOP', 'MOP'), ('MRO', 'MRO'), ('MUR', 'MUR'), ('MVR', 'MVR'), ('MWK', 'MWK'), ('MXN', 'MXN'), ('MXV', 'MXV'), ('MYR', 'MYR'), ('MZN', 'MZN'), ('NAD', 'NAD'), ('NGN', 'NGN'), ('NIO', 'NIO'), ('NOK', 'NOK'), ('NPR', 'NPR'), ('NZD', 'NZD'), ('OMR', 'OMR'), ('PAB', 'PAB'), ('PEN', 'PEN'), ('PGK', 'PGK'), ('PHP', 'PHP'), ('PKR', 'PKR'), ('PLN', 'PLN'), ('PYG', 'PYG'), ('QAR', 'QAR'), ('RON', 'RON'), ('RSD', 'RSD'), ('RUB', 'RUB'), ('RWF', 'RWF'), ('SAR', 'SAR'), ('SBD', 'SBD'), ('SCR', 'SCR'), ('SDG', 'SDG'), ('SEK', 'SEK'), ('SGD', 'SGD'), ('SHP', 'SHP'), ('SLL', 'SLL'), ('SOS', 'SOS'), ('SRD', 'SRD'), ('SSP', 'SSP'), ('STD', 'STD'), ('SVC', 'SVC'), ('SYP', 'SYP'), ('SZL', 'SZL'), ('THB', 'THB'), ('TJS', 'TJS'), ('TMM', 'TMM'), ('TMT', 'TMT'), ('TND', 'TND'), ('TOP', 'TOP'), ('TRY', 'TRY'), ('TTD', 'TTD'), ('TVD', 'TVD'), ('TWD', 'TWD'), ('TZS', 'TZS'), ('UAH', 'UAH'), ('UGX', 'UGX'), ('USD', 'USD'), ('USN', 'USN'), ('UYI', 'UYI'), ('UYU', 'UYU'), ('UZS', 'UZS'), ('VEF', 'VEF'), ('VND', 'VND'), ('VUV', 'VUV'), ('WST', 'WST'), ('XAF', 'XAF'), ('XAG', 'XAG'), ('XAU', 'XAU'), ('XBA', 'XBA'), ('XBB', 'XBB'), ('XBC', 'XBC'), ('XBD', 'XBD'), ('XCD', 'XCD'), ('XDR', 'XDR'), ('XFO', 'XFO'), ('XFU', 'XFU'), ('XOF', 'XOF'), ('XPD', 'XPD'), ('XPF', 'XPF'), ('XPT', 'XPT'), ('XSU', 'XSU'), ('XTS', 'XTS'), ('XUA', 'XUA'), ('XYZ', 'XYZ'), ('YER', 'YER'), ('ZAR', 'ZAR'), ('ZMK', 'ZMK'), ('ZMW', 'ZMW'), ('ZWD', 'ZWD'), ('ZWL', 'ZWL'), ('ZWN', 'ZWN')]), | ||
), | ||
migrations.AlterField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. MoneyField
extends DecimalField
. Precision and default value stay the same; field name (price
) also remains the same. In the database, currency is stored in a separate field (price_currency
). Also, in the REST API field price
looks the same as before; currency is displayed in a separate price_currency
fideld.
src/ralph/licences/models.py
Outdated
@@ -191,8 +192,8 @@ class Licence(Regionalizable, AdminAbsoluteUrlMixin, BaseObject): | |||
help_text="Leave blank if this licence is perpetual", | |||
) | |||
order_no = models.CharField(max_length=50, null=True, blank=True) | |||
price = models.DecimalField( | |||
max_digits=10, decimal_places=2, default=0, null=True, blank=True, | |||
price = MoneyField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field price_currency
is automatically created by the library. MoneyField
combines the two fields (price
and price_currency
) into one model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for resolving my concerns. LGTM
@szymi- I've just got one more concern -- does this change the format of an existing object field that Ralph returns via the API? |
Field |
@romcheg @MarekBleschke I am considering unifying |
Yes, unifying price field in all models IMO is the way to go. |
from factory.django import DjangoModelFactory | ||
from factory.fuzzy import FuzzyText | ||
from factory.fuzzy import FuzzyDecimal, FuzzyText |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FuzzyDecimal
looks unused
No description provided.