Project

General

Profile

Développement #56116

ovh : exposer le crédit consommé par un SMS

Added by Valentin Deniaud over 3 years ago. Updated about 3 years ago.

Status:
Fermé
Priority:
Normal
Target version:
-
Start date:
11 August 2021
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

C'est une info que l'API OVH nous renvoie mais on ne l'a pas dans les logs.


Files

0001-sms-warn-when-credit-consumption-is-high-56116.patch (7.13 KB) 0001-sms-warn-when-credit-consumption-is-high-56116.patch Valentin Deniaud, 11 August 2021 05:31 PM
1628759340.png (66.4 KB) 1628759340.png Valentin Deniaud, 12 August 2021 11:13 AM
0001-sms-record-credits-used-56116.patch (8.35 KB) 0001-sms-record-credits-used-56116.patch Valentin Deniaud, 08 September 2021 05:44 PM
0001-sms-record-credits-used-56116.patch (12.9 KB) 0001-sms-record-credits-used-56116.patch Valentin Deniaud, 15 September 2021 12:07 PM

Associated revisions

Revision 7f57c504 (diff)
Added by Valentin Deniaud about 3 years ago

sms: record credits used (#56116)

History

#1

Updated by Valentin Deniaud over 3 years ago

  • Certains connecteurs SMS retournent un dictionnaire avec des infos hétérogènes et ce n'est exploité nulle part, je retire tout ça.
  • À la place, retour du nombre de crédits consommés par un SMS.
  • Info exploitée de deux manières :
    • Stockage dans le log SMSLog correspondant parce que ça ne mange pas de pain.
    • Log d'une alerte si le nombre de crédits est supérieur à 1.
#2

Updated by Thomas Noël over 3 years ago

J'ai un peu de mal à avoir où l'information va être visible... ou bien c'est juste un log dans un coin (vus les tests j'ai l'impression) ?

Idéalement on aimerait un affichage au niveau des logs des jobs. Et je me dis qu'on pourrait enregistrer des trucs dans job.status_details quand un job réussi -- actuellement on ne s'en sert qu'en cas d'échec. (job.status_details est affiché quand on clique sur un job, donc ça serait bien).

On pourrait aussi sans doute ajouter une série dans SmsStatisticsView (sms/models.py) ? (Ok ça peut être un autre ticket, mais je me dis que c'est peut-être pas mortel)

#3

Updated by Valentin Deniaud over 3 years ago

Thomas Noël a écrit :

J'ai un peu de mal à avoir où l'information va être visible... ou bien c'est juste un log dans un coin (vus les tests j'ai l'impression) ?

Au contraire c'est très visible, je joins le screen nécessaire à la compréhension du patch.

Idéalement on aimerait un affichage au niveau des logs des jobs. Et je me dis qu'on pourrait enregistrer des trucs dans job.status_details quand un job réussi -- actuellement on ne s'en sert qu'en cas d'échec. (job.status_details est affiché quand on clique sur un job, donc ça serait bien).

Et donc par rapport à cette idée actuellement c'est dans le log des appels (kif-kif avec le log des jobs IMO) et ce n'est pas caché dans une pop-up. As you want pour faire l'un ou l'autre.

On pourrait aussi sans doute ajouter une série dans SmsStatisticsView (sms/models.py) ? (Ok ça peut être un autre ticket, mais je me dis que c'est peut-être pas mortel)

Pas convaincu par l'utilité, autre ticket si besoin :)

#4

Updated by Thomas Noël over 3 years ago

Valentin Deniaud a écrit :

Thomas Noël a écrit :

J'ai un peu de mal à avoir où l'information va être visible... ou bien c'est juste un log dans un coin (vus les tests j'ai l'impression) ?

Au contraire c'est très visible, je joins le screen nécessaire à la compréhension du patch.

Ah oui, mais pour le coup on dirait une erreur alors que non (je pense qu'aucun message qu'on envoie ne consomme qu'un seul sous-message). Ca me gratouille.

Idéalement on aimerait un affichage au niveau des logs des jobs. Et je me dis qu'on pourrait enregistrer des trucs dans job.status_details quand un job réussi -- actuellement on ne s'en sert qu'en cas d'échec. (job.status_details est affiché quand on clique sur un job, donc ça serait bien).

Et donc par rapport à cette idée actuellement c'est dans le log des appels (kif-kif avec le log des jobs IMO) et ce n'est pas caché dans une pop-up. As you want pour faire l'un ou l'autre.

J'aime bien l'autre (avoir des infos en plus dans le job).

Pour que ça soit visible sans un clic, on pourrait par convention décider que la liste des jobs (resource-jobs-table.html) affiche les clés-valeur de job.status_details.additionnal_status_info quand ce dictionnaire existe. Charge au job de savoir qu'il faut retourner un dictionnaire du genre {'status_details': {'additionnal_status_info': {"sms-count": 2}}} quand il veut profiter de cette possibilité.

On pourrait aussi sans doute ajouter une série dans SmsStatisticsView (sms/models.py) ? (Ok ça peut être un autre ticket, mais je me dis que c'est peut-être pas mortel)

Pas convaincu par l'utilité, autre ticket si besoin :)

L'idée c'est que dans les stats, on voit le nombre de messages envoyés à côté de la consommation correspondante. Mais c'est pas important, ça sera un autre ticket, un jour, quand ça sera demandé.

#5

Updated by Valentin Deniaud over 3 years ago

Thomas Noël a écrit :

Ah oui, mais pour le coup on dirait une erreur alors que non

Il y a un deuxième truc pas clair de ce ticket c'est que la description est imprécise borderline fausse

on ne l'a pas dans les logs

En fait si on l'a, en allant manuellement mettre le niveau de log du connecteur à info, ça déclenche le log de la réponse de l'API d'OVH qui contient la clé avec le nombre de crédits. Et donc reformulation de l'objectif du ticket, rendre l'info 1) loggée par défaut 2) visible, d'où la solution évidente de logger ça à un niveau plus haut pour cocher les deux points en même temps.

je pense qu'aucun message qu'on envoie ne consomme qu'un seul sous-message

Là par contre je suis parti de l'hypothèse inverse, que c'est effectivement limite une erreur d'avoir un SMS qui coûte plus que un SMS.

Pour que ça soit visible sans un clic, on pourrait par convention décider que la liste des jobs (resource-jobs-table.html) affiche les clés-valeur de job.status_details.additionnal_status_info quand ce dictionnaire existe. Charge au job de savoir qu'il faut retourner un dictionnaire du genre {'status_details': {'additionnal_status_info': {"sms-count": 2}}} quand il veut profiter de cette possibilité.

Pour moi l'info ne sera toujours pas assez visible, mais je peux quand même partir là dessus et ajouter un message tout en haut de la page du connecteur « attention, ce connecteur a envoyé récemment un message qui a consommé plusieurs crédits (voir le message) ». Va pour ça ?

#6

Updated by Thomas Noël over 3 years ago

Valentin Deniaud a écrit :

Là par contre je suis parti de l'hypothèse inverse, que c'est effectivement limite une erreur d'avoir un SMS qui coûte plus que un SMS.

Ok c'est là dessus qu'on est en désaccord. Avec les smiley et les accents la limite réelle c'est 70 caractères, n'importe quel message dépasse... c'est même pour ça que plus aucun opérateur ne facture les SMS aux particuliers, plus personne n'y comprenait rien :-)

Donc non, y'a pas d'erreur, c'est normal.

Pour moi l'info ne sera toujours pas assez visible, mais je peux quand même partir là dessus et ajouter un message tout en haut de la page du connecteur « attention, ce connecteur a envoyé récemment un message qui a consommé plusieurs crédits (voir le message) ». Va pour ça ?

Bah non parce que pour moi c'est pas du tout une erreur ou un warning, c'est normal. Autrement dit, ce message va se retrouver sur tous les connecteurs SMS :) Aussi, cette page Passerelle, personne n'y va jamais. C'est aussi pour ça que ça serait plus utile sans doute de penser plutôt à l'export statistique qui, lui, sera affiché sur le portail agent (et on pourra voir que la conso SMS est double du nombre de message envoyés)

Désolé d'être un peu relou sur ce patch simple, mais comme je pressens qu'on va avoir du retour de bâton de quelques clients, autant faire un truc clair et propre, en évitant de faire croire qu'il y a des erreurs ou des bogues (parce que y'en a pas).

#7

Updated by Valentin Deniaud over 3 years ago

Thomas Noël a écrit :

autant faire un truc clair et propre

Tout à fait d'accord et donc

un peu relou

Pas relou du tout :)

Avec les smiley et les accents la limite réelle c'est 70 caractères, n'importe quel message dépasse...

Plot twist ici, les accents passent avec la limite à 160, c'est visible de manière dégueulasse dans mon screen (le premier message a 140 caractères et un accent et ne déclenche pas de warning, le second pareil avec des caractères en plus pour faire 160 le déclenche). C'est confirmé par la doc ici https://docs.ovh.com/gb/en/sms/send_sms_messages_via_url_-_http2sms/#appendix_2.

Bah non parce que pour moi c'est pas du tout une erreur ou un warning, c'est normal.

Pour toi d'accord, mais pas pour un CPF : on a fait à maints reprises des facturations de SMS sur la base des SMSLog enregistrés, ce n'est venu à l'idée de personne qu'un SMS pouvait coûter deux crédits.
Ça se ressent aussi dans la doc du paramétrage du connecteur (« vous êtes safe avec la limite de 160).
Donc aux yeux d'un admin fonctionnel je suis assez persuadé que c'est presque un bug (dont la correction est tout à fait envisageable et souhaitable, il s'agira d'enlever un smiley ou de reformuler un poil).

Autrement dit, ce message va se retrouver sur tous les connecteurs SMS :)

Tout à fait le but, du coup

Aussi, cette page Passerelle, personne n'y va jamais.

Plus maintenant, elle affiche le nombre de crédits restants sur le compte, info amenée à être consultée de temps en temps j'imagine.

#8

Updated by Benjamin Dauvergne over 3 years ago

Valentin Deniaud a écrit :

Plus maintenant, elle affiche le nombre de crédits restants sur le compte, info amenée à être consultée de temps en temps j'imagine.

Je ne vois pas le code pour afficher une alerte "ce connecteur a envoyé %(n)d messages qui ont coûté plus de 1 crédit récemment." néanmoins ça me semble largement suffisant, on a le coût unitaire de chaque envoie quelque part, le coût global en page d'accueil du connecteur. Les apprentis comptables chez nos clients demanderont mieux s'ils en ont besoin (je ne vois aucun ticket client lié donc je ne saurai dire ce qui est souhaité par nos clients en fin de compte). Personnellement je me dis que si effectivement un SMS coûte 2, 3 voir 4 crédits pour des raisons idiotes (genre un smiley ou une faucille et un marteau en UTF-8), ça aiderait de l'indiquer au client en implémentant l'algo pour le calculer, mais ça attendra que quelqu'un se pose vraiment la question.

#9

Updated by Benjamin Dauvergne over 3 years ago

  • Status changed from Solution proposée to Solution validée
#10

Updated by Valentin Deniaud over 3 years ago

  • Status changed from Solution validée to En cours

Benjamin Dauvergne a écrit :

Je ne vois pas le code pour afficher une alerte "ce connecteur a envoyé %(n)d messages qui ont coûté plus de 1 crédit récemment."

Non c'est resté au stade d'idée.

néanmoins ça me semble largement suffisant, on a le coût unitaire de chaque envoie quelque part

Le quelque part n'allait pas à Thomas, je comptais attendre son retour et en discuter avec lui. C'est vrai qu'une ligne de log en rouge à chaque envoi, c'est pas ouf. Et d'un autre côté, je ne vois pas comment faire pour « que la liste des jobs (resource-jobs-table.html) affiche les clés-valeur », il y a 0 place dans ce tableau. Donc actuellement ce ticket est bloqué par manque d'idée sur l'endroit où afficher l'info.

#11

Updated by Valentin Deniaud over 3 years ago

Valentin Deniaud a écrit :

Donc actuellement ce ticket est bloqué par manque d'idée sur l'endroit où afficher l'info.

Et donc débloquons ça, patch avec le nombre de crédits consommés par un SMS consultable dans le manager, en allant cliquer sur un job.
C'est à dire que l'info est plutôt cachée, mais si il faut la rendre plus visible ce sera dans un autre ticket.

#12

Updated by Thomas Noël about 3 years ago

Après rererelecture, tout le mystère est là :

credits_spent = self.send_msg(**kwargs) or 1

et je ne trouve ça pas clair... send_msg doit retourner le crédit consommé, mais peut retourner None qui veut dire "je ne sait pas combien de crédit ont été consommés". Je préférerais voir dans les méthodes send_msg un « return None # credit consumed is unknown » final bien explicite.

Et dans ce cas de retour None, on ne sait pas combien, alors stockons-le. C'est à dire que je préférerais que credits doit effectivement None, et donc :

credits = models.PositiveSmallIntegerField(null=True)

Je suis pénible hein ?

#13

Updated by Valentin Deniaud about 3 years ago

J’adore pas le mot de pénibilité parce que ça donne le sentiment que le travail serait pénible.

#14

Updated by Thomas Noël about 3 years ago

  • Status changed from Solution proposée to Solution validée
#15

Updated by Valentin Deniaud about 3 years ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 7f57c5046967d68c0c7a94f6343305ca8ddae74c
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Wed Aug 11 17:17:37 2021 +0200

    sms: record credits used (#56116)
#16

Updated by Frédéric Péters about 3 years ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF