Code

Opened 7 years ago

Closed 6 years ago

#5917 closed (fixed)

django.newforms.extras.widgets.SelectDateWidget

Reported by: Camille Harang Owned by: SmileyChris
Component: Forms Version: master
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: UI/UX:

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@…> 7 years ago.
Fix + ID's
newforms.extra.widgets.py.2.diff (2.0 KB) - added by Camille Harang <mammique@…> 7 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@…> 7 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@…> 7 years ago.
Good one, forgget above :-/
newforms.extra.widgets.py.5.diff (2.0 KB) - added by Camille Harang <mammique@…> 7 years ago.
Fix + ID's 5
SelectDate_datetime.patch (819 bytes) - added by Martin Conte Mac Donell <Reflejo@…> 7 years ago.
This is IMHO a better way to do that
SelectDate_datetime.2.patch (819 bytes) - added by Martin Conte Mac Donell <Reflejo@…> 7 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@…> 7 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@…> 6 years ago.
Change to render() for SelectDateWidget and test to ensure it works
SelectDateWidget_with_test.2.diff (1.2 KB) - added by josePhoenix 6 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 josePhoenix 6 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 SmileyChris 6 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 Changed 7 years ago by Camille Harang <mammique@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
if type(value) == type(u''): => isinstance(value, unicode)

Sorry ;-/

Changed 7 years ago by Camille Harang <mammique@…>

Fix + ID's

comment:2 Changed 7 years ago by Camille Harang <mammique@…>

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.

Changed 7 years ago by Camille Harang <mammique@…>

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

Changed 7 years ago by Camille Harang <mammique@…>

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

Changed 7 years ago by Camille Harang <mammique@…>

Good one, forgget above :-/

Changed 7 years ago by Camille Harang <mammique@…>

Fix + ID's 5

Changed 7 years ago by Martin Conte Mac Donell <Reflejo@…>

This is IMHO a better way to do that

Changed 7 years ago by Martin Conte Mac Donell <Reflejo@…>

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

Changed 7 years ago by Martin Conte Mac Donell <Reflejo@…>

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

comment:3 Changed 6 years ago by anonymous

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

Changed 6 years ago by Joseph Long <josephoenix@…>

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

comment:4 Changed 6 years ago by josePhoenix

  • 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 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Ready 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.

Changed 6 years ago by josePhoenix

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 Changed 6 years ago by SmileyChris

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

Changed 6 years ago by josePhoenix

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

comment:7 Changed 6 years ago by SmileyChris

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:8 follow-up: Changed 6 years ago by ryan@…

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

comment:9 in reply to: ↑ 8 Changed 6 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Status changed from new to assigned
  • Triage Stage changed from Ready for checkin to Accepted

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.

Changed 6 years ago by SmileyChris

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

comment:10 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 6 years ago by mtredinnick

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

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

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.