Merge pull request #157 from BaseAdresseNationale/housenumber-positions

Replace HouseNumber.center by HouseNumber.positions

Author: yohanboniface

ban/core/models.py

@@ -23,6 +23,8 @@ class BaseModel(BaseResource, BaseVersioned):
 class Model(ResourceModel, Versioned, metaclass=BaseModel):
     resource_fields = ['version', 'created_at', 'created_by', 'modified_at',
                        'modified_by', 'attributes']
+    exclude_for_collection = ['version', 'created_at', 'created_by',
+                              'modified_at', 'modified_by']
     # 'version' is validated by us.
     resource_schema = {'version': {'required': False},
                        'created_at': {'readonly': True},
@@ -56,6 +58,7 @@ def __str__(self):
 class Municipality(NamedModel):
     identifiers = ['siren', 'insee']
     resource_fields = ['name', 'alias', 'insee', 'siren', 'postcodes']
+    exclude_for_version = ['postcodes']
 
     insee = db.CharField(max_length=5, unique=True)
     siren = db.CharField(max_length=9, unique=True, null=True)
@@ -135,9 +138,9 @@ def housenumbers(self):
 class HouseNumber(Model):
     identifiers = ['cia', 'laposte', 'ign']
     resource_fields = ['number', 'ordinal', 'parent', 'cia', 'laposte',
-                       'ancestors', 'center', 'ign', 'postcode']
+                       'ancestors', 'positions', 'ign', 'postcode']
     resource_schema = {'cia': {'readonly': True},
-                       'center': {'readonly': True}}
+                       'positions': {'readonly': True}}
 
     number = db.CharField(max_length=16, null=True)
     ordinal = db.CharField(max_length=16, null=True)
@@ -167,10 +170,8 @@ def compute_cia(self):
                            self.number, self.ordinal)
 
     @property
-    def center(self):
-        position = self.position_set.first()
-        return (position.center.geojson
-                if position and position.center else None)
+    def positions_extended(self):
+        return list(self.positions.as_resource_list())
 
     @property
     def ancestors_extended(self):
@@ -224,13 +225,13 @@ class Position(Model):
 
     name = db.CharField(max_length=200, null=True)
     center = db.PointField(verbose_name=_("center"), null=True, index=True)
-    housenumber = db.ForeignKeyField(HouseNumber)
+    housenumber = db.ForeignKeyField(HouseNumber, related_name='positions')
     parent = db.ForeignKeyField('self', related_name='children', null=True)
     source = db.CharField(max_length=64, null=True)
     kind = db.CharField(max_length=64, choices=KIND)
     positioning = db.CharField(max_length=32, choices=POSITIONING)
     ign = db.CharField(max_length=24, null=True, unique=True)
-    comment = peewee.TextField(null=True)
+    comment = db.TextField(null=True)
 
     class Meta:
         indexes = (

ban/core/resource.py

@@ -34,9 +34,8 @@ def as_resource_list(self):
 
 class BaseResource(peewee.BaseModel):
 
-    def include_field_for_compact(cls, name):
-        if name in ['version', 'created_at', 'created_by', 'modified_at',
-                    'modified_by']:
+    def include_field_for_collection(cls, name):
+        if name in cls.exclude_for_collection:
             return False
         attr = getattr(cls, name, None)
         exclude = (db.ManyToManyField, peewee.ReverseRelationDescriptor,
@@ -49,6 +48,8 @@ def __new__(mcs, name, bases, attrs, **kwargs):
         # Inherit and extend instead of replacing.
         resource_fields = attrs.pop('resource_fields', None)
         resource_schema = attrs.pop('resource_schema', None)
+        exclude_for_collection = attrs.pop('exclude_for_collection', None)
+        exclude_for_version = attrs.pop('exclude_for_version', None)
         cls = super().__new__(mcs, name, bases, attrs, **kwargs)
         if resource_fields is not None:
             inherited = getattr(cls, 'resource_fields', {})
@@ -58,10 +59,20 @@ def __new__(mcs, name, bases, attrs, **kwargs):
             inherited = getattr(cls, 'resource_schema', {})
             resource_schema.update(inherited)
             cls.resource_schema = resource_schema
-        cls.extended_fields = cls.resource_fields
-        cls.compact_fields = [
-            n for n in cls.extended_fields
-            if mcs.include_field_for_compact(cls, n)] + ['resource']
+        if exclude_for_collection is not None:
+            inherited = getattr(cls, 'exclude_for_collection', [])
+            exclude_for_collection.extend(inherited)
+            cls.exclude_for_collection = exclude_for_collection
+        if exclude_for_version is not None:
+            inherited = getattr(cls, 'exclude_for_version', [])
+            exclude_for_version.extend(inherited)
+            cls.exclude_for_version = exclude_for_version
+        cls.collection_fields = [
+            n for n in cls.resource_fields
+            if mcs.include_field_for_collection(cls, n)] + ['resource']
+        cls.versioned_fields = [
+            n for n in cls.resource_fields
+            if n not in cls.exclude_for_version]
         cls.build_resource_schema()
         return cls
 
@@ -70,6 +81,8 @@ class ResourceModel(db.Model, metaclass=BaseResource):
     resource_fields = ['id']
     identifiers = []
     resource_schema = {'id': {'readonly': True}}
+    exclude_for_collection = []
+    exclude_for_version = []
 
     id = db.CharField(max_length=50, unique=True, null=False)
 
@@ -91,7 +104,7 @@ def build_resource_schema(cls):
         """Map Peewee models to Cerberus validation schema."""
         schema = dict(cls.resource_schema)
         for name, field in cls._meta.fields.items():
-            if name not in cls.extended_fields:
+            if name not in cls.resource_fields:
                 continue
             if field.primary_key:
                 continue
@@ -133,17 +146,17 @@ def resource(self):
     @property
     def as_resource(self):
         """Resource plus relations."""
-        return {f: self.extended_field(f) for f in self.extended_fields}
+        return {f: self.extended_field(f) for f in self.resource_fields}
 
     @property
     def as_relation(self):
         """Resources plus relation references without metadata."""
-        return {f: self.compact_field(f) for f in self.compact_fields}
+        return {f: self.compact_field(f) for f in self.collection_fields}
 
     @property
     def as_version(self):
         """Resources plus relations references and metadata."""
-        return {f: self.compact_field(f) for f in self.extended_fields}
+        return {f: self.compact_field(f) for f in self.versioned_fields}
 
     def extended_field(self, name):
         value = getattr(self, '{}_extended'.format(name), getattr(self, name))

ban/core/validators.py

@@ -20,6 +20,11 @@ def _validate_type_point(self, field, value):
         if not isinstance(value, (str, list, tuple, Point)):
             self._error(field, 'Invalid Point: {}'.format(value))
 
+    def _validate_type_string(self, field, value):
+        if value is None and not self.schema[field].get('required'):
+            return True
+        return super()._validate_type_string(field, value)
+
     def _validate_type_foreignkey(self, field, value):
         if not isinstance(value, int):
             self._error(field, 'No matching resource for {}'.format(value))

ban/core/versioning.py

@@ -75,7 +75,7 @@ def store_version(self):
         old = None
         if self.version > 1:
             old = self.load_version(self.version - 1)
-            old.close_period(new.period[0])
+            old.close_period(new.period.lower)
         if Diff.ACTIVE:
             Diff.create(old=old, new=new, created_at=self.modified_at)
 

ban/db/fields.py

@@ -11,7 +11,7 @@
 __all__ = ['PointField', 'ForeignKeyField', 'CharField', 'IntegerField',
            'HStoreField', 'UUIDField', 'ArrayField', 'DateTimeField',
            'BooleanField', 'BinaryJSONField', 'PostCodeField', 'FantoirField',
-           'ManyToManyField', 'PasswordField', 'DateRangeField']
+           'ManyToManyField', 'PasswordField', 'DateRangeField', 'TextField']
 
 
 lonlat_pattern = re.compile('^[\[\(]{1}(?P<lon>-?\d{,3}(:?\.\d*)?), ?(?P<lat>-?\d{,3}(\.\d*)?)[\]\)]{1}$')  # noqa
@@ -99,6 +99,8 @@ class ForeignKeyField(peewee.ForeignKeyField):
     schema_type = 'foreignkey'
 
     def coerce(self, value):
+        if not value:
+            return None
         if isinstance(value, peewee.Model):
             value = value.pk
         elif isinstance(value, dict):
@@ -117,10 +119,19 @@ def _get_related_name(self):
 class CharField(peewee.CharField):
     schema_type = 'string'
 
-    def db_value(self, value):
+    def coerce(self, value):
         if self.null and not value:
             return None
-        return super().db_value(value)
+        return super().coerce(value)
+
+
+class TextField(peewee.TextField):
+    schema_type = 'string'
+
+    def coerce(self, value):
+        if self.null and not value:
+            return None
+        return super().coerce(value)
 
 
 class IntegerField(peewee.IntegerField):
@@ -177,6 +188,8 @@ class FantoirField(CharField):
     max_length = 9
 
     def coerce(self, value):
+        if not value:
+            return None
         value = str(value)
         if len(value) == 10:
             value = value[:9]

ban/db/model.py

@@ -44,3 +44,9 @@ def first(cls, *expressions):
             qs = qs.where(*expressions)
         # See https://github.com/coleifer/peewee/commit/eeb6d4d727da8536906a00c490f94352465e90bb  # noqa
         return qs.limit(1).first()
+
+    def __setattr__(self, name, value):
+        attr = getattr(self.__class__, name, None)
+        if attr and hasattr(attr, 'coerce'):
+            value = attr.coerce(value)
+        return super().__setattr__(name, value)

ban/http/resources.py

@@ -94,7 +94,7 @@ def on_get(self, req, resp, **params):
         """Get {resource} collection."""
         qs = self.get_collection(req, resp, **params)
         qs = self.get_where_clause(req, qs)
-        self.collection(req, resp, qs.as_resource())
+        self.collection(req, resp, qs.as_resource_list())
 
     @auth.protect
     @app.endpoint(path='/{identifier}')
@@ -279,7 +279,7 @@ def get_collection(self, req, resp, **kwargs):
     def on_get_positions(self, req, resp, *args, **kwargs):
         """Retrieve {resource} positions."""
         instance = self.get_object(**kwargs)
-        qs = instance.position_set.as_resource_list()
+        qs = instance.positions.as_resource_list()
         self.collection(req, resp, qs)
 
 

ban/tests/http/test_housenumber.py

@@ -71,7 +71,7 @@ def test_get_housenumber_collection(get, url):
     resp = get(url('housenumber'))
     assert resp.json['total'] == 5
     for obj in objs:
-        assert json.loads(dumps(obj.as_resource)) in resp.json['collection']
+        assert json.loads(dumps(obj.as_relation)) in resp.json['collection']
 
 
 @authorize
@@ -82,7 +82,7 @@ def test_get_housenumber_collection_can_be_filtered_by_bbox(get, url):
     resp = get(url('housenumber', query_string=bbox))
     assert resp.json['total'] == 1
     # JSON transform internals tuples to lists.
-    resource = position.housenumber.as_resource
+    resource = position.housenumber.as_relation
     assert resp.json['collection'][0] == json.loads(dumps(resource))
 
 
@@ -142,16 +142,16 @@ def test_housenumber_with_two_positions_is_not_duplicated_in_bbox(get, url):
     resp = get(url('housenumber', query_string=bbox))
     assert resp.json['total'] == 1
     # JSON transform internals tuples to lists.
-    data = json.loads(dumps(position.housenumber.as_resource))
+    data = json.loads(dumps(position.housenumber.as_relation))
     assert resp.json['collection'][0] == data
 
 
 @authorize
 def test_get_housenumber_with_position(get, url):
     housenumber = HouseNumberFactory()
-    PositionFactory(housenumber=housenumber, center=(1, 1))
+    position = PositionFactory(housenumber=housenumber, center=(1, 1))
     resp = get(url('housenumber-resource', identifier=housenumber.id))
-    assert resp.json['center'] == {'coordinates': [1, 1], 'type': 'Point'}
+    assert resp.json['positions'] == [json.loads(dumps(position.as_relation))]
 
 
 @authorize

ban/tests/http/test_position.py

@@ -392,7 +392,7 @@ def test_get_position_collection_can_be_filtered_by_bbox(get, url):
     resp = get(url('position', query_string=bbox))
     assert resp.json['total'] == 1
     # JSON transform internals tuples to lists.
-    resource = position.as_resource
+    resource = position.as_relation
     assert resp.json['collection'][0] == json.loads(dumps(resource))
 
 

ban/tests/http/test_postcode.py

@@ -45,8 +45,8 @@ def test_postcode_select_use_default_orderby(get, url):
     assert resp.status == falcon.HTTP_200
     assert resp.json['total'] == 3
     assert resp.json['collection'][0]['code'] == '90101'
-    assert resp.json['collection'][1]['municipality']['insee'] == "90002"
-    assert resp.json['collection'][2]['municipality']['insee'] == "90001"
+    assert resp.json['collection'][1]['municipality'] == mun1.id
+    assert resp.json['collection'][2]['municipality'] == mun2.id
 
 
 @authorize

ban/tests/test_diff.py

@@ -0,0 +1,36 @@
+from .factories import MunicipalityFactory
+
+
+def test_only_changed_columns_should_be_in_diff():
+    municipality = MunicipalityFactory(name='Moret-sur-Loing', insee='77316',
+                                       siren='987654321')
+    municipality.name = 'Orvanne'
+    municipality.siren = '123456789'
+    # "Changed" with same value.
+    municipality.insee = '77316'
+    municipality.increment_version()
+    municipality.save()
+    diff = municipality.versions[1].diff
+    assert len(diff.diff) == 2  # name, siren
+    assert diff.diff['name']['old'] == 'Moret-sur-Loing'
+    assert diff.diff['name']['new'] == 'Orvanne'
+    assert diff.diff['siren']['old'] == '987654321'
+    assert diff.diff['siren']['new'] == '123456789'
+    municipality.insee = '77319'
+    municipality.increment_version()
+    municipality.save()
+    diff = municipality.versions[2].diff
+    assert len(diff.diff) == 1  # insee
+    assert diff.diff['insee']['old'] == '77316'
+    assert diff.diff['insee']['new'] == '77319'
+
+
+def test_adding_value_to_arrayfield_column():
+    municipality = MunicipalityFactory(name='Moret-sur-Loing')
+    municipality.alias = ['Orvanne']
+    municipality.increment_version()
+    municipality.save()
+    diff = municipality.versions[1].diff
+    assert len(diff.diff) == 1  # name, siren
+    assert diff.diff['alias']['old'] is None
+    assert diff.diff['alias']['new'] == ['Orvanne']

ban/tests/test_managers.py

@@ -1,6 +1,6 @@
 from ban.core import models
 
-from .factories import MunicipalityFactory, GroupFactory
+from .factories import GroupFactory, MunicipalityFactory
 
 
 def test_municipality_as_resource():