Code

Opened 5 months ago

Closed 5 months ago

#21397 closed Bug (fixed)

forms.TypedChoiceField validation problem

Reported by: Elec Owned by: claudep
Component: Forms Version: 1.6
Severity: Normal Keywords:
Cc: 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

I get ValidationError on this code:

POST_DAYS_CHOICE = (
    (7, '1 week'),
    (14, '2 weeks'),
    (30, '1 month'),
    (60, '2 months'),
    (90, '3 months'),
)

def days_to_till_date(val):
    print val
    try:
        till_date = date.today() + timedelta(int(val))
    except ValueError:
        till_date = date.today() + timedelta(30)
    return till_date

class PostForm(forms.ModelForm):
    till_date = forms.TypedChoiceField(label='Period', choices=POST_DAYS_CHOICE, coerce=days_to_till_date, required=True, initial=30)

This is was removed from TypedChoiceField class:

    def validate(self, value):
        pass

Attachments (1)

21397-1.diff (4.9 KB) - added by claudep 5 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 months ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

This was changed in [9883551d50e].

Interesting use case, which is currently not tested in Django. Having the coerced value not in choices is a bit of a corner case (and not clearly documented), but I admit this might be considered as a regression.

I'll see if I can come up with a fix which is not too invasive...

In any case, the documentation will have to be clarified on this point.

Changed 5 months ago by claudep

comment:2 Changed 5 months ago by claudep

  • Has patch set

Attached a patch with a possible fix. Test suite passes with SQLite. I'll let other core developers judge if this warrants to be added in a stable release.

comment:3 Changed 5 months ago by claudep

Thinking a bit more about this, I think that it's too risky to push my patch in the 1.6 stable branch. We can document the regression in the 1.6 release notes, but you will probably have to create a custom field to workaround the issue until the next release.
Please, next time try to test your project with beta or at least RC releases, so we can catch those issues before the official release.

comment:4 Changed 5 months ago by Elec

Hi, no problem. I just test my project with 1.6 release. I create custom field.

comment:5 Changed 5 months ago by claudep

  • Owner changed from nobody to claudep
  • Status changed from new to assigned

Assigned to me, but would love to receive another core dev opinion.

comment:6 Changed 5 months ago by charettes

  • Severity changed from Release blocker to Normal
  • Triage Stage changed from Accepted to Ready for checkin

While you did a great job at putting a non-invasive patch together I'm not sure we should commit to backport it.

First because it doesn't meet any of the policy criterias and second because it concerns an undocumented and corner use case.

IMHO the small risk of introducing another regression, there's a small one given the original to_python override, is not worth the backport.

On the other hand -- apart the missing a release note -- the patch looks RFC for master.

comment:7 Changed 5 months ago by claudep

Proposal for a 1.6 release note addition (misc backwards incompatibilities):

* Due to a change in the form validation workflow,
  :class:`~django.forms.TypedChoiceField` ``coerce`` method should always
  return a value present in the ``choices`` field attribute. That limitation
  should be lift again in Django 1.7.

Does it look sane?

comment:8 Changed 5 months ago by charettes

LGTM

comment:9 Changed 5 months ago by Claude Paroz <claude@…>

In 4a00f132e0cc5246f7e7bd04b6d84a9d9ea4a0c1:

Added release note for TypedChoiceField coerce limitation

Thanks Elec for the report and Simon Charette for the review.
Refs #21397.

comment:10 Changed 5 months ago by Claude Paroz <claude@…>

In 9f59149cfe97b3b0026bbc4ff1d2121e4cf04d0e:

[1.6.x] Added release note for TypedChoiceField coerce limitation

Thanks Elec for the report and Simon Charette for the review.
Refs #21397.
Backport of 4a00f132e0 from master.

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

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

In a0f3eeccf3d5a90f9914bfe20b15df05673ea59d:

Fixed #21397 -- Re-added flexibility to TypedChoiceField coercion

Thanks Elec for the report and Simon Charette for the review.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.