jrief / django-angular

Let AngularJS play well with Django
http://django-angular.awesto.com/
MIT License
1.23k stars 293 forks source link

Cant pass instance to ModelForm inheriting when using NgModelFormMixin #122

Closed gbozee closed 8 years ago

gbozee commented 10 years ago

I keep on getting the following error in my test suit when passing an instance to my Form class.

class TutorProfileForm(NgModelFormMixin,forms.ModelForm):
    form_name = 'tutor_profile_form'
    scope_prefix = 'tutor'

    video = EmbedVideoFormField(required=False)
    identification = CloudinaryJsFileField(label=_('ID Verification'))
    terms = forms.BooleanField(widget=forms.CheckboxInput,required=True,
                          label=_('I accept the terms and conditions'))

    class Meta:
        model = Tutor
        exclude = ['skills', 'profile', 'verified']

I am initializing my form like this

def create_tutor_profile_form(self, tutor, data=None):
    if data is None:
        data = {
            'identification':
                'image/upload/v1414503418/preset_folder/ypm375l05wonqhzy5zvv.png#23f903762afef9f9872873a6be9538877b1f7f57',
            'terms': True,
            'years_of_teaching': 5,
            'response_time': '1 hour',
            'description': 'The best programmer to be your teacher',
            'travel_policy': 'near',
            'cancellation_policy': 12,
            'penalty': 30
        }
    return TutorProfileForm(data, instance=tutor)

but any time i run any test using the above method, I get the following error.

return TutorProfileForm(data, instance=tutor)
TypeError: __new__() takes exactly 1 argument (3 given)

I traced this error down to the base class NgFormBaseMixin

def __new__(cls, **kwargs):
    field_mixins_module = import_module(cls.field_mixins_module)
    field_mixins_fallback_module = import_module(cls.field_mixins_fallback_module)
    new_cls = super(NgFormBaseMixin, cls).__new__(cls)
    # add additional methods to django.form.fields at runtime
    for field in new_cls.base_fields.values():
        FieldMixinName = field.__class__.__name__ + 'Mixin'
        try:
            FieldMixin = getattr(field_mixins_module, FieldMixinName)
        except AttributeError:
            try:
                FieldMixin = getattr(field_mixins_fallback_module, FieldMixinName)
            except AttributeError:
                FieldMixin = field_mixins_fallback_module.DefaultFieldMixin
        field.__class__ = type(field.__class__.__name__, (field.__class__, FieldMixin), {})
    return new_cls

Any help would be greatly appreciated.

jrief commented 10 years ago

Can you please try to apply this patch:

diff --git a/djangular/forms/angular_base.py b/djangular/forms/angular_base.py
index af2dae5..30df4e4 100644
--- a/djangular/forms/angular_base.py
+++ b/djangular/forms/angular_base.py
@@ -160,10 +160,10 @@ class NgFormBaseMixin(object):
     field_error_css_classes = 'djng-field-errors'
     field_mixins_module = field_mixins_fallback_module = 'djangular.forms.field_mixins'

-    def __new__(cls, **kwargs):
+    def __new__(cls, *args, **kwargs):
         field_mixins_module = import_module(cls.field_mixins_module)
         field_mixins_fallback_module = import_module(cls.field_mixins_fallback_module)
-        new_cls = super(NgFormBaseMixin, cls).__new__(cls)
+        new_cls = super(NgFormBaseMixin, cls).__new__(cls, *args, **kwargs)
         # add additional methods to django.form.fields at runtime
         for field in new_cls.base_fields.values():
             FieldMixinName = field.__class__.__name__ + 'Mixin'

yes, I'd prefer to call new as __new__(cls, name, bases, attrs) but some weird reasons this does not work.

gbozee commented 10 years ago

Thanks for the above patch. Worked like a charm

jrief commented 10 years ago

This is not fixed yet.

adrienbrunet commented 10 years ago

What is the exact status of this issue? The above patch seems to work for me as well but what are you planning to code? __new__(cls, name, bases, attrs) is the aim?

Any help needed? If yes, do you have a branch or some tests to show what should actually be done? (I need that part to be fixed...)

adrienbrunet commented 10 years ago

To use __new__(cls, name, bases, attrs), we shoud useclass NgFormBaseMixin(type):``instead ofclass NgFormBaseMixin(object):` right? I've never encountered such modification before so I'm not sure what is the next step but I guess then, this Mixin should actually be called in another way (metaclass). As it's the first time I'm using that kind of things, I may be plain wrong. If you have some guidances... I'm listening!

jrief commented 10 years ago

@adrienbrunet sure! Thank for reporting. Inheritance from the mixin should be done using six.with_metaclass(...). This will be fixed in 0.7.10. I hope to find some time soon.

adrienbrunet commented 10 years ago

I tried:

class NgFormBaseMeta(type):
     field_mixins_module = field_mixins_fallback_module = 'djangular.forms.field_mixins'

    def __new__(cls, name, bases, attrs):
        field_mixins_module = import_module(cls.field_mixins_module)
        field_mixins_fallback_module = import_module(cls.field_mixins_fallback_module)
        new_cls = super(NgFormBaseMeta, cls).__new__(cls, name, bases, attrs)
        # add additional methods to django.form.fields at runtime
        for field in new_cls.base_fields.values():
            FieldMixinName = field.__class__.__name__ + 'Mixin'
            try:
                FieldMixin = getattr(field_mixins_module, FieldMixinName)
            except AttributeError:
                try:
                    FieldMixin = getattr(field_mixins_fallback_module, FieldMixinName)
                except AttributeError:
                    FieldMixin = field_mixins_fallback_module.DefaultFieldMixin
            field.__class__ = type(field.__class__.__name__, (field.__class__, FieldMixin), {})
        return new_cls

@add_metaclass(NgFormBaseMeta)
class NgFormBaseMixin(object):
    form_error_css_classes = 'djng-form-errors'
    field_error_css_classes = 'djng-field-errors'

    def __init__(self, data=None, *args, **kwargs):
        try:
            form_name = self.form_name
     .....

But at this point, in __new__, the object does not have base_fields yet. Do you think it's that important to stick to name, base, attrs? Is it to stick to django FormMixinBase?

I guess I need to wait for version 0.7.10 :smile:

By the way, my company (luna technology) is actually using django-angular but we are still under heavy development so I don't have any working link to provide. I'll tell you as soon as I get one.

jrief commented 9 years ago

This weekend a tried out a few things. The problem is, that I have to add a mixin class, built on the fly, to all of our form fields. This is, because each form field requires an additional method get_potential_errors to render potential errors activated by AngularJS's form validation code.

Now, if I do it in __init__, its too late. If I do it in a __new__ of a meta-class, the member base_fields is not available yet. I have to experiment a little to find out the best hook for patching the form fields as required by class NgFormValidationMixin.

jrief commented 9 years ago

@gbozee @adrienbrunet Now I managed to find a clean solution. The big problem here is that django.forms.Form and django.forms.ModelForm required a metaclass themselves. Since you can't mix metaclasses in a friendly manner, I had to inherit my own metaclass BaseFieldsModifierMetaclass from the ones, given by Django: DeclarativeFieldsMetaclass and ModelFormMetaclass.

Now however we can't just add some mixins to our own Forms or ModelForms. Therefore instead you now have to derive your classes from djangular.forms.NgForm or djangular.forms.NgModelForm respectively.

Please test with the latest revision, so that I can release version 0.7.10.

gbozee commented 9 years ago

Just tested the above patch. Working as expected.

adrienbrunet commented 9 years ago

:+1: Tested with your last commits. I have to rewrite a lot of my forms now but it's for the best.. Thanks for your work!