Ticket/22502 #2638

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

melinath commented May 7, 2014

Nixed microseconds on datetime BoundField values generated from a callable default. Fixed Django ticket 22502.

Explanation: If a callable default creates an initial datetime value with microseconds, those microseconds will be reflected in the hidden input used to track the initial value - but not in the datetime field presented to the user. That means that the field will always show up as changed when the user submits the form. This is particularly relevant for extra forms on a formset (which, due to this bug, will run validation and potentially be saved even though if the user hasn't touched them).

The only potential edge case where I could see this causing a problem would be if someone wrote a datetime field which had microsecond support. I don't think that's something folks will want to do, though.

@claudep claudep and 1 other commented on an outdated diff May 8, 2014

django/forms/forms.py
@@ -590,6 +591,14 @@ def value(self):
data = self.form.initial.get(self.name, self.field.initial)
if callable(data):
data = data()
+ # If this is an auto-generated default date, nix the
+ # microseconds for standardized handling. See #22502.
+ if ((isinstance(data, datetime.datetime) or
+ isinstance(data, datetime.time)) and
+ not getattr(self.field.widget,
+ 'supports_microseconds',
+ False)):
@claudep

claudep May 8, 2014

Member

nitpick: we are not so attached to the 80 chars limit, I think having those 3 lines on one should be fine.

@melinath

melinath May 8, 2014

Contributor

It will be done.

@melinath

melinath May 8, 2014

Contributor

And lo, it is done. ;-)

@claudep claudep and 1 other commented on an outdated diff May 9, 2014

django/forms/forms.py
@@ -590,6 +591,12 @@ def value(self):
data = self.form.initial.get(self.name, self.field.initial)
if callable(data):
data = data()
+ # If this is an auto-generated default date, nix the
+ # microseconds for standardized handling. See #22502.
+ if ((isinstance(data, datetime.datetime) or
+ isinstance(data, datetime.time)) and
+ not getattr(self.field.widget, 'supports_microseconds', False)):
@claudep

claudep May 9, 2014

Member

Now I wonder if the False default is right. Imagine someone uses a plain TextInput, does it not support microseconds? What's your opinion?

@melinath

melinath May 9, 2014

Contributor

That seems like a reasonable thought. Though a BooleanField doesn't really
"support" microseconds, in that it doesn't support datetimes... But that's
the user's responsibility at that point.
On May 8, 2014 11:28 PM, "Claude Paroz" notifications@github.com wrote:

In django/forms/forms.py:

@@ -590,6 +591,12 @@ def value(self):
data = self.form.initial.get(self.name, self.field.initial)
if callable(data):
data = data()

  •            # If this is an auto-generated default date, nix the
    
  •            # microseconds for standardized handling. See #22502.
    
  •            if ((isinstance(data, datetime.datetime) or
    
  •                    isinstance(data, datetime.time)) and
    
  •                    not getattr(self.field.widget, 'supports_microseconds', False)):
    

Now I wonder if the False default is right. Imagine someone uses a plain
TextInput, does it not support microseconds? What's your opinion?


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/2638/files#r12465491
.

@melinath

melinath May 9, 2014

Contributor

Although... Does TextInput support datetimes?
On May 9, 2014 1:56 AM, stephen.r.burrows@gmail.com wrote:

That seems like a reasonable thought. Though a BooleanField doesn't really
"support" microseconds, in that it doesn't support datetimes... But that's
the user's responsibility at that point.
On May 8, 2014 11:28 PM, "Claude Paroz" notifications@github.com wrote:

In django/forms/forms.py:

@@ -590,6 +591,12 @@ def value(self):
data = self.form.initial.get(self.name, self.field.initial)
if callable(data):
data = data()

  •            # If this is an auto-generated default date, nix the
    
  •            # microseconds for standardized handling. See #22502.
    
  •            if ((isinstance(data, datetime.datetime) or
    
  •                    isinstance(data, datetime.time)) and
    
  •                    not getattr(self.field.widget, 'supports_microseconds', False)):
    

Now I wonder if the False default is right. Imagine someone uses a plain
TextInput, does it not support microseconds? What's your opinion?


Reply to this email directly or view it on GitHubhttps://github.com/django/django/pull/2638/files#r12465491
.

@claudep

claudep May 9, 2014

Member

Although... Does TextInput support datetimes?

Yes, with TextInput, the date is serialized like this: 2014-05-09 14:08:21.805873 and you can manually edit any part, including microseconds.

@melinath

melinath May 9, 2014

Contributor

Switched default to true.

@melinath melinath Made explicit lack of microsecond handling by built-in datetime fields.
Used that explicitness to appropriately nix microsecond values in bound fields.
Fixed Django ticket 22502.
905a94f
Member

claudep commented May 10, 2014

I've just slightly modified the patch (isolating the test in a new test method) and pushed in a5de0df. Thanks!

claudep closed this May 10, 2014

melinath deleted the melinath:ticket/22502 branch May 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment