search

assemblee-virtuelle / archipelago

Fostering interconnections between communities by creating synergies between their platforms
Apache License 2.0
14 stars 6 forks source link

Proposition d'alternative au DereferenceMixin - Gestion de MembershipAssociation côté frontend #175

Closed mguihal closed 2 weeks ago

mguihal commented 4 months ago

Hello,

Suite aux récents déboires avec la gestion des relations réifiées (Organization <-> MembershipAssociation <-> Person), et du dereferenceMixin qui avait été rajouté dans Archipelago mais qui un coup fonctionne (#133, #175), un coup ne fonctionne plus (#5, #45, #173), un coup est commenté dans le code (#158), je me suis mis en peine de réfléchir à une solution alternative, plus dans l'esprit de react-admin, en gérant le fetch des données de la relation côté front, au lieu de le faire côté middleware.

J'aimerais bien avoir votre avis éclairé et constructif @srosset81 et @simonLouvet sur cette proposition, pour savoir si selon vous cette solution répond à tous les usages pour lesquels nous avions besoin de déférencement d'une part. Et pour savoir s'il est préférable de faire cette logique côté front (comme proposé ici), ou côté middleware (comme c'est fait actuellement).

Je donne mon avis personnel, je trouve cette solution côté frontend plus élégante que l'actuelle, et pouvant être généralisée plus facilement (sans la nécessité de créer un nouveau service pour y inclure le container avec le dereferenceMixin côté middleware). Après, je manque peut-être de contexte et d'historique pour me faire un avis complet.

PS : @simonLouvet J'ai commencé à réfléchir et à implémenter cette proposition avant que tu ne proposes la correction sur le derefenceMixin hier, ne sachant pas si le bug détecté pouvait être résolu facilement et rapidement 😬

PS2 : Je n'ai fait pour l'instant que la modification côté frontend, mais si cette solution venait à être validée, il faudra faire les modifications nécessaires côté middleware aussi (ainsi que sur Semapps pour supprimer les composants liés dans le semantic-data-provider éventuellement)

simonLouvet commented 3 months ago

C'est ok pour moi après une petite h de revue. Un grand merci @mguihal pour faire avancer ce chantier.

Les composants étaient génériques et deviennent spécifiques mais ils ne servent que dans archipelago dans le cas des relations réifiées donc on peut rester comme ca pour le moment. Un petit commentaire pour faire du 100% spécifique pour éviter les confusions d'avoir des composants paramétrables qui ne le sont pas. Je suis donc ok pour faire du nettoyage dans les composants d'interface qui servaient pour les relations réifiées. Pour FilterHandler, qui est dans @semapps/semantic-data-provider, c'est également ok pour moi de le supprimer car il n'a pas servi et du code spécifique est utilisé dans les projets qui ont besoin de filtrer les données. On peut donc supprimer le répertoire réification de @semapps/semantic-data-provider

Je rappelle que la migration des relations réifiées vers le front n'est pas terminée puisque c'est le mixin disassembly de @semapps/ldp qui permet d'enregistrer les bons triplets dans les bons containers sur le middleware.

Je pense qu'il faut garder les mixins disassembly et dereference dans @semapps/ldp pour pouvoir faire des APIs LDP qui gèrent les réifications côtés middleware avec des ControlledContainerMixin . Je m'engage à les maintenir. Je ne ferais pas objection à leur suppression mais je demande à les garder.

Je me suis permis de merge #174 suite à la prise en compte de la review de @srosset81 : Ne pas oublier de supprimer le mixin sur les containers users et oganizations.

srosset81 commented 3 months ago

@simonLouvet Je suis OK pour intégrer le DereferenceMixin dans le package @semapps/ldp. Le DisassemblyMixin est déjà dans ce package. Idéalement il faudrait que ces deux mixins soient documentés sur le site SemApps comme p.ex. https://semapps.org/docs/middleware/ldp/controlled-container ... Ce n'est pas une obligation de mon point de vue, mais sans documentation il faut assumer que personne d'autre que nous ne va les utiliser (en tout cas pour le moment). Le DereferenceMixin me semble avoir plus de possibilités d'usage que le DisassemblyMixin (dont le nom n'est pas clair je trouve).

mguihal commented 3 weeks ago

Hello, Concernant cette PR, j'ai vu que le dereferenceMixin avait été déjà ajouté à Semapps. J'ai rajouté un commit pour supprimer la partie middleware. Si c'est ok pour vous, je merge d'ici la fin de semaine :)