Opened 18 years ago

Closed 15 years ago

Last modified 15 years ago

#4092 closed (fixed)

django/contrib/localflavor/usa/forms.py USStateSelect missing blank options

Reported by: polpak <polpak@…> Owned by: rokclimb15
Component: contrib.localflavor Version: dev
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Malcolm Tredinnick)

This field needs to have the default starting option of ('', '----------') so people don't accidentally forget to change it and have a bunch of submissions for Alabama.

I can submit a patch for this later on if needed

Chad Simmons

Attachments (4)

4092_6194_simple.diff (20.3 KB ) - added by dougn 17 years ago.
Simple patch which does not include a new base widget, but adds an empty_label to each localflavor Select widget
4092_6248_nullselect.diff (139.7 KB ) - added by dougn 17 years ago.
New NullSelect widget which is used as the base for all the localflavor select widgets, and replaces the ModelField default widget,.
4093_6352_nullselect.diff (143.4 KB ) - added by dougn 17 years ago.
Updated patch with better NullSelect implementation which accepts empty_label on render method, and has full tests
4092_9912_nullselect.diff (163.3 KB ) - added by rokclimb15 16 years ago.
Updated patch to make it compatible with r9912. Remove modifications to forms.models

Download all attachments as: .zip

Change History (26)

comment:1 by polpak <polpak@…>, 18 years ago

.... it stripped off the blank value ticktick

comment:2 by Simon G. <dev@…>, 18 years ago

Component: Uncategorizeddjango.contrib.localflavor
Owner: changed from Jacob to Adrian Holovaty
Triage Stage: UnreviewedAccepted

comment:3 by Malcolm Tredinnick, 17 years ago

Description: modified (diff)

(Fixed description formatting)

I'm not really sure what to do about this. The blank entry (if we include it), should be inserted by the widget, since it's not part of the data. And possibly only inserted if no choice is selected. We should also do the same thing for every other localflavor widget; nothing special about the US version here.

Patches welcome, certainly. Something to look at might make it easier to decide.

comment:4 by dougn, 17 years ago

I was looking at this and there are 20 Select widgets which would need to be changed.
It feels like there should be a new base widget which deals with the empty choice (The same way there is for the ModelChoiceField.

I could just be over thinking things and making them more complex than necessary.

from itertools import chain

class NullSelect(Select):
    def __init__(self, attrs=None, choices=(), empty_label=u"---------"):
        empty_choice = ((u'', empty_label),)
        super(NullSelect, self).__init__(attrs, chain(empty_choice, choices))

With this addition, the newforms.models.ModelChoiceField would no longer need the empty_label,
and would use the newforms.widgets.NullSelect widget instead.

comment:5 by dougn, 17 years ago

Owner: changed from nobody to dougn

Working on a patch

comment:6 by Allagappan (IRC:vegpuff), 17 years ago

Owner: changed from dougn to Allagappan (IRC:vegpuff)

comment:7 by Allagappan (IRC:vegpuff), 17 years ago

Owner: changed from Allagappan (IRC:vegpuff) to dougn

Didn't notice that it was claimed. Sorry. Is there going to be a new base widget?

comment:8 by dougn, 17 years ago

I am making two different patches. One w/o the new base, and one with a new base, updated docs, and a regression test.

by dougn, 17 years ago

Attachment: 4092_6194_simple.diff added

Simple patch which does not include a new base widget, but adds an empty_label to each localflavor Select widget

comment:9 by dougn, 17 years ago

NOTE: holding off on setting the 'has_patch' flag until I get the second extended patch done, then will open up the discussion to the developer list.

by dougn, 17 years ago

Attachment: 4092_6248_nullselect.diff added

New NullSelect widget which is used as the base for all the localflavor select widgets, and replaces the ModelField default widget,.

comment:10 by dougn, 17 years ago

Has patch: set

The nullselect patch includes documentation and regression tests.
It is as up to date as I can make it.

The older simple patch is now out of data and does not include required test fixes.
I can easily make a new simple patch if the NullSelect widget is rejected.

comment:11 by Jacob, 17 years ago

This looks pretty good. I'm not 100% happy with the name "NullSelect", but I'm not sure if there's a better color for that particular bikeshed.

Oh, and this needs a test for NullSelect itself (there are tests for subclasses, but no the widget itself).

by dougn, 17 years ago

Attachment: 4093_6352_nullselect.diff added

Updated patch with better NullSelect implementation which accepts empty_label on render method, and has full tests

comment:12 by dougn, 17 years ago

I really don't care for NullSelect either. I thought of other names (EmptySelect, BlankSelect, etc) but NullSelect fit the meme of NullBooleanSelect and others.

I also considered adding the empty_label to the !Select widget it's self, but there is no good way of doing this
without breaking most implementations out there. The nice part about NullSelect is that it does not change any existing behavior.

One option would be to add it to !Select, defaulting to None, and providing a EMPTY_LABEL global for people to use
(so that u'--------' is defined somewhere). This would be a rather verbose way of doing it though.

comment:13 by tim.saylor@…, 17 years ago

Without this patch I can't make an optional address in my model using USStateField. This should be committed soon.

Hmm, why is this still marked as new? It's been assigned and has a patch. Wierd.

comment:14 by Malcolm Tredinnick, 17 years ago

Tim Saylor: if you want to help advance the ticket, feel free to fix the problems that were mentioned above. Complaining adds no value.

comment:15 by Malcolm Tredinnick, 17 years ago

Patch needs improvement: set

comment:16 by Malcolm Tredinnick, 17 years ago

Patch needs improvement: unset

comment:17 by tim.saylor@…, 17 years ago

What complaints? The only things complained about recently were the name NullSelect, and Jacob said he can't think of anything better, and a lack of tests for the base NullSelect class, which dougn appears to have fixed. I'd be glad to help, but I don't know what's needed.

comment:18 by rokclimb15, 16 years ago

Owner: changed from dougn to rokclimb15@…
Patch needs improvement: set

Since this hasn't been touched in 11 months, I'm assuming that the original assignee isn't interested in completing this ticket anymore. For now, I will assign it to myself to test and polish up this patch to be compatible with the current trunk. If dougn would like to continue working on this, feel free to reassign it to him.

by rokclimb15, 16 years ago

Attachment: 4092_9912_nullselect.diff added

Updated patch to make it compatible with r9912. Remove modifications to forms.models

comment:19 by rokclimb15, 16 years ago

Owner: changed from rokclimb15@… to rokclimb15
Patch needs improvement: unset

I have updated dougn's patch so it is now compatible with r9912. I removed modifications to forms.models since I don't think we want all model generated choicefields to have this type of widget. Eliminating the empty label option produced failures in the forms test cases. If someone wants to use this widget with their ModelChoiceField, they can override the widget for the ChoiceField in kwargs.

comment:20 by anonymous, 15 years ago

What further needs to be done on this issue in order to get this patch accepted into SVN?

#10969 is also an alternate solution to dealing with this issue.

comment:21 by aditya, 15 years ago

Has patch: unset
Resolution: fixed
Status: newclosed
Summary: django/contrib/localflavor/usa/forms.py USStateSelect missing blank optionsTicket closed. This issue has been taken care of in #10969.

comment:22 by Alex Gaynor, 15 years ago

Summary: Ticket closed. This issue has been taken care of in #10969.django/contrib/localflavor/usa/forms.py USStateSelect missing blank options

Reverting the summary. This issue was fixed as a part of #10969.

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