Code

Opened 3 years ago

Closed 3 years ago

#16570 closed Bug (fixed)

allow DecimalFields with max_digits == decimal_places

Reported by: dcwatson Owned by: kenth
Component: Database layer (models, ORM) Version: 1.3
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This was discussed on django-dev here:

https://groups.google.com/d/topic/django-developers/TvMNbPWwetY/discussion

Essentially, the fix for #7726 caused a validation error for max_digits == decimal_places, which is a valid numeric type in the database.

Attachments (3)

16570.diff (2.9 KB) - added by dcwatson 3 years ago.
Patch against trunk r16579, all tests pass.
16570-2.diff (3.5 KB) - added by dcwatson 3 years ago.
Updated with documentation patch
16570-3.diff (3.6 KB) - added by kenth 3 years ago.
Updated patch to account for refactoring in #16681

Download all attachments as: .zip

Change History (11)

Changed 3 years ago by dcwatson

Patch against trunk r16579, all tests pass.

comment:1 Changed 3 years ago by mk

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

The documentation needs to be changed, too:

.. attribute:: DecimalField.max_digits

    The maximum number of digits allowed in the number. Note that this number
    must be greater than ``decimal_places``, if it exists.

Changed 3 years ago by dcwatson

Updated with documentation patch

comment:2 Changed 3 years ago by dcwatson

  • Patch needs improvement unset

comment:3 Changed 3 years ago by mtredinnick

  • Needs tests set
  • Patch needs improvement set

Feels like there should be a test included here. However, my reading of the original ticket we are modifying is that there was a problem with Django normalising .1234 to be 0.1234, so maybe a test is hard to write.

However, failing an argument that it's impossible to test at the Django level (and hence only relevant when Django creates the table), it needs a test to avoid future regressions.

comment:4 Changed 3 years ago by dcwatson

I think the problem was that Django used to do it's own validation of decimals (see #7064 and the associated patch). It seems now, Django simply tries the Decimal constructor and catches any exception. Since Decimal('0.123') == Decimal('.123') (both get normalized to include the leading 0) I don't think this is an issue.

comment:5 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:6 Changed 3 years ago by kenth

  • Owner changed from nobody to kenth

Changed 3 years ago by kenth

Updated patch to account for refactoring in #16681

comment:7 Changed 3 years ago by kenth

  • Needs tests unset
  • Patch needs improvement unset

comment:8 Changed 3 years ago by kmtracey

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

In [17089]:

Fix #16570: Restore ability to have decimal fields where max_digits equals decimal_places. Thanks dcwatson and kenth.

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.