Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#22502 closed Bug (fixed)

Datetime fields break empty form validation on first attempt (but not second)

Reported by: melinath Owned by: melinath
Component: Forms Version: master
Severity: Normal Keywords:
Cc: melinath Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It turns out that if you have a form with empty_permitted=True (for example, an extra form in a formset) and that form has a datetime field defaulting to (timezone.)now, the form will fail validation the first time you save *even if you touch nothing* – but it will succeed if you immediately save again.

The reason for this seems to lie in the way that initial values for datetime fields with defaults are calculated. Specifically, for some reason, on first page load, the hidden "initial" field will include microseconds - but the displayed datetime field will not. Example:

<input class="form-control" type="datetime" name="options-3-available_start" value="2014-04-24 08:03:22" required="" id="id_options-3-available_start">
<input type="hidden" name="initial-options-3-available_start" value="2014-04-24 08:03:22.688836" id="initial-options-3-id_options-3-available_start">

This means that the field (and thus the form) is marked as "changed". However, when the page is re-rendered (with validation errors) the initial field's microseconds vanish. Example:

<input class="form-control" type="datetime" name="options-1-available_start" value="2014-04-24 08:03:22" required="" id="id_options-1-available_start">
<input type="hidden" name="initial-options-1-available_start" value="2014-04-24 08:03:22" id="initial-options-1-id_options-1-available_start">

So the form validates just fine on the second go-round.

I'm technically using the latest code on the 1.7.X branch, not 1.7-beta-2.

Change History (12)

comment:1 Changed 16 months ago by melinath

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I am also using floppyforms, so there's a chance this is a bug in their code rather than django. Investigating further.

comment:2 Changed 16 months ago by melinath

Nope, this is definitely a django bug. The problem is that the hidden widget used to keep track of the initial value preserves the microsecond information, while the datetime widget discards it. Solution to this bug should then be to either display the microseconds (see also #12139?) or (what I'd prefer) perform the same conversion on the data for the hidden input as for the displayed input.

Note that there is a race condition: it is possible (though unlikely) for the displayed input and hidden input to be different seconds, as well, since BoundField.value() is called twice and the results not cached.

Regarding the first vs. second page render: On the second page render, since the form is now bound, BoundField.value() returns the submitted data for the hidden initial value (as well as the displayed value) instead of the old initial value. Which seems a bit odd, since it means that data changes on any field with show_hidden_initial will first cause validation errors and then (on resubmit) simply be ignored. (That being said, I don't know that there would be a better way to handle this kind of data.)

(P.S. this bug is maybe related to #12139?)

Version 0, edited 16 months ago by melinath (next)

comment:3 Changed 16 months ago by melinath

  • Resolution set to invalid
  • Status changed from new to closed

... nope, this is a floppyforms bug. :-p Apologies for the extraneous bug. Closing now.

comment:4 Changed 16 months ago by melinath

  • Resolution invalid deleted
  • Status changed from closed to new

Re-tested, and this is present even with django-floppyforms disabled. So I'm re-opening. Sorry for the confusion.

comment:5 Changed 16 months ago by claudep

  • Triage Stage changed from Unreviewed to Someday/Maybe
  • Version changed from 1.7-beta-2 to master

Confirmed the issue, however clueless about a possible resolution. The thing is that has_changed should behave differently depending on the initial value coming from a saved value or coming from a default value:

  • 2014-04-24 08:03:22.688836 (default value) -> 2014-04-24 08:03:22 (posted value) -> field._has_changed should return False
  • 2014-04-24 08:03:22.688836 (saved value) -> 2014-04-24 08:03:22 (posted value) -> field._has_changed should return True

When the widget renders himself, he knows nothing about the origin of the data. IMHO it will be hard to change that.

comment:6 Changed 16 months ago by melinath

... or maybe we should have consistent handling of milliseconds. This problem would be resolved if we were either using datetime form fields that displayed millisecond data or if we didn't include millisecond data in default values.

comment:7 Changed 16 months ago by melinath

  • Has patch set
  • Owner changed from nobody to melinath
  • Status changed from new to assigned

I've written a patch to fix this.

From the pull request:

Nixed microseconds on datetime BoundField values generated from a callable default. Fixed [Django ticket 22502](https://code.djangoproject.com/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.

comment:8 Changed 16 months ago by claudep

  • Triage Stage changed from Someday/Maybe to Accepted

Great, the patch looks nice.
However, to mitigate the last concern, what about querying the field.widget about a supports_microseconds property (with DateTimeBaseInput.supports_microseconds defaulting to False):
if isinstance(data, datetime.datetime) and not getattr(self.field.widget, 'supports_microseconds', False):

comment:9 Changed 16 months ago by melinath

Updated the patch/pull request to use that suggestion. Also added handling of datetime.time values and a test for widgets that *do* support microseconds.

Last edited 16 months ago by melinath (previous) (diff)

comment:10 Changed 16 months ago by melinath

Updated the pull request to default to True, as per suggestion.

comment:11 Changed 16 months ago by Claude Paroz <claude@…>

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

In a5de0df58b5431b25a3246a57432db73843be87f:

Fixed #22502 -- Fixed microseconds/default/form interaction

Made explicit lack of microsecond handling by built-in datetime form
fields. Used that explicitness to appropriately nix microsecond
values in bound fields. Thanks Claude Paroz for the review.

comment:12 Changed 16 months ago by Claude Paroz <claude@…>

In 0c198035e958a3caa0bd1e0fd9f419bf1308399d:

[1.7.x] Fixed #22502 -- Fixed microseconds/default/form interaction

Made explicit lack of microsecond handling by built-in datetime form
fields. Used that explicitness to appropriately nix microsecond
values in bound fields. Thanks Claude Paroz for the review.
Backport of a5de0df58 from master.

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