Fixed #13883 -- Added optgroups for SelectBox in Admin

[mmclar]

Modified SelectBox.js so that optgroups are honored. When an item is moved from unselected to selected box, order is maintained by group and then item name. Also added a test.

Thanks ᴙɘɘᴙgYmɘᴙɘj for the nice repro.

[github-actions[bot]]

Hello @mmclar! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

[felixxm]

@mmclar Thanks for the patch :+1:

I found that this doesn't play nice with :heavy_plus_sign: , see

https://user-images.githubusercontent.com/2865885/165691060-84313333-39c3-452e-a546-4ba7cb786f54.mp4

I also cannot save chosen values anymore:

https://user-images.githubusercontent.com/2865885/165692282-9c56aa48-dddc-4a1c-bdd6-a06c344c3d2d.mp4

[mmclar]

Thanks for the feedback!

For saving values: this is just an error with the demo app. In ProfileForm, the pairs added to the choices lists should be (pk, name), not (name, name) as seen here: https://github.com/reergymerej/ticket_13883/blob/master/demo/admin.py on lines 14 and 16.

For the + button: this is quite a tricky bit. Since the logic for partitioning the choices into optgroups lives in the admin object, I don't see a way to discern a new choice's optgroup without initializing ProfileForm again. To do that, the popup window needs to know the source form, so that needs to be passed to the add form. I've written out those changes and they are here: https://github.com/django/django/pull/15568/commits/e60d5f371e4985bae1658146cbf4d4965035d600

[silentbugs]

Are there any news on this?

[nessita]

Hello @silentbugs @mmclar

I see that this PR needs a new round of reviews, in order to progress with that, would you please rebase onto main, fix conflicts, and re-ping for a re-review?

Thank you!

[mmclar]

@nessita I've resolved the conflicts and updated some new tests -- how do I get a re-review?

[nessita]

@nessita I've resolved the conflicts and updated some new tests -- how do I get a re-review?

Thanks for that work. Before pinging specific people, did you solve the issues reported by Mariusz in their videos? If yes, to get a re-review, you could either mention the last reviewer that asked for changes, and also be sure that the ticket in the tracker has the "needs tests" and "patch needs improvement" flags unset. Does that make sense?

[felixxm]

For saving values: this is just an error with the demo app. In ProfileForm, the pairs added to the choices lists should be (pk, name), not (name, name) as seen here: https://github.com/reergymerej/ticket_13883/blob/master/demo/admin.py on lines 14 and 16.

Yes, thanks it works now.

For the + button: this is quite a tricky bit. Since the logic for partitioning the choices into optgroups lives in the admin object, I don't see a way to discern a new choice's optgroup without initializing ProfileForm again. To do that, the popup window needs to know the source form, so that needs to be passed to the add form. I've written out those changes and they are here: e60d5f3

Unfortunately, I can still reproduce this issue.

[mmclar]

Hm. I am not able to reproduce the issue using the same steps. I am not sure what the delta between my test system and yours is.

Screencast from 06-16-2023 06:57:37 PM.webm

[felixxm]

Hm. I am not able to reproduce the issue using the same steps. I am not sure what the delta between my test system and yours is.

Probably because I'm checking on the "Add" view, not the "Change" view:

https://github.com/django/django/assets/2865885/b47b1b9a-cc8f-4ac7-bf03-980acc484a2a

[mmclar]

I think I'm also using the "Add" view, and the plus icon to add a new sport item. Here's a screen recording with more context: Screencast from 06-18-2023 03:51:12 PM.webm

[felixxm]

I think I'm also using the "Add" view, and the plus icon to add a new sport item. Here's a screen recording with more context: Screencast from 06-18-2023 03:51:12 PM.webm

It still doesn't work for me. Can you share your test project? I think you've changed a structure of choices, from 2-tuples of (sport.pk, sport.name) to (sport.name, sport) :thinking: As far as I'm aware this, may break the way how ChoiceWidget.optgroups() works.

[Christophe31]

I used optgroup with select2 and chosen, those JS widgets searched also in group name. I humbly suggest that search may also match optgroup string.

Here my naive implementation change.

SelectBox.js:45:

                const group_text = (node.group && node.group.toLowerCase() || '')
                const node_text = group_text + " " + node.text.toLowerCase();

[Christophe31]

I don't know much about JS performances, but as this node_text chain is also used for sorting, I ask myself if somebody knowing something about JS perf would rather add an index field to options, or compute it in both cases.