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

Implement DRY to all autocompletes #1067

Open
jpic opened this issue Nov 2, 2018 · 5 comments
Open

Implement DRY to all autocompletes #1067

jpic opened this issue Nov 2, 2018 · 5 comments
Assignees

Comments

@jpic
Copy link
Member

jpic commented Nov 2, 2018

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
Copy link

spapas commented Nov 2, 2018

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
Copy link
Member

Guilouf commented Nov 2, 2018

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

@spapas
Copy link

spapas commented Nov 3, 2018

@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
Copy link
Member

Guilouf commented Nov 3, 2018

@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
Copy link
Member Author

jpic commented Nov 8, 2018

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 😹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants