Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vérifie si l'utilisateur a une adresse mail avant de lui envoyer un mail. #5019

Merged
merged 5 commits into from Sep 9, 2018

Conversation

@nils-van-zuijlen
Copy link
Contributor

commented Aug 22, 2018

Lors de l'inscription via OAuth (réseaux sociaux), un utilisateur peut être crée sans adresse mail. Ceci cause des erreurs 500 lorsque l'on essaye de lui envoyer une notification par ce moyen.

Cette PR introduit une condition préalable à l'envoi de mails pour éviter ces erreurs.

Numéro du ticket concerné : #4943

Contrôle qualité

  • Créez un compte avec une adresse mail vide (allez dans la BDD si besoin)
  • Sur ce compte, demandez à recevoir toutes les notifications de MP
  • Envoyez un MP à cet utilisateur à l'aide d'un autre compte normal.
  • Il ne doit pas y avoir d'erreurs lors de l'envoi du MP.
@nils-van-zuijlen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

Le test que j'ai écrit fail, mais je ne sais pas pourquoi...
J'ai l'output :

(zdsenv) [nils@pineapple:zestedesavoir/zds-site][1]$ ./manage.py test zds.notification.tests.tests_tricky.SubscriptionsTest
Creating test database for alias 'default'...
 DEBUG  zds.utils.templatetags.emarkdown Result , {'disableToc': True, 'languages': [], 'depth': -1}, []
DEBUG:zds.utils.templatetags.emarkdown:Result , {'disableToc': True, 'languages': [], 'depth': -1}, []
 DEBUG  zds.utils.templatetags.emarkdown Result , {'disableToc': True, 'languages': [], 'depth': -1}, []
DEBUG:zds.utils.templatetags.emarkdown:Result , {'disableToc': True, 'languages': [], 'depth': -1}, []
F
======================================================================
FAIL: test_no_emails_for_those_who_have_none (zds.notification.tests.tests_tricky.SubscriptionsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/nils/Developpement/zestedesavoir/zds-site/zds/notification/tests/tests_tricky.py", line 402, in test_no_emails_for_those_who_have_none
    self.assertEqual(1, len(mail.outbox))
AssertionError: 1 != 0

----------------------------------------------------------------------
Ran 1 test in 0.818s

FAILED (failures=1)
Destroying test database for alias 'default'...
(zdsenv) [nils@pineapple:zestedesavoir/zds-site][1]$

@artragis est-ce que tu sais ce qui peut faire que le mail n'est pas envoyé à l'utilisateur normal ?

@coveralls

This comment has been minimized.

Copy link

commented Aug 22, 2018

Coverage Status

Coverage increased (+0.008%) to 74.103% when pulling 1b1f519 on nils-van-zuijlen:mail-mp-4943 into 173d0e0 on zestedesavoir:dev.

@firm1

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

Tu n'aurai pas tout simplement oublié de t'authentifier ?

self.assertTrue(self.client.login(username=self.userOAuth1.username, password='hostel77'))
@nils-van-zuijlen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

@firm1 Je viens d'essayer, merci pour l'idée, mais ce n'est pas ça.

zds/notification/tests/tests_tricky.py Outdated Show resolved Hide resolved
zds/notification/tests/tests_tricky.py Outdated Show resolved Hide resolved
zds/notification/tests/tests_tricky.py Show resolved Hide resolved
@@ -87,6 +87,11 @@ def send_email(self, notification):
settings.ZDS_APP['site']['email_noreply'])

receiver = self.user

# This can happen when an user subscribes via social networks without providing an e-mail adress

This comment has been minimized.

Copy link
@gustavi

gustavi Aug 23, 2018

Member

s/adress/address

@@ -87,6 +87,11 @@ def send_email(self, notification):
settings.ZDS_APP['site']['email_noreply'])

receiver = self.user

# This can happen when an user subscribes via social networks without providing an e-mail address

This comment has been minimized.

Copy link
@gcodeur

gcodeur Aug 23, 2018

Member

An user.

@nils-van-zuijlen nils-van-zuijlen force-pushed the nils-van-zuijlen:mail-mp-4943 branch from 87bca85 to 46081e2 Aug 23, 2018

@artragis

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

Peux-tu rebaser ta MR s'il te plait. Je regarderai le tout ce week end. Ce bug est trop important pour êtr elaissé de côté.

@artragis artragis self-assigned this Sep 5, 2018

self.userStandard1 = ProfileFactory().user
self.userOAuth1 = ProfileFactory().user

self.userStandard1.profile.email_for_answer = True

This comment has been minimized.

Copy link
@artragis

artragis Sep 8, 2018

Member

la cause du fail du test est ici : tu save l'object user, mais pas l'object profile. J'ai un petit diff qui corrige tout ça :

--- a/zds/notification/tests/tests_tricky.py
+++ b/zds/notification/tests/tests_tricky.py
@@ -371,13 +371,13 @@ class ContentNotification(TestCase, TutorialTestMixin):
 @override_settings(EMAIL_BACKEND='django.core.mail.backends.locmem.EmailBackend')
 class SubscriptionsTest(TestCase):
     def setUp(self):
-        self.userStandard1 = ProfileFactory().user
-        self.userOAuth1 = ProfileFactory().user
-
-        self.userStandard1.profile.email_for_answer = True
-        self.userOAuth1.profile.email_for_answer = True
-        self.userStandard1.profile.email_for_new_mp = True
-        self.userOAuth1.profile.email_for_new_mp = True
+        self.userStandard1 = ProfileFactory(
+            email_for_answer=True,
+            email_for_new_mp=True
+        ).user
+        self.userOAuth1 = ProfileFactory(
+            email_for_answer=True,
+            email_for_new_mp=True).user
 
         self.userOAuth1.email = ''
 
@@ -389,7 +389,6 @@ class SubscriptionsTest(TestCase):
         Test that we do not try to send e-mails to those who have not registered one.
         """
         self.assertEqual(0, len(mail.outbox))
-
         topic = send_mp(author=self.userStandard1, users=[self.userOAuth1],
                         title='Testing', subtitle='', text='',
                         send_by_mail=True, leave=False)
Fixed tests
Thank you @artragis
@@ -87,6 +87,11 @@ def send_email(self, notification):
settings.ZDS_APP['site']['email_noreply'])

receiver = self.user

# This can happen when a user subscribes via social networks without providing an e-mail address
if not receiver.email:

This comment has been minimized.

Copy link
@artragis

artragis Sep 8, 2018

Member

ici j'ajouterais même :

from django.core.validators import email_re

"""
...
"""
if not receiver.email or not email_re.match(receiver.email):

This comment has been minimized.

Copy link
@nils-van-zuijlen

nils-van-zuijlen Sep 8, 2018

Author Contributor

Bonne idée.

This comment has been minimized.

Copy link
@nils-van-zuijlen

nils-van-zuijlen Sep 8, 2018

Author Contributor

Hum, y'a un petit pb :

  File "/home/nils/Developpement/zestedesavoir/zds-site/zds/notification/models.py", line 8, in <module>
    from django.core.validators import email_re
ImportError: cannot import name 'email_re'

This comment has been minimized.

Copy link
@artragis

artragis Sep 8, 2018

Member

en effet, les choses semblent avoir changé, le code serait aujourd'hui :

from django.core.validators import validate_email, ValidationError

try:
    validate_email(receiver.email)  # il fait lui même le check de "ça existe et c'est valide"
except ValidationError:
    return
@artragis

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

QA OK

@artragis artragis merged commit 6d7f12b into zestedesavoir:dev Sep 9, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nils-van-zuijlen nils-van-zuijlen deleted the nils-van-zuijlen:mail-mp-4943 branch Sep 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.