Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

The default amount should be set via defaults #257

Merged

Conversation

kjagiello
Copy link
Contributor

@kjagiello kjagiello commented Jan 31, 2017

I've noticed a regression in the codebase, introduced in #227 (a09a2d4 to be exact). It broke the following use case of mine:

class Wallet(UUIDModel):
    owner_id = models.UUIDField()
    balance = MoneyField(
        default=Money(0, settings.DEFAULT_CURRENCY),
        max_digits=10,
        decimal_places=2,
        default_currency=settings.DEFAULT_CURRENCY
    )

    class Meta:
        unique_together = ('owner_id', 'balance_currency',)

Every user can own multiple wallets with different currencies, thus the UNIQUE index on both owner_id and balance_currency.

The problem lies in the changes to the behaviour of _expand_money_kwargs. They made following code snippet...

>>> instance = ModelWithUniqueIdAndCurrency.objects.create(money=Money(0, 'SEK'))
>>> Wallet.objects.get_or_create(owner_id=instance.id, balance_currency='SEK')

...produce following query and error:

query = 'INSERT INTO "testapp_modelwithuniqueidandcurrency" ("id", "money_currency", "money") SELECT ?, ?, ?', params = (1, 'SEK', '123.00')

    def execute(self, query, params=None):
        if params is None:
            return Database.Cursor.execute(self, query)
        query = self.convert_query(query)
>       return Database.Cursor.execute(self, query, params)
E       django.db.utils.IntegrityError: UNIQUE constraint failed: testapp_modelwithuniqueidandcurrency.id

_expand_money_kwargs set the value of money to the default value of the field, which should not be the correct behaviour.

This pull requests fixes the issues with all the tests passing. It is however a quickfix and feels a bit hacky. I feel also like _expand_money_kwargs is a bit messy and there may be more bugs hiding in there. You are welcome to review the change and suggest improvements! 馃檶馃徎

@Stranger6667
Copy link
Collaborator

Hello @kjagiello !
I'm happy to see this PR :) I'll add a release note

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants