Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#13390 closed (fixed)

Using AdminSplitDateTime and clean method in DateTimeField

Reported by: vaxXxa Owned by: nobody
Component: Forms Version: 1.2-beta
Severity: Keywords: AdminSplitDateTime
Cc: gregor@… 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

models.py

class Order(models.Model):
    delivery_time_start = models.DateTimeField('Delivery Start', blank=True, null=True)
    delivery_time_end = models.DateTimeField('Delivery End', blank=True, null=True)

forms.py

from django.contrib.admin import widgets

class OrderForm(forms.ModelForm):
    class Meta:
        model = Order
        widgets = {
            'delivery_time_start': widgets.AdminSplitDateTime(),
            'delivery_time_end': widgets.AdminSplitDateTime(),
        }

When I try to leave field delivery_time_start and delivery_time_end empty, I have error: "Enter a valid date/time." cause clean method in DateTimeField get value - list of 2 empty string [u"", u""]:

    def clean(self, value):
        """
        Validates that the input can be converted to a datetime. Returns a
        Python datetime.datetime object.
        """
        super(DateTimeField, self).clean(value)
        if value in EMPTY_VALUES:
            return None
        if isinstance(value, datetime.datetime):
            return value
        if isinstance(value, datetime.date):
            return datetime.datetime(value.year, value.month, value.day)
        if isinstance(value, list):
            # Input comes from a SplitDateTimeWidget, for example. So, it's two
            # components: date and time.
            if len(value) != 2:
                raise ValidationError(self.error_messages['invalid'])
            value = '%s %s' % tuple(value)

, but EMPTY_VALUES has only None and "".

So, solution is - EMPTY_VALUES must contains list of 2 empty string.

Attachments (3)

13390.tests.patch (670 bytes ) - added by Gregor Müllegger 14 years ago.
Failing test.
13390.fix.patch (591 bytes ) - added by Gregor Müllegger 14 years ago.
Naive bug fix, needs review.
13390.patch (1.7 KB ) - added by Gregor Müllegger 14 years ago.
Fix and tests in one patch.

Download all attachments as: .zip

Change History (10)

by Gregor Müllegger, 14 years ago

Attachment: 13390.tests.patch added

Failing test.

by Gregor Müllegger, 14 years ago

Attachment: 13390.fix.patch added

Naive bug fix, needs review.

comment:1 by Gregor Müllegger, 14 years ago

Cc: gregor@… added

comment:2 by Chris Beaven, 14 years ago

milestone: 1.2
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Really, a SplitDateTimeField should be used with the widget, but since this logic already exists in DateTimeField.clean it should be fixed to work correctly.
(and for future reference, a single patch with both fix and tests is preferred)

The patch doesn't ensure validation for datetime fields which are required (and use a the multi widget). You'd need something like this in the patch:

        if self.required:
            raise ValidationError(self.error_messages['required'])
        return None

I'm dropping the 1.2 milestone (that's only for bugs introduced by 1.2 features)

by Gregor Müllegger, 14 years ago

Attachment: 13390.patch added

Fix and tests in one patch.

comment:3 by Gregor Müllegger, 14 years ago

I added a few more assertions to the test to make sure that required fields are handled corretly. I think you're point is already covered with the code. The to_python method of DateTimeField returns None if the value is a list of two times which are empty values. So the clean method in Field.clean will get None if an empty value from SplitedDateTimeWidget is passed into the field and will handle the required attribute correctly.

comment:4 by Chris Beaven, 14 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Right you are, was looking at 1.1 code (the clean method in the initial description of the ticket).

comment:5 by vaxXxa, 14 years ago

So, I thought, that this patch:

if value[0] in validators.EMPTY_VALUES and value[1] in validators.EMPTY_VALUES: 
    return None

will be integrated in Django 1.2 RC or Django 1.2. Will it?

comment:6 by jbronn, 14 years ago

Resolution: fixed
Status: newclosed

(In [13753]) Fixed #13390 -- SplitDateTimeWidget now recognizes when it's no longer required. Thanks vaxXxa for bug report and patch.

comment:7 by jbronn, 14 years ago

(In [13754]) [1.2.X] Fixed #13390 -- SplitDateTimeWidget now recognizes when it's no longer required. Thanks vaxXxa for bug report and patch.

Backport of r13753 from trunk.

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