jonashaag / django-addanother

"Add another" buttons outside the Django admin
http://django-addanother.readthedocs.org/
ISC License
50 stars 18 forks source link

ModelSelect2AddAnother #29

Closed afs25 closed 6 years ago

afs25 commented 6 years ago

Hi, thank you for developing django-addanother; I have been using it with the default AddAnotherWidgetWrapper widget for a while now (in Django 1.11) and it works very well. However, I am now attempting to use it in a project that requires an autocomplete widget (currently using dal.autocomplete.ModelSelect2, but would really like a popup form to create new choices).

Following your documentation (http://django-addanother.readthedocs.io/en/latest/select2.html and http://django-addanother.readthedocs.io/en/latest/reference/select2_widgets.html#select2ref), I had no trouble to reproduce the example given with Select2AddAnother, but no luck so far in my attempts to use ModelSelect2AddAnother instead.

Could you please add an example using ModelSelect2AddAnother to http://django-addanother.readthedocs.io/en/latest/select2.html ?

Many thanks!

jpic commented 6 years ago

Seems like the code in contrib generates a bunch of subclasses. I thought you didn't need to subclass at all, why not use the decorator pattern as in the howto ? ie. AddAnotherWidgetWrapper(ModelSelect2()) ?

For media loading, I assume the decorator (AddAnotherWidgetWrapper) could patch its media with the decorated (ModelSelect2)'s media and then inheritance would not be needed for media loading /if that's the root cause for changing from composition to inheritance here, otherwise, perhaps tackle the relevant root cause ?/ Then, there would be no need for a specific example and we could close this issue ?

I suspect the change from manual script loading (templates/autocomplete_scripts.html) to using Media could have broken support for using the decorator pattern of django-addanother out of the box, but maybe there's another reason, @afs25 what happens when you try using DAA and DAL together as described in both tutorials ?

afs25 commented 6 years ago

Thank you for your prompt reply @jpic!

what happens when you try using DAA and DAL together as described in both tutorials ?

What tutorials? I am not aware of any tutorial showing how to use DAA and DAL together. The only DAA tutorial I have found is in the project's documentation: http://django-addanother.readthedocs.io/en/latest/usage.html

This thread (https://github.com/yourlabs/django-autocomplete-light/issues/648) mentions using both together, but it does not give much detail and some of the links are broken.

Anyway, I tried to use both together by using the decorator pattern you suggested. In forms.py I tried:

class NodeForm(forms.ModelForm):
    vetted = forms.BooleanField(disabled=True)

    class Meta:
        model = models.Node
        fields = '__all__'
        widgets = {
            'synonym_of': AddAnotherWidgetWrapper(
                autocomplete.ModelSelect2(url='policies:node_autocomplete', forward=['type']),
                reverse_lazy('policies:node_add'),
            ),
        }

This resulted in a TypeError:

File "/usr/local/venvs/orpheus/lib/python3.5/site-packages/django_addanother/widgets.py", line 82, in render
    'widget': self.widget.render(name, value, *args, **kwargs),
TypeError: render() got an unexpected keyword argument 'renderer'

Is this what you had in mind?

afs25 commented 6 years ago

The solution you suggested works with django-select2 widgets:

widgets = {
            'synonym_of': AddAnotherWidgetWrapper(
                Select2Widget,
                reverse_lazy('policies:node_add'),
            ),
        }

But, unfortunately, I am not being able to test it with ModelSelect2Widget, possibly because of this issue: https://github.com/applegrew/django-select2/issues/341

If I can get ModelSelect2Widget to work and it satisfies the performance requirements of my application, the integration with django-addanother will not be an issue.

However, any ideas on solving the issue with autocomplete.ModelSelect2 posted above would be much appreciated, because that widget is already working fine in my Django 1.11 app.

Many thanks!

jpic commented 6 years ago

I mean that there should be no tutorial for using both together, as using both together should work out of the box. This is the reason why there is no specific tutorial for DAL + DRF: using a DAL widget in the DRF filter works out of the box. So yeah, i think the idea is to make this just work as expected:

AddAnotherWidgetWrapper(
                autocomplete.ModelSelect2(url='policies:node_autocomplete', forward=['type']),
                reverse_lazy('policies:node_add'),
            )

Can you contribute an example app with DAA+DAL, a minimal, isolated example, in one of the test project ? This will help fixing the exception.

Also, what DAL version are u on ?

jpic commented 6 years ago

I see we've had something matching renderer yourlabs/django-autocomplete-light@823247eb4a0e1b97ae9bd54e1e7eef05cc8ea868

But then DAL moved on to just proxying that kwarg in yourlabs/django-autocomplete-light@823247eb4a0e1b97ae9bd54e1e7eef05cc8ea868

afs25 commented 6 years ago

Thank you! I was using DAL version 3.2.10. Upgrading DAL to version 3.3.0rc6 solved the issue. I am pleased to confirm that this now works:

widgets = {
            'synonym_of': AddAnotherWidgetWrapper(
                autocomplete.ModelSelect2(url='policies:node_autocomplete', forward=['type']),
                reverse_lazy('policies:node_add'),
            ),
        }

Many thanks for your prompt help with this! It's much appreciated.

jonashaag commented 6 years ago

@afs25 I haven't followed this thread in detail. Do you think someone else having this problem in the future could profit from better documentation somewhere?

jpic commented 6 years ago

@jonashaag this is a recurring question from my experience, in DRF every now and then somebody asks for better documentation about integrating DAL, but just using DAL works out of the box with django-filter so, even when somebody wants to contribute an example, it turns out just using each app as respectively documented works out of the box (or at least, it should), so no extra documentation should be needed.

afs25 commented 6 years ago

@jonashaag, I agree with @jpic that your documentation is fine. The only catch here is that the latest stable version of DAL does not seem to work with DAA on Django 1.11, but this seems to be a DAL issue that will be fixed in their next release. Thank you again for developing this package.