Changed managers patching

[Stranger6667]

I propose a solution to the problem, which I mentioned here #28.
It's based on class_prepared signal usage.

[toudi]

Hi. @Stranger6667 , If i read issue #28 correctly then the removal of objects is an intention, not a bug. However, if i do a simple model:

class Mymodel(models.Model):
  money = MoneyField( )

then it does not have objects attribute, so i cannot even do .objects.get() neither .objects.filter on my class.

For now, i'll do:

class Mymodel(models.Model):
  money = MoneyField()
  objects = models.Manager()

but this seems really unintuitive approach :/

or am i doing something wrong here?

[Stranger6667]

class Mymodel(models.Model):
  money = MoneyField( )

For me it leads to exception during object instantiation:

Mymodel.objects.create(money=Money("100.0", "USD"))
Exception: You have to provide a max_digits attribute to Money fields.

[Stranger6667]

I've added some tests for this situation

[toudi]

@Stranger6667 , sorry, i was trying to be quick with this snippet so i ommited max_digits and decimal_places.. :( but the bug persist nevertherless

[Stranger6667]

Can you add some code, please? to make it clear for me :)

[toudi]

@Stranger6667
Sure thing, sorry for not responding in such long while.

Here's the core excerpt:

from djmoney.models.fields import MoneyField
class MyModel(models.Model):
    amount = MoneyField(decimal_places=2, max_digits=10, default_currency='USD')

and the error in question:

$ pip freeze | grep django-money
django-money==0.3.3.2
$ find . -name '*.pyc' -delete
$ ./manage.py shell
Python 2.7.5 (default, Sep  6 2013, 09:55:21) 
[GCC 4.8.1 20130725 (prerelease)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from core.models import MyModel
>>> MyModel.objects.count()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AttributeError: type object 'MyModel' has no attribute 'objects'

However, when i just add the objects manager, everything works as expected.

class MyModel(models.Model):
    amount = MoneyField(decimal_places=2, max_digits=10, default_currency='USD')
    objects = models.Manager()

i believe (but this is just a long shot) that the problem may lie here: https://github.com/jakewins/django-money/blob/master/djmoney/models/fields.py#L173

       if getattr(cls, 'objects', None):
            cls.objects = money_manager(cls.objects)
            if cls.objects.model is None:
                cls.objects.model = cls

So if there is no 'objects' attribute, the following if statement will be ommited, leaving the class without even the most basic objects manager. I believe previously it was something like this:

        if getattr(cls, 'objects', None):
            cls.objects = money_manager(cls.objects)
        else:
            cls.objects = money_manager(models.Manager())

which caused the manager to be assigned regardless of whether it was there or not.

best regards ;)

[benjaoming]

This is the cause, right?

6505d26

[Stranger6667]

I think all this situation was caused by wrong choice of place of managers initialization. Let me explain myself.
Manager 'objects' added in ensure_default_manager from django.db.models.manager after model class initialization (via class_prepared signal). Other managers are initialized in ModelBase metaclass.
Using MoneyField, managers initialization may occur several times on the same model without any respect to inheritance. I don't know exactly where the error occurs, but these things are clearly related to mangers manipulations in contribute_to_class.

[benjaoming]

Thanks, @Stranger6667 for explaining. I think there was an issue somewhere about multiple MoneyFields on the same Model not working -- so that might be an explanation, too.

We could optimize it a bit by, say, not looping through the fields of the model, but simply looking for some _meta.has_money_field attribute that we set in contribute_to_class ?

    if getattr(sender._meta, '_has_money_field', False):
        for _id, name, manager in sender._meta.concrete_managers:
            setattr(sender, name, money_manager(manager))

[Stranger6667]

Seems very reasonable :) but it would be better with hasattr

[benjaoming]

Well alright then :)

[inureyes]

@benjaoming This code has a potential problem when model uses two or more managers. This code overwrites every manager to MoneyManager, which causes critical errors during migration.

e.g. If model has CurrentSiteManager as a field,

on_site = CurrentSiteManager()

makemigrations command generates the migration code to alter on_site manager from CurrentSiteManager to MoneyManager.

[benjaoming]

@inureyes that sounds reasonable, not sure if it's supposed to be so. Current code setattr(sender, name, money_manager(manager)) looks like it's supposed to alter a manager, not entirely overwrite it. Maybe the fix should be done in money_manager(manager) ? You're most welcome to do a PR, I think we have a reasonable test level to ensure that if tests are green, your code is good...

[inureyes]

@benjaoming Ok. I'll make a quick fix for that.

[inureyes]

#194

[Stranger6667]

I think we should cover this migration issue in tests. I can add them later, thank you for the report :)