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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ModelWithNullableCurrency): error was raised when trying to do a select #427

Merged
merged 1 commit into from Jun 27, 2018

Conversation

woile
Copy link
Contributor

@woile woile commented Jun 26, 2018

Hello everyone, I found this weird error, I also added a patch, but I would like someone to take a first look.

For the error observe the first test I've added:

def test_no_fail_all(self):
    money = Money(100, 'EUR')
    instance = ModelWithNullableCurrency(money=money)
    instance.save()
    r = ModelWithNullableCurrency.objects.all()
    assert r[0].money == money

After doing that an error like this appears:

~/projects/tmp/venv/lib/python3.6/site-packages/django/db/models/base.py in __init__(self, *args, **kwargs)
    500                 if val is _DEFERRED:
    501                     continue
--> 502                 _setattr(self, field.attname, val)
    503         else:
    504             # Slower, kwargs-ready version.

~/projects/tmp/venv/lib/python3.6/site-packages/djmoney/models/fields.py in __set__(self, obj, value)
    115         if value is not None and self.field._currency_field.null and not isinstance(value, MONEY_CLASSES):
    116             # For nullable fields we need either both NULL amount and currency or both NOT NULL
--> 117             raise ValueError('Missing currency value')
    118         if isinstance(value, BaseExpression):
    119             if isinstance(value, Value):

ValueError: Missing currency value

Steps to reproduce:

  1. Create instance an assigned a Money value
  2. Save instance
  3. Do a Model.objects.all()
  4. Observe "ValueError: Missing currency value"

@codecov-io
Copy link

codecov-io commented Jun 26, 2018

Codecov Report

Merging #427 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #427   +/-   ##
=======================================
  Coverage   97.55%   97.55%           
=======================================
  Files          29       29           
  Lines         900      900           
  Branches      153      153           
=======================================
  Hits          878      878           
  Misses         15       15           
  Partials        7        7
Impacted Files Coverage Δ
djmoney/models/fields.py 98.97% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad2b348...b5f5052. Read the comment docs.

@Stranger6667
Copy link
Collaborator

Stranger6667 commented Jun 27, 2018

Hello @woile !
Thank you so much for reporting the issue :) Your efforts are very appreciated!

Could you, please, rewrite the tests in the following way, to be more in line with the rest of the code?

    def test_query_not_null(self):
        money = Money(100, 'EUR')
        ModelWithNullableCurrency.objects.create(money=money)
        instance = ModelWithNullableCurrency.objects.get()
        assert instance.money == money

    def test_query_null(self):
        ModelWithNullableCurrency.objects.create()
        instance = ModelWithNullableCurrency.objects.get()
        assert instance.money is None
        assert instance.money_currency is None

And add an entry to the changelog?

Thank you

…select

Steps to reproduce:
1. Create instance an assigned a Money value
2. Save instance
3. Do a Model.objects.all()
4. Observe "ValueError: Missing currency value"
@woile
Copy link
Contributor Author

woile commented Jun 27, 2018

Done!

@Stranger6667 Stranger6667 merged commit 4386da4 into django-money:master Jun 27, 2018
@Stranger6667
Copy link
Collaborator

Thank you! :)

davidszotten added a commit to davidszotten/django-money that referenced this pull request Jan 19, 2022
TODO: is the currency field guaranteed to appear before the amount (and main)
field? yes because of the creation_counters?
@davidszotten davidszotten mentioned this pull request Jan 19, 2022
davidszotten added a commit to davidszotten/django-money that referenced this pull request Jan 23, 2022
TODO: is the currency field guaranteed to appear before the amount (and main)
field? yes because of the creation_counters?
SunShiny712 pushed a commit to SunShiny712/Django-moeny that referenced this pull request Aug 16, 2023
TODO: is the currency field guaranteed to appear before the amount (and main)
field? yes because of the creation_counters?
alexkoren-dev pushed a commit to alexkoren-dev/django_money that referenced this pull request Oct 3, 2023
TODO: is the currency field guaranteed to appear before the amount (and main)
field? yes because of the creation_counters?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants