Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5355 closed (fixed)

DecimalField may "clean" itself and then raise an Exception

Reported by: Weipin Xia <webbing@…> Owned by: PhiR
Component: Forms Version: master
Severity: Keywords: newforms, sprintsept14
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

DecimalField may "clean" itself and then raise an Exception. In my case, it happens when I put a DecimalField in a django.newforms.Form class which will be used to "Edit" the model.

I suppose it would be necessary to check the type of parameter value like some of the other "Field" classes, such as DateField.

         super(DecimalField, self).clean(value)
         if not self.required and value in EMPTY_VALUES:
             return None
         value = value.strip() # raise AttributeError, 'Decimal' object has no attribute 'strip'
         try:
             value = Decimal(value)

Attachments (2)

fields.py.patch (441 bytes) - added by Weipin Xia <webbing@…> 8 years ago.
5355.diff (1.3 KB) - added by PhiR 8 years ago.
correct patch with regression tests

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by Weipin Xia <webbing@…>

comment:1 Changed 8 years ago by PhiR

  • Has patch unset
  • Keywords newforms added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

There's no reason value would already be a decimal unless the data was a decimal in the first place. If you use a form with string input this will never happen. Maybe we should check in form inputs that the values are really strings and/or enforce this ?

comment:2 Changed 8 years ago by PhiR

  • Keywords sprintsept14 added
  • Owner changed from nobody to PhiR
  • Triage Stage changed from Design decision needed to Accepted

Changed 8 years ago by PhiR

correct patch with regression tests

comment:3 Changed 8 years ago by PhiR

  • Has patch set
  • Status changed from new to assigned

comment:4 Changed 8 years ago by Fredrik Lundh <fredrik@…>

  • Triage Stage changed from Accepted to Ready for checkin

comment:5 Changed 8 years ago by mtredinnick

So, for reference, there were a few problems with this patch.

  • The first version didn't check the precision.
  • The second version didn't work on input like " 3.14".
  • The test in the second version showed that IntegerField was cleaning 3.14 (the number) to 3, rather than raising an error.

I'm committing something that fixes all of these.

comment:6 Changed 8 years ago by mtredinnick

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

(In [6282]) Fixed #5355 -- Fixed data cleaning for DecimalField.

In passing, fixed a problem with cleaning in IntegerField.

Includes tests from PhiR.

comment:7 Changed 8 years ago by Fredrik Lundh <fredrik@…>

There were an extensive discussion last night (european time); I simply assumed that tests in the final patch matched what was agreed upon.

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