base-adresse: provide ordering options to cities endpoint (#74374) #103

Merged
pmarillonnet merged 1 commits from wip/74374-base-adresse-cities-endpoint-ordering into main 2023-02-16 14:12:43 +01:00
2 changed files with 68 additions and 3 deletions

View File

@ -6,6 +6,7 @@ from io import StringIO
from urllib import parse as urlparse
from django.contrib.postgres.fields import JSONField
from django.core.exceptions import FieldError
from django.db import connection, models
from django.db.models import Q
from django.utils import timezone
@ -346,9 +347,19 @@ class BaseAdresse(AddressResource):
},
'region_code': {'description': _('Region code'), 'example_value': '11'},
'department_code': {'description': _('Department code'), 'example_value': '75'},
'ordering': {
'description': _(
Outdated
Review

Ici il faudrait préciser la liste des clés de tri (name, unaccent_name, code, zipcode, population, departement, region) et je mettrais "-population,zipcode,unaccent_name,name" en exemple, sous-entendant que c'est la valeur par défaut, et "documentant" au passage l'usage du "-".

Ici il faudrait préciser la liste des clés de tri (name, unaccent_name, code, zipcode, population, departement, region) et je mettrais "-population,zipcode,unaccent_name,name" en exemple, sous-entendant que c'est la valeur par défaut, et "documentant" au passage l'usage du "-".

Ok, j’ai ajouté la liste des clés, et j’ai modifié l’exemple comme tu le suggères ici.

Ok, j’ai ajouté la liste des clés, et j’ai modifié l’exemple comme tu le suggères ici.
'Comma-separated ordering field list (the fields are "name", "unaccent_name", '
'"code", "zipcode", "population", "department" and "region", and can each be '
'prefixed with "-" for reverse ordering)'
),
'example_value': '-population,zipcode,unaccent_name,name',
},
},
)
def cities(self, request, id=None, q=None, code=None, region_code=None, department_code=None):
def cities(
self, request, id=None, q=None, code=None, region_code=None, department_code=None, ordering=None
):
cities = self.citymodel_set.all()
if id is not None:
@ -374,6 +385,13 @@ class BaseAdresse(AddressResource):
cities = cities.filter(department__code=department_code)
cities = cities.select_related('department', 'region')
Outdated
Review

Ce "if" n'est pas utile, car cities.order_by(*ordering.split(',')) fonctionne aussi avec une chaîne sans virgule.

Ce "if" n'est pas utile, car `cities.order_by(*ordering.split(','))` fonctionne aussi avec une chaîne sans virgule.

Bien vu, en effet, j’avais loupé ça. C’est corrigé, c’est plus simple ainsi.

Bien vu, en effet, j’avais loupé ça. C’est corrigé, c’est plus simple ainsi.
Outdated
Review

Ici il faut juste modifier la query_set cities et laisser la suite (le return des cities) s'appliquer tranquillement. A voir d'ailleurs d'il faut faire le select_related('department', 'region') avant ou après.

Ici il faut juste modifier la query_set cities et laisser la suite (le return des cities) s'appliquer tranquillement. A voir d'ailleurs d'il faut faire le select_related('department', 'region') avant ou après.

Le problème est que le FieldError n’est pas levé lors du order_by (comportement “lazy”), seulement lorsque le sql est réellement exécuté, c’est-à-dire lors de

return {'data': [city.to_json() for city in cities]}

Le problème est que le FieldError n’est pas levé lors du order_by (comportement “lazy”), seulement lorsque le sql est réellement exécuté, c’est-à-dire lors de > return {'data': [city.to_json() for city in cities]}
if ordering:
try:
ordered = cities.order_by(*ordering.split(','))
return {'data': [city.to_json() for city in ordered]}
Review

en mode pénible, je dirais de sortir le return du try/except

en mode pénible, je dirais de sortir le return du try/except
except FieldError:
Outdated
Review

Il faut faire une APIError qui retourne une 400 (un self.logger.error ça fait surtout une trace sentry, non merci ;) )

Il faut faire une APIError qui retourne une 400 (un self.logger.error ça fait surtout une trace sentry, non merci ;) )

Ok, j’avais mélangé ces deux choses. C’est corrigé dans la branche.

Ok, j’avais mélangé ces deux choses. C’est corrigé dans la branche.
raise APIError(f'cities: erroneous ordering query {ordering}')
return {'data': [city.to_json() for city in cities]}
@endpoint(

View File

@ -157,6 +157,19 @@ def city(db, region, department):
)
@pytest.fixture
def city2(db, region, department):
return CityModel.objects.create(
name='Aix-les-Bains',
code='73008',
zipcode='73010',
population=30000,
region=region,
department=department,
resource=BaseAdresse.objects.first(),
)
@pytest.fixture
def miquelon(db):
return CityModel.objects.create(
@ -565,7 +578,7 @@ def test_base_adresse_command_update_multiple(mocked_get, db, base_adresse_multi
assert StreetModel.objects.count() == 5
def test_base_adresse_cities(app, base_adresse, city, miquelon, department, region):
def test_base_adresse_cities(app, base_adresse, city, city2, miquelon, department, region):
resp = app.get('/base-adresse/%s/cities?q=chambe' % base_adresse.slug)
assert len(resp.json['data']) == 1
result = resp.json['data'][0]
@ -581,8 +594,11 @@ def test_base_adresse_cities(app, base_adresse, city, miquelon, department, regi
assert result['department_name'] == city.department.name
resp = app.get('/base-adresse/%s/cities?q=73' % base_adresse.slug)
assert len(resp.json['data']) == 1
assert len(resp.json['data']) == 2
assert resp.json['data'][0] == result
assert resp.json['data'][1]['name'] == city2.name
assert resp.json['data'][1]['zipcode'] == city2.zipcode
assert resp.json['data'][1]['id'] == '%s.%s' % (city2.code, city2.zipcode)
resp = app.get('/base-adresse/%s/cities?code=73065' % base_adresse.slug)
assert len(resp.json['data']) == 1
@ -593,6 +609,37 @@ def test_base_adresse_cities(app, base_adresse, city, miquelon, department, regi
assert resp.json['data'][0] == result
assert resp.json['data'][1]['name'] == 'Miquelon-Langlade'
# default ordering, descending number of inhabitants
resp = app.get('/base-adresse/%s/cities?department_code=73' % base_adresse.slug)
assert len(resp.json['data']) == 2
assert resp.json['data'][0]['name'] == city.name
assert resp.json['data'][1]['name'] == city2.name
# sorted by population (ascending) then name
resp = app.get('/base-adresse/%s/cities?department_code=73&ordering=population,name' % base_adresse.slug)
assert resp.json['data'][0]['name'] == city2.name
assert resp.json['data'][1]['name'] == city.name
# sorted by name
resp = app.get('/base-adresse/%s/cities?department_code=73&ordering=name' % base_adresse.slug)
assert resp.json['data'][0]['name'] == city2.name
assert resp.json['data'][1]['name'] == city.name
# sorted by name (reverse) then by code
resp = app.get('/base-adresse/%s/cities?department_code=73&ordering=-name,code' % base_adresse.slug)
assert resp.json['data'][0]['name'] == city.name
assert resp.json['data'][1]['name'] == city2.name
# sorted by code
resp = app.get('/base-adresse/%s/cities?department_code=73&ordering=code' % base_adresse.slug)
assert resp.json['data'][0]['name'] == city2.name
assert resp.json['data'][1]['name'] == city.name
# nonexistent ordering field, fallback on default ordering
resp = app.get('/base-adresse/%s/cities?department_code=73&ordering=foobar' % base_adresse.slug)
assert resp.json['err'] == 1
assert resp.json['err_desc'] == 'cities: erroneous ordering query foobar'
def test_base_adresse_cities_missing_region_and_department(app, base_adresse, miquelon):
resp = app.get('/base-adresse/%s/cities?q=miqu' % base_adresse.slug)