Connecteur Isere ESRH (#82880) #371

Merged
csechet merged 3 commits from wip/82880-isere-esrh into main 2023-11-14 16:36:57 +01:00
Owner
No description provided.
csechet added 1 commit 2023-10-26 18:06:52 +02:00
Author
Owner

Pour les entités, comme la nomenclature est pas bien claire entre directions et services, j'ai ajouté des filtres avec des expressions régulières sur les codes et les libellés des entités. ca permettra de paramétrer ça comme ils veulent sans faire des allers retours dans le code (il y a des "Z(his) DIR. blablabla" en base, par exemple, je ne sais pas si ces entrées doivent être prises en compte ou pas).

Pour les entités, comme la nomenclature est pas bien claire entre directions et services, j'ai ajouté des filtres avec des expressions régulières sur les codes et les libellés des entités. ca permettra de paramétrer ça comme ils veulent sans faire des allers retours dans le code (il y a des "Z(his) DIR. blablabla" en base, par exemple, je ne sais pas si ces entrées doivent être prises en compte ou pas).
tnoel requested changes 2023-11-06 11:39:52 +01:00
tnoel left a comment
Owner

Une première relecture où il s'agira d'être moins confiant sur les retours reçus (et faire des APIError si besoin), et en même temps renvoyer plus "directement" les infos quand on les a (faire un minimum de transformation des objets reçus)

Une première relecture où il s'agira d'être moins confiant sur les retours reçus (et faire des APIError si besoin), et en même temps renvoyer plus "directement" les infos quand on les a (faire un minimum de transformation des objets reçus)
@ -0,0 +42,4 @@
def _get(self, endpoint):
response = self.requests.get(f'{self.base_url}/api/v2/{endpoint}')
response.raise_for_status()
Owner

Il faut lever une APIError en cas de raise (encadrer d'un try/except donc).

Il faut lever une APIError en cas de raise (encadrer d'un try/except donc).
csechet marked this conversation as resolved
@ -0,0 +43,4 @@
def _get(self, endpoint):
response = self.requests.get(f'{self.base_url}/api/v2/{endpoint}')
response.raise_for_status()
return response.json()
Owner

Là aussi en cas de json.JSONDecodeError, requests.exceptions.JSONDecodeError, lever une APIError

Et tu pourrais aller plus loin, vérifier que c'est bien un dictionnaire qui est reçu, et qu'il contient une clé "values" qui est une liste de dictionnaires. Et que dans le cas contraire, lever une APIError détaillant le pépin.

Là aussi en cas de json.JSONDecodeError, requests.exceptions.JSONDecodeError, lever une APIError Et tu pourrais aller plus loin, vérifier que c'est bien un dictionnaire qui est reçu, et qu'il contient une clé "values" qui est une liste de dictionnaires. Et que dans le cas contraire, lever une APIError détaillant le pépin.
csechet marked this conversation as resolved
@ -0,0 +53,4 @@
},
)
def official(self, request, number, authority):
agents = self._get(f'Agent?numero={number}&collectivite={authority}').get('values', [])
Owner

Ici il faudrait pouvoir faire un self._get('Agent', params=...) où params sera celui attendu par self.requests.get, ça fera l'encodage et tout ce qu'il faut, c'est plus propre.

Ici il faudrait pouvoir faire un self._get('Agent', params=...) où params sera celui attendu par self.requests.get, ça fera l'encodage et tout ce qu'il faut, c'est plus propre.
csechet marked this conversation as resolved
@ -0,0 +56,4 @@
agents = self._get(f'Agent?numero={number}&collectivite={authority}').get('values', [])
if len(agents) == 0:
raise Http404(_('Official not found'))
Owner

On évite ça dans Passerelle, on renvoie plutôt data=None et voilà. Si on pense que c'est vraiment une erreur, alors on lève une APIError.

On évite ça dans Passerelle, on renvoie plutôt data=None et voilà. Si on pense que c'est vraiment une erreur, alors on lève une APIError.
csechet marked this conversation as resolved
@ -0,0 +60,4 @@
agent = agents[0]
agent_id = agent['agentId']
Owner

Je ne sais pas trop si leur API marche toujours bien, mais dans ce genre moment on a tendance à lever des APIError si agents n'est pas une liste, si le premier élément n'est pas un dico, s'il ne contient pas de clé agentId ... Si on ne fait pas ça le code va crasher et remonter des erreurs sentry dont on ne veut pas.

Je ne sais pas trop si leur API marche toujours bien, mais dans ce genre moment on a tendance à lever des APIError si agents n'est pas une liste, si le premier élément n'est pas un dico, s'il ne contient pas de clé agentId ... Si on ne fait pas ça le code va crasher et remonter des erreurs sentry dont on ne veut pas.
csechet marked this conversation as resolved
@ -0,0 +68,4 @@
categorie_gestion = {}
position = {}
files = self._get(f'Agent/{agent_id}/DossiersStatutaire?aDate={encoded_now()}').get('values', [])
Owner

Ici aussi, faire plutôt un params={'aDate', ...} plutôt que de gérer l'encodage à la main.

Au passage, je ne sais pas si c'est vraiment utile mais c'est pas cher, je permettrais un champ "date" dans le endpoint : on utiliserait la date du jour si date est None.

Ici aussi, faire plutôt un `params={'aDate', ...}` plutôt que de gérer l'encodage à la main. Au passage, je ne sais pas si c'est vraiment utile mais c'est pas cher, je permettrais un champ "date" dans le endpoint : on utiliserait la date du jour si date est None.
Author
Owner

Au passage, je ne sais pas si c'est vraiment utile mais c'est pas cher, je permettrais un champ "date" dans le endpoint : on utiliserait la date du jour si date est None.

Leurs spécifications disaient explicitement d'utiliser la date du jour, donc je ne pense pas que ça soit utile.

> Au passage, je ne sais pas si c'est vraiment utile mais c'est pas cher, je permettrais un champ "date" dans le endpoint : on utiliserait la date du jour si date est None. Leurs spécifications disaient explicitement d'utiliser la date du jour, donc je ne pense pas que ça soit utile.
csechet marked this conversation as resolved
@ -0,0 +77,4 @@
categorie_gestion = file.get('categorieGestion', {}) or {}
position = file.get('position', {}) or {}
result = {
Owner

Là j'ai pas trop compris la construction de ce "result". On peut renvoyer un JSON complexe. Pour moi, il faut juste renvoyer les deux JSON, celui qui vient de Agent et celui qui vient de Agent/{agent_id}/DossiersStatutaire. Et tant pis s'il contient des clés vides (None).

Je penserais donc plus à quelque chose du genre :

agent['DossiersStatutaire'] = files[0]
return {'data': agent}

Là j'ai pas trop compris la construction de ce "result". On peut renvoyer un JSON complexe. Pour moi, il faut juste renvoyer les deux JSON, celui qui vient de Agent et celui qui vient de Agent/{agent_id}/DossiersStatutaire. Et tant pis s'il contient des clés vides (None). Je penserais donc plus à quelque chose du genre : ``` agent['DossiersStatutaire'] = files[0] return {'data': agent} ```
csechet marked this conversation as resolved
@ -0,0 +107,4 @@
result = {k: v for (k, v) in result.items() if v is not None}
return {'err': 0, 'data': result}
Owner

A noter qu'il n'est pas nécessaire de renvoyer la clé err:0, c'est ajouté automatiquement par la mécanique du décorateur endpoint

A noter qu'il n'est pas nécessaire de renvoyer la clé err:0, c'est ajouté automatiquement par la mécanique du décorateur endpoint
csechet marked this conversation as resolved
@ -0,0 +143,4 @@
if code_pattern and not code_pattern.match(code):
continue
result.append({'id': id, 'text': label, 'code': code})
Owner

Ici encore, agir plus comme une passerelle : ajouter des clés "id" et "text" sur entity, et faire un result.append(entity). Comme ça s'il y a d'autres clés dedans un jour, on les aura aussi.

Ici encore, agir plus comme une passerelle : ajouter des clés "id" et "text" sur entity, et faire un result.append(entity). Comme ça s'il y a d'autres clés dedans un jour, on les aura aussi.
csechet marked this conversation as resolved
@ -0,0 +165,4 @@
labels = job_type.get('libelles', [])
label = 'N/A' if len(labels) == 0 else labels[0]['libelle']
result.append({'id': id, 'text': label})
Owner

Même remarque que plus haut, ajouter "id" et "text" sur job_type, et dans un result.append(job_type).

Même remarque que plus haut, ajouter "id" et "text" sur job_type, et dans un result.append(job_type).
csechet marked this conversation as resolved
csechet force-pushed wip/82880-isere-esrh from d77f03ee8e to 10774937c7 2023-11-07 12:15:40 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from 10774937c7 to 9c05a28857 2023-11-07 14:28:55 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from 9c05a28857 to 6c5396147d 2023-11-07 14:34:50 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from 6c5396147d to 7677f75f72 2023-11-07 14:38:07 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from 7677f75f72 to f4767e092f 2023-11-07 15:00:47 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from f4767e092f to 6d98811d9f 2023-11-07 15:17:10 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from 6d98811d9f to ea8df50356 2023-11-07 15:39:35 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from ea8df50356 to d17bbaf1d2 2023-11-07 15:39:58 +01:00 Compare
tnoel requested changes 2023-11-10 16:22:42 +01:00
tnoel left a comment
Owner

On touche au but :)

On touche au but :)
@ -0,0 +54,4 @@
if not isinstance(response_json, dict) or response_json.get('values') is None:
raise APIError('ESRH returned malformed json : expecting a dictionary with a "values" key')
Owner

Je crois qu'ici tu peux pousser d'un cran la vérification : response_json['values'] doit une liste, et chacun de ses éléments doit être un dictionnaire, et hop, c'est toujours ça de gagné.

Je crois qu'ici tu peux pousser d'un cran la vérification : response_json['values'] doit une liste, et chacun de ses éléments doit être un dictionnaire, et hop, c'est toujours ça de gagné.
csechet marked this conversation as resolved
@ -0,0 +89,4 @@
description=_('Get entities'),
parameters={
'label_pattern': {
'description': _('Filter entities whose label matches this pattern (case insensitive)'),
Owner

Pour la personne qui va faire les traductions, dire plutôt "this regex" au lieu de "this pattern" (on traduira par "Filtrer les entités qui correspondant à l'expression régulière")

Pour la personne qui va faire les traductions, dire plutôt "this regex" au lieu de "this pattern" (on traduira par "Filtrer les entités qui correspondant à l'expression régulière")
csechet marked this conversation as resolved
@ -0,0 +109,4 @@
result = []
for entity in entities:
id = entity.pop('entiteId')
Owner

Pas de pop, on laisse entity telle quelle, on y ajoute juste deux clés "id" et "text"

Pas de pop, on laisse entity telle quelle, on y ajoute juste deux clés "id" et "text"
Author
Owner

Quel intérêt de renvoyer deux fois la même donnée ?

Quel intérêt de renvoyer deux fois la même donnée ?
csechet marked this conversation as resolved
@ -0,0 +137,4 @@
result = []
for job_type in job_types:
id = job_type.pop('posteId')
Owner

même chose, pas de pop, ajouter juste "id" et "text" et hop.

même chose, pas de pop, ajouter juste "id" et "text" et hop.
Author
Owner

Même remarque.

Même remarque.
csechet marked this conversation as resolved
csechet force-pushed wip/82880-isere-esrh from d17bbaf1d2 to 787f897449 2023-11-14 11:10:11 +01:00 Compare
csechet force-pushed wip/82880-isere-esrh from 787f897449 to 9692a4f7df 2023-11-14 15:05:21 +01:00 Compare
tnoel reviewed 2023-11-14 15:21:45 +01:00
@ -0,0 +43,4 @@
response = self.requests.get(f'{self.base_url}/api/v2/{endpoint}', params=params)
response.raise_for_status()
except (ConnectionError, Timeout) as e:
raise APIError('ESRH is down', data={'exception': str(e)})
Owner

Ça peut être autre chose que « ESRH is down » donc laisse ici un générique « HTTP request timeout »

Ça peut être autre chose que « ESRH is down » donc laisse ici un générique « HTTP request timeout »
csechet marked this conversation as resolved
tnoel approved these changes 2023-11-14 15:24:14 +01:00
tnoel left a comment
Owner

Que des détails peu importants, tu peux envoyer ainsi.

Que des détails peu importants, tu peux envoyer ainsi.
@ -0,0 +79,4 @@
agent = agents[0]
if not isinstance(agent, dict) or 'agentId' not in agent:
Owner

Grâce aux puissants contrôles dans _get, « not isinstance(agent, dict) » est déjà vérifié, youpi.

Grâce aux puissants contrôles dans _get, « not isinstance(agent, dict) » est déjà vérifié, youpi.
csechet marked this conversation as resolved
csechet force-pushed wip/82880-isere-esrh from 9692a4f7df to 0426fdc063 2023-11-14 15:30:23 +01:00 Compare
csechet merged commit 230d424571 into main 2023-11-14 16:36:57 +01:00
csechet deleted branch wip/82880-isere-esrh 2023-11-14 16:36:57 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/passerelle#371
No description provided.