Currency not being updated

[nigelcopley]

I've been experiencing some strange behaviour when trying to update MoneyField in bulk updates. I have a product model which may store products with money fields in different currencies so i have been using nullable money fields. The problem arises when trying to update the MoneyField after the product has already been created.

I've tried to outline the steps below to reproduce, hope it helps.

Stack
Django==4.0
django-money==2.1.1
postgres 14

Model

class Product(models.Model):
    price = MoneyField(max_digits=14, decimal_places=2, null=True, blank=True)
    cost_of_goods_sold = MoneyField(max_digits=14, decimal_places=2, default_currency=None, null=True, blank=True)
    sale_price = MoneyField(max_digits=14, decimal_places=2, default_currency=None, null=True, blank=True)

Create & Update
Normal operation on single items seems to work fine

money = Money('13.99', 'GBP')

test = Product.objects.create(
    price=Money('99.99', 'GBP'),
    sale_price=None
)

test_2 = Product.objects.create(
    price=Money('39.99', 'GBP'),
)

test.sale_price = money
test.save()
test_2.sale_price = money
test_2.save()

Bulk Update
This results in the amount being updated but not the currency, the same occurs when using Product.objects.bulk_update()

import random
for i in range(1, 35):
    Product.objects.create(
        price=Money(round(random.uniform(42.99, 99.99), 2), 'GBP')
    )
test_products = Product.objects.filter(sale_price=None)
test_products.update(sale_price=money)

test_products = Product.objects.filter(sale_price__gt=0)
for product in test_products:
    print(product.sale_price)

raises CurrencyDoesNotExist(code) as there is an amount but no currency
moneyed.classes.CurrencyDoesNotExist: No currency with code NONE is defined.

Bulk Create with Money Field

for i in range(1, 35):
    identifier = ''.join(random.choices(string.ascii_letters + string.digits, k=3))
    Product.objects.create(
        source_id=3,
        identifier='test-random' + identifier ,
        price=Money(round(random.uniform(42.99, 99.99), 2), 'GBP'),
        sale_price=money
    )
test_products = Product.objects.filter(sale_price__gt=0, sale_price_currency__isnull=False)
for product in test_products:
    print(product.sale_price)

returns expected value £13.99

**Updating to change currency**
money_new_currency = Money('13.99', 'USD')
test_products.update(sale_price=money_new_currency)
for product in test_products:
    print(product.sale_price)
returns unexpected value £13.99

Updating individual entries

test.sale_price=money_new_currency
test.sale_price.save()
print(test.sale_price)  = Money('13.99', 'USD')

[nitsujri]

A year old but...

This expected behavior. You're using .update in both scenarios which performs DB updates directly in Django. This isn't a quirk of DJMoney it's an expected behavior of Django itself.

DJMoney uses __set__ to perform currency updates which means it has to go through object.sale_price = as you show in your "updating individual entries"

If you want to use .update to perform bulk work you have to manage the currency field manually.

[benjaoming]

@nitsujri I think you are right. Maybe django-money should be doing a log.warning or similar if possible from the .update(), rather than magic?

Or maybe it should try to automatically map field names provided to .update() calls so the *_currency field is added if the value provided is a Money object?

I'd try to argue to not use magic here, just because people might have more complex queries for their update statements and maybe it's best to just emit the warning.

[nitsujri]

I'm probably talking beyond my paygrade here as I'm relatively new to Django, but I'm not sure if that is realistically possible or even if it should be done.

Looking at how .update works there's no direct hooks/effects/easy entry?

But more philosophically, I don't believe DJMoney needs to take this on. While it's "surprising and weird" to new comers to Django, it's very much a Django design choice. You go .update when you need speed and bulk because you're not marshalling Django Objects which is where all the processing slowness lives in bulk updates. Having a warning there is really annoying if you're properly using this piece of code.

Admittedly, I'm very familiar with from Rails (.update performs validations+effects and .update_columns is pure DB) and it bites people there as well. To be fair, here in Django, .update is named in such a way that does seem like it should perform effects.

[Edit]
Personally, I recommend closing this.

[benjaoming]

Looking at how .update works there's no direct hooks/effects/easy entry?

This is a good point. I can only say that from experience, "when there is a will, there is a way". But most likely, the most stable way will be to just try to warn the developer.

Having a warning there is really annoying if you're properly using this piece of code.

Definitely good to only display relevant warnings or offer a way to silence them when doing deliberate actions 👍

[benjaoming]

Personally, I recommend closing this.

This is a good action of no one comes up with a PR :) I wouldn't rule out that if it is reasonably problematic for someone and they want to propose a way forward and add test cases, then it's fine.. otherwise, yeah seems likely that this will be closed as a #wontfix.

[nitsujri]

@benjaoming in Rails we use the linter, Rubocop, it has a specific rule against this:

https://docs.rubocop.org/rubocop-rails/cops_rails.html#railsskipsmodelvalidations

Following this, the recommendation becomes getting it with Flake8 or Black. Flake8 has this library:

https://github.com/rocioar/flake8-django

Contributing an .update would be good?