yourlabs / django-autocomplete-light

A fresh approach to autocomplete implementations, specially for Django. Status: v4 alpha, v3 stable, v2 & v1 deprecated.
https://django-autocomplete-light.readthedocs.io
MIT License
1.8k stars 467 forks source link

Implement DRY to all autocompletes #1067

Open jpic opened 5 years ago

jpic commented 5 years ago

if you want DRY, you need to make either the view generate the form field, either the form field generate the view, like what @Guilouf does

@Guilouf found a pattern , it's currently in dal_queryset_sequence, but can be extended and used in others.

then he does urlpatterns += form.as_urls() or something

https://github.com/yourlabs/django-autocomplete-light/blob/master/src/dal_select2_queryset_sequence/fields.py#L44

spapas commented 5 years ago

Hello @jpic, actually I've found a nice solution for this: Using the existing django modelform_factory and passing it the widgets parameter instead of explicitly creating a form. Here's an example of an actual admin class:

@admin.register(models.ServiceTypeCorrelation)
class ServiceTypeCorrelationAdmin(admin.ModelAdmin):
    list_display = ("classification_society", "service_type_str", "service_type")
    search_fields = ("service_type_str", "service_type__name")
    list_filter = ("classification_society", ServiceTypeIsNullFilter)
    form = modelform_factory(
        models.ServiceTypeCorrelation,
        fields="__all__",
        widgets={"service_type": autocomplete.ModelSelect2(url="service-type-ac")}
    )

I know that this doesn't actually save many keystrokes (you just define the form using the modelform_factory instead of using a separate form class) but it definitely is more DRY than having an extra ServiceTypeCorrelationForm!

I recommend putting this as a hint to the users to the tutorial in the "Using autocompletes in the admin" section (https://django-autocomplete-light.readthedocs.io/en/master/tutorial.html#using-autocompletes-in-the-admin), i.e something like

If you don't need to create a form you could use the Django modelform_factory directly passing it the widgets parameter with the autocompeltes you'd like to use for example:

class PersonAdmin(admin.ModelAdmin):
form = modelform_factory(
model=Person
fields='__all__'
widgets={'birth_country': autocomplete.ModelSelect2(url='country-autocomplete')}
)
Guilouf commented 5 years ago

But you will still need to create a view and register an url for the widget ?

spapas commented 5 years ago

@Guilouf yes I'd need to create both the view and widget. What I mainly wanted was to avoid creating an explicit form class for this and having something similar to the django-autocomplete-light v2 modelform_factory (https://django-autocomplete-light.readthedocs.io/en/2.0.9/tutorial.html#autocomplete-light-modelform-factory-shortcut-to-generate-modelforms-in-the-admin).

Also probably the v2 modelform_factory behavior could be better emulated if v3 provided an modelform_factory that would search the urls.py for autocompletes for the provided model's foreign keys and automatically created the widgets parameter to be passed to the django's modelform_factory. I'll take a look at it ti see how well it's working.

Guilouf commented 5 years ago

@spapas Yeah you will need to create a custom modelform_factory to replace each defaults django field by the corresponding one in autocomplete. But because of the variety of widgets and parameters in DAL, im not so sure if its usefull to bypass the creation of a proper form. It could be convenient in some cases, but will add some complexity and kind break the django model + form pattern. Eventually working with FutureModelForm and modifying it in order to replace the fields widgets of the form with default DAL fields could be a nice way. And it would be very easy then to create a FutureModelForm factory.

So then you will have the choice between:

class TForm(autocomplete.FutureModelForm):
    """ 
    django widgets converted to dal widgets automatically, but still the possibility to customize
    as in standard django's forms
    """
    class Meta:
        model = TModel
        fields = __all__

or futuremodelform_factory(TModel)

jpic commented 5 years ago

You're both right that we can have it like in v2 from the outside but also not like v2 in the inside.

Creating the fields on the fly is possible safely in the metaclass, but unlike in v2 it should all be contained in FutureFormMetaclass.new before calling super().new to emulate that the user has defined them normally - in the way that is currently tested.

However with @Guilouf's pattern I suppose you will have to solve a chicken and egg problem if the form class is also supposed to register urls, that means after being defined by the metaclass.

This means that the widgets will have to initialize without a url, and be set afterwards when the form generates the urls...

Sounds like a lovely hack :joy_cat: