Fixed #14370 -- Added select2 widget for related object fields in admin. #6385

codingjoe · 2016-04-02T23:27:18Z

Adds jQuery Select2 version 4 to support async select inputs
including a search feature.

I split the PR in two commits, one is vendoring select2, one contains my code.

Links & Discussions

Changes:

~~jQuery noConflict is set to false, jQuery itself does not get removed form global~~
the new select2 widget is automatically used if the related object
has a registered admin and defines search fields
only str representation is supported at this point
the search feature uses the same field as the model admin

Todo:

codingjoe · 2016-04-02T23:29:15Z

@jpic this is a first draft, I tested it manually, it seems fine
@timgraham I only added tests for the new json view, I'll add the widget tests in the sprint tomorrow

syphar · 2016-04-03T07:16:37Z

django/contrib/admin/views/main.py

@@ -384,3 +389,97 @@ def url_for_result(self, result):
                                               self.opts.model_name),
                       args=(quote(pk),),
                       current_app=self.model_admin.admin_site.name)
+
+
+class ForeignKeyJsonView(BaseListView):


Shouldn't there be a LoginRequiredMixin to prevent unauthorized access?

my bad, wrap does it (thanks @codingjoe)

jpic · 2016-04-03T08:04:50Z

Nice !

Did you try it on dynamically added formset rows ?

codingjoe · 2016-04-03T08:59:00Z

@jpic no that can't work yet, but I'm on it today!
I also got a lot of documentation ahead of me, but it really depends where this is going. I'm gonna talk to a couple of people here about it.

jpic · 2016-04-03T09:12:21Z

@codingjoe we use DOMNodeInserted event for dynamically added selects: https://github.com/yourlabs/django-autocomplete-light/blob/master/src/dal/static/autocomplete_light/autocomplete.init.js#L25-L27

We also have a really small snippet for option renaming using the edit button which you might like: https://github.com/yourlabs/django-autocomplete-light/blob/master/src/dal_select2/static/autocomplete_light/select2.js#L100-L106

collinanderson · 2016-04-03T16:58:05Z

I might be able to try test this out later this week. Seems to me we should give this a release or two in the real world before deprecating raw_id_fields. Like, make sure it's good enough in practice to replace raw_id_fields.

collinanderson · 2016-04-03T17:08:04Z

Does /foreignkey_json/ do permission checks? It seems to me it should require the same permissions as raw_id_fields (so if you're logged in to the admin, you can't just query that table unless you actually have permission, but maybe that's ok)

Edit: Actually, maybe we just need to make sure that you have the change permission for the Model that has the foreign key. That way it follows the permissions of a normal ChoiceField.

codingjoe · 2016-04-03T18:05:23Z

@collinanderson good point, that's what I thought. Lets see how it performs.
Regarding the permissions, you are correct, the change permission would the right thing to check. I'll put it on my list, thanks!

guettli · 2016-04-04T06:14:23Z

There is a small typo in the diff of docs/ref/models/fields.txt: you mean admin not admon.

guettli · 2016-04-04T06:16:20Z

@codingjoe first: Thank you for this great improvement. Next: It would be nice if the autocomplete would be easily re-usable outside the admin, too. Do you plan to support this?

codingjoe · 2016-04-04T15:42:45Z

@guettli no, not really. We have django-select2 and django-autocomplete-light for that.
The really tricky part is to know which queryset to server as a JSON. django-select2 solves this by using the cache as a persistent storage shared by all application servers. In django-autocomplete-light you'll have to specify that explicitly.

I don't see a way to get this into core. This should remain a admin only feature for now, just like the raw ID field.

collinanderson · 2016-04-05T14:58:59Z

@codingjoe btw, at some point we'll need to eventually get django-developers mailing list approval for vendoring select2.

Also, you've been mentioning raw_id_fields. I'm also hoping we can eventually use select2 to replace filter_horizontal/filter_vertical for m2m's.

guettli · 2016-04-05T18:35:59Z

@codingjoe you said:

The really tricky part is to know which queryset to server as a JSON

Yes, this is true, since the django admin interface needs a generic ajax server part.

But I still think it would be great to have a autocomplete component in django which can be used in django apps and the django admin.

You can make a BaseWidget available which gets subclassed once in the admin interface and once for the usage in custom apps.

I like small systems and having select2 twice in my static directory gives me a bad feeling. Yes it works and does not hurt, but somehow I think "less is more - avoid redundancy".

jpic · 2016-04-05T20:02:28Z

You don't need to have it twice, you can reuse it. For example, this public facing-app reuses admin scripts: https://github.com/jonashaag/django-addanother/blob/master/django_addanother/widgets.py#L44-L52 Correct me if I'm wrong but the idea here is to first incorporate autocompletion in the admin first and then perhaps extract it into django.forms.

timgraham

This completes my detailed review of the patch:

Make sure a selenium test covers the changes in RelatedObjectLookups.js and all lines of autocomplete.js (e.g. as per Collin's comment, formset tests look missing).
test_select_multiple selenium test is erroring
I don't see any test failures if AutocompleteJsonView.paginate_by/get_paginator,and self.paginator_class=... are removed. Also, there aren't any tests that result in 'pagination': {'more': True}, I think.
test_search_use_distinct may be adding test coverage but it doesn't fail if qs = qs.distinct() is commented out as it should to be a good test.

timgraham · 2017-09-13T16:02:16Z

django/contrib/admin/widgets.py

+        return attrs
+
+    def optgroups(self, name, value, attr=None):
+        """Return only selected options and set QuerySet from `ModelChoiceIterator`."""


what does "set QuerySet" mean?

timgraham · 2017-09-13T16:09:18Z

docs/ref/contrib/admin/index.txt

+    in the drop-down.
+
+    ``autocomplete_fields`` is a list of fields that you would like to change
+    to `Select2 <contrib-admin-select2>`_ autocomplete inputs.


This link is broken, I'm not sure what you intended to link to.

seems to be old. I remove it.

timgraham · 2017-09-13T16:26:09Z

docs/ref/contrib/admin/index.txt

+        :func:`ModelAdmin.get_search_results` implementation based on a
+        full text index search.
+
+        It's also advisable to change the ```Paginator`` on very large tables


This leaves me wondering how to do that.

timgraham · 2017-09-13T16:28:59Z

docs/ref/models/fields.txt

@@ -1329,8 +1329,9 @@ The possible values for :attr:`~ForeignKey.on_delete` are found in

    If ``limit_choices_to`` is or returns a :class:`Q object
    <django.db.models.Q>`, which is useful for :ref:`complex queries
-    <complex-lookups-with-q>`, then it will only have an effect on the choices
-    available in the admin when the field is not listed in
+    <complex-lookups-with-q>`.


Can you remind me why this change is related to this patch?

I don't remember writing this. I don't believe it's too. I will revert this.

timgraham · 2017-09-13T16:39:20Z

tests/admin_views/models.py

@@ -957,3 +963,12 @@ def __str__(self):

 class RelatedWithUUIDPKModel(models.Model):
    parent = models.ForeignKey(ParentWithUUIDPK, on_delete=models.SET_NULL, null=True, blank=True)
+
+
+class Author(models.Model):


Are new models really needed here?

I believe so, yes. I can go ahead and alter other models more and more, eventually it lead to many side effects and will make the test suite hard to maintain. I removed as many models as I deemed unnecessary without making thing overly complex.

timgraham · 2017-09-13T16:48:10Z

tests/admin_views/test_autocomplete_view.py

+        # We can not know if the field is an autocomplete field,
+        # since `get_autocomplete_fields` requires a request.
+        # The final url is therefore not available during the url setup.
+        resolve('/test_admin/admin/admin_views/answer/autocomplete/wrong_field/')


I think this test isn't working as expected -- it's resolving to RedirectView, same with test_inline_urls -- probably the result of the resolve should be checked.

I believe things moved around here, did you already fix it @timgraham ?

No, this comment is still relevant. The comments at the "top" of this review that aren't connected to any specific lines are also still outstanding. Did you see those?

codingjoe · 2017-09-16T21:05:48Z

@timgraham I added a lot new selenium tests overing all the cases you described and all the cases I can think of.
While doing so I discovered and fix two bugs, including the one described by @collinanderson
Furthermore I added tests for the paginator and distinct behavior.

Puh, that took the the whole day, but it's done. Just in time, sorry for that :/

timgraham · 2017-09-16T21:20:07Z

tests/admin_views/test_autocomplete_view.py

        elem = self.selenium.find_element_by_css_selector('.select2-selection')
        elem.click()
        results = self.selenium.find_element_by_css_selector('.select2-results')
        self.assertTrue(results.is_displayed())
        elem = results.find_element_by_css_selector('.select2-results__option')
        elem.click()
+
+    def test_inline_add(self):
+        self.selenium.get(self.live_server_url + reverse('admin:admin_views_question_add'))


Is this test complete?

No, just a leftover. I currently going over my changelog. I seem to have missed this. I'll remove it.

timgraham · 2017-09-16T21:45:59Z

tests/admin_views/test_autocomplete_view.py


    def test_select_multiple(self):
-        self.selenium.get(self.live_server_url + reverse('admin:admin_views_lecture_add'))
+        self.selenium.get(self.live_server_url + reverse('admin:admin_views_question_add'))
        elem = self.selenium.find_element_by_css_selector('.select2-selection')
        elem.click()
        results = self.selenium.find_element_by_css_selector('.select2-results')
        self.assertTrue(results.is_displayed())
        elem = results.find_element_by_css_selector('.select2-results__option')
        elem.click()


Is this test complete? What's the purpose of clicking on "No result found."?

It does tests no results, which I didn't test in the test before. I will add it to the test before and test multiple select here in more detail including selecting many.

codingjoe · 2017-09-18T08:12:40Z

@timgraham is this happening in 2.0? I'll watch my notifications closely today, incase anything comes up. But I am CEST.

…nd ManyToManyField in the admin. Thanks Florian Apolloner and Tim Graham for review and contributing to the patch.

auvipy · 2017-09-19T03:26:31Z

@codingjoe thanks for all your hard work and patience

GeyseR · 2017-09-19T05:57:52Z

docs/ref/contrib/admin/index.txt

+    <https://select2.org/>`_ autocomplete inputs.
+
+    By default, the admin uses a select-box interface (``<select>``) for fields
+    that are . Sometimes you don't want to incur the overhead of selecting all


This looks like not finished sentence

thx :) see #9109

Finished incomplete sentense in `autocomplete_fields` documenation. Ref django#6385 Ref django#14370

codingjoe · 2017-09-19T09:35:34Z

Thanks everyone for all the support, be it mental or though reviews. This has been a crazy but also amazing experience for me.

Thanks to @jpic and @apollo13 for your feedback and the help during the DjangoCon and DUTH sprints.

Special thanks to @kevin-brown who build the foundation this feature rests upon. I hope this will also send more contributors your way to make Select2 even better.

Last but not least, special thanks to @timgraham. Thank you for your guidance and thoroughness and seemingly limitless patience.

Thank you <3

rom1dep · 2017-09-23T15:26:45Z

Hi @codingjoe , I just migrated my test project to django 2a1, and got to test your feature. Congrats for having it merged!
It looks nice on the admin model creation form, but I was somewhat expecting to see the same select2 control showing up in the filter pane (considering that I passed this field to both autocomplete_fields and list_filter). Do you think this could be considered as a nice to have addition?

For me, the whole point of this autocomplete kind of control is to better handle long lists of items, and list_filter has always been a pain for that. My current workaround is to use an approach like https://stackoverflow.com/a/39618385 , but it'd be much better to have it available out of the box, as part of django 2, now that select2 has been vendored :)

In any case, keep up the good work 👍

collinanderson · 2017-09-24T18:09:42Z

@rom1dep you could propose something like that here: https://code.djangoproject.com/ticket/1873

codingjoe · 2017-09-25T08:19:54Z

@rom1dep love the idea! Lets put that into a separate ticket. Now that we have a version of Select2 vendored in Django's admin, it should be simple and not invoice too much discussion 👍

paultiplady · 2018-02-03T20:55:24Z

Added this ticket to facilitate discussion - https://code.djangoproject.com/ticket/29111#ticket

apollo13 · 2018-02-04T15:26:08Z

@paultiplady There isn't really much discussion needed (in the sense that we'd want that feature), if you feel up to the task a PR would be great!

codingjoe · 2018-02-07T12:22:14Z

I agree. Just build it, this would be an amazing addition and actually not too hard to implement.

paultiplady · 2018-02-22T00:33:58Z

Good to know, thanks -- just wanted to check whether anyone else was already looking at it. I'm unlikely to have time in the next couple weeks but my team needs this feature so I'll hopefully find time to have a crack at it soon.

carltongibson · 2018-06-18T07:46:07Z

tests/admin_views/test_autocomplete_view.py

+    def test_has_change_permission_required(self):
+        """
+        Users require the change permission for the related model to the
+        autocomplete view for it.


The requirement for the change permission means the autocomplete doesn't work for a user with only the new view permission on the related model.

See https://code.djangoproject.com/ticket/29502

This should probably be weakened. (No extra permission is needed here if you're not using autocomplete.)

@codingjoe: Your thoughts welcomed! 🙂

I wasn't even aware of the view permission. This is fine.

codingjoe force-pushed the issues/14370 branch 6 times, most recently from bcaa59d to 1ba7b5f Compare April 3, 2016 00:38

syphar reviewed Apr 3, 2016
View reviewed changes

codingjoe force-pushed the issues/14370 branch 7 times, most recently from 10bc3a2 to c183593 Compare April 3, 2016 16:37

codingjoe force-pushed the issues/14370 branch from c183593 to 17343cd Compare April 3, 2016 17:46

timgraham changed the title ~~Resolves #14370 -- Adds select2 widget for foreign keys in admin~~ Fixed #14370 -- Added select2 widget for foreign keys in admin. Apr 4, 2016

timgraham force-pushed the issues/14370 branch from 3c862f0 to a674a36 Compare September 13, 2017 17:42

timgraham reviewed Sep 13, 2017

View reviewed changes

django deleted a comment from codingjoe Sep 16, 2017

codingjoe force-pushed the issues/14370 branch from c579f9e to e127c72 Compare September 16, 2017 21:13

timgraham reviewed Sep 16, 2017

View reviewed changes

codingjoe force-pushed the issues/14370 branch from 87dacd9 to 55c8898 Compare September 16, 2017 21:25

timgraham reviewed Sep 16, 2017

View reviewed changes

codingjoe force-pushed the issues/14370 branch 2 times, most recently from aed1a72 to c838620 Compare September 17, 2017 00:31

django deleted a comment from codingjoe Sep 18, 2017

timgraham force-pushed the issues/14370 branch from c838620 to d596a9e Compare September 18, 2017 16:20

Fixed django#14370 -- Allowed using a Select2 widget for ForeignKey a…

94cd8ef

…nd ManyToManyField in the admin. Thanks Florian Apolloner and Tim Graham for review and contributing to the patch.

timgraham force-pushed the issues/14370 branch from d596a9e to 94cd8ef Compare September 18, 2017 17:48

timgraham merged commit 94cd8ef into django:master Sep 18, 2017

codingjoe deleted the issues/14370 branch September 18, 2017 18:09

GeyseR reviewed Sep 19, 2017

View reviewed changes

codingjoe added a commit to codingjoe/django that referenced this pull request Sep 19, 2017

Fixed docs –– Finish incomplete sentence

09fb411

Finished incomplete sentense in `autocomplete_fields` documenation. Ref django#6385 Ref django#14370

codingjoe mentioned this pull request Sep 19, 2017

Refs #14370 -- Fixed typo in ModelAdmin.autocomplete_fields docs. #9109

Merged

carltongibson reviewed Jun 18, 2018

View reviewed changes

Fixed #14370 -- Added select2 widget for related object fields in admin. #6385

Fixed #14370 -- Added select2 widget for related object fields in admin. #6385

Conversation

codingjoe commented Apr 2, 2016 • edited

Links & Discussions

Changes:

Todo:

codingjoe commented Apr 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpic commented Apr 3, 2016

codingjoe commented Apr 3, 2016

jpic commented Apr 3, 2016

collinanderson commented Apr 3, 2016

collinanderson commented Apr 3, 2016

codingjoe commented Apr 3, 2016

guettli commented Apr 4, 2016

guettli commented Apr 4, 2016

codingjoe commented Apr 4, 2016

collinanderson commented Apr 5, 2016

guettli commented Apr 5, 2016

jpic commented Apr 5, 2016 via email

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingjoe commented Sep 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingjoe commented Sep 18, 2017 • edited

auvipy commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingjoe commented Sep 19, 2017

rom1dep commented Sep 23, 2017

collinanderson commented Sep 24, 2017

codingjoe commented Sep 25, 2017

paultiplady commented Feb 3, 2018

apollo13 commented Feb 4, 2018

codingjoe commented Feb 7, 2018

paultiplady commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingjoe commented Apr 2, 2016 •

edited

codingjoe commented Sep 18, 2017 •

edited