Opened 5 years ago

Last modified 5 months ago

#13883 new Bug

SelectBox.js with grouping (optgroup elements)

Reported by: SardarNL Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords: admin, SelectBox, optgroup, sprintdec2010
Cc: minimallyuseless@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Description

Current implementation ignores optgroup elements in selects. Here is an example of SelectBox.js which takes optgroups in account, but uses jQuery (already used by django admin anyway).
The script is not perfectly written. I can rewrite this to meet django's coding/quality standards if needed.

Attachments (8)

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

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by SardarNL

Current SelectBox.js screenshot

Changed 5 years ago by SardarNL

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

Changed 5 years ago by SardarNL

Better SelectBox.js which handles optgroup, depends on jQuery

comment:1 Changed 5 years ago by SardarNL

  • Component changed from Contrib apps to django.contrib.admin
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by minimallyuseless@…

  • Cc minimallyuseless@… added

subscribing

comment:3 Changed 4 years ago by hvdklauw

  • milestone set to 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 Changed 4 years ago by julien

  • Keywords sprintdec2010 added
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 4 years ago by julien

comment:5 Changed 4 years ago by SardarNL

  • 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.

Changed 4 years ago by SardarNL

Patch to SelectBox.js

Changed 4 years ago by SardarNL

takes group in account

Changed 4 years ago by SardarNL

Django 1.2.4 patch, let admin recognize groups in selects

comment:6 Changed 4 years ago by graham_king

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

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

comment:8 Changed 4 years ago by julien

  • UI/UX set

Changed 4 years ago by bsimons

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

comment:9 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:10 Changed 5 months ago by rynomster

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

comment:11 Changed 5 months ago by timgraham

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

Note: See TracTickets for help on using tickets.
Back to Top