Pilotage : Mise en place d'une route pour accéder aux tableaux de bord par leur nom #4466
Conversation
rsebille
added
the
no-changelog
rsebille
force-pushed
the
rsebille/c2/one-route-to-rule-them-all
branch
from
20ee68e to
0fcf2c9
Compare
rsebille
force-pushed
the
rsebille/c2/one-route-to-rule-them-all
branch
from
0fcf2c9 to
d8a4afd
Compare
| def stats_redirect(request, dashboard_name): | ||
| match request.user.kind: | ||
| case UserKind.LABOR_INSPECTOR: | ||
| normalized_organization_kind = request.current_organization.kind.lower().replace(" ", "_") |
There was a problem hiding this comment.
Ça donne envie de virer l’espace de la valeur pour kind, mais j’ai un peu peur qu’on l’utilise directement.
There was a problem hiding this comment.
J'ai pas regardé si c'était la merde (faut faire des migrations) ou LA MERDE (faut toucher à des requêtes SQL jusque dans le pilotage) mais oui l'espace m'a aussi un peu étonné.
There was a problem hiding this comment.
Pour le moment, je ne vois pas bien le gain, mais je suis sûr qu’un PR à venir le mettra en valeur :)
J'ai essayer d'expliquer au mieux dans le "Pourquoi" mais si tu veux un exemple c'est pour faire ceci : 6f13b27#diff-223fd8697068ddce3d37c8ff1af642529bacdb3720e156c939bebb52edb73852R43
Et ainsi avoir exactement le même template pour tout les types d'institution sans avoir à passer le bon nom de route suivant le type de l'organisation : stats_ddets_iae_tension, stats_dreets_iae_tension, stats_dgefp_tension
| def stats_redirect(request, dashboard_name): | ||
| match request.user.kind: | ||
| case UserKind.LABOR_INSPECTOR: | ||
| normalized_organization_kind = request.current_organization.kind.lower().replace(" ", "_") |
There was a problem hiding this comment.
J'ai pas regardé si c'était la merde (faut faire des migrations) ou LA MERDE (faut toucher à des requêtes SQL jusque dans le pilotage) mais oui l'espace m'a aussi un peu étonné.
| path("dgefp/hiring", views.stats_dgefp_iae_hiring, name="stats_dgefp_iae_hiring"), | ||
| path("dgefp/state", views.stats_dgefp_iae_state, name="stats_dgefp_iae_state"), | ||
| path("dgefp/siae_evaluation", views.stats_dgefp_iae_siae_evaluation, name="stats_dgefp_iae_siae_evaluation"), | ||
| path("dgefp/af", views.stats_dgefp_iae_af, name="stats_dgefp_iae_af"), |
There was a problem hiding this comment.
Je suis choqué que tu n'en profites pas pour les trier 🔡
There was a problem hiding this comment.
Je l'ai pas fait pour limiter la diff 👼, mais j'aimerais bien virer toutes ces routes donc je garde quelques trucs qui grattent pour ne pas que ça soit trop "propre" non plus 😁.
| case UserKind.LABOR_INSPECTOR: | ||
| normalized_organization_kind = request.current_organization.kind.lower().replace(" ", "_") | ||
| case _: | ||
| return HttpResponseNotFound() |
There was a problem hiding this comment.
Je pense au cas d'un membre DDETS_IAE/DDETS_GEIQ connecté en GEIQ qui se prendrait une 404:
je me demande si on ne peut pas améliorer le message de la 404 ?
There was a problem hiding this comment.
Je pense au cas d'un membre
DDETS_IAE/DDETS_GEIQconnecté enGEIQ
Pas sûr de comprendre 🤔.
Un compte LABOR_INSPECTOR ne peux normalement pas se lier à un CompanyMembership, non ?
Et pour le coup j'ai pris le parti de faire la fonction très simple quitte à avoir quelques erreurs au début, plutôt que commencer à mettre des vérifications dans tout les sens.
There was a problem hiding this comment.
Je parle de https://github.com/gip-inclusion/les-emplois/blob/master/itou/institutions/enums.py#L7-L10
Si sa current_organization est DDETS_GEIQ et qu'il va sur un tableau de bord IAE, il aura une 404 sans forcément faire le lien avec son institution ?
There was a problem hiding this comment.
Ah, compris ! 😫 🥵
Dans ce cas là il aura pas la 404 mais la 500 [1] car son type d'utilisateur est bon mais la route stats_ddets_geiq_{dashboard_name} n'existera sans doute pas.
C'est toujours la même problématique de prendre current_organization ou organizations pour les ACL, et la perméabilité entre les deux, mais de toute manière vu comment son fait les ACL pour les TB ça serais pas vraiment possible de lui dire "Change d'organisation pour voir ce TB".
[1] Je suis parti sur cette idée car une 500 fera une erreur sentry et donc on verra les erreurs réelles plutôt qu'ajouter du code défensif dés le départ sur un truc qui petit et non-critique
There was a problem hiding this comment.
Oui pas de super solution à proposer 😬
🤔 Pourquoi ?
Le même tableau de bord peut être utiliser pour des types d'institution partenaire différente, le fonctionnement actuel fait que l'on crée une route pour chaque combinaison (don't feed the troll), le problème est que pour un autre ticket j'ai besoin de mettre en place des liens (affichés pour le moment à la DGEFP, aux DREETS IAE et aux DDETS IAE) qui pointent vers la bonne URL du tableau de bord privé donc plutôt que de faire une tambouille dans la vue ou le template je crée une vue qui fait le taf et qu'on pourra à terme utiliser comme point d'entrée histoire de simplifier tout ça.
🍰 Comment ?
Le code actuel est certe ultra-redondant mais il suit une nomenclature bien précise, je me base donc dessus pour implémenter un comportement générique.