Opened 18 years ago
Closed 16 years ago
#2037 closed enhancement (fixed)
Doesn't make sense to have a blank option in dropdowns for non-nullable, non-blankable fields with defaults
Reported by: | Joeboy | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | minor | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
It makes sense if null or blank is a valid option, or if there's no default (so the user can't mistakenly skip the field). Otherwise it's a (admittedly minor) blemish on your spangly admin section.
Attachments (2)
Change History (13)
comment:1 by , 18 years ago
comment:2 by , 18 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Here's one:
from django.db import models # Create your models here. class TestModel(models.Model): """ Simple Test Model. """ CHOICES = ( (0, 'State A'), (1, 'State B'), ) emptynotallowed = models.CharField(maxlength=1, choices=CHOICES) emptyallowed = models.CharField(maxlength=1, choices=CHOICES, null=True, blank=True) defaultemptynotallowed = models.CharField(maxlength=1, choices=CHOICES, default=2) defaultemptyallowed = models.CharField(maxlength=1, choices=CHOICES, null=True, blank=True, default=2) class Admin: pass
So - only emptynotallowed should have the blank drop-down option
comment:3 by , 17 years ago
From my POV, Field.get_choices() actually adds the BLANK line if the parameter include_blank is True (which is the default) it does not consider self.blank which is the parameter given when defining the field.
The expected behaviour is that if self.blank = False then Field.get_choices must not add the blank line, I will upload a patch following this reasoning in a while.
by , 17 years ago
Patching the behaviour of Field.get_choices() + tests
comment:4 by , 17 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
The attached patch does:
- Change the behaviour of Field.get_choices() so it never adds the Blank option if self.blank = False
- Adds Field.get_choices() tests on the choices tests
- Changes tests/modeltests/manipulators/models.py
The issue reported in this ticket also affects ForeignKeys, on [source:django/trunk/tests/modeltests/manipulators/models.py] there's Album which has a ForeignKey to Musician and on the validation below it expects a blank choice for the relation while it's defined null=False so I stripped the blank choice as the patch above will strip it.
Maybe a second test should be added with a ForeignKey set null=True to make sure that there's always a blank choice ;)
(mtredinnick: this is the patch I owe for #4330 (comment 6) hehe).
comment:5 by , 17 years ago
Patch needs improvement: | unset |
---|
Ok, get_choices() has to check self.blank and self.null, if any of both is True the blank choice is given.
I also added tests on ForeignKey for the future. That should be "Ready for Check-in" but I'll leave that in hands of a triager ;))
comment:6 by , 17 years ago
Patch needs improvement: | set |
---|
The patch looks fine (well, I'm not great fan of inline "and and and or" constructs), but no longer applies cleanly. Got time to clean it up?
comment:7 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
Releasing.... should be patched against newforms
comment:9 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Will port it to newforms (as asked at #django-sprint)
comment:10 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:11 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
r6733 accomplished what was being asked for here.
Could you paste a sample model? That'd really save some time in verifying and fixing this.