Send activation emails from a worker process

Send new user account activation emails from a worker process, instead of from the main application thread. This avoids a problem when the register view is called and sending the email (mailer.send(message)) raises an exception *but the email was actually sent*. The transaction would be rolled back and the user account not created, and the user would receive an unusable "please activate your account" email. Changes: - The register view callable now puts a message on a new "activations" nsq topic, instead of actually sending the email. - The new h/accounts/worker.py is a hypothesis-worker that subscribes to the new "activations" topic and actually sends an email for each message published to the toipic. - Update accounts/test/views_test.py to fix broken tests. These were mainly broken because _register() now accesses request.get_queue_writer() and Pyramid's DummyRequest() objects don't have that method. Just use a mock.Mock() instead. In a couple of places assertions had to be updated as well. In sending the email fails (if pyramid_mailer's send_immediately() raises) then the message will be requeued (this is gnsq's default behavior when a worker function raises an exception) and the worker will try again to send it. Note that we don't attempt to prevent sending the same activation email multiple times if a message published to the "activations" topic is received by the worker multiple times. This may happen, since nsq guarantees "at least once" delivery. If this becomes a problem we may need to set a flag in the database to record when a particular activation email has already been sent successfully.
hypothesis · Feb 2, 2016 · 49b2412 · 49b2412
1 parent 2d0e3ca
commit 49b2412
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 27 deletions.
diff --git a/Procfile b/Procfile
@@ -1,5 +1,6 @@
 web: gunicorn -w ${WEB_CONCURRENCY:-1} --paster conf/${HYP_ENV:-production}.ini
 notification: hypothesis-worker conf/${HYP_ENV:-production}.ini notification
 nipsa: hypothesis-worker conf/${HYP_ENV:-production}.ini nipsa
+activation: hypothesis-worker conf/${HYP_ENV:-production}.ini activation
 assets: hypothesis assets conf/${HYP_ENV:-production}.ini
 initdb: hypothesis initdb conf/${HYP_ENV:-production}.ini
diff --git a/h/accounts/test/views_test.py b/h/accounts/test/views_test.py
@@ -349,7 +349,7 @@ def test_forgot_password_creates_no_activations_when_validation_fails(activation
 @patch('h.accounts.views.reset_password_link')
 @forgot_password_fixtures
 def test_forgot_password_generates_reset_link(reset_link, authn_policy):
-    request = DummyRequest(method='POST')
+    request = mock.Mock(method="POST", authenticated_userid=None)
     request.registry.password_reset_serializer = FakeSerializer()
     authn_policy.authenticated_userid.return_value = None
     user = FakeUser(username='giraffe', email='giraffe@thezoo.org')
@@ -413,7 +413,7 @@ def test_forgot_password_redirects_on_success(authn_policy):
 
 @pytest.mark.usefixtures('routes_mapper')
 def test_forgot_password_form_redirects_when_logged_in(authn_policy):
-    request = DummyRequest()
+    request = mock.Mock(method="POST", authenticated_userid="jane")
     authn_policy.authenticated_userid.return_value = "acct:jane@doe.org"
 
     with pytest.raises(httpexceptions.HTTPFound):
@@ -476,7 +476,6 @@ def test_reset_password_redirects_on_success():
 
 
 register_fixtures = pytest.mark.usefixtures('activation_model',
-                                            'mailer',
                                             'notify',
                                             'routes_mapper',
                                             'user_model')
@@ -494,7 +493,7 @@ def test_register_returns_errors_when_validation_fails():
 
 @register_fixtures
 def test_register_creates_user_from_form_data(user_model):
-    request = DummyRequest(method='POST')
+    request = mock.Mock(method="POST", authenticated_userid=None)
     controller = RegisterController(request)
     controller.form = form_validating_to({
         "username": "bob",
@@ -512,7 +511,7 @@ def test_register_creates_user_from_form_data(user_model):
 
 @register_fixtures
 def test_register_adds_new_user_to_session(user_model):
-    request = DummyRequest(method='POST')
+    request = mock.Mock(method="POST", authenticated_userid=None)
     controller = RegisterController(request)
     controller.form = form_validating_to({
         "username": "bob",
@@ -522,13 +521,13 @@ def test_register_adds_new_user_to_session(user_model):
 
     controller.register()
 
-    assert user_model.return_value in request.db.added
+    request.db.add.assert_any_call(user_model.return_value)
 
 
 @register_fixtures
 def test_register_creates_new_activation(activation_model,
                                          user_model):
-    request = DummyRequest(method='POST')
+    request = mock.Mock(method="POST", authenticated_userid=None)
     controller = RegisterController(request)
     controller.form = form_validating_to({
         "username": "bob",
@@ -546,7 +545,7 @@ def test_register_creates_new_activation(activation_model,
 @register_fixtures
 def test_register_generates_activation_email_from_user(activation_email,
                                                        user_model):
-    request = DummyRequest(method='POST')
+    request = mock.Mock(method="POST", authenticated_userid=None)
     controller = RegisterController(request)
     controller.form = form_validating_to({
         "username": "bob",
@@ -562,9 +561,8 @@ def test_register_generates_activation_email_from_user(activation_email,
 
 @patch('h.accounts.views.activation_email')
 @register_fixtures
-def test_register_sends_activation_email(activation_email,
-                                         mailer):
-    request = DummyRequest(method='POST')
+def test_register_publishes_activation_message_to_nsq(activation_email):
+    request = mock.Mock(method="POST", authenticated_userid=None)
     controller = RegisterController(request)
     controller.form = form_validating_to({
         "username": "bob",
@@ -574,7 +572,8 @@ def test_register_sends_activation_email(activation_email,
 
     controller.register()
 
-    assert activation_email.return_value in mailer.outbox
+    request.get_queue_writer.return_value.publish.assert_called_once_with(
+        'activations', activation_email.return_value)
 
 
 @patch('h.accounts.views.RegistrationEvent')
@@ -592,8 +591,8 @@ def test_register_no_event_when_validation_fails(event, notify):
 
 @patch('h.accounts.views.RegistrationEvent')
 @register_fixtures
-def test_register_event_when_validation_succeeds(event, notify, user_model):
-    request = DummyRequest(method='POST')
+def test_register_event_when_validation_succeeds(event, user_model):
+    request = mock.Mock(method="POST", authenticated_userid=None)
     controller = RegisterController(request)
     controller.form = form_validating_to({
         "username": "bob",
@@ -605,12 +604,12 @@ def test_register_event_when_validation_succeeds(event, notify, user_model):
     controller.register()
 
     event.assert_called_with(request, new_user)
-    notify.assert_called_with(event.return_value)
+    request.registry.notify.assert_called_once_with(event.return_value)
 
 
 @register_fixtures
 def test_register_event_redirects_on_success():
-    request = DummyRequest(method='POST')
+    request = mock.Mock(method="POST", authenticated_userid=None)
     controller = RegisterController(request)
     controller.form = form_validating_to({
         "username": "bob",

diff --git a/h/accounts/views.py b/h/accounts/views.py
@@ -421,8 +421,7 @@ def _register(self, username, email, password):
 
         # Send the activation email
         message = activation_email(self.request, user)
-        mailer = get_mailer(self.request)
-        mailer.send(message)
+        self.request.get_queue_writer().publish('activations', message)
 
         self.request.session.flash(jinja2.Markup(_(
             'Thank you for creating an account! '
@@ -542,20 +541,20 @@ def _user_notifications(self):
 
 
 def activation_email(request, user):
-    """
-    Generate an 'activate your account' email for the specified user.
+    """Return the data for an 'activate your account' email for the given user.
+
+    :rtype: dict
 
-    :rtype: pyramid_mailer.message.Message
     """
     link = request.route_url('activate', id=user.id, code=user.activation.code)
-
     emailtext = ("Please validate your email and activate your account by "
                  "visiting: {link}")
     body = emailtext.format(link=link)
-    msg = Message(subject=_("Please activate your account"),
-                  recipients=[user.email],
-                  body=body)
-    return msg
+    return {
+        "subject": _("Please activate your account"),
+        "recipients": [user.email],
+        "body": body
+    }
 
 
 def reset_password_email(user, reset_code, reset_link):

diff --git a/h/accounts/worker.py b/h/accounts/worker.py
@@ -0,0 +1,18 @@
+# -*- coding: utf-8 -*-
+import json
+
+import pyramid_mailer
+
+
+def worker(request):
+    """Subscribe to the "activations" topic and send activation emails."""
+    def handle_message(reader, message):
+        body = json.loads(message.body)
+        email = pyramid_mailer.message.Message(
+            subject=body['subject'], recipients=body['recipients'],
+            body=body['body'])
+        pyramid_mailer.get_mailer(request).send_immediately(email)
+
+    reader = request.get_queue_reader('activations', 'mailer')
+    reader.on_message.connect(handle_message)
+    reader.start(block=True)
diff --git a/setup.py b/setup.py
@@ -125,8 +125,9 @@ def run_tests(self):
             'hypothesis-worker=h.worker:main',
         ],
         'h.worker': [
-            'notification=h.notification.worker:run',
+            'activation=h.accounts.worker:worker',
             'nipsa=h.api.nipsa.worker:worker',
+            'notification=h.notification.worker:run',
         ],
         'h.annotool': [
             'prepare=h.api.transform:prepare',