Produits Entr'ouvert » Passerelle

Agate Berriot a écrit :

(le problème de fond ici est que le token est pas bon

Juste pour parler : ce n'est pas possible de détecter cela au niveau de self.request ?

Thomas Noël a écrit :

Agate Berriot a écrit :

(le problème de fond ici est que le token est pas bon

Juste pour parler : ce n'est pas possible de détecter cela au niveau de self.request ?

Si carrément, c'est même beaucoup mieux :D

mon second patch fait ça ;)

Bon en fait y'avait des erreurs dans les tests un peu moins triviales à gérer (notamment le multi destinataire). Avec un peu de chance ça passe sur jenkins et tout le monde est content.

raise_on_response_error = kwargs.pop('raise_on_response_error', True)

Ajoutons plutôt un explicite raise_on_response_error=True dans la fonction.

Par ailleurs mon expérience avec nos amies les webservices me ferait ajouter une vérification que result est un dictionnaire, avant de faire le result.get('message').

Perso je pense qu'il faut revenir en arrière, on a deux endpoints qui ne sont pas homogènes et n'ont pas la même manière d'indiquer une erreur, il ne faut pas chercher à mutualiser le code de gestion d'erreur dans self.request. Là ce code on se retrouve à l'utiliser pour un endpoint et le désactiver pour l'autre, sa place est donc directement dans self.update_credit_left (c'est revenir sur l'idée de Thomas, à voir ce qu'il en dit).

PS : ajouter un test pour couvrir le nouveau code c'est toujours mieux (j'aurais pu dire ça avant de valider tout à l'heure), bonus ça rendrait explicite à la lecture du patch l'erreur qu'on est en train de gérer (« le token est pas bon »).

Valentin Deniaud a écrit :

Perso je pense qu'il faut revenir en arrière, on a deux endpoints qui ne sont pas homogènes et n'ont pas la même manière d'indiquer une erreur, il ne faut pas chercher à mutualiser le code de gestion d'erreur dans self.request. Là ce code on se retrouve à l'utiliser pour un endpoint et le désactiver pour l'autre, sa place est donc directement dans self.update_credit_left (c'est revenir sur l'idée de Thomas, à voir ce qu'il en dit).

Là, comme ça, rien. En fait je ne comprends finalement pas vraiment pourquoi il y a un "raise_on_response_error", dans ma tête self.request devrait toujours planter quand une erreur de token est détectée. Mais si c'est pas possible alors le faire uniquement pour self.update_credit_left oui, pourquoi pas. Mais c'est bizarre non ? (je dois rater un truc, as usual)

Thomas Noël a écrit :

Là, comme ça, rien. En fait je ne comprends finalement pas vraiment pourquoi il y a un "raise_on_response_error", dans ma tête self.request devrait toujours planter quand une erreur de token est détectée.

On a 0 garantie que ce soit une erreur de token (message != 'OK' ça couvre toutes les erreurs).
Or il n'y a pas d'endpoint qui permette d'envoyer un SMS à plusieurs destinataires. Donc dans self.send_msg il y a une boucle qui self.request('/send/', un_numéro).
Au cas où une erreur soit retournée pour un numéro et pas pour les autres, le code essaye d'abord tous les numéros, puis remonte les erreurs. Si on ne met pas raise_on_response_error=False, alors on essaye plus tous les numéros, on plante à la première erreur.

(c'est juste un résumé de la situation, je ne sais pas comment bien gérer cette histoire qui peut aboutir à des envois partiels ingérables, mais c'est la faute de l'API et on sort du cadre de ce ticket)

le faire uniquement pour self.update_credit_left oui, pourquoi pas. Mais c'est bizarre non ?

C'est l'API qui est bizarre, j'insiste sur

on a deux endpoints qui ne sont pas homogènes et n'ont pas la même manière d'indiquer une erreur

Car d'un côté /send/ renvoie une clé status en plus de la clé message, de l'autre /credits/ renvoie juste message. (fun fact, quand il n'y a pas d'erreur status vaut 1, on a donc affaire à une API qualité platine)

Automatic expiration