Repository: level12/keg-elements

Issue:
ModelForm Requires Precision and Scale to be Set on Numeric Columns
It seems to create validators for a `ModelForm` any Numeric columns on the model are assumed to have both precision and scale set: (focus on lines 367-368)
https://github.com/level12/keg-elements/blob/0ea69ab464c61c93338196d167c220859130196b/keg_elements/forms/__init__.py#L365-L371
https://github.com/level12/keg-elements/blob/0ea69ab464c61c93338196d167c220859130196b/keg_elements/forms/__init__.py#L312-L313

I was setting up a `ModelForm` for a model with a Numeric column on which precision and scale were _not_ set. It failed like this:
```
keg_elements/forms/__init__.py:315: in _max_for_numeric
    return Decimal('{}.{}'.format('9' * (digits - scale), '9' * scale))
E   TypeError: unsupported operand type(s) for -: 'NoneType' and 'NoneType'
```

In my mind there are two possible directions to go with a fix for this:

1. We still want to require Numeric columns to have precision and scale set in order for us to generate validators for that form field. In which case, we should probably assert that precision and scale are set, and if they aren't give a more helpful error message.
2. We do not want to require Numeric columns to have precision and scale set. In which case, we should probably not call `_max_for_numeric` if either value is missing.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/level12/keg-elements/pull/134?src=pr&el=h1) Report
> Merging [#134](https://codecov.io/gh/level12/keg-elements/pull/134?src=pr&el=desc) into [master](https://codecov.io/gh/level12/keg-elements/commit/0ea69ab464c61c93338196d167c220859130196b&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/level12/keg-elements/pull/134/graphs/tree.svg?width=650&height=150&src=pr&token=jf627QC3Wy)](https://codecov.io/gh/level12/keg-elements/pull/134?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   96.53%   96.55%   +0.02%     
==========================================
  Files          28       28              
  Lines        2511     2529      +18     
==========================================
+ Hits         2424     2442      +18     
  Misses         87       87              
```


| [Impacted Files](https://codecov.io/gh/level12/keg-elements/pull/134?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [keg\_elements/forms/\_\_init\_\_.py](https://codecov.io/gh/level12/keg-elements/pull/134/diff?src=pr&el=tree#diff-a2VnX2VsZW1lbnRzL2Zvcm1zL19faW5pdF9fLnB5) | `95.45% <100.00%> (+0.02%)` | :arrow_up: |
| [keg\_elements/forms/validators.py](https://codecov.io/gh/level12/keg-elements/pull/134/diff?src=pr&el=tree#diff-a2VnX2VsZW1lbnRzL2Zvcm1zL3ZhbGlkYXRvcnMucHk=) | `92.85% <100.00%> (ø)` | |
| [keg\_elements/testing.py](https://codecov.io/gh/level12/keg-elements/pull/134/diff?src=pr&el=tree#diff-a2VnX2VsZW1lbnRzL3Rlc3RpbmcucHk=) | `89.25% <100.00%> (+0.56%)` | :arrow_up: |
| [keg\_elements/tests/test\_forms/test\_form.py](https://codecov.io/gh/level12/keg-elements/pull/134/diff?src=pr&el=tree#diff-a2VnX2VsZW1lbnRzL3Rlc3RzL3Rlc3RfZm9ybXMvdGVzdF9mb3JtLnB5) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/level12/keg-elements/pull/134?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/level12/keg-elements/pull/134?src=pr&el=footer). Last update [0ea69ab...3cd6366](https://codecov.io/gh/level12/keg-elements/pull/134?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/keg_elements/forms/__init__.py b/keg_elements/forms/__init__.py
index c045e2c..4bde279 100644
--- a/keg_elements/forms/__init__.py
+++ b/keg_elements/forms/__init__.py
@@ -365,6 +365,8 @@ class FormGenerator(FormGeneratorBase):
     def create_validators(self, prop, column):
         validators = super(FormGenerator, self).create_validators(prop, column)
         if isinstance(column.type, sa.Numeric) and not isinstance(column.type, sa.Float):
+            if column.type.precision is None or column.type.scale is None:
+                raise ValueError('Numeric fields must specify precision and scale')
             max_ = _max_for_numeric(column.type.precision, column.type.scale)
             validators.append(NumberRange(min=-max_, max=max_))
             validators.append(NumberScale(column.type.scale))
diff --git a/keg_elements/forms/validators.py b/keg_elements/forms/validators.py
index 62be761..56e0dc1 100644
--- a/keg_elements/forms/validators.py
+++ b/keg_elements/forms/validators.py
@@ -44,7 +44,7 @@ def numeric(form, field):
 
 
 class NumberScale(object):
-    def __init__(self, scale=-1, message=None):
+    def __init__(self, scale, message=None):
         self.scale = scale
         if not message:
             message = _(u'Field must have no more than {scale} decimal places.', scale=scale)
@@ -56,7 +56,7 @@ class NumberScale(object):
             return
         # use decimal's quantization to see if it's equal to field data at the full scale.
         #   If it isn't, user entered too many decimal places
-        if field.data != field.data.quantize(Decimal('0.{}1'.format('0' * (self.scale - 1)))):
+        if field.data != field.data.quantize(Decimal(1) / 10 ** self.scale):
             raise ValidationError(self.message)
 
 
diff --git a/kegel_app/model/entities.py b/kegel_app/model/entities.py
index 59c5a0e..83a6abe 100644
--- a/kegel_app/model/entities.py
+++ b/kegel_app/model/entities.py
@@ -219,3 +219,9 @@ class SoftDeleteTester(mixins.SoftDeleteMixin, mixins.DefaultMixin, db.Model):
     def testing_create(cls, **kwargs):
         kwargs['hdp_id'] = kwargs.get('hpd_id') or HardDeleteParent.testing_create().id
         return super().testing_create(**kwargs)
+
+
+class DefaultNumeric(mixins.DefaultMixin, db.Model):
+    __tablename__ = 'default_numeric'
+
+    number = sa.Column(sa.Numeric)


Test patch:
diff --git a/keg_elements/testing.py b/keg_elements/testing.py
index c82551e..963cddf 100644
--- a/keg_elements/testing.py
+++ b/keg_elements/testing.py
@@ -130,6 +130,15 @@ class EntityBase(object):
                 assert col.onupdate, 'Column "{}" should have onupdate set'.format(col.name)
             assert col.server_default, 'Column "{}" should have server_default set'.format(col.name)
 
+    def test_column_numeric_scale_precision_set(self):
+        for col_check in self.column_check_generator():
+            col = getattr(self.entity_cls, col_check.name)
+            if isinstance(col.type, sa.Numeric) and not isinstance(col.type, sa.Float):
+                assert col.type.precision is not None, \
+                    'Column "{}" does not specify precision'.format(col.name)
+                assert col.type.scale is not None, \
+                    'Column "{}" does not specify scale'.format(col.name)
+
     def test_all_columns_are_constraint_tested(self):
         """Checks that all fields declared on entity are in the constraint tests"""
 
diff --git a/keg_elements/tests/test_forms/test_form.py b/keg_elements/tests/test_forms/test_form.py
index 0db8afe..01b92a6 100644
--- a/keg_elements/tests/test_forms/test_form.py
+++ b/keg_elements/tests/test_forms/test_form.py
@@ -284,6 +284,19 @@ class TestValidators(FormBase):
 
         assert len(form.float_check.validators) == 1
 
+    def test_numeric_scale_precision_required(self):
+        with pytest.raises(ValueError, match='Numeric fields must specify precision and scale'):
+            class TestForm1(ke_forms.ModelForm):
+                class Meta:
+                    model = ents.DefaultNumeric
+
+        class TestForm2(ke_forms.ModelForm):
+            class Meta:
+                model = ents.DefaultNumeric
+                exclude = ('number',)
+
+            number = wtf.DecimalField('Number', validators=[])
+
     def test_length_validation_not_applied_for_enums(self):
         form = self.compose_meta()
         for validator in form.units.validators: