Project

General

Profile

Development #20174

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

Added by Serghei Mihai over 6 years ago. Updated over 5 years ago.

Status:
Fermé
Priority:
Normal
Assignee:
Target version:
-
Start date:
20 November 2017
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:

Description

Comme pour le mail


Files

Associated revisions

Revision 8e8b10ae (diff)
Added by Serghei Mihai over 6 years ago

manager: add test sms send (#20174)

History

#2

Updated by Benjamin Dauvergne over 6 years ago

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

Updated by Serghei Mihai over 6 years ago

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

Updated by Thomas Noël over 6 years ago

  • 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

Updated by Serghei Mihai over 6 years ago

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

Updated by Thomas Noël over 6 years ago

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

Updated by Serghei Mihai over 6 years ago

  • Status changed from Nouveau to 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

Updated by Frédéric Péters over 5 years ago

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

Also available in: Atom PDF