-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
Standardise currency value storage as cents (StripeQuantumCurrencyAmountField) #955
Comments
Added #958 for a special case (probably would need a 3rd CurrencyAmountField class to support). |
@therefromhere I think it's time we finally bit the bullet and moved fully to cents. We might want to have |
Yeah, I'm happy for us to move towards storing cents instead of decimal dollars. So for now, shall we say that all new fields should be in cents (with comments on the fields to that effect), plus a note in the CHANGELOG about an upcoming migration of existing fields (probably for 3.0), and yeah we could look at adding *_cents and/or *_decimal properties on the models to help the migration. We should also check the help text of existing CurrencyAmount fields, since some of them are definitely misleading. I've not looked into this, but maybe subclassing Decimal and using that subclass for the base type of StripeDecimalCurrencyAmountField would also help with adding checks? Note that I think we should hold off on #958 since it's likely to cause more confusion since it's decimal but holding cents.
I'll mention this in discord. |
Resolves dj-stripe#972, part of dj-stripe#955
Resolves dj-stripe#972, part of dj-stripe#955
We are using the price like Currently for this small price we are having this exception when we are trying to print the price: /home/user/venv/lib/python3.7/site-packages/djstripe/models/core.py in __str__(self)
2242 subscriptions = Subscription.objects.filter(plan__id=self.id).count()
2243 if self.recurring:
-> 2244 return f"{self.human_readable_price} for {self.product.name} ({subscriptions} subscriptions)"
2245 return f"{self.human_readable_price} for {self.product.name}"
2246
/home/user/venv/lib/python3.7/site-packages/djstripe/models/core.py in human_readable_price(self)
2255 flat_amount_tier_1 = tier_1["flat_amount"]
2256 formatted_unit_amount_tier_1 = get_friendly_currency_amount(
-> 2257 tier_1["unit_amount"] / 100, self.currency
2258 )
2259 amount = f"Starts at {formatted_unit_amount_tier_1} per unit"
TypeError: unsupported operand type(s) for /: 'NoneType' and 'int' And the dictionary of the object is showing: ...
'billing_scheme': 'tiered',
'lookup_key': '',
'tiers': [{'flat_amount': 0,
'flat_amount_decimal': '0',
'unit_amount': None,
'unit_amount_decimal': '7.5',
'up_to': 5},
{'flat_amount': 0,
'flat_amount_decimal': '0',
'unit_amount': None,
'unit_amount_decimal': '3.8',
'up_to': None}],
'tiers_mode': 'volume',
'transform_quantity': None} |
Stripe handles all currency values as integer cents, but we currently have a mixture of storage as cent and as decimals.
StripeDecimalCurrencyAmountField
stores as decimal dollar (etc) values, ie $1.99 =Decimal("1.99")
)StripeQuantumCurrencyAmountField
stores as integer cents values, ie $1.99 =int(199)
Currently we have something like 16 Decimal fields and 12 Quantum fields (see below). 3 of the Quantum fields are new since 2.0 (those in PaymentIntent), so we could potentially switch them to decimal before 2.1 release.
We should have a documented policy on this (eg a "don't use this for new fields" note in the docstring of one of those field classes?) , and ultimately aim to switch all values to either cents or decimal.
Obviously migrating existing fields needs to be done very carefully so users don't accidentally send value 100x to large or small - I think the approach is a good idea -
dj-stripe/djstripe/models/core.py
Lines 727 to 728 in b247268
Thoughts @kavdev (cc @jleclanche )?
Related tickets: #247, #468
dump of current field usage, (editted from result
ack ".* = StripeDecimalCurrencyAmountField|^class [^\(]*" -o
on current master)Decimal:
Quantum:
The text was updated successfully, but these errors were encountered: