log history

[bouttier]

Regroupe #1835 et lpofredc#38

Liée à #789

[codecov]

Codecov Report

Patch coverage: 84.33% and project coverage change: +0.17 🎉

Comparison is base (fa7673a) 67.44% compared to head (45c7272) 67.62%.

❗ Current head 45c7272 differs from pull request most recent head 40f2e7f. Consider uploading reports for the commit 40f2e7f to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2337      +/-   ##
===========================================
+ Coverage    67.44%   67.62%   +0.17%     
===========================================
  Files           82       82              
  Lines         7007     7086      +79     
===========================================
+ Hits          4726     4792      +66     
- Misses        2281     2294      +13     

Flag	Coverage Δ
pytest	67.62% <84.33%> (+0.17%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
backend/geonature/core/gn_synthese/models.py	87.94% <82.25%> (-1.18%)	⬇️
backend/geonature/core/gn_synthese/routes.py	79.95% <90.47%> (+0.48%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bouttier]

• La query_class SyntheseQuery est adapté aux objets Synthese, pas aux SyntheseLogEntry : ne pas l’utiliser pour les SyntheseLogEntry et supprimer les modifications qui lui ont été apporté.
• Les fonctions utilitaires proposé dans utils/routes.py sont soit superflu, soit le mauvais endroit (certaines fonctions auraient leur place dans une query_class), et ne respectent pas ce qui a été discuté ici : [Dev] Bonnes pratiques API RESTful : query string pour le tri #2273 → implémenter tout ceci directement dans le code de la route.
• Éviter le filtre générique automatique. Il n’a que peu de chance de marcher à tout les coup, et en l’état il ne marche pas justement pour le filtre le plus important qui est de pouvoir filtrer sur la date. Ceci pourrait être fait avec ?meta_last_action_date:gt=<date iso 8601> (gt - greather than / lt - lower than). On a pas encore de convention sur ce type de filtre, et il faudra discuter de cela avant d’en faire des utilitaires génériques réutilisable.
• supprimer @json_resp : https://docs.geonature.fr/development.html#le-decorateur-json-resp
• Il manque des tests unitaires

[bouttier]

Voici un certain nombres d’observations ayant amené aux dernières modifications sur cette PR :

• Il ne faut pas recopier le code à tester dans les tests, mais tester le vrai code (cas de l’union). En effet, cela annule l’intérêt du code coverage (le vrai code n’étant pas testé), et d’autre part, les tests ont un objectif de tester les régressions ce qui ne sera pas le cas ici.
• Les fixtures ne doivent contenir que des opérations simples que l’on sait ne pas planter et dont l’objectif est de créer des données utiles à une situation de test, mais ne doivent pas contenir du code de test à proprement dit.
• Il est une bonne pratique que la QueryClass lève des ValueError, car celle-ci peut être utilisé en dehors d’une route, une BadRequest n’ayant alors pas de sens. Cependant, nous avons du code de gestion d’erreur qui met en forme les exceptions dans un format commun à tous GeoNature : se contenter de lever une BadRequest sans créer son propre format de retour d’erreurs.
• La QueryClass n’est pas forcément l’emplacement pour les fonctions utilitaires. Si une fonction ne peut s’utiliser via MonModel.query.ma_fonction, c’est sans doute que la fonction doit aller ailleurs.
• Le paramètre get_role (dont le rôle été par ailleurs inutilisé) a été supprimé : Suppression du paramètre get_role du décorateur check_cruved_scope #2162
• J’ai séparé les tests de logs de ceux de la synthèse pour plus de clareté. Un préfix commun aux fonctions de tests est pratique pour les sélectionner spécifiquement lorsque l’on lance des tests partiels.
• Il est immaintenable de vérifier le code SQL compilé. Toute évolution de l’ORM, de monter de version de PostgreSQL avec évolution de la syntaxe fera planter le test. D’autre part, peut-être que la requête SQL elle même est erroné et qu’elle ne fait pas exactement ce qui est attendu. Il faut vérifier que le retour de l’exécution de la requête SQL est correcte : j’appelle ma route, et je vérifie que les données qu’elles me renvoit sont celle attendu. Par exemple, si j’utilise un filtre date > D, je vérifie que dans les données renvoyé j’ai bien que des dates > D, et je peux aussi vérifier qu’il y a bien une observation pour laquelle je sais la date être > D.
• J’ai modifié la syntaxe date:gt=D vers date=gt:D afin qu’il soit plus facile de récupérer le nom des champs.
• Attention au formattage, l’action github te l’avais remonté. Tu peux activer l’auto-formatage dans ton IDE, ou utiliser également le hook pre-commit.
• Limitons le code générique, moins facile à lire, et plus prompte aux erreurs. Si l’on souhaite faire du code générique, on le fera dans une fonction / classe utilitaire réutilisable avec une très très bonne couverture de code, et non pas pour une route unique de GeoNature.
• Utilisation d’une datetime plutôt qu’une date. On peut vouloir faire des synchronisations toute les heures par exemple.
• Utilisation du format de date ISO plutôt que d’inventer des formats propre à une route donnée.
• Éviter les DROP … IF EXISTS. Si l’objet est censé exister, c’est qu’il y a un problème à comprendre, je préfère donc avoir une erreur plutôt que de ne pas me rendre compte du problème.
• La colonne synthese.last_action n’est pas fiable ! Elle est parfois NULL, et est mise à "U" uniquement pour les updates fait dans OccTax. Utilisation d’un case sur meta_{create,update}_date à la place.
• Utilisation de loadonly au lieu de with_entities, afin d’obtenir des objets SyntheseLogEntry, et de pouvoir appeler as_dict dessus.