Opened 5 years ago

Closed 5 years ago

Last modified 5 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: UI/UX:

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 phxx 5 years ago.
Failing test.
13390.fix.patch (591 bytes) - added by phxx 5 years ago.
Naive bug fix, needs review.
13390.patch (1.7 KB) - added by phxx 5 years ago.
Fix and tests in one patch.

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by phxx

Failing test.

Changed 5 years ago by phxx

Naive bug fix, needs review.

comment:1 Changed 5 years ago by phxx

  • Cc gregor@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by SmileyChris

  • milestone 1.2 deleted
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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)

Changed 5 years ago by phxx

Fix and tests in one patch.

comment:3 Changed 5 years ago by phxx

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

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

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

comment:5 Changed 5 years ago by vaxXxa

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 Changed 5 years ago by jbronn

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

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

comment:7 Changed 5 years ago by jbronn

(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