Opened 10 years ago

Closed 8 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: master
Severity: minor Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

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)

2037.diff (2.5 KB) - added by Marc Fargas <telenieko@…> 9 years ago.
Patching the behaviour of Field.get_choices() + tests
2037_2.diff (2.8 KB) - added by Marc Fargas <telenieko@…> 9 years ago.
ForeignKeys didn't work as expected

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by Adrian Holovaty

Could you paste a sample model? That'd really save some time in verifying and fixing this.

comment:2 Changed 10 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedAccepted

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 Changed 9 years ago by Marc Fargas <telenieko@…>

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.

Changed 9 years ago by Marc Fargas <telenieko@…>

Attachment: 2037.diff added

Patching the behaviour of Field.get_choices() + tests

comment:4 Changed 9 years ago by Marc Fargas <telenieko@…>

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

Changed 9 years ago by Marc Fargas <telenieko@…>

Attachment: 2037_2.diff added

ForeignKeys didn't work as expected

comment:5 Changed 9 years ago by Marc Fargas <telenieko@…>

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 Changed 9 years ago by Fredrik Lundh <fredrik@…>

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 Changed 9 years ago by Thomas Kerpe

Owner: changed from nobody to Thomas Kerpe
Status: newassigned

comment:8 Changed 9 years ago by Thomas Kerpe

Owner: changed from Thomas Kerpe to nobody
Status: assignednew

Releasing.... should be patched against newforms

comment:9 Changed 9 years ago by Thomas Kerpe

Owner: changed from nobody to Thomas Kerpe
Status: newassigned

Will port it to newforms (as asked at #django-sprint)

comment:10 Changed 8 years ago by Thomas Kerpe

Owner: changed from Thomas Kerpe to nobody
Status: assignednew

comment:11 Changed 8 years ago by Karen Tracey

Resolution: fixed
Status: newclosed

r6733 accomplished what was being asked for here.

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