Should be possible to change and save money back to default currency

[benjaoming]

#221

[benjaoming]

The following test breaks when changing the behavior:

    @pytest.mark.parametrize(
        'kwargs, currency',
        (
            ({'money_currency': 'PLN'}, 'PLN'),
            ({'money': Money(0, 'EUR')}, 'EUR')
        )
    )
    def test_get_or_create_respects_currency(self, kwargs, currency):
        instance, created = ModelWithVanillaMoneyField.objects.get_or_create(**kwargs)
>       assert str(instance.money.currency) == currency, 'currency should be taken into account in get_or_create'
E       AssertionError: currency should be taken into account in get_or_create
E       assert 'XYZ' == 'PLN'
E         - XYZ
E         + PLN

[benjaoming]

The explanation is already in an older comment:

         # we have to determine whether to replace the currency.
         # i.e. if we do the following:
         # .objects.get_or_create(money_currency='EUR')
         # then the currency is already set up, before this code hits
         # __set__ of MoneyField. This is because the currency field
         # has less creation counter than money field.

[benjaoming]

Any advice? Ideas?

[Stranger6667]

@benjaoming
I'll take a look tomorrow :)

[Stranger6667]

It seems like our builds are broken, I'll fix it tomorrow and rerun all PRs

[Stranger6667]

For me it seems perfectly fine! Probably we could just set currency like this:

    def prepare_value(self, obj, value):
        validate_money_value(value)
        currency = get_currency(value)
        if currency:
            setattr(obj, self.currency_field_name, currency)
        return self.field.to_python(value)

get_or_create fails only if there is no info about money field:

Good:

{'money': Money(0, 'EUR')}

Also good:

{'money': 10, 'money_currency': 'PLN'}

Bad:

{'money_currency': 'PLN'}

Now we manipulate with currency if currency is absent - we're setting default value for it.
Probably we should consider opposite situation, when we have only currency and no amount. We can just take default value for amount.
Something like this (models.managers.py):

from .fields import MoneyField, CurrencyField

...

def _expand_money_kwargs(model, args=(), kwargs=None, exclusions=()):
    """
    Augments kwargs so that they contain _currency lookups.
    """
    involved_fields = [_get_clean_name(name) for name in kwargs]
    for name, value in list(kwargs.items()):
        if name in exclusions:
            continue
        if isinstance(value, Money):
            clean_name = _get_clean_name(name)
            kwargs[name] = value.amount
            kwargs[get_currency_field_name(clean_name)] = smart_unicode(value.currency)
        else:
            field = _get_field(model, name)
            if isinstance(field, MoneyField):
                if isinstance(value, (BaseExpression, F)):
                    clean_name = _get_clean_name(name)
                    if not isinstance(value, F):
                        value = prepare_expression(value)
                    kwargs[get_currency_field_name(clean_name)] = F(get_currency_field_name(value.name))
                if is_in_lookup(name, value):
                    args += (_convert_in_lookup(model, name, value), )
                    del kwargs[name]
            elif isinstance(field, CurrencyField):
                money_field_name = name[:-9]  # Remove '_currency'
                if money_field_name not in involved_fields:
                    money_field = _get_field(model, money_field_name)
                    kwargs[money_field_name] = money_field.default.amount

    return args, kwargs

What do you think? All tests pass with these changes. I tried Django 1.4, 1.10.

[benjaoming]

@Stranger6667 thanks, seems to make sense -- so with the changes, it makes it possible to do all this?

MyModel.objects.filter(money_currency="USD")

...or was this functional even before the change?

[Stranger6667]

It was functional :)

[benjaoming]

Hmm, not sure :) would you like to write a sentence for the changelog? :)

[codecov-io]

Current coverage is 90.48% (diff: 100%)

Merging #227 into master will increase coverage by 0.07%

@@             master       #227   diff @@
==========================================
  Files            16         16          
  Lines           719        725     +6   
  Methods           0          0          
  Messages          0          0          
  Branches        136        139     +3   
==========================================
+ Hits            650        656     +6   
  Misses           48         48          
  Partials         21         21          

Powered by Codecov. Last update 1ac7116...5aaae14

[Stranger6667]

Don't know, it should be drop in replacement :) Also, I'll check more queries, which could be wrong

[Stranger6667]

Ah, I thought, that you mean some extra line about get_or_create behaviour.
Sure, I'll add some about this issue :) Thank you

[benjaoming]

Oh snap, I thought I had already written release notes for the fix... I did that only in the other PR yesterday.. I'll add something here...

[benjaoming]

Rebased and added release note

[Stranger6667]

Its probably should not be there :)

[Stranger6667]

I'll fix it

[benjaoming]

No indeed :)

[benjaoming]

Oh I just force-pushed the fix, wonder who made it first

[benjaoming]

You did ;)