Projet

Général

Profil

Development #20174

ajouter l'option d'envoi d'un sms de test

Ajouté par Serghei Mihai il y a plus de 6 ans. Mis à jour il y a plus de 5 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Version cible:
-
Début:
20 novembre 2017
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:

Description

Comme pour le mail


Fichiers

Révisions associées

Révision 8e8b10ae (diff)
Ajouté par Serghei Mihai il y a environ 6 ans

manager: add test sms send (#20174)

Historique

#1

Mis à jour par Serghei Mihai il y a plus de 6 ans

#2

Mis à jour par Benjamin Dauvergne il y a plus de 6 ans

Remarque de forme:
  • quand tu as ça:
        assert resp.location == 'http://testserver/manage/'
        resp = app.get('http://testserver/manage/')
    

    tu peux remplacer la deuxième ligne par
        resp = resp.follow()
    

    c'est plus clair je trouve, ça suit plus le cheminement d'un navigo.
  • URL invalide with override_settings(SMS_GATEWAY_URL='http:/passerelle.com')
  • inutile assert send_form.method 'post'
  • sauf si l'URL à une importance (pour une API par exemple), assert resp.location 'http://testserver/manage/announce/1/' c'est mieux de faire ce test avec reverse je pense (ou alors on dit que toutes nos URLs doivent être immuables), anecdotique.
  • with override_settings(SMS_GATEWAY_URL='http:/passerelle.com'): avec pytest-django on utilise directement la fixture settings et on modifie directement la valeur du setting dessus
Sur le fond:
  • je ne vois pas de test que l'action du formulaire à procéder à un appel du backend de SMS (pas de mock rien, bizarre que ça ne pète pas une erreur)
  • même remarque dans form_valid on dirait que send_sms() ne peut jamais foirer, send_sms() devrait retourner un résultat genre return True, None ou return False, error ou alors émettre une exception (je suis de moins en moins fan des exceptions)
  • Send test {{ action }} c'est quoi action ? J'ai la vague impression que c'est mal nommé ou inutile (ce serait "SMS"?) Tout le jeu d'héritage avec corbo/send_test_announce_form.html" me semble inutile mais je rate peut-être un truc, si c'est juste pour pas mettre {{ form.as_p }} ne nous prions pas de nous répéter ici.
#3

Mis à jour par Serghei Mihai il y a plus de 6 ans

Benjamin Dauvergne a écrit :

  • sauf si l'URL à une importance (pour une API par exemple), assert resp.location == 'http://testserver/manage/announce/1/' c'est mieux de faire ce test avec reverse je pense (ou alors on dit que toutes nos URLs doivent être immuables), anecdotique.

je préfère ici que les tests "detectent" quand les urls changent.

  • with override_settings(SMS_GATEWAY_URL='http:/passerelle.com'): avec pytest-django on utilise directement la fixture settings et on modifie directement la valeur du setting dessus

ok.

  • même remarque dans form_valid on dirait que send_sms() ne peut jamais foirer, send_sms() devrait retourner un résultat genre return True, None ou return False, error ou alors émettre une exception (je suis de moins en moins fan des exceptions)

send_sms comme pour l'envoi des mails retourne le nombre de destinations auxquelles le message a été délivré. S'il y a exception elle est logguée et la fonction retourne 0.
La vue d'envoi du SMS de test devrait au moins vérifier qu'il y a eu envoi.

#4

Mis à jour par Thomas Noël il y a environ 6 ans

  • Dans les tests, je ne serais pas contre quelques lignes vides (respiration après une série d'action+assert) et quelques commentaires, pour mieux comprendre ce qui se passe.
  • settings.SMS_GATEWAY_URL='http:/passerelle.com' manque d'espace pèpuite. *

Serghei Mihai a écrit :

Benjamin Dauvergne a écrit :

  • sauf si l'URL à une importance (pour une API par exemple), assert resp.location == 'http://testserver/manage/announce/1/' c'est mieux de faire ce test avec reverse je pense (ou alors on dit que toutes nos URLs doivent être immuables), anecdotique.

je préfère ici que les tests "detectent" quand les urls changent.

Comme dit Benj, seules les URL d'API ne doivent pas bouger ; mais les autres on s'en fiche, donc ici ça peut être du reverse (mais c'est anecdotique, comme dit Benjamin)

  • même remarque dans form_valid on dirait que send_sms() ne peut jamais foirer, send_sms() devrait retourner un résultat genre return True, None ou return False, error ou alors émettre une exception (je suis de moins en moins fan des exceptions)

send_sms comme pour l'envoi des mails retourne le nombre de destinations auxquelles le message a été délivré. S'il y a exception elle est logguée et la fonction retourne 0.
La vue d'envoi du SMS de test devrait au moins vérifier qu'il y a eu envoi.

Dans ce cas, ça serait plus lisible de faire

sms_sent = utils.send_sms(announce.text, [mobile])
if sms_sent == 1:
    ...
else:
    ...

dans le SMSAnnounceView::form_valid

Mais c'est quand même très dommage de ne pas avoir le message d'erreur, qui ne sera qu'au fond des logs (genre, quota SMS dépassé, message trop long, etc). Le retour des fonctions send_xxx pourraient être amélioré dans cet objectif (autre ticket à faire).

#5

Mis à jour par Serghei Mihai il y a environ 6 ans

Commentaires et un peu d'espaces rajoutés.

Au passage, je prend en compte les remarques de Benj pour l'usage de reverse pour les urls. Ainsi au passage j'avance vers la compatibilité avec Django 1.11

#6

Mis à jour par Thomas Noël il y a environ 6 ans

Une ligne vide ajoutée dans corbo/templates/corbo/email_test_announce_form.html pour rien, je pense. Tu la retires et c'est un ack :)

#7

Mis à jour par Serghei Mihai il y a environ 6 ans

  • Statut changé de Nouveau à Résolu (à déployer)

Yep, c'est juste un renommage de fichier:

commit 8e8b10aeb5c438653e50dfd85d694f665571bf10 (origin/master)
Author: Serghei Mihai <smihai@entrouvert.com>
Date:   Mon Dec 4 17:30:06 2017 +0100

    manager: add test sms send (#20174)

#8

Mis à jour par Frédéric Péters il y a plus de 5 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF