Code

Opened 8 years ago

Closed 6 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@…> 7 years ago.
Patching the behaviour of Field.get_choices() + tests
2037_2.diff (2.8 KB) - added by Marc Fargas <telenieko@…> 7 years ago.
ForeignKeys didn't work as expected

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by adrian

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

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

  • Triage Stage changed from Unreviewed to 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 Changed 7 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 7 years ago by Marc Fargas <telenieko@…>

Patching the behaviour of Field.get_choices() + tests

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

ForeignKeys didn't work as expected

comment:5 Changed 7 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 7 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 7 years ago by toke

  • Owner changed from nobody to toke
  • Status changed from new to assigned

comment:8 Changed 7 years ago by toke

  • Owner changed from toke to nobody
  • Status changed from assigned to new

Releasing.... should be patched against newforms

comment:9 Changed 7 years ago by toke

  • Owner changed from nobody to toke
  • Status changed from new to assigned

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

comment:10 Changed 6 years ago by toke

  • Owner changed from toke to nobody
  • Status changed from assigned to new

comment:11 Changed 6 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from new to closed

r6733 accomplished what was being asked for here.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.