mangopay2-python-sdk icon indicating copy to clipboard operation
mangopay2-python-sdk copied to clipboard

[Question] Can we improve representation of `Money`?

Open samitnuk opened this issue 4 years ago • 4 comments
trafficstars

For example, I have Pay-In with the next data (screenshot from the dashboard): image

When I check this Pay-In in the terminal I have:

In [3]: p = DirectDebitWebPayIn.get('xxxxxxxxxx')

In [4]: p.debited_funds
Out[4]: EUR 18043

In [5]: p.fees
Out[5]: EUR 6542

From my point of view, these values should be represented as EUR 180.43 and EUR 65.42 (as in the dashboard).

To fix this I can suggest to use self.amount / 100 instead of self.amount here: https://github.com/Mangopay/mangopay2-python-sdk/blob/2a656a8a50597e3ba6b991c4f01b6a98e2e47793/mangopay/utils.py#L62-L63 and here: https://github.com/Mangopay/mangopay2-python-sdk/blob/2a656a8a50597e3ba6b991c4f01b6a98e2e47793/mangopay/utils.py#L65-L66

But of course, this "fix" will work only for the currencies with 2 digits after the decimal separator. So maybe there can be a better solution.

Also, I understand that Money in the SDK represents An amount of money in the smallest sub-division of the currency, so showing 18043 instead of 180.43 can be correct from some point of view. But we show currency near it and this can be confusing.

But I think the minimum that can be done here is to use for __str__ the same functionality as for __repr__ (means remove :,.2f. For example, when I want to log Pay-In values I have something like next in my logs:

Booking #000000001: Created Pay-In #0000000002, debited funds - EUR 18,043.00, fees - EUR 6,542.00

As you can see, the __str__ method is definitely produces an incorrect representation of the values from above.

samitnuk avatar Sep 30 '21 10:09 samitnuk

In [3]: preauthorization.debited_funds
Out[3]: EUR 2495

In [4]: str(preauthorization.debited_funds)
Out[4]: 'EUR 2,495.00'

In [5]: vars(preauthorization.debited_funds)
Out[5]: {'amount': Decimal('2495'), 'currency': 'EUR'}

But the correct number to show is 24.95. So to show the "blocked" amount of money for a user we need somehow format this value by ourselves. E.g.

In [6]: preauthorization.debited_funds.amount / 100
Out[6]: Decimal('24.95')

or

In [7]: preauthorization.debited_funds / 100
Out[7]: EUR 24.95

samitnuk avatar Nov 17 '21 11:11 samitnuk

It is not always correct to divide by 100 because not all currencies have two decimals, see ISO 4217.

In fact, there's no need to use Decimal here at all and it can get confusing as the Mangopay documentation indicates that int type is always used for MoneyCard amounts:

An amount of money in the smallest sub-division of the currency, e.g. 12.60 EUR 
would be represented as 1260 whereas 12 JPY would be represented as just 12

So the easiest option is to just use int here, and avoid the {:,.2f} format which introduces a lot of confusion.

If you want to improve the Money class, it makes sense to use Decimal internally, but the amount should be corrected accordingly to the number of decimals, like amount = Decimal(amount) / (10 **currency.decimals). But for that ISO 4217 support is needed (some packages exists).

yoch avatar Mar 14 '23 14:03 yoch

In our project we are actually patching MoneyField to hide the division/multiplication as an implementation detail instead of sprinkling them all over our codebase. Is there any specific reason why customer implementations are made responsible to deal with this? In my view the mangopay SDK's have all the information they need to hide this. I think also not hiding this is a dangerous vector for client codebases to introduce bugs on their end.

shanx avatar Jun 30 '23 08:06 shanx

Not reason to not encapsulate this dance inside the SDK, I mean, mango.Money(amount=Decimal('10.10'), currency='EUR') means actually 0.10€ :joy:

jpic avatar Mar 27 '24 09:03 jpic