Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: ajout des filtres departement et departement_postes sur la route /api/v1/siaes/ #4270

Merged
merged 5 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 71 additions & 51 deletions itou/api/siae_api/viewsets.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import logging

from django.db.models import Prefetch
from django_filters.filters import OrderingFilter
from django.db.models import Exists, OuterRef, Prefetch, Q
from django_filters.filters import CharFilter, ChoiceFilter, NumberFilter, OrderingFilter
from django_filters.rest_framework import DjangoFilterBackend, FilterSet
from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import OpenApiExample, OpenApiParameter, extend_schema
Expand All @@ -11,6 +11,7 @@
from rest_framework.throttling import UserRateThrottle

from itou.cities.models import City
from itou.common_apps.address.departments import DEPARTMENTS
from itou.companies.models import Company, JobDescription

from .serializers import SiaeSerializer
Expand All @@ -32,12 +33,77 @@
}


class SiaeOrderingFilter(FilterSet):
def noop(queryset, name, value):
return queryset


class CompanyFilterSet(FilterSet):
# Mapping of the model property names -> query parameter names used to order the results:
# - keys: the name of the property in the model in order to order the results
# - values: the name of the ordering criterion in the query parameter
# If you want to query https://some_api?o=cree_le, it will perform queryset.order_by("created_at")
o = OrderingFilter(fields=SIAE_ORDERING_FILTER_MAPPING)
o = OrderingFilter(
fields=SIAE_ORDERING_FILTER_MAPPING,
help_text="Critère de tri",
)

code_insee = CharFilter(
method=noop, # Noop filter since filtering happens in filter_queryset method
help_text="Filtre par code INSEE de la ville. À utiliser avec `distance_max_km`.",
)
distance_max_km = NumberFilter(
method=noop, # Noop filter since filtering happens in filter_queryset method
min_value=0,
max_value=MAX_DISTANCE_RADIUS_KM,
help_text=(
"Filtre par rayon de recherche autour de la ville, en kilomètres. "
f"Maximum {MAX_DISTANCE_RADIUS_KM} kilomètres. À utiliser avec `code_insee`."
),
)

departement = ChoiceFilter(
field_name="department", choices=list(DEPARTMENTS.items()), help_text="Département de la structure"
)

postes_dans_le_departement = ChoiceFilter(
choices=list(DEPARTMENTS.items()),
help_text="Département d'un poste de la structure.",
method="having_job_in_department",
)

def having_job_in_department(self, queryset, name, value):
# Either a Job in one of the department cities
# or a Job without city when the company department matches
return queryset.filter(
Exists(JobDescription.objects.filter(company=OuterRef("pk"), location__department=value))
| (
Q(department=value)
& Exists(JobDescription.objects.filter(company=OuterRef("pk"), location__isnull=True))
)
)

def filter_queryset(self, queryset):
filtered_queryset = super().filter_queryset(queryset)
code_insee = self.form.cleaned_data.get("code_insee")
distance = self.form.cleaned_data.get("distance_max_km")
department = self.form.cleaned_data.get("departement")
jobs_in_department = self.form.cleaned_data.get("postes_dans_le_departement")

if distance and code_insee:
try:
city = City.objects.get(code_insee=code_insee)
return filtered_queryset.within(city.coords, distance)
except City.DoesNotExist:
# Ensure the error comes from a missing city, which may not be that clear
# with get_object_or_404
raise NotFound(f"Pas de ville avec pour code_insee {code_insee}")
elif not (department or jobs_in_department):
raise ValidationError(
f"Les paramètres `{CODE_INSEE_PARAM_NAME}` et `{DISTANCE_FROM_CODE_INSEE_PARAM_NAME}` sont "
"obligatoires si ni `departement` ni `postes_dans_le_departement` ne sont spécifiés."
)
# Here the department/job_department filters have been applied
Copy link
Contributor

@francoisfreitag francoisfreitag Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

departement_postes pour correspondre au nom du filtre ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postes_dans_le_departement ?

return filtered_queryset


class RestrictedUserRateThrottle(UserRateThrottle):
Expand Down Expand Up @@ -93,7 +159,7 @@ class SiaeViewSet(viewsets.ReadOnlyModelViewSet):

serializer_class = SiaeSerializer
filter_backends = [DjangoFilterBackend]
filterset_class = SiaeOrderingFilter
filterset_class = CompanyFilterSet
ordering = ["id"]

authentication_classes = [TokenAuthentication, SessionAuthentication]
Expand All @@ -119,32 +185,10 @@ class SiaeViewSet(viewsets.ReadOnlyModelViewSet):
response_only=True,
status_codes=["404"],
)
sort_description = """
Critère de tri.

On peut spécifier la direction de tri :
- o=critère pour l’ordre croissant
- o=-critère pour l’ordre décroissant
"""

@extend_schema(
parameters=[
OpenApiParameter(
name=CODE_INSEE_PARAM_NAME, description="Filtre par code INSEE de la ville", required=True, type=str
),
OpenApiParameter(
name=DISTANCE_FROM_CODE_INSEE_PARAM_NAME,
description=f"Filtre par rayon de recherche autour de la ville, en kilomètres. Maximum {MAX_DISTANCE_RADIUS_KM} kilomètres", # noqa: E501
required=True,
type=str,
),
OpenApiParameter(name="format", description="Format de sortie", required=False, enum=["json", "api"]),
OpenApiParameter(
name="o",
description=sort_description,
required=False,
enum=SIAE_ORDERING_FILTER_MAPPING.values(),
),
],
responses={200: SiaeSerializer, 404: OpenApiTypes.OBJECT},
examples=[
Expand All @@ -159,9 +203,6 @@ def get_queryset(self):
# We only get to this point if permissions are OK
queryset = super().get_queryset()

# Get (registered) query parameters filters
queryset = self._filter_by_query_params(self.request, queryset)

try:
return queryset.order_by("id")
finally:
Expand All @@ -170,24 +211,3 @@ def get_queryset(self):
"User-Agent: %s",
self.request.headers.get("User-Agent"),
)

def _filter_by_query_params(self, request, queryset):
params = request.query_params
code_insee = params.get(CODE_INSEE_PARAM_NAME)
t = f"Les paramètres `{CODE_INSEE_PARAM_NAME}` et `{DISTANCE_FROM_CODE_INSEE_PARAM_NAME}` sont obligatoires."
if params.get(DISTANCE_FROM_CODE_INSEE_PARAM_NAME) and code_insee:
distance_filter = int(params.get(DISTANCE_FROM_CODE_INSEE_PARAM_NAME))
if distance_filter < 0 or distance_filter > MAX_DISTANCE_RADIUS_KM:
raise ValidationError(
f"Le paramètre `{DISTANCE_FROM_CODE_INSEE_PARAM_NAME}` doit être compris entre 0 et {MAX_DISTANCE_RADIUS_KM}." # noqa: E501
)

try:
city = City.objects.get(code_insee=code_insee)
return queryset.within(city.coords, distance_filter)
except City.DoesNotExist:
# Ensure the error comes from a missing city, which may not be that clear
# with get_object_or_404
raise NotFound(f"Pas de ville avec pour code_insee {code_insee}")
else:
raise ValidationError(t)
106 changes: 80 additions & 26 deletions tests/api/siae_api/tests.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import json

from django.urls import reverse
from rest_framework.test import APIClient, APITestCase

from itou.companies.enums import CompanyKind, ContractType
from tests.cities.factories import create_city_guerande, create_city_saint_andre
from tests.companies.factories import CompanyFactory, JobDescriptionFactory
from tests.users.factories import EmployerFactory
from tests.utils.test import BASE_NUM_QUERIES

from ..utils import _str_with_tz
Expand All @@ -16,18 +13,19 @@


class SiaeAPIFetchListTest(APITestCase):
@classmethod
def setUpTestData(cls):
# We create 2 cities and 2 siaes in Saint-Andre.
cls.saint_andre = create_city_saint_andre()
cls.guerande = create_city_guerande()
cls.company_without_jobs = CompanyFactory(kind=CompanyKind.EI, department="44", coords=cls.saint_andre.coords)
cls.company_with_jobs = CompanyFactory(
with_jobs=True, romes=("N1101", "N1105", "N1103", "N4105"), department="44", coords=cls.saint_andre.coords
)

def setUp(self):
super().setUp()
self.client = APIClient()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

À se demander si le setUp est encore utile...

self.user = EmployerFactory()

# We create 2 cities and 2 siaes in Saint-Andre.
self.saint_andre = create_city_saint_andre()
self.guerande = create_city_guerande()
CompanyFactory(kind=CompanyKind.EI, department="44", coords=self.saint_andre.coords)
CompanyFactory(
with_jobs=True, romes=("N1101", "N1105", "N1103", "N4105"), department="44", coords=self.saint_andre.coords
)

def test_performances(self):
num_queries = BASE_NUM_QUERIES
Expand All @@ -41,13 +39,15 @@ def test_performances(self):

def test_fetch_siae_list_without_params(self):
"""
The query parameters for INSEE code and distance are mandatories
The query parameters need to contain either a department or both an INSEE code and a distance
"""
response = self.client.get(ENDPOINT_URL, format="json")

self.assertContains(
response, "Les paramètres `code_insee` et `distance_max_km` sont obligatoires.", status_code=400
)
assert response.status_code == 400
assert response.json() == [
"Les paramètres `code_insee` et `distance_max_km` sont obligatoires si ni `departement` ni "
"`postes_dans_le_departement` ne sont spécifiés."
]

def test_fetch_siae_list_with_too_high_distance(self):
"""
Expand All @@ -56,9 +56,8 @@ def test_fetch_siae_list_with_too_high_distance(self):
query_params = {"code_insee": 44056, "distance_max_km": 200}
response = self.client.get(ENDPOINT_URL, query_params, format="json")

self.assertContains(
response, "Le paramètre `distance_max_km` doit être compris entre 0 et 100", status_code=400
)
assert response.status_code == 400
assert response.json() == {"distance_max_km": ["Assurez-vous que cette valeur est inférieure ou égale à 100."]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faudrait-il également une mention dans la release note de ce changement ? C’est un breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certes mais comme on est sur un cas d'erreur et que j'espère qu'aucun système automatique ne s'amuse à envoyer des données invalides j'aurais tendance à laisser tel quel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Juste une mention dans le changelog ne me semble pas trop lourde, et évite des surprises aux utilisateurs ?

Copy link
Contributor Author

@xavfernandez xavfernandez Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mais comment ajouter des choses dans le changelog en dehors du titre de cette PR ?
Je rajoute ainsi qu'une modification du format de certaines erreurs à la fin du titre ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvim CHANGELOG.md ? La génération automatique est bien dans la plupart des cas, mais c’est pas la seule manière ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est un coup à péter la prochaine PR de changelog non ?


def test_fetch_siae_list_with_negative_distance(self):
"""
Expand All @@ -67,9 +66,8 @@ def test_fetch_siae_list_with_negative_distance(self):
query_params = {"code_insee": 44056, "distance_max_km": -10}
response = self.client.get(ENDPOINT_URL, query_params, format="json")

self.assertContains(
response, "Le paramètre `distance_max_km` doit être compris entre 0 et 100", status_code=400
)
assert response.status_code == 400
assert response.json() == {"distance_max_km": ["Assurez-vous que cette valeur est supérieure ou égale à 0."]}

def test_fetch_siae_list_with_invalid_code_insee(self):
"""
Expand Down Expand Up @@ -162,9 +160,20 @@ def test_fetch_siae_list(self):
query_params = {"code_insee": self.saint_andre.code_insee, "distance_max_km": 100}
response = self.client.get(ENDPOINT_URL, query_params, format="json")

body = json.loads(response.content)
assert body["count"] == 2
assert response.json()["count"] == 2
assert response.status_code == 200

# Add a department filter matching the companies
query_params["departement"] = "44"
response = self.client.get(ENDPOINT_URL, query_params, format="json")
assert response.status_code == 200
assert response.json()["count"] == 2

# Add a department filter NOT matching the companies
query_params["departement"] = "33"
response = self.client.get(ENDPOINT_URL, query_params, format="json")
assert response.status_code == 200
assert response.json()["count"] == 0

def test_fetch_siae_list_too_far(self):
"""
Expand All @@ -174,10 +183,55 @@ def test_fetch_siae_list_too_far(self):
query_params = {"code_insee": self.guerande.code_insee, "distance_max_km": 10}
response = self.client.get(ENDPOINT_URL, query_params, format="json")

body = json.loads(response.content)
assert body["count"] == 0
assert response.json()["count"] == 0
assert response.status_code == 200

def test_fetch_siae_list_by_department(self):
# Declare company in 56 despite its coordinates
company56 = CompanyFactory(kind=CompanyKind.EI, department="56", coords=self.saint_andre.coords)
query_params = {"departement": "44"}
response = self.client.get(ENDPOINT_URL, query_params, format="json")

body = response.json()
assert body["count"] == 2
assert response.status_code == 200
assert company56.siret not in {company["siret"] for company in body["results"]}

def test_fetch_siae_list_by_postes_dans_le_departement(self):
# Declare company in 56
company56 = CompanyFactory(kind=CompanyKind.EI, department="56")
query_params = {"postes_dans_le_departement": "56"}
response = self.client.get(ENDPOINT_URL, query_params, format="json")

assert response.status_code == 200
assert response.json()["count"] == 0 # No job in 56

# Add a job without location, it should use the company department
JobDescriptionFactory(company=company56, location=None)
response = self.client.get(ENDPOINT_URL, query_params, format="json")
assert response.status_code == 200
assert response.json()["count"] == 1

query_params = {"postes_dans_le_departement": "44"}
response = self.client.get(ENDPOINT_URL, query_params, format="json")

assert response.status_code == 200
body = response.json()
assert body["count"] == 1
assert body["results"][0]["siret"] == self.company_with_jobs.siret

xavfernandez marked this conversation as resolved.
Show resolved Hide resolved
# Add a new job for company 56 in department 44
JobDescriptionFactory(company=company56, location=self.guerande)
response = self.client.get(ENDPOINT_URL, query_params, format="json")

assert response.status_code == 200
body = response.json()
assert body["count"] == 2
assert {body["results"][0]["siret"], body["results"][1]["siret"]} == {
self.company_with_jobs.siret,
company56.siret,
}

def test_fetch_siae_list_rate_limits(self):
query_params = {"code_insee": self.saint_andre.code_insee, "distance_max_km": 100}
# Declared in itou.api.siae_api.viewsets.RestrictedUserRateThrottle.
Expand Down