Opened 16 years ago

Closed 16 years ago

#5917 closed (fixed)

django.newforms.extras.widgets.SelectDateWidget

Reported by: Camille Harang Owned by: Chris Beaven
Component: Forms Version: dev
Severity: Keywords: newforms widgets SelectDateWidget
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

django.newforms.extras.widgets.SelectDateWidget is not correctly passing the previous selected value to the HTML renderer because it sometimes receives the value as a datetime.date field or a unicode string. The code only handles it as unicode, it splits it and converts it to datetime.date, so when it receives it directly as a datetime.date try/except resets the default value as None :

    def render(self, name, value, attrs=None):
        try:
            value = datetime.date(*map(int, value.split('-')))
            year_val, month_val, day_val = value.year, value.month, value.day
        except (AttributeError, TypeError, ValueError):
            year_val = month_val = day_val = None

I added a type value test and now it works:

    def render(self, name, value, attrs=None):
        try:
            if type(value) == type(u''):
                value = datetime.date(*map(int, value.split('-')))
            year_val, month_val, day_val = value.year, value.month, value.day
        except (AttributeError, TypeError, ValueError):
            year_val = month_val = day_val = None

Regards.

Camille.

Attachments (12)

newforms.extra.widgets.py.diff (1.8 KB ) - added by Camille Harang <mammique@…> 16 years ago.
Fix + ID's
newforms.extra.widgets.py.2.diff (2.0 KB ) - added by Camille Harang <mammique@…> 16 years ago.
Fix + ID's 2 (same but uses month_field = '%s_month', etc...)
newforms.extra.widgets.py.3.diff (2.0 KB ) - added by Camille Harang <mammique@…> 16 years ago.
Fix + ID's 3 (same as Fix + ID's 2 but uses isinstance, this is the good one, sorry for flooding)
newforms.extra.widgets.py.4.diff (2.0 KB ) - added by Camille Harang <mammique@…> 16 years ago.
Good one, forgget above :-/
newforms.extra.widgets.py.5.diff (2.0 KB ) - added by Camille Harang <mammique@…> 16 years ago.
Fix + ID's 5
SelectDate_datetime.patch (819 bytes ) - added by Martin Conte Mac Donell <Reflejo@…> 16 years ago.
This is IMHO a better way to do that
SelectDate_datetime.2.patch (819 bytes ) - added by Martin Conte Mac Donell <Reflejo@…> 16 years ago.
This is IMHO a better way to do that (Sorry for the other one)
SelectDate_datetime.3.patch (825 bytes ) - added by Martin Conte Mac Donell <Reflejo@…> 16 years ago.
This is IMHO a better way to do that (Err.. Sorry for the other ones)
SelectDateWidget_with_test.diff (1.1 KB ) - added by Joseph Long <josephoenix@…> 16 years ago.
Change to render() for SelectDateWidget and test to ensure it works
SelectDateWidget_with_test.2.diff (1.2 KB ) - added by Joseph Long 16 years ago.
Checks isinstance of string in render, and adds a test. Removes TypeError from exceptions, since isinstance means that can't be raised (as pointed out by SmileyChris)
SelectDateWidget_with_test.3.diff (1.1 KB ) - added by Joseph Long 16 years ago.
Nevermind removing TypeError. It could still be raised.. this is the better version of the patch
5917.diff (3.6 KB ) - added by Chris Beaven 16 years ago.
Allow widget to receive a datetime and also render invalid dates (validation doesn't belong in the widget)

Download all attachments as: .zip

Change History (23)

comment:1 by Camille Harang <mammique@…>, 16 years ago

if type(value) == type(u''): => isinstance(value, unicode)

Sorry ;-/

by Camille Harang <mammique@…>, 16 years ago

Fix + ID's

comment:2 by Camille Harang <mammique@…>, 16 years ago

Here is the patch. I alos added individual id_* to each <select> inspired by CheckboxSelectMultiple()'s way to do it, it works, i hope it fits Django coding guidelines.

Regards.

Camille.

by Camille Harang <mammique@…>, 16 years ago

Fix + ID's 2 (same but uses month_field = '%s_month', etc...)

by Camille Harang <mammique@…>, 16 years ago

Fix + ID's 3 (same as Fix + ID's 2 but uses isinstance, this is the good one, sorry for flooding)

by Camille Harang <mammique@…>, 16 years ago

Good one, forgget above :-/

by Camille Harang <mammique@…>, 16 years ago

Fix + ID's 5

by Martin Conte Mac Donell <Reflejo@…>, 16 years ago

Attachment: SelectDate_datetime.patch added

This is IMHO a better way to do that

by Martin Conte Mac Donell <Reflejo@…>, 16 years ago

Attachment: SelectDate_datetime.2.patch added

This is IMHO a better way to do that (Sorry for the other one)

by Martin Conte Mac Donell <Reflejo@…>, 16 years ago

Attachment: SelectDate_datetime.3.patch added

This is IMHO a better way to do that (Err.. Sorry for the other ones)

comment:3 by anonymous, 16 years ago

I suggest using isinstance(value, basestring), or even wrapping the value in unicode().

def render(self, name, value, attrs=None):
    try:
        if isinstance(value, basestring):
            value = datetime.date(*map(int, value.split('-')))
        year_val, month_val, day_val = value.year, value.month, value.day
    except (AttributeError, TypeError, ValueError):
        year_val = month_val = day_val = None

    # -- snip assumed --

def render(self, name, value, attrs=None):
    try:
       
        value = datetime.date(*map(int, unicode(value).split('-')))
        year_val, month_val, day_val = value.year, value.month, value.day
    except (AttributeError, TypeError, ValueError):
        year_val = month_val = day_val = None
 
    # -- snip assumed --

by Joseph Long <josephoenix@…>, 16 years ago

Change to render() for SelectDateWidget and test to ensure it works

comment:4 by Joseph Long, 16 years ago

Has patch: set

The file I just attached (wasn't logged in, oops) adds tests for SelectDateWidget rendering for this issue, and uses isinstance instead of type() in * to see whether the value is a string. Hopefully with these, this can be patched in the trunk. (I don't know about the IDs.. should those be patched for this ticket as well, or is that a separate issue?)

Joseph

comment:5 by Chris Beaven, 16 years ago

Triage Stage: UnreviewedReady for checkin

I think that value_from_datadict should return a date anyway. But that's a side point from this bug, of which the latest patch looks good to go.

by Joseph Long, 16 years ago

Checks isinstance of string in render, and adds a test. Removes TypeError from exceptions, since isinstance means that can't be raised (as pointed out by SmileyChris)

comment:6 by Chris Beaven, 16 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Actually, I was wrong. TypeError could be raised still:
datetime.date(*map(int, '1-2'.split('-')))

by Joseph Long, 16 years ago

Nevermind removing TypeError. It could still be raised.. this is the better version of the patch

comment:7 by Chris Beaven, 16 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:8 by ryan@…, 16 years ago

If "years" is not specified but initial date value is, perhaps self.years should be range(value.year, this_year+10)

Also, if date validation fails (eg. February 31), it would be nice if the widget would repopulate with the failed date, rather than resetting everything back to the first entry in the list.

Otherwise nice... :)

in reply to:  8 comment:9 by Chris Beaven, 16 years ago

Owner: changed from nobody to Chris Beaven
Status: newassigned
Triage Stage: Ready for checkinAccepted

Replying to ryan@pressure.net.nz:

If "years" is not specified but initial date value is, perhaps self.years should be range(value.year, this_year+10)

This is a separate proposal to the fix here, feel free to open a new ticket.

Also, if date validation fails (eg. February 31), it would be nice if the widget would repopulate with the failed date, rather than resetting everything back to the first entry in the list.

While this is a separate issue (bug IMO), I'll fix it here since it's not that difficult.

Rolling back to Accepted and assigning to me.

by Chris Beaven, 16 years ago

Attachment: 5917.diff added

Allow widget to receive a datetime and also render invalid dates (validation doesn't belong in the widget)

comment:10 by Chris Beaven, 16 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [7337]) Fixed #5917 -- More error robustness in date parsing in SelectDateWidget, plus
keep the original date selected on redisplay, even if it was bogus (e.g. 31
Feb). Patch from SmileyChris.

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