Test and fix territories routing

.circleci/config.yml

@@ -187,6 +187,9 @@ workflows:
               only:
                 - master
                 - /[0-9]+(\.[0-9]+)+/
+                - /pull/[0-9]+/
+                - /pyup-update-.+/
+                - l10n_master
             tags:
               only: /v[0-9]+(\.[0-9]+)+/
       - assets:
@@ -195,6 +198,9 @@ workflows:
               only:
                 - master
                 - /[0-9]+(\.[0-9]+)+/
+                - /pull/[0-9]+/
+                - /pyup-update-.+/
+                - l10n_master
             tags:
               only: /v[0-9]+(\.[0-9]+)+/
       - dist:
@@ -206,6 +212,9 @@ workflows:
               only:
                 - master
                 - /[0-9]+(\.[0-9]+)+/
+                - /pull/[0-9]+/
+                - /pyup-update-.+/
+                - l10n_master
             tags:
               only: /v[0-9]+(\.[0-9]+)+/
       - publish:

CHANGELOG.md

@@ -6,6 +6,7 @@
 - Prevent API tracking errors with unicode [#1602](https://github.com/opendatateam/udata/pull/1602)
 - Prevent a race condition error when uploading file with concurrent chunking [#1606](https://github.com/opendatateam/udata/pull/1606)
 - Disallow resources dict in API [#1603](https://github.com/opendatateam/udata/pull/1603)
+- Test and fix territories routing [#1611](https://github.com/opendatateam/udata/pull/1611)
 
 ## 1.3.6 (2018-04-16)
 

udata/core/spatial/models.py

@@ -82,6 +82,8 @@ def resolve(self, geoid, id_only=False):
 
 
 class GeoZone(db.Document):
+    SEPARATOR = ':'
+
     id = db.StringField(primary_key=True)
     slug = db.StringField(required=True)
     name = db.StringField(required=True)

udata/routing.py

@@ -10,6 +10,7 @@
 from werkzeug.urls import url_quote
 
 from udata import models
+from udata.core.spatial.models import GeoZone
 from udata.i18n import ISO_639_1_CODES
 
 
@@ -111,8 +112,8 @@ class PostConverter(ModelConverter):
     model = models.Post
 
 
-class TerritoryConverter(ModelConverter, PathConverter):
-    model = models.GeoZone
+class TerritoryConverter(PathConverter):
+    DEFAULT_PREFIX = 'fr'  # TODO: make it a setting parameter
 
     def to_python(self, value):
         """
@@ -126,10 +127,18 @@ def to_python(self, value):
         if '/' not in value:
             return
 
-        level, code, slug = value.split('/')
-        return self.model.objects.get_or_404(
-            id=':'.join(['fr', level, code]),
-            level='fr:{level}'.format(level=level))
+        level, code = value.split('/')[:2]  # Ignore optionnal slug
+
+        geoid = GeoZone.SEPARATOR.join([level, code])
+        zone = GeoZone.objects.resolve(geoid)
+
+        if not zone and GeoZone.SEPARATOR not in level:
+            # Try implicit default prefix
+            level = GeoZone.SEPARATOR.join([self.DEFAULT_PREFIX, level])
+            geoid = GeoZone.SEPARATOR.join([level, code])
+            zone = GeoZone.objects.resolve(geoid)
+
+        return zone or NotFound()
 
     def to_url(self, obj):
         """
@@ -139,26 +148,14 @@ def to_url(self, obj):
         if not level_name:
             raise ValueError('Unable to serialize "%s" to url' % obj)
 
-        zone_id = getattr(obj, 'id', None)
-        if not zone_id:
-            # Fallback on code for old redirections.
-            code = getattr(obj, 'code', None)
-            if not code:
-                raise ValueError('Unable to serialize "%s" to url' % obj)
-            territory = self.model.objects.get_or_404(
-                code=code, level='fr:{level}'.format(level=level_name))
-            return '{level_name}/{code}/{slug}'.format(
-                level_name=level_name, code=territory.code,
-                slug=territory.slug)
-
         code = getattr(obj, 'code', None)
         slug = getattr(obj, 'slug', None)
         validity = getattr(obj, 'validity', None)
         if code and slug:
             return '{level_name}/{code}@{start_date}/{slug}'.format(
                 level_name=level_name,
                 code=code,
-                start_date=getattr(validity, 'start') or 'latest',
+                start_date=getattr(validity, 'start', None) or 'latest',
                 slug=slug
             )
         else:

udata/tests/test_routing.py

@@ -9,7 +9,10 @@
 from flask import url_for
 
 from udata import routing
+from udata.core.spatial.models import GeoZone
+from udata.core.spatial.factories import GeoZoneFactory
 from udata.models import db
+from udata.tests.helpers import assert200
 
 
 class UUIDConverterTest:
@@ -111,3 +114,86 @@ def test_resolve_model_from_utf8_slug(self, client):
 
     def test_model_not_found(self, client):
         assert client.get('/model/not-found').status_code == 404
+
+
+@pytest.mark.usefixtures('clean_db')
+@pytest.mark.options(TERRITORY_DEFAULT_PREFIX='fr')  # Not implemented
+class TerritoryConverterTest:
+
+    @pytest.fixture(autouse=True)
+    def setup(self, app):
+
+        @app.route('/territory/<territory:territory>')
+        def territory_tester(territory):
+            assert isinstance(territory, GeoZone)
+            return 'ok'
+
+    def test_serialize_zone_with_validity(self):
+        zone = GeoZoneFactory()
+        url = url_for('territory_tester', territory=zone)
+        expected = '/territory/{level}/{code}@{validity.start}/{slug}'
+        assert url == expected.format(**zone._data)
+
+    def test_serialize_zone_without_validity(self):
+        zone = GeoZoneFactory(validity=None)
+        url = url_for('territory_tester', territory=zone)
+        expected = '/territory/{level}/{code}@latest/{slug}'
+        assert url == expected.format(**zone._data)
+
+    def test_serialize_default_prefix_zone_with_validity(self):
+        zone = GeoZoneFactory(level='fr:level')
+        url = url_for('territory_tester', territory=zone)
+        expected = '/territory/level/{code}@{validity.start}/{slug}'
+        assert url == expected.format(**zone._data)
+
+    def test_serialize_default_prefix_zone_without_validity(self):
+        zone = GeoZoneFactory(level='fr:level', validity=None)
+        url = url_for('territory_tester', territory=zone)
+        expected = '/territory/level/{code}@latest/{slug}'
+        assert url == expected.format(**zone._data)
+
+    def test_resolve_default_prefix_zone_with_validity(self, client):
+        zone = GeoZoneFactory(level='fr:level')
+        url = '/territory/level/{code}@{validity.start}/{slug}'
+        response = client.get(url.format(**zone._data))
+        assert200(response)
+
+    def test_resolve_default_prefix_zone_without_validity(self, client):
+        zone = GeoZoneFactory(level='fr:level', validity=None)
+        url = '/territory/level/{code}@latest/{slug}'
+        response = client.get(url.format(**zone._data))
+        assert200(response)
+
+    def test_resolve_zone_with_validity(self, client):
+        zone = GeoZoneFactory()
+        url = '/territory/{level}/{code}@{validity.start}/{slug}'
+        response = client.get(url.format(**zone._data))
+        assert200(response)
+
+    def test_resolve_zone_with_latest_validity(self, client):
+        zone = GeoZoneFactory(validity=None)
+        url = '/territory/{level}/{code}@latest/{slug}'
+        response = client.get(url.format(**zone._data))
+        assert200(response)
+
+    def test_resolve_zone_without_validity(self, client):
+        zone = GeoZoneFactory(validity=None)
+        url = '/territory/{level}/{code}/{slug}'
+        response = client.get(url.format(**zone._data))
+        assert200(response)
+
+    def test_resolve_zone_without_optionnal_slug(self, client):
+        zone = GeoZoneFactory(validity=None)
+        url = '/territory/{level}/{code}@latest/'
+        response = client.get(url.format(**zone._data))
+        assert200(response)
+
+    def test_resolve_zone_without_slug_nor_trailing_slash(self, client):
+        zone = GeoZoneFactory(validity=None)
+        url = '/territory/{level}/{code}@latest'
+        response = client.get(url.format(**zone._data))
+        assert200(response)
+
+    def test_model_not_found(self, client):
+        assert client.get('/territory/l/c@latest').status_code == 404
+