Produits Entr'ouvert » Passerelle

Testé de bout en bout en local via la création du connecteur + paramétrage du fournisseur SMS dans hobo, avec une URL de type {{ passerelle_url }}smsfactor/sms-factor/send

SMS Factor supporte des appels sandbox en ajoutant un paramètre simulate (aucun SMS n'est envoyé aux destinataires), donc vous pouvez aussi faire {{ passerelle_url }}smsfactor/sms-factor/send?simulate=true dans la configuration SMS de hobo.

Ce qu'on voulait dire sur le salon avec Benj c'est que le schéma et la méthode send sont déjà définis dans SMSResource, il n'y a pas besoin de les réimplémenter comme dans le patch.

Valentin Deniaud a écrit :

Ce qu'on voulait dire sur le salon avec Benj c'est que le schéma et la méthode send sont déjà définis dans SMSResource, il n'y a pas besoin de les réimplémenter comme dans le patch.

Le sender est légèrement différent (ça ne supporte pas de numéro mais seulement un ID si je me base sur la doc SMS factor). Et le paramètre nostop n'a pas de sens pour SMS Factor. Mais je peux factoriser et retirer le support du simulate si on considère que ça n'en vaut pas la chandèle.

Yep c'est toujours mieux de ne pas introduire de particularités, que tous les connecteurs SMS aient la même tête.

Le paramètre stop, il y a moyen qu'il ne soit utilisé que par le connecteur OVH, donc déjà affiché de manière incorrecte sur cinq autres connecteurs : on peut garder cette bataille pour un autre endroit (pas sûr de l'utilité, puisqu'on appelle pas les connecteurs directement on ne va pas non plus voir le détail des endpoints).

Ce serait étonnant que l'ID ne puisse pas être alphanumérique, en tout cas plutôt ajouter un check en python avec une erreur explicative et utiliser le schéma de base.

Agate Berriot a écrit :

Le sender est légèrement différent (ça ne supporte pas de numéro mais seulement un ID si je me base sur la doc SMS factor). Et le paramètre nostop n'a pas de sens pour SMS Factor. Mais je peux factoriser et retirer le support du simulate si on considère que ça n'en vaut pas la chandèle.

Pourant dans la doc (https://dev.smsfactor.com/en/api/sms/send/send-single) je vois https://api.smsfactor.com/send?text=Hello world&to=33612345678&token=your.token le to ressemble beaucoup à un numéro.

Valentin Deniaud a écrit :

Le paramètre stop, il y a moyen qu'il ne soit utilisé que par le connecteur OVH, donc déjà affiché de manière incorrecte sur cinq autres connecteurs : on peut garder cette bataille pour un autre endroit (pas sûr de l'utilité, puisqu'on appelle pas les connecteurs directement on ne va pas non plus voir le détail des endpoints).

Le paramètre stop existe sur tous les backends mais ne se nomme pas forcément comme ça, sur d'autres backends ils font plutôt la différence entre un sms commercial ou une notification, commercial => stop=1, notification => stop=0.

Ici c'est stop=1 => pushtype=marketing, stop=0 => pushtype=alert.

Benjamin Dauvergne a écrit :

Agate Berriot a écrit :

Le sender est légèrement différent (ça ne supporte pas de numéro mais seulement un ID si je me base sur la doc SMS factor). Et le paramètre nostop n'a pas de sens pour SMS Factor. Mais je peux factoriser et retirer le support du simulate si on considère que ça n'en vaut pas la chandèle.

Pourant dans la doc (https://dev.smsfactor.com/en/api/sms/send/send-single) je vois https://api.smsfactor.com/send?text=Hello world&to=33612345678&token=your.token le to ressemble beaucoup à un numéro.

Le to est effectivement le numéro de téléphone du destinataire. Le sender (décrit dans la même doc) est un champ optionnel textuel par exemple « Mairie » affiché dans le SMS à la place du numéro un peu bateau d'SMS Factor d'après mes tests. S'il n'est pas spécifié, le message est marqué comme provenant d'un numéro genre 38082.

J'ai notés vos autres points, je fixe

Donc dernière version :

- retrait du simulate
- retrait de la surcharge du endpoint (on a juste ce qui est hérité de SMSResource) et du schema
- gestion du paramètre nostop pour déterminer le pushtype

Coolos ! Deuxième remarque générale, tu penserais quoi de mutualiser le code d'envoi d'alertes en le mettant dans SMSResource ? (un indicateur qu'on utilise parfois c'est d'attendre d'avoir le même code à trois endroits avant de factoriser, mais là comme on a déjà tout ce qu'il faut avec le modèle de base je me dis qu'on pourrait prendre un peu d'avance)

Agate Berriot a écrit :

Le to est effectivement le numéro de téléphone du destinataire. Le sender (décrit dans la même doc) est un champ optionnel textuel par exemple « Mairie » affiché dans le SMS à la place du numéro un peu bateau d'SMS Factor d'après mes tests. S'il n'est pas spécifié, le message est marqué comme provenant d'un numéro genre 38082.

Oui pardon j'étais à coté de la plaque, je ne vois rien qui limite la valeur de sender dans le schéma de base donc on pourrait le laisser vide et le passer à smsfactor s'il ne l'est pas, mais coté hobo où c'est configuré ça ne peut par contre pas être laissé vide, et coté w.c.s. on ne peut pas non plus le surcharger. Comme je ne sais pas ce que va faire le CD06 avec tout ça, je dirai bien de faire de sender un champ du modèle, vide par défaut, pour l'instant (ça paraîtrait logique que ça y soit déjà pour tous les backends vu que c'est déjà non variable entre hobo/wcs, mais bon l'historique veut que ce ne soit pas comme ça).

PS: et donc ça veut dire continuer à ignorer la valeur passer dans l'appel, c'est pas trop mais en l'état je ne vois pas mieux, l'API de base n'est pas future proof.

Valentin Deniaud a écrit :

Coolos ! Deuxième remarque générale, tu penserais quoi de mutualiser le code d'envoi d'alertes en le mettant dans SMSResource ? (un indicateur qu'on utilise parfois c'est d'attendre d'avoir le même code à trois endroits avant de factoriser, mais là comme on a déjà tout ce qu'il faut avec le modèle de base je me dis qu'on pourrait prendre un peu d'avance)

Bonne idée, mais dans un autre ticket.

Valentin Deniaud a écrit :

Coolos ! Deuxième remarque générale, tu penserais quoi de mutualiser le code d'envoi d'alertes en le mettant dans SMSResource ? (un indicateur qu'on utilise parfois c'est d'attendre d'avoir le même code à trois endroits avant de factoriser, mais là comme on a déjà tout ce qu'il faut avec le modèle de base je me dis qu'on pourrait prendre un peu d'avance)

Plutôt contre, c'est aussi mon indicateur d'attendre 3 répétitions du code avant de commencer à se poser la question :D

Si un troisième connecteur similaire se pointe, je me désigne volontaire pour prendre le sujet !

Benjamin Dauvergne a écrit :

Bonne idée, mais dans un autre ticket.

Je suis tout pour découper les tickets mais ici l'idée c'est de simplifier la relecture (ne relire que des lignes qui sont vraiment nouvelles). Sous ce prisme le faire dans un autre ticket n'a pas d'intérêt, on peut zapper.

Agate Berriot a écrit :

Plutôt contre

No problem.

Benjamin, je viens de faire le test avec le sender et ça marche très bien en fait de saisir un sender textuel dans la configuration SMS, j'ai juste amendé mon patch précédent pour passer cette information à SMS factor.

Si quelqu'un veut relire et valider, on peut passer à autre chose :)

Remarques de forme puisque je passe par là :)

• Le copyright devrait dire 2022
• La variable endpoint dans send_msg n'est plus utile (comparé à la première version du patch)
• La construction de destinations serait plus lisible sur une ligne : destinations = [dest[2:] for dest in destinations]
• Dans les try: on préfère ne mettre que les lignes qui sont susceptible de lever l'exception, le self.save devrait venir après dans un else:
• Le pytest.skip pour le test du paramètre ?nostop correspond peut-être aussi à une ancienne version du patch

Et sur le fond, le raise APIError à la fin de send_msg pose question, est-ce que SMS factor peut se retrouver à envoyer le message à certains destinataires et pas à d'autres ? Si en face on a un système qui réessaye quand il reçoit une erreur, ça conduirait à des messages envoyés en double : pour cette raison je préférerais dire au monde que tout s'est bien passé et juste logger les erreurs (mais il n'y a pas de solution idéale, et je ne sais pas si le cas que j'imagine est réel).

Dernier patch validé avec le modifications suggérées par Valentin.

Et sur le fond, le raise APIError à la fin de send_msg pose question, est-ce que SMS factor peut se retrouver à envoyer le message à certains destinataires et pas à d'autres ? Si en face on a un système qui réessaye quand il reçoit une erreur, ça conduirait à des messages envoyés en double : pour cette raison je préférerais dire au monde que tout s'est bien passé et juste logger les erreurs (mais il n'y a pas de solution idéale, et je ne sais pas si le cas que j'imagine est réel).

Je me suis basé sur le code du connecteur Twilio, dont l'API est bien plus proche de SMS factor que celle d'OVH. Le code du connecteur oxyd fait également la même chose.

(patch avec du linting, j'avais zappé l'arrivée de djhtml un peu partout)

Valentin Deniaud a écrit :

Agate Berriot a écrit :

Je me suis basé sur le code du connecteur Twilio, dont l'API est bien plus proche de SMS factor que celle d'OVH. Le code du connecteur oxyd fait également la même chose.

Ouep qu'il y ait des précédents annule tout à fait ma remarque, allons-y comme ça !

Pour entretenir ma réputation de pénible : les connecteurs SMS Twilio ou Oxyd n'ont jamais été utilisés nulle part. Ne pas en faire des références...

Automatic expiration