Opened 5 years ago

Closed 5 years ago

#17165 closed Bug (duplicate)

SelectDateWidget does not work correctly with has_changed()

Reported by: rene@… Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords: SelectDateWidget changed has_changed changed_data
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

See http://groups.google.com/group/django-users/browse_thread/thread/45c2a4b6a2f0ba67

Attachments (2)

1.3.X.diff (1.8 KB) - added by René Fleschenberg 5 years ago.
Patch against the 1.3.X branch.
trunk.diff (1.9 KB) - added by René Fleschenberg 5 years ago.
Patch against trunk

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by René Fleschenberg

Owner: changed from nobody to René Fleschenberg
Status: newassigned

Changed 5 years ago by René Fleschenberg

Attachment: 1.3.X.diff added

Patch against the 1.3.X branch.

comment:2 Changed 5 years ago by René Fleschenberg

Has patch: set
Owner: René Fleschenberg deleted
Status: assignednew
Version: 1.3SVN

comment:3 Changed 5 years ago by René Fleschenberg

The problem is that Widget._has_changed compares a localized string representation of the date to a datetime.date object. The patch addresses the issue by overriding _has_changed in SelectDateWidget and passing the data value as a datetime.date object to Widget._has_changed.

comment:4 Changed 5 years ago by anonymous

Owner: set to nobody

Changed 5 years ago by René Fleschenberg

Attachment: trunk.diff added

Patch against trunk

comment:5 Changed 5 years ago by Julien Phalip

Triage Stage: UnreviewedAccepted

Thanks for the report. The patch looks good, I'm just about to push it. You'll note that I've just fleshed out the tests a bit to be sure it works if the field's show_hidden_initial is True. Cheers!

comment:6 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: newclosed

In [17071]:

Fixed #17165 -- Fixed SelectDateWidget._has_changed() to work correctly with a localized date format.

comment:7 Changed 5 years ago by Julien Phalip

rene_f, my apologies for not mentioning you in the commit message. Thanks again for your contribution.

comment:8 Changed 5 years ago by René Fleschenberg

julien, no problem. Thanks for your quick reaction.

Will the fix also be applied to the 1.3.X branch? It might be nice to have it included in a possible 1.3.2 release. I attached a separate diff for 1.3.X (avoids datetime.strptime for Python 2.4 compatibility).

comment:9 Changed 5 years ago by Julien Phalip

The policy is to only commit severe bug fixes to older branches; see: https://docs.djangoproject.com/en/dev/internals/release-process/#supported-versions
I don't think the severity of this bug qualifies, so you'll have to wait till the next release which should hopefully happen soon.

comment:10 Changed 5 years ago by fon.vosi@…

Patch needs improvement: set

doesn't work for me
when SelectDateWidget is empty and i check "if 'fieldname' in self.changed_data" for some other field
django fires TypeError "must be string, not None" in django/forms/extras/widgets.py "data = datetime_safe.datetime.strptime(data, input_format).date()"

so, you should make something like

if data is not None:

data = datetime_safe.datetime.strptime(data, input_format).date()

else:

data = None

comment:11 Changed 5 years ago by Volodymyr Tartynskyi

sorry, previous one was rather stupid )))
it would be better to

try:
  input_format = get_format('DATE_INPUT_FORMATS')[0]
  data = datetime.datetime.strptime(data, input_format).date()
except (TypeError, ValueError):
  pass
Last edited 5 years ago by Volodymyr Tartynskyi (previous) (diff)

comment:12 Changed 5 years ago by Volodymyr Tartynskyi

Resolution: fixed
Status: closedreopened

comment:13 Changed 5 years ago by anonymous

This is a duplicate of: #17542 that is already fixed

comment:14 Changed 5 years ago by Jannis Leidel

Resolution: duplicate
Status: reopenedclosed
Note: See TracTickets for help on using tickets.
Back to Top