Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5355 closed (fixed)

DecimalField may "clean" itself and then raise an Exception

Reported by: Weipin Xia <webbing@…> Owned by: Philippe Raoult
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@…> 9 years ago.
5355.diff (1.3 KB) - added by Philippe Raoult 9 years ago.
correct patch with regression tests

Download all attachments as: .zip

Change History (9)

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

Attachment: fields.py.patch added

comment:1 Changed 9 years ago by Philippe Raoult

Has patch: unset
Keywords: newforms added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign 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 9 years ago by Philippe Raoult

Keywords: sprintsept14 added
Owner: changed from nobody to Philippe Raoult
Triage Stage: Design decision neededAccepted

Changed 9 years ago by Philippe Raoult

Attachment: 5355.diff added

correct patch with regression tests

comment:3 Changed 9 years ago by Philippe Raoult

Has patch: set
Status: newassigned

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

Triage Stage: AcceptedReady for checkin

comment:5 Changed 9 years ago by Malcolm Tredinnick

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 9 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(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 9 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