Opened 19 years ago
Closed 17 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 , 19 years ago
comment:2 by , 19 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 , 18 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 , 18 years ago
Patching the behaviour of Field.get_choices() + tests
comment:4 by , 18 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 , 18 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 , 18 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 , 18 years ago
| Owner: | changed from to | 
|---|---|
| Status: | new → assigned | 
comment:8 by , 18 years ago
| Owner: | changed from to | 
|---|---|
| Status: | assigned → new | 
Releasing.... should be patched against newforms
comment:9 by , 18 years ago
| Owner: | changed from to | 
|---|---|
| Status: | new → assigned | 
Will port it to newforms (as asked at #django-sprint)
comment:10 by , 17 years ago
| Owner: | changed from to | 
|---|---|
| Status: | assigned → new | 
comment:11 by , 17 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.