Opened 14 years ago

Last modified 12 days ago

#13883 assigned Bug

SelectBox.js with grouping (optgroup elements)

Reported by: SardarNL Owned by: seanhelvey
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin, SelectBox, optgroup, sprintdec2010
Cc: minimallyuseless@…, Narbonne, Jignesh Kotadiya, Michael McLarnon, Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: yes

Description (last modified by Narbonne)


Attachments (9)

bad_select.png (20.4 KB ) - added by SardarNL 14 years ago.
Current SelectBox.js screenshot
good_select.png (21.9 KB ) - added by SardarNL 14 years ago.
Better SelectBox.js screenshot. optgroups are recreated on redisplay()
SelectBox.js (4.7 KB ) - added by SardarNL 14 years ago.
Better SelectBox.js which handles optgroup, depends on jQuery
13883_initial_patch_as_diff.diff (8.2 KB ) - added by Julien Phalip 14 years ago.
SelectBox.js.diff (8.7 KB ) - added by SardarNL 14 years ago.
Patch to SelectBox.js
RelatedObjectLookups.js.diff (2.1 KB ) - added by SardarNL 14 years ago.
takes group in account
options.py.diff (1.1 KB ) - added by SardarNL 14 years ago.
Django 1.2.4 patch, let admin recognize groups in selects
options.py-corrected.diff (1.1 KB ) - added by bsimons 14 years ago.
update of patch for options.py made to work with trunk
13883-fix.patch (5.1 KB ) - added by Michael McLarnon 3 years ago.
Patch to retain option groups, and test

Download all attachments as: .zip

Change History (43)

by SardarNL, 14 years ago

Attachment: bad_select.png added

Current SelectBox.js screenshot

by SardarNL, 14 years ago

Attachment: good_select.png added

Better SelectBox.js screenshot. optgroups are recreated on redisplay()

by SardarNL, 14 years ago

Attachment: SelectBox.js added

Better SelectBox.js which handles optgroup, depends on jQuery

comment:1 by SardarNL, 14 years ago

Component: Contrib appsdjango.contrib.admin

comment:2 by minimallyuseless@…, 14 years ago

Cc: minimallyuseless@… added

subscribing

comment:3 by Harro, 14 years ago

milestone: 1.3

Works great, it doesn't change the current working of the select but adds extra functionality.

I dunno if there are tests for the admin javascript, so it might need that and I really wouldn't know what documentation it would need.

comment:4 by Julien Phalip, 14 years ago

Keywords: sprintdec2010 added
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

It looks good and it mostly works, except that the add popup (which appears after clicking the green cross icon) does not work properly. Could you revisit your patch to fix this?

By the way, in the future it would make it a lot easier for anyone to review your patch if you:

  • sent a diff instead of a whole file.
  • provided a simple test case so that it's easier for people to reproduce the problem you're trying to solve.

I'm uploading a diff now. I've also produced the following test case:

Models:

from django.db import models

class Sport(models.Model):
    name = models.CharField(max_length=100)
    is_team_sport = models.BooleanField(default=False)

    def __unicode__(self):
        return self.name
    
class Profile(models.Model):
    sports = models.ManyToManyField(Sport)

Admin:

from django.contrib import admin
from django.forms import ModelForm

from .models import Sport, Profile

class ProfileForm(ModelForm):
    def __init__(self, *args, **kwargs):
        super(ProfileForm, self).__init__(*args, **kwargs)

        team_sports = []
        single_sports = []
        for sport in Sport.objects.all():
            if sport.is_team_sport:
                team_sports.append((sport.name, sport.name))
            else:
                single_sports.append((sport.name, sport.name))
        self.fields['sports'].choices = [['Team Sports', team_sports], ['Single Sports', single_sports]]

class ProfileAdmin(admin.ModelAdmin):
    form = ProfileForm
    filter_horizontal = ('sports',)

admin.site.register(Profile, ProfileAdmin)
admin.site.register(Sport)

This can be considered as a usability bug so keeping in the 1.3 milestone.

by Julien Phalip, 14 years ago

comment:5 by SardarNL, 14 years ago

Patch needs improvement: unset

julien thanks for the test case.

except that the add popup (which appears after clicking the green cross icon) does not work properly.

Fixed, but involves a little more changes than a thought first time.

Drop-in replacement SelectBox.js has now fully compatible API as the original SlectBox.js, the group argument is optional. If there is no group, then option is added to 'not-grouped' option list. RelatedObjectLookups.js is updated to take optional group in account.

A more painful update is needed for django.contrib.admin.options.ModelAdmin.response_add as it knows nothing about groups. Small patch looks for 'list_grouping' attribute, if it exists, then it is assumed to refer to the property returning the object's group. This assumes a group is a single property that can be turned into string.

In short: if men needs just proper optgroup handling and doesn't mind if new objects will be added to 'no-group' list, then only SelectBox.js patch is needed.
If men wants that new elements will be placed in the proper group, then RelatedObjectLookups.js and options.py patches are needed too. Of course this is only graphical issue.

Updated test case:

from django.db import models

class Sport(models.Model):
    name = models.CharField(max_length=100)
    is_team_sport = models.BooleanField(default=False)

    def __unicode__(self):
        return self.name

    @property
    def group(self):
        return 'Team Sports' if self.is_team_sport else 'Single Sports'
    
class Profile(models.Model):
    sports = models.ManyToManyField(Sport)

Admin:

from django.contrib import admin
from django.forms import ModelForm

from .models import Sport, Profile

class ProfileForm(ModelForm):
    def __init__(self, *args, **kwargs):
        super(ProfileForm, self).__init__(*args, **kwargs)

        choices = {}
        for sport in Sport.objects.all():
            choices.setdefault(sport.group, []).append(sport)
        self.fields['sports'].choices = choices.items()

class ProfileAdmin(admin.ModelAdmin):
    form = ProfileForm
    filter_horizontal = ('sports',)

class SportAdmin(admin.ModelAdmin):
    # currently is used only by response_add() but can be used by changelist_view() too
    list_grouping = 'group'


admin.site.register(Profile, ProfileAdmin)
admin.site.register(Sport, SportAdmin)

The ModelAdmin passes 'group' to RelatedObjectLookups.js, which passes it to SelectBox.js, which properly handles it.

by SardarNL, 14 years ago

Attachment: SelectBox.js.diff added

Patch to SelectBox.js

by SardarNL, 14 years ago

takes group in account

by SardarNL, 14 years ago

Attachment: options.py.diff added

Django 1.2.4 patch, let admin recognize groups in selects

comment:6 by Graham King, 14 years ago

Severity: Normal
Type: Bug

comment:7 by patchhammer, 14 years ago

Easy pickings: unset
Patch needs improvement: set

options.py.diff fails to apply cleanly on to trunk

comment:8 by Julien Phalip, 14 years ago

UI/UX: set

by bsimons, 14 years ago

Attachment: options.py-corrected.diff added

update of patch for options.py made to work with trunk

comment:9 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:10 by Ryno, 10 years ago

Did this change ever make it to trunk?
I can't seem to find it.

comment:11 by Tim Graham, 10 years ago

No, the ticket is still open. It doesn't appear we have a tested patch.

comment:12 by Narbonne, 6 years ago

Cc: Narbonne added
Type: BugNew feature
Version: 1.2master

subscribing too. a bit sad to see the feature I was looking for in an abandoned old ticket.

comment:13 by Simon Charette, 6 years ago

Type: New featureBug

Hello Narbonne,

I get your feeling about discovering a still unsolved long standing issue. Most of Django contributors are volunteers though and issues only get fixed if someone commits to spend a part of their limited amount of time to submitting a patch and testing it. Given this issue hasn't received a lot of attention in the past years I assume it wasn't critical enough for someone to some of their time to it.

If it is to you I'd encourage you to go through the contributing guide and try to revive the previous patch. From a quick look it seems like adding optgroup support to SelectBox.js and adding a group_by option to ModelChoiceField à la #27331.

comment:14 by Jignesh Kotadiya, 5 years ago

Owner: changed from nobody to Jignesh Kotadiya
Status: newassigned

comment:15 by Jignesh Kotadiya, 5 years ago

Cc: Jignesh Kotadiya added
Easy pickings: set

comment:16 by Jignesh Kotadiya, 5 years ago

Last edited 5 years ago by Jignesh Kotadiya (previous) (diff)

comment:18 by Abhiraj Chatterjee, 4 years ago

Owner: changed from Jignesh Kotadiya to Abhiraj Chatterjee

comment:19 by Marius Räsener, 4 years ago

Hello everybody,

I'm trying to contribute to Django with this "easy picking" issue. The PR on Github was closed by Jignesh Kotadiya and currently the PR says jigneshkotadiya000 wants to merge 0 commits into 'django:master' from 'unknown repository' - so I'm already stuck I guess.
Is this still issue still open? If yes, should I give it a try?

comment:20 by ᴙɘɘᴙgYmɘᴙɘj, 3 years ago

Here's a minimal repro for anyone who is interested.
https://github.com/reergymerej/ticket_13883

Last edited 3 years ago by ᴙɘɘᴙgYmɘᴙɘj (previous) (diff)

comment:21 by Mariusz Felisiak, 3 years ago

Owner: Abhiraj Chatterjee removed
Status: assignednew

by Michael McLarnon, 3 years ago

Attachment: 13883-fix.patch added

Patch to retain option groups, and test

comment:22 by Mariusz Felisiak, 3 years ago

Thanks, please send PR via GitHub.

comment:23 by Narbonne, 3 years ago

Description: modified (diff)

I don't see an update of the filter method. I suppose search should also search in the optgroup name (a bit like sort before token search).
I update your patch line 45 in filter:

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

Also, maybe it could make sense to move optgroup as a block.

comment:25 by Michael McLarnon, 3 years ago

Cc: Michael McLarnon added

comment:26 by Mariusz Felisiak, 3 years ago

Owner: set to Michael McLarnon
Patch needs improvement: unset
Status: newassigned

comment:27 by Mariusz Felisiak, 3 years ago

Needs tests: set
Patch needs improvement: set

comment:28 by Jacob Walls, 2 years ago

Patch needs improvement: unset

comment:29 by Natalia Bidart, 19 months ago

Needs tests: unset

The PR author reported that the PR is ready for re-review, so I'm clearing the flags.

comment:30 by Mariusz Felisiak, 19 months ago

Patch needs improvement: set

comment:31 by rzrwolf, 17 months ago

13 years to fix the issue...

What is current status? Any beta that can be used, please?

in reply to:  31 comment:32 by Mariusz Felisiak, 17 months ago

Replying to rzrwolf:

13 years to fix the issue...

What is current status? Any beta that can be used, please?

This is an open-source, these kind of comments are unhelpful. Feel-free to prepare a patch, or test PR.

comment:33 by Aditya Chaudhary, 3 months ago

Last edited 3 months ago by Aditya Chaudhary (previous) (diff)

comment:34 by Ülgen Sarıkavak, 7 weeks ago

Cc: Ülgen Sarıkavak added

comment:35 by seanhelvey, 12 days ago

Owner: changed from Michael McLarnon to seanhelvey
Note: See TracTickets for help on using tickets.
Back to Top