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 )
Attachments (9)
Change History (43)
by , 14 years ago
Attachment: | bad_select.png added |
---|
by , 14 years ago
Attachment: | good_select.png added |
---|
Better SelectBox.js screenshot. optgroups are recreated on redisplay()
by , 14 years ago
Attachment: | SelectBox.js added |
---|
Better SelectBox.js which handles optgroup, depends on jQuery
comment:1 by , 14 years ago
Component: | Contrib apps → django.contrib.admin |
---|
comment:3 by , 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 , 14 years ago
Keywords: | sprintdec2010 added |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → 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.
by , 14 years ago
Attachment: | 13883_initial_patch_as_diff.diff added |
---|
comment:5 by , 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 , 14 years ago
Attachment: | options.py.diff added |
---|
Django 1.2.4 patch, let admin recognize groups in selects
comment:6 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:7 by , 14 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
options.py.diff fails to apply cleanly on to trunk
comment:8 by , 14 years ago
UI/UX: | set |
---|
by , 14 years ago
Attachment: | options.py-corrected.diff added |
---|
update of patch for options.py made to work with trunk
comment:11 by , 10 years ago
No, the ticket is still open. It doesn't appear we have a tested patch.
comment:12 by , 6 years ago
Cc: | added |
---|---|
Type: | Bug → New feature |
Version: | 1.2 → master |
subscribing too. a bit sad to see the feature I was looking for in an abandoned old ticket.
comment:13 by , 6 years ago
Type: | New feature → Bug |
---|
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 , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:15 by , 5 years ago
Cc: | added |
---|---|
Easy pickings: | set |
comment:16 by , 5 years ago
comment:18 by , 4 years ago
Owner: | changed from | to
---|
comment:19 by , 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 , 3 years ago
Here's a minimal repro for anyone who is interested.
https://github.com/reergymerej/ticket_13883
comment:21 by , 3 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:23 by , 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 , 3 years ago
Cc: | added |
---|
comment:27 by , 3 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:28 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:29 by , 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 , 19 months ago
Patch needs improvement: | set |
---|
follow-up: 32 comment:31 by , 17 months ago
13 years to fix the issue...
What is current status? Any beta that can be used, please?
comment:32 by , 17 months ago
comment:34 by , 7 weeks ago
Cc: | added |
---|
comment:35 by , 12 days ago
Owner: | changed from | to
---|
Created PR: https://github.com/django/django/pull/18934.
Current SelectBox.js screenshot