#196 Inscription autonome aux mailling lists depuis coin

Fusionné
jocelyn a fusionné 17 commits à partir de FFDN/jd-maillist-management vers FFDN/master il y a 6 ans
jocelyn a commenté il y a 6 ans

Permettre aux adhérent·e·s de s'inscrire/désinscrire seul·e·s aux listes mail de l'association.

capture pour mettre l'eau à la bouche.

Cela se veut une première version basique. Plusieurs améliorations sont possibles ; je les ai pour l'instant regroupées dans ce ticket https://code.ffdn.org/FFDN/coin/issues/195

Je suis bien friand de retours, pour vérifier que ça marche, me donner votre avis sur mon code, améliorer la doc… etc.

Ce qui me reste à faire (mais je pense que la review peut se faire ou a minima commencer sans) :

  • finir d'écrire les tests unitaires
  • bien border les cas où la commande de synchronisation échoue
    • si synchro lancée par une modif de la fiche adhérent·e : ne pas bloquer coin pour autant, mais afficher un message d'erreur explicite à l'admin : Trop compliqué à gérer, donc je laisse comme ça : le code plantera (erreur 500) dans ce cas.
    • si synchro lancée explicitement dans l'admin (via les admin actions), ne pas planter coin, mais afficher des messages pour indiquer quelles listes ont réussi et quelles listes ont échoué.
    • si synchro lancée par l'inscription d'un·e adhérent·e de manière autonome, l'avertir si la synchro a échoué (et ne pas enregistrer son adhésion en base).
Permettre aux adhérent·e·s de s'inscrire/désinscrire seul·e·s aux listes mail de l'association. ![capture pour mettre l'eau à la bouche](https://screenshotscdn.firefoxusercontent.com/images/6327d1e3-f4e8-4824-9e17-38ed3c457783.png). Cela se veut une première version basique. Plusieurs améliorations sont possibles ; je les ai pour l'instant regroupées dans ce ticket https://code.ffdn.org/FFDN/coin/issues/195 Je suis bien friand de retours, pour vérifier que ça marche, me donner votre avis sur mon code, améliorer la doc… etc. Ce qui me reste à faire (mais je pense que la review peut se faire ou a minima commencer sans) : - [x] finir d'écrire les tests unitaires - [x] bien border les cas où la commande de synchronisation échoue - <strike>si synchro lancée par une modif de la fiche adhérent·e : ne pas bloquer coin pour autant, mais afficher un message d'erreur explicite à l'admin</strike> : Trop compliqué à gérer, donc je laisse comme ça : le code plantera (erreur 500) dans ce cas. - [x] si synchro lancée explicitement dans l'admin (via les admin actions), ne pas planter coin, mais afficher des messages pour indiquer quelles listes ont réussi et quelles listes ont échoué. - [x] si synchro lancée par l'inscription d'un·e adhérent·e de manière autonome, l'avertir si la synchro a échoué (et ne pas enregistrer son adhésion en base).
jocelyn a commenté il y a 6 ans
Propriétaire

j'ai écrit les tests.

j'ai écrit les tests.
jocelyn a commenté il y a 6 ans
Propriétaire

J'ai géré les cas d'erreur au mieux.

J'ai géré les cas d'erreur au mieux.
capslock a commenté il y a 6 ans
Collaborateur

à la vue très rapide du code je pense qu'il y a un risque d'accès concurrent : rien ne semble empêcher l'exécution de deux commande de synchronisation en même temps.

Vu les soucis, cas d'erreurs, blocages évoqués dans ta PR je me dis que l'utilisation de taches asynchrones en arrière plan auraient une bonne place pour résoudre ces soucis

à la vue très rapide du code je pense qu'il y a un risque d'accès concurrent : rien ne semble empêcher l'exécution de deux commande de synchronisation en même temps. Vu les soucis, cas d'erreurs, blocages évoqués dans ta PR je me dis que l'utilisation de taches asynchrones en arrière plan auraient une bonne place pour résoudre ces soucis
jocelyn a commenté il y a 6 ans
Propriétaire

Hey, merci :-)

à la vue très rapide du code je pense qu'il y a un risque d'accès concurrent : rien ne semble empêcher l'exécution de deux commande de synchronisation en même temps.

Non, c'est vrai. Dans la plupart des cas, cela ne posera pas souci majeur (à mon avis) vu que la liste est régénérée en entier à chaque modif.

Vu les soucis, cas d'erreurs, blocages évoqués dans ta PR je me dis que l'utilisation de taches asynchrones en arrière plan auraient une bonne place pour résoudre ces soucis

Dans l'idée oui, ça serait mieux. J'ai choisi de ne pas utiliser des tâches asynchrones (type celery) pour l'instant :

  • complexité (l'autre nom de la complexité est « flemme »)
  • on reste sur des tâches pas très longues non plus (donc la latence n'était pas un critère énorme à mon sens, mais ça reste à voir…)
  • plus complexe de produire un retour utilisateur (passe par un système de messages/alertes asynchrones)

Bref, j'ai l'impression qu'utiliser un système de tâches asynchrones pourrait être une solution mais demanderait un travail un peu trop conséquent et repousserait pas mal cette fonctionnalité :-/.

Hey, merci :-) > à la vue très rapide du code je pense qu'il y a un risque d'accès concurrent : rien ne semble empêcher l'exécution de deux commande de synchronisation en même temps. Non, c'est vrai. Dans la plupart des cas, cela ne posera pas souci majeur (à mon avis) vu que la liste est régénérée en entier à chaque modif. > Vu les soucis, cas d'erreurs, blocages évoqués dans ta PR je me dis que l'utilisation de taches asynchrones en arrière plan auraient une bonne place pour résoudre ces soucis Dans l'idée oui, ça serait mieux. J'ai choisi de ne pas utiliser des tâches asynchrones (type celery) pour l'instant : - complexité (l'autre nom de la complexité est « flemme ») - on reste sur des tâches pas très longues non plus (donc la latence n'était pas un critère énorme à mon sens, mais ça reste à voir…) - plus complexe de produire un retour utilisateur (passe par un système de messages/alertes asynchrones) Bref, j'ai l'impression qu'utiliser un système de tâches asynchrones pourrait être une solution mais demanderait un travail un peu trop conséquent et repousserait pas mal cette fonctionnalité :-/.
capslock a commenté il y a 6 ans
Collaborateur

(je ne sais pas comment tu as fait pour citer une portion de message, gogs est une énigme)

Oui ca rajoute de la complexité tant sur le flow de traitement que sur la mise en place des environnements. Après j'ai pas suivi depuis un certain temps les développements de Coin mais si il y a d'autres besoins de ce type ca peut valoir le coup (générations des factures, relances sans passer par du cron etc, ...)

Dans l'absolu je rejoins ton avis, au pire ca sera pété jusqu'à ce qu'un gentil membre change ses abonnements

(je ne sais pas comment tu as fait pour citer une portion de message, gogs est une énigme) Oui ca rajoute de la complexité tant sur le flow de traitement que sur la mise en place des environnements. Après j'ai pas suivi depuis un certain temps les développements de Coin mais si il y a d'autres besoins de ce type ca peut valoir le coup (générations des factures, relances sans passer par du cron etc, ...) Dans l'absolu je rejoins ton avis, au pire ca sera pété jusqu'à ce qu'un gentil membre change ses abonnements
jocelyn a commenté il y a 6 ans
Propriétaire

(je ne sais pas comment tu as fait pour citer une portion de message, gogs est une énigme)

markdown + copié-collé

Oui ca rajoute de la complexité tant sur le flow de traitement que sur la mise en place des environnements. Après j'ai pas suivi depuis un certain temps les développements de Coin mais si il y a d'autres besoins de ce type ca peut valoir le coup (générations des factures, relances sans passer par du cron etc, ...)

Yep, je pense que ça pourrait valoir le coup pour ce genre de cas dans l'absolu. Juste le rapport temps passé / bénéfice n'est pas très favorable à mon avis. Si on fait un dévcamp et que quelqu'un·e veut jouer avec celery, ça peut être un truc rigolo en revanche. À voir.

Dans l'absolu je rejoins ton avis, au pire ca sera pété jusqu'à ce qu'un gentil membre change ses abonnements

Sachant que les admins ont la possibilité de déclencher manuellement la synchro.

> (je ne sais pas comment tu as fait pour citer une portion de message, gogs est une énigme) markdown + copié-collé > Oui ca rajoute de la complexité tant sur le flow de traitement que sur la mise en place des environnements. Après j'ai pas suivi depuis un certain temps les développements de Coin mais si il y a d'autres besoins de ce type ca peut valoir le coup (générations des factures, relances sans passer par du cron etc, ...) Yep, je pense que ça pourrait valoir le coup pour ce genre de cas dans l'absolu. Juste le rapport temps passé / bénéfice n'est pas très favorable à mon avis. Si on fait un dévcamp et que quelqu'un·e veut jouer avec celery, ça peut être un truc rigolo en revanche. À voir. > Dans l'absolu je rejoins ton avis, au pire ca sera pété jusqu'à ce qu'un gentil membre change ses abonnements Sachant que les admins ont la possibilité de déclencher manuellement la synchro.
ljf a commenté il y a 6 ans
Collaborateur

ARN ne compte pas utiliser ce module pour l'instant (on met en place un forum discourse pour remplacer nos ML) Donc on vous laisse gérer cette MR.

ARN ne compte pas utiliser ce module pour l'instant (on met en place un forum discourse pour remplacer nos ML) Donc on vous laisse gérer cette MR.
Sim a commenté il y a 6 ans
Collaborateur

On a fait quelques modifs.

Le truc qui reste nul c'est que si la synchro plante :

  • on peut éditer ses abo depuis l'interface user (et il y a un warning) donc c'est bien
  • par contre pas depuis l'interface d'admin
  • et même pire ça empêche l'édition de l'adresse mail (puisqu'elle déclenche la synchro)

Pour les deux derniers points on pourrait rattraper l'erreur mais pas afficher de "message" parce qu'on n'a pas accès à la "request" dans les signaux ...

Je propose de merger en l'état après relecture.

On a fait quelques modifs. Le truc qui reste nul c'est que si la synchro plante : - on peut éditer ses abo depuis l'interface user (et il y a un warning) donc c'est bien - par contre pas depuis l'interface d'admin - et même pire ça empêche l'édition de l'adresse mail (puisqu'elle déclenche la synchro) Pour les deux derniers points on pourrait rattraper l'erreur mais pas afficher de "message" parce qu'on n'a pas accès à la "request" dans les signaux ... Je propose de merger en l'état après relecture.
jocelyn a commenté il y a 6 ans
Propriétaire

OK pour le compromis sur la gestion d'erreur, c'est comme ça :-)

Relu, testé, validé !

… Et je n'ai pas pu m'empêcher, j'ai apporté quelques peaufinements d'interfaces (cf dernier commit dont le message est détaillé).

@sim Vu que c'est des changements mineurs, je merge dans la foulée, qu'on en finisse :-)

Youhou !

OK pour le compromis sur la gestion d'erreur, c'est comme ça :-) Relu, testé, validé ! … Et je n'ai pas pu m'empêcher, j'ai apporté quelques peaufinements d'interfaces (cf dernier commit dont le message est détaillé). @sim Vu que c'est des changements mineurs, je merge dans la foulée, qu'on en finisse :-) Youhou !
jocelyn a commenté il y a 6 ans
Propriétaire

Et j'ai documenté un petit warning également pour éviter d'être surpris par les 200 appels à SSH.

Et j'ai documenté un petit warning également pour éviter d'être surpris par les 200 appels à SSH.
Cette Pull Request a été fusionnée avec succès !
Connectez-vous pour rejoindre cette conversation.
Aucun jalon
Pas d'assignataire
4 Participants
Chargement…
Annuler
Enregistrer
Il n'existe pas encore de contenu.