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

feat/load-info-media-inpn #325

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

mvergez
Copy link
Contributor

@mvergez mvergez commented May 9, 2022

Contexte

Suite à plusieurs demandes notamment du PNR de la Forêt D'Orient et de la LPO PACA, un bouton pouvant récupérer automatiquement les informations depuis l'API de l'INPN serait fortement utile pour générer rapidement les fiches espèces, notamment pour l'atlas.

Implémentation

Un nouveau bouton a été ajouté dans l'édition d'une fiche espèce et récupère depuis 2 routes de l'API INPN la description et les média.
Si une description est déjà présente : elle est écrasée
Si un média est déjà présent (vérifié par l'url) : il n'est pas écrasé

Limitations

Les médias s'insèrent automatiquement dans la liste mais pas le type de média : il suffit donc de recharger la page. Il faudrait que l'API retourne après le POST/PUT le nom du type de média en plus de l'id_type.
Pour le moment il est obligatoire d'insérer en base de donnée l'attribut atlas_description (doit absolument s'appeler comme cela) en plus d'un thème si besoin:

INSERT INTO bib_themes (nom_theme, desc_theme, ordre, id_droit) 
VALUES ('Atlas', 'Informations relatives à GeoNature-atlas', 2, 3);

-- Insertion des attributs utilisés par GeoNature-atlas
INSERT INTO bib_attributs (id_attribut, nom_attribut, label_attribut, liste_valeur_attribut, obligatoire, desc_attribut, type_attribut, type_widget, regne, group2_inpn, id_theme, ordre) VALUES (100, 'atlas_description', 'Description', '{}', false, 'Donne une description du taxon pour l''atlas', 'text', 'textarea', NULL, NULL, (SELECT max(id_theme) FROM taxonomie.bib_themes), 1);

This button enables to get taxon info and media from inpn API
Erases the description if there is already one
If the media already exists, it does not post it
@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #325 (a31bbd9) into develop (bc11c9b) will decrease coverage by 0.88%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #325      +/-   ##
===========================================
- Coverage    65.86%   64.97%   -0.89%     
===========================================
  Files           22       22              
  Lines         1576     1576              
===========================================
- Hits          1038     1024      -14     
- Misses         538      552      +14     
Flag Coverage Δ
pytest 64.97% <ø> (-0.89%) ⬇️

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

Impacted Files Coverage Δ
apptax/utils/utilssqlalchemy.py 49.25% <0.00%> (-5.98%) ⬇️
apptax/taxonomie/repositories.py 59.06% <0.00%> (-5.37%) ⬇️
apptax/taxonomie/models.py 94.73% <0.00%> (-0.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc11c9b...a31bbd9. Read the comment docs.

@camillemonchicourt
Copy link
Member

OK intéressant.
Est-ce que ça utilise les scripts existants ?

A rendre activable ou non, car tout le monde ne souhaitera pas l'utiliser.

Je m'interroge quand même sur la pertinence de faire ça taxon par taxon.
Lancer le script global qui renseigne tous les taxons sans infos depuis l'API INPN comme prévu dans les scripts existants le semble plus adapté et pertinent.

Je ne comprends pas pourquoi les éventuels textes existants localement sont écrasés ?
Si on a fait une description locale, selon moi, elle prévaut sur la description générique de l'INPN.

Il me semble aussi qu'il faut distinguer l'import de média et l'import des description.

In media list
LIMITATION: type is not retrieved
@mvergez mvergez marked this pull request as ready for review May 9, 2022 13:15
@Adrien-Pajot
Copy link

Adrien-Pajot commented May 9, 2022

Salut @camillemonchicourt ,

Je prends la main pour le côté fonctionnel.

A rendre activable ou non, car tout le monde ne souhaitera pas l'utiliser.

J'étais d'accord dans le fond mais après discussion avec Max, cela ne paraît forcément intéressant de rendre activable un bouton qui n'est pas forcément gênant et obligatoire à utiliser.

Je m'interroge quand même sur la pertinence de faire ça taxon par taxon.
Lancer le script global qui renseigne tous les taxons sans infos depuis l'API INPN comme prévu dans les scripts existants le semble plus adapté et pertinent.

C'est une fonctionnalité qui nous est tout le temps demandée. A l'initiation d'un Atlas, on remplit toutes les espèces via les scripts mais dès qu'une nouvelle espèce apparaît, le bouton sert. On l'a déjà expérimenté sur nos atlas.

Il me semble aussi qu'il faut distinguer l'import de média et l'import des description.

Pour le coup on demande aussi tout le temps le "all in one". On parle d'utilisateurs sans notion d'informatiques dans notre cas qui veulent récupérer tout l'INPN puis aviser, faire deux boutons ou deux clics nous paraissait beaucoup, qu'est-ce que tu en penses ?

@mvergez
Copy link
Contributor Author

mvergez commented May 9, 2022

  • Non cela n'utilise pas les scripts existants car, si tu parles de celui là : https://github.com/PnX-SI/TaxHub/blob/7f349f5f115736ba7f415698b1d5927d602efc8a/data/scripts/import_inpn_media/import_inpn_media.py alors il récupère uniquement les média via l'API INPN et les insère via la base de donnée. Cette PR appelle l'API directement depuis le front et appelle l'API de taxhub pour insérer les infos et les médias
  • Je ne vois pas vraiment pourquoi il faudrait le rendre désactivable. Cela ferait un nouveau paramètre qui n'a pas forcément d'utilité à mon sens. Voilà a quoi ressemble ce bouton :
    image
  • Je suis d'accord avec toi que lancer un script avec tous les taxons qu'on souhaite est plus pratique mais :
    • Cela fait plusieurs fois qu'on nous demande cette feature, pour que l'admin taxhub puisse lui même ajouter les infos automatiquement
    • Un script avec potentiellement 500+ taxons risque de surcharger l'API de l'INPN
    • Si nouveau taxon ou changement de média/info etc... L'utilisateur peut le faire plutôt que de passer par l'admin serveur
  • Effectivement pas de problème pour ne pas écraser la description, je ferai un commit en ce sens.
  • Séparer le media et l'info est une bonne idée mais je pense que ça nécessiterait plus de clics de la part de l'utilisateur

@mvergez
Copy link
Contributor Author

mvergez commented May 9, 2022

Maintenant : s'il y a déjà une description : un message d'information dit qu'il faut supprimer les infos existantes pour pouvoir récupérer celles de l'INPN

@camillemonchicourt
Copy link
Member

OK pour les textes non écrasés.

Si l'idée est de lancer le script global d'import au départ, ça n'a pas de sens que l'utilisateur doive ensuite faire manuellement les ajouts suivants.
Dans ce cas, l'enjeu serait plutôt de relancer le script chaque jour automatiquement, d'importer les médias et descriptions des espèces ajoutées mais aussi de mettre à les descriptions qui auraient changé niveau INPN et/ou les nouvelles photos.

Le fait d'activer ou non, c'est car côté PNE par exemple, je ne suis pas certain de vouloir proposer cette fonctionnalité à nos utilisateurs.
Je préfère lancer le script global chaque nuit et bien gérer ça globalement.

@Adrien-Pajot
Copy link

Pour le coup le script global est une autre demande que l'on va essayer de satisfaire sous peu aussi. Ce sera en effet intéressant de lancer ce script régulièrement pour un remplissage automatisé. Le bouton, aussi discutable qu'il soit, est vraiment très beaucoup demander, peut-être que le script global annihilera celui-ci mais quand on le propose, les utilisateurs aiment ce bouton. Bizarre ? ;)

@mvergez
Copy link
Contributor Author

mvergez commented May 9, 2022

Ajout de la possibilité de cacher le bouton dans le fichier static/app/constant.js comme ceci :

app.constant("backendCfg", {
  "api_url": "api/",
  "medias_path": "static/medias/",
  "user_admin_privilege":6,
  "user_high_privilege":4,
  "user_medium_privilege":3,
  "user_low_privilege":2,
  "hideInpnButton": false
})

@camillemonchicourt
Copy link
Member

Si le script global est lancé une seule fois au départ, puis que les données ne sont pas mises à jour régulièrement, c'est normal que les utilisateurs demandent le "bouton" pour importer les photos des taxons qu'ils ajoutent ensuite.
Ça veut pas dire que c'est la bonne solution. :-)

Il y aura des photos et descriptions importées sur certains taxons, pas d'autres, ça sera hétérogène et lourd à gérer par les utilisateurs si ils veulent être à jour et complets.

Selon moi l'enjeu aurait plutôt été de compléter le script global, de l'améliorer justement pour gérer les mises à jour, éventuellement de l'interfacer au niveau de l'interface de TaxHub pour pouvoir le lancer directement depuis l'interface de TaxHub, pourquoi pas de lister le log des imports réalisés.

Et si on regarde un peu plus global, le vrai enjeu selon nous serait de ne plus importer les contenus de l'API INPN, car c'est lourd, cela duplique des contenus. Maintenant que l'API de l'INPN est plus complète et stable, le vrai enjeu serait de ne pas importer les contenus, mais uniquement de les afficher au niveau de GeoNature-atlas, en interrogeant l'API en direct, sur les fiches où il n'y a pas de contenu provenant de TaHub.

Et ainsi concentrer TaxHub sur la création de contenu local, spécifique et non redondant.

Donc le bouton d'import par fiche... pourquoi pas, pas de soucis pour l'intégrer, mais plus j'y réfléchis, plus j'en vois l'incohérence.
Donc le rendre activable ou non est important, peut-être le masquer par défaut pour ne pas encourager cette pratique.

PS : Attention, cette PR réduit la couverture des tests pour le moment

@DonovanMaillard
Copy link
Contributor

Salut à tous,

Idem de mon coté, je préfèrerais aussi pouvoir désactiver cette fonctionnalité et gérer coté admin les contenus sur l'ensemble des taxons. Comme dit Camille, avoir certaines infos qui remontent et d'autres pas, en fonction de l'utilisateur et des taxons visités, pose question à mon avis.

En revanche appeler l'API en direct sans charger les contenus, ca crée aussi une dépendance forte à des outils sur lesquels on a pas la main (les sites INPN étaient en vrac il y a quelques jours encore). Sur un outil de comm comme l'atlas, ca peut poser question. En revanche on peut imaginer de proposer les 2 alternatives à l'administrateur...

@Adrien-Pajot
Copy link

Adrien-Pajot commented May 10, 2022

Ok pour la gestion en back complète de toutes façons, qui n'est pas forcément le sujet de la PR puisque celle-ci concerne le bouton qui pourra être installé par qui veut. Le script de gestion des taxons et mises à jour est travaillé en ce moment en interne avant de le mettre à disposition dès qu'il sera prêt :)

En revanche appeler l'API en direct sans charger les contenus, ca crée aussi une dépendance forte à des outils sur lesquels on a pas la main (les sites INPN étaient en vrac il y a quelques jours encore). Sur un outil de comm comme l'atlas, ca peut poser question. En revanche on peut imaginer de proposer les 2 alternatives à l'administrateur...

Je ne crois pas que ce soit qui était développé par @mvergez , il appelle directement l'API pour remplir TaxHub, c'est ça Max ?

@camillemonchicourt
Copy link
Member

Je ne crois pas que ce soit qui était développé par @mvergez , il appelle directement l'API pour remplir TaxHub, c'est ça Max ?

Oui @DonovanMaillard disait ça en lien avec mon propos sur le fait de ne plus importer les contenus dans TaxHub, mais d'interroger et d'afficher en direct les contenus de l'API INPN au niveau de GeoNature-atlas.

@mvergez
Copy link
Contributor Author

mvergez commented May 10, 2022

Je pense que cette solution est là pour aussi laisser le choix à l'utilisateur de compléter manuellement des informations ou via l'API de l'INPN les infos. Cas d'usage (peut-être un peu tordus) :
J'ai une description que j'ai changé par erreur => j'appelle l'API de l'INPN pour la remettre automatiquement.
J'ai supprimé par erreur des média => idem

Encore une fois, je suis d'accord avec vous pour avoir un script global (c'était mon idée première et je ne suis pas loin d'arriver à un outil fonctionnel), mais ce bouton selon moi laisse indépendant l'utilisateur vis à vis de l'admin bdd.

Lancer un script toutes les nuits ou à la demande de l'utilisateur pour vérifier si les informations sont à jour entre Taxhub et l'INPN n'est pas une solution car solliciterait trop l'API de l'INPN...

@camillemonchicourt : Si on réfléchit de manière plus globale le vrai enjeu pour moi serait de ne plus avoir TaxRef en base de données mais d'utiliser l'API de l'INPN directement lorsque l'on veut rechercher un taxon et l'importer dans une liste ou dans "Mes taxons" (et de pouvoir ajouter les taxons que l'on souhaite également). Ainsi la description et les médias pourront venir avec. Pour la cohérence et la pérennité des données, il est possible, via cet API, de changer de version de Taxref. On stockerait alors la version associée pour retrouver plus facilement les infos. Mais je pense que c'est un autre débat.

Je suis d'accord pour le moment avec @DonovanMaillard pour les appels directement depuis l'atlas. Cela ne laissera aussi pas place à la personnalisation mais à réfléchir car je suis aussi assez peu partisan de la duplication des données (chacun son taxref...).

PS : Attention, cette PR réduit la couverture des tests pour le moment

Je ne comprends pas pourquoi : je n'ai modifié que 2 fichiers de front qui n'est, si je ne me trompe pas, pas testé. Le coverage ne devrait pas être réduit. Si vous pouvez m'éclairer là dessus, je suis preneur.

@camillemonchicourt
Copy link
Member

Cela ne laissera aussi pas place à la personnalisation mais à réfléchir car je suis aussi assez peu partisan de la duplication des données (chacun son Taxref...).

Si si, l'idée est justement de se baser sur les médias et les descriptions locales saisies dans TaxHub, mais d'interroger l'API INPN pour les espèces qui n'ont pas de médias ou de descriptions locales. Du coup ce serait un bonus, et pas catastrophique si l'API ne répond pas pendant quelques heures. Et ainsi ne plus dupliquer, gérer, mettre à jour les contenus INPN qui sont disponibles sous forme d'API.


Concernant Taxref, on a beaucoup d'usages, de requêtes, de jointures, de synthèses dans la BDD de GeoNature ou avec des outils tiers pour pouvoir se passer d'avoir Taxref dans la BDD.

Un des enjeux pour simplifier la gestion des médias, des attributs additionnels mais aussi les MAJ de Taxref est de supprimer bib_noms pour n'avoir plus que Taxref, des listes de taxons, des attributs additionnels et des médias.

@mvergez
Copy link
Contributor Author

mvergez commented May 11, 2022

Si si, l'idée est justement de se baser sur les médias et les descriptions locales saisies dans TaxHub, mais d'interroger l'API INPN pour les espèces qui n'ont pas de médias ou de descriptions locales. Du coup ce serait un bonus, et pas catastrophique si l'API ne répond pas pendant quelques heures. Et ainsi ne plus dupliquer, gérer, mettre à jour les contenus INPN qui sont disponibles sous forme d'API.

Ce mode hybride peut être effectivement pas mal ! On amène un peu plus de complexité dans le code mais pourquoi pas.

Concernant Taxref, on a beaucoup d'usages, de requêtes, de jointures, de synthèses dans la BDD de GeoNature ou avec des outils tiers pour pouvoir se passer d'avoir Taxref dans la BDD.

Je pense vraiment qu'on pourrait prendre un temps pour réfléchir à cette question. Le fait de dupliquer Taxref à chaque fois que l'on installe GeoNature ne me parait pas optimal (tout comme le ref_geo d'ailleurs). On stockerait toujours l'information (pour ne pas plomber l'API de l'INPN à chaque recherche) et donc cela permettrait toujours de faire des jointures etc. On pourrait ouvrir un ticket pour en discuter ?
Car je serais plus OK de garder bib_noms (taxons importés depuis l'API ou autre) et de supprimer Taxref. Mais encore une fois, à discuter, je n'ai pas une vision globale des choses...

@DonovanMaillard
Copy link
Contributor

Concernant Taxref, on a beaucoup d'usages, de requêtes, de jointures, de synthèses dans la BDD de GeoNature ou avec des outils tiers pour pouvoir se passer d'avoir Taxref dans la BDD.

Je pense vraiment qu'on pourrait prendre un temps pour réfléchir à cette question. Le fait de dupliquer Taxref à chaque fois que l'on installe GeoNature ne me parait pas optimal (tout comme le ref_geo d'ailleurs). On stockerait toujours l'information (pour ne pas plomber l'API de l'INPN à chaque recherche) et donc cela permettrait toujours de faire des jointures etc. On pourrait ouvrir un ticket pour en discuter ? Car je serais plus OK de garder bib_noms (taxons importés depuis l'API ou autre) et de supprimer Taxref. Mais encore une fois, à discuter, je n'ai pas une vision globale des choses...

A discuter sur un ticket dédié, il y a trop de débats possibles, on va tuer ce ticket là ;)

@camillemonchicourt
Copy link
Member

En effet, gros sujets bien plus larges.

  • Concernant la suppression de bib_noms pour moi c'est un sujet clair et important, mais il faut s'y atteler. bib_noms a un rôle historique au moment où on a créé TaxHub et où Taxref n'avait pas du tout la maturité actuelle, notamment au niveau des noms français. Ça a évolué depuis et bib_noms est devenu une contrainte forte pour les utilisateurs, pour la gestion des médias, des attributs additionnels, mais aussi pour la gestion des mises à jour de Taxref, etc... Pour moi, il est important de supprimer cette table. Cela a été évoqué (en partie) ici : Supprimer id_nom de bib_noms #111 et ici : Proposition d'évolution du MCD #163. Ces tickets datent et mériteraient d'être complété pour aller plus loin dans l'explication des raisons de la suppression de bib_noms et pas uniquement du champs id_nom (qui est la première étape).
  • Pour la suppression de Taxref, pour moi c'est clairement une mauvaise piste car il a un rôle et un usage central dans GeoNature, mais on peut en (re)discuter dans un ticket dédié si besoin
  • Pour la suppression du ref_geo, il est moins central que Taxref, mais par contre son usage et intérêt au sein de la BDD de GeoNature est encore plus fort, avec tous les traitements et requêtes que l'on fait au niveau BDD et qui ont encore pris de l'importance en terme de volumes et de performances avec le floutage des données sensibles, et l'agrégation des données par maille ou autre zonage. Discuté ici si besoin : [ref_geo] Transformer le schéma ref_geo de la base de données de GeoNature en un service indépendant GeoNature#1607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants