Feat/add circulation model - #3578

[IdrissaD]

Face au besoin du Parc national des Cévennes d'intégrer son plan de circulation à Geotrek (cf. #3578), j'ai essayé d'ajouter le modèle CirculationEdge, sur le modèle déjà réalisé de PhysicalEdge et LandEdge.

Le but : permettre d'attribuer un statut "Autorisation de circulation" aux tronçons du réseau, afin de gérer les différentes autorisations sur un territoire réglementé. Par exemple en cœur de Parc avec les véhicules motorisés, les vélos, etc.

Quatre valeurs de CirculationType ont été intégrées aux fixtures :

• Pédestre
• Motorisée
• Équestre
• Cycliste

Je n'ai ajouté qu'un champ spécifique au modèle CirculationEdge : "authorized", de type booléen. Il permet de qualifier le Circulation edge en précisant si la circulation est autorisée (authorized=True), interdite (authorized=False) ou si l'on ne connaît pas son statut (authorized=None).

Je précise qu'il s'agit de ma première PR dans Geotrek-admin, donc je suis preneur de tout retour pour améliorer cette contribution !

J'ai notamment eu des problèmes lors de l'exécution des tests Cypress (les fichiers static ne se chargeaient jamais en environnement tests).

Une chose qui est déjà à corriger est la sélection de couleurs pour circulation dans le paramètre COLORS_POOLS : j'ai mis la même que pour land, ne sachant pas comment trouver une bonne palette.

[cypress]

Passing run #7600 ↗︎

0

24

0

0

0

Details:

Merge 81b9a37 into 94eee3c...
Project: Geotrek-admin	Commit: f7aaf9a2b9 ℹ️
Status: Passed	Duration: 02:14 💡
Started: Nov 29, 2023 10:18 AM	Ended: Nov 29, 2023 10:20 AM

Review all test suite changes for PR #3663 ↗︎

[camillemonchicourt]

Très intéressant.

Étant donné que pour chaque événement que l'on va localiser, on va renseigner "autorisé : oui ou non", je pense qu'il faudrait juste parler de "Circulation" et non pas d'"Autorisation de circulation".

[babastienne]

Super, merci pour cette contribution @IdrissaD !! 🎉

On va regarder ça en détail pour voir si tout est ok niveau code, mais de ce que j'ai vu rapidement ça a l'air plutôt bon j'ai l'impression.

Juste une question, concernant le choix de définir la notion de Circulation sous forme de booléen. En fait si je comprends bien on utilise un booléen mais il faudra pouvoir conserver la possibilité de switcher entre trois valeurs : autorisée (= True), non autorisée (= False) et non définie (= None ou null). Ma question est la suivante : dans ton implémentation actuelle est-ce que dans l'interface graphique l'utilisateur à la possibilité de repasser son statut à null ? Ou alors à partir du moment où il a coché une fois la case (même par erreur, ca le condamne à devoir choisir entre vrai/faux ?
D'après la doc de Django je vois qu'il existe un composant graphique pour gérer ce cas de "triple switch (vrai / faux / null)", il faut utiliser NullBooleanSelect et pas le composant CheckboxInput comme pour les autres booléens.

Sinon j'ai l'impression qu'un test en mode sans segmentation dynamique ne passe plus. Tu peux les lancer en local sur ton poste avec la commande docker-compose run --rm -e ENV=tests_nds web ./manage.py test. Une fois que les tests passeront on pourra regarder la couverture de code.

En tout cas c'est top comme contribution, merci encore 🙏

[IdrissaD]

J'ai renommé "Autorisation de circulation" en "Circulation".

@babastienne j'ai essayé d'intégrer le widget NullBooleanSelect mais sans succès... En essayant dans l'interface graphique j'avais toujours une erreur me disant que les valeurs nulles n'étaient pas autorisées pour ce champ. J'ai essayé de cette manière dans land.forms.py :

class CirculationEdgeForm(EdgeForm):
    authorized = forms.NullBooleanField(widget=forms.NullBooleanSelect, required=False)
    class Meta(EdgeForm.Meta):
        model = CirculationEdge
        fields = EdgeForm.Meta.fields + ['circulation_type', 'authorized']

Concernant les traductions, tout ce qui concerne CirculationEdge reste en anglais dans mon interface graphique, pourtant j'ai bien mis à jour land/locale/fr/LC_MESSAGES/django.po, il y a autre chose à faire ?

Le composant graphique du champ authorized est une liste déroulante plutôt qu'une checkbox, est-ce qu'on le garde comme ça ou on le transforme en checkbox ?

Pour les tests, j'ai bien la segmentation dynamique activée pourtant... J'ai lancé ta commande, j'ai toujours quelques dizaines d'erreurs mais uniquement reliées à de la traduction (je n'avais pas l'espagnol et l'italien activés dans la base de données). Les tests Cypress ne fonctionnent toujours pas chez moi par contre

[babastienne]

@IdrissaD

• Pour le champ authorized si on a le choix avec les trois valeurs dans la liste déroulante alors c'est ok pour moi, c'est mieux qu'une checkbox 👍
• Pour les traductions tu as bien utilisé les commandes makemessages et collectmessages ?
• Les tests ne prennent pas en compte la configuration de ton système, ils sont lancé dans un environnement qui leur est propre (ça utilise les fichiers env_tests.py et env_tests_nds.py). Pour la fonctionnalité il faut donc s'assurer que rien ne casse à la fois en segmentation dynamique mais également sans. C'est pour ça qu'il faut lancer les tests dans les deux environnements, et la CI sur Github vérifie ça aussi. En l’occurrence les langues ne devraient pas être un problème car pour les tests une base de données temporaire est créée il me semble, tu ne devrais pas avoir à activer toutes les langues sur ton instance de dev.
• Pour les tests cypress je ne peux pas t'aider. @Chatewgne va te faire une review prochainement sur la PR.

[IdrissaD]

• Ok, je laisse la liste déroulante, mais par contre il y a toujours une IntegrityError quand la valeur "Indéterminé" est sélectionnée.
• ah non, je ne savais pas que ces commandes étaient à lancer, mais en relançant le build ça a dû les lancer car les textes sont bien traduits à présent
• j'ai relancé les tests et voilà les erreurs qui remontent :
  errors.txt

[IdrissaD]

Le problème du champ authorized qui n'était pas nullable est résolu, il fallait retirer l'argument default=None, je pense que ça rentrait en conflit quelque part avec null=True.
Par contre quand la valeur "Indéterminée" est renseignée, l'interface graphique affiche "Autorisée : Non", ce qui peut prêter à confusion.

[camillemonchicourt]

Oui quand le champs n'est pas défini, c'est pas souhaitable d'afficher NON.
Je ne sais pas si on peut faire quelque chose à ce sujet.

[babastienne]

On peut sûrement afficher quelque chose oui, mais la question c'est quoi ? Je suis d'avis de ne rien afficher si le champ n'est pas renseigné, juste laisser vide.

[IdrissaD]

Je suis d'accord pour laisser vide, par contre je n'ai pas réussi à trouver comment faire.
D'ailleurs, est-ce qu'il y a réellement une utilité d'avoir une valeur indéterminée pour ce champ ? Vous pensez à des cas de figures où cette valeur se révélerait utile ?

[Chatewgne]

Je suis d'accord pour laisser vide, par contre je n'ai pas réussi à trouver comment faire. D'ailleurs, est-ce qu'il y a réellement une utilité d'avoir une valeur indéterminée pour ce champ ? Vous pensez à des cas de figures où il se révélerait utile ?

Si on a pas l'information pour ce tronçon, il suffit de ne pas créer de circulation edge non ? Je ne vois pas bien l'intérêt de la valeur "indéterminée" de mon côté

[babastienne]

Pour moi cette information a du sens. Le statut sert à renseigner le type de circulation (vélo, pédestre, motorisé, etc.), mais on peut ne pas forcément déterminer si la circulation est autorisée ou non.
De même si je suis un utilisateur qui travaille sur les statuts de circulation je dois pouvoir filtrer mes éléments pour rapidement accéder à ceux dans la circulation est autorisée, interdite ou non définie.

[IdrissaD]

Après discussion avec @amandine-sahl, la gestion des autorisations de manière booléenne n'est peut-être pas la plus adaptée. Circulations autorisée ou interdite sont deux catégories assez absolues qui comprennent de nombreuses nuances comme :

• circulation motorisée interdite sauf ayant-droits
• circulation motorisée interdite sauf macarons
• circulation cycliste autorisée si le vélo est porté (roues ne touchant pas le sol)

Et sûrement bien d'autres exemples de réglementations spécifiques. Une liste de valeurs serait plus adaptée à ces nuances, qu'en pensez-vous ?

[babastienne]

Une liste de valeurs c'est peut-être en effet ce qu'il y a de plus simple, l'avantage étant que chacun peut adapter les valeur comme il l'entend.

[Chatewgne]

You can update failing test with self.assertNumQueries(22) in file geotrek/altimetry/tests/test_commands.py line 74, it's asserting that circulation edges geom will be updated when altimetry is updated 👍

[IdrissaD]

La PR nous semble prête !

• ajout de CirculationType et AuthorizationType
• ajout des fixtures
• mise à jour du Changelog

La dernière chose que j'ai identifié mais que je vous laisserai faire, c'est la sélection de couleurs pour circulation dans le paramètre COLORS_POOLS (advanced-configuration.rst et settings/base.py): j'ai mis la même que pour land, ne sachant pas comment trouver une bonne palette.
Cf. ce commit : e1d2498

[submarcos]

merci @IdrissaD . Par contre il faudrait fixer les conflits dans les deux fichiers pour que la CI se lance

[marcantoinedupre]

Ça m'a l'air bien, merci pour la contribution 👍

Question ouverte : est-il pertinent d'ajouter les circulations au panneau de contrôle des couches Leaflet ?

[babastienne]

Oui bien vu @marcantoinedupre je pense qu'il faudrait également l'ajouter parmi les couches leaflet pour garder une cohérence avec les autres types de status.

[camillemonchicourt]

Oui si c'est un sixième sous-module de Statuts, il faut qu'il soit complet et similaire aux 5 autres sous-modules.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (94eee3c) 98.33% compared to head (81b9a37) 98.34%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3663      +/-   ##
==========================================
+ Coverage   98.33%   98.34%   +0.01%     
==========================================
  Files         292      292              
  Lines       21792    21932     +140     
==========================================
+ Hits        21429    21569     +140     
  Misses        363      363              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Requested changes have been carried on.