(Re-)added error handling for twisted smtp sender.

- Issue #249
- Fixed all tests with that rely on sendmail deferred.

8 files changed, 181 insertions, 30 deletions

diff --git a/service/pixelated/adapter/services/mail_sender.py b/service/pixelated/adapter/services/mail_sender.py
index d29b7d49..24ae839d 100644
--- a/service/pixelated/adapter/services/mail_sender.py
+++ b/service/pixelated/adapter/services/mail_sender.py
@@ -22,7 +22,7 @@ from twisted.internet import reactor
 from pixelated.support.functional import flatten
 
 
-class MailSender():
+class MailSender(object):
     def __init__(self, account_email_address, smtp_client=None):
         self.account_email_address = account_email_address
 
@@ -41,11 +41,11 @@ class MailSender():
 
     def sendmail(self, mail):
         recipients = flatten([mail.to, mail.cc, mail.bcc])
-        normalized_recepients = self.get_email_addresses(recipients)
+        normalized_recipients = self.get_email_addresses(recipients)
         resultDeferred = Deferred()
         senderFactory = SMTPSenderFactory(
             fromEmail=self.account_email_address,
-            toEmail=normalized_recepients,
+            toEmail=normalized_recipients,
             file=StringIO(mail.to_smtp_format()),
             deferred=resultDeferred)
 
diff --git a/service/pixelated/adapter/services/mail_service.py b/service/pixelated/adapter/services/mail_service.py
index 6e309ee0..1e0a0414 100644
--- a/service/pixelated/adapter/services/mail_service.py
+++ b/service/pixelated/adapter/services/mail_service.py
@@ -55,10 +55,12 @@ class MailService:
         mail = InputMail.from_dict(content_dict)
         draft_id = content_dict.get('ident')
 
-        self.mail_sender.sendmail(mail)
-        sent_mail = self.move_to_sent(draft_id, mail)
+        def move_to_sent(_):
+            return self.move_to_sent(draft_id, mail)
 
-        return sent_mail
+        deferred = self.mail_sender.sendmail(mail)
+        deferred.addCallback(move_to_sent)
+        return deferred
 
     def move_to_sent(self, last_draft_ident, mail):
         if last_draft_ident:
diff --git a/service/pixelated/resources/mails_resource.py b/service/pixelated/resources/mails_resource.py
index 77a47cda..f387076b 100644
--- a/service/pixelated/resources/mails_resource.py
+++ b/service/pixelated/resources/mails_resource.py
@@ -1,7 +1,8 @@
 import json
 from pixelated.adapter.model.mail import InputMail
-from pixelated.resources import respond_json
+from pixelated.resources import respond_json, respond_json_deferred
 from twisted.web.resource import Resource
+from twisted.web import server
 from leap.common.events import (
     register,
     events_pb2 as proto
@@ -86,9 +87,20 @@ class MailsResource(Resource):
 
     def render_POST(self, request):
         content_dict = json.loads(request.content.read())
-        sent_mail = self._mail_service.send_mail(content_dict)
 
-        return respond_json(sent_mail.as_dict(), request)
+        deferred = self._mail_service.send_mail(content_dict)
+
+        def onSuccess(sent_mail):
+            data = sent_mail.as_dict()
+            respond_json_deferred(data, request)
+
+        def onError(error):
+            respond_json_deferred({'message': str(error)}, request, status_code=422)
+
+        deferred.addCallback(onSuccess)
+        deferred.addErrback(onError)
+
+        return server.NOT_DONE_YET
 
     def render_PUT(self, request):
         content_dict = json.loads(request.content.read())
diff --git a/service/test/integration/test_drafts.py b/service/test/integration/test_drafts.py
index c555cb89..a5901b67 100644
--- a/service/test/integration/test_drafts.py
+++ b/service/test/integration/test_drafts.py
@@ -15,39 +15,68 @@
 # along with Pixelated. If not, see <http://www.gnu.org/licenses/>.
 
 from test.support.integration import *
+from mockito import *
+from twisted.internet.defer import Deferred
 
 
 class DraftsTest(SoledadTestBase):
 
+    def tearDown(self):
+        unstub()
+
     def test_post_sends_mail_and_deletes_previous_draft_if_it_exists(self):
+        # act is if sending the mail by SMTP succeeded
+        sendmail_deferred = Deferred()
+        when(self.client.mail_sender).sendmail(any()).thenReturn(sendmail_deferred)
+
         # creates one draft
         first_draft = MailBuilder().with_subject('First draft').build_json()
         first_draft_ident = self.put_mail(first_draft)[0]['ident']
 
         # sends an updated version of the draft
         second_draft = MailBuilder().with_subject('Second draft').with_ident(first_draft_ident).build_json()
-        self.post_mail(second_draft)
+        deferred_res = self.post_mail(second_draft)
 
-        sent_mails = self.get_mails_by_tag('sent')
-        drafts = self.get_mails_by_tag('drafts')
+        sendmail_deferred.callback(None)  # SMTP succeeded
+
+        def onSuccess(mail):
+            sent_mails = self.get_mails_by_tag('sent')
+            drafts = self.get_mails_by_tag('drafts')
+
+            # make sure there is one email in the sent mailbox and it is the second draft
+            self.assertEquals(1, len(sent_mails))
+            self.assertEquals('Second draft', sent_mails[0].subject)
 
-        # make sure there is one email in the sent mailbox and it is the second draft
-        self.assertEquals(1, len(sent_mails))
-        self.assertEquals('Second draft', sent_mails[0].subject)
+            # make sure that there are no drafts in the draft mailbox
+            self.assertEquals(0, len(drafts))
 
-        # make sure that there are no drafts in the draft mailbox
-        self.assertEquals(0, len(drafts))
+        deferred_res.addCallback(onSuccess)
+        return deferred_res
 
     def test_post_sends_mail_even_when_draft_does_not_exist(self):
+        # act is if sending the mail by SMTP succeeded
+        sendmail_deferred = Deferred()
+        when(self.client.mail_sender).sendmail(any()).thenReturn(sendmail_deferred)
+
         first_draft = MailBuilder().with_subject('First draft').build_json()
-        self.post_mail(first_draft)
+        deferred_res = self.post_mail(first_draft)
+        sendmail_deferred.callback(True)
 
-        sent_mails = self.get_mails_by_tag('sent')
-        drafts = self.get_mails_by_tag('drafts')
+        def onSuccess(result):
+            sent_mails = self.get_mails_by_tag('sent')
+            drafts = self.get_mails_by_tag('drafts')
+
+            self.assertEquals(1, len(sent_mails))
+            self.assertEquals('First draft', sent_mails[0].subject)
+            self.assertEquals(0, len(drafts))
+
+        deferred_res.addCallback(onSuccess)
+        return deferred_res
 
-        self.assertEquals(1, len(sent_mails))
-        self.assertEquals('First draft', sent_mails[0].subject)
-        self.assertEquals(0, len(drafts))
+    def post_mail(self, data):
+        deferred_res, req = self.client.post('/mails', data)
+        deferred_res.callback(None)
+        return deferred_res
 
     def test_put_creates_a_draft_if_it_does_not_exist(self):
         mail = MailBuilder().with_subject('A new draft').build_json()
diff --git a/service/test/support/integration/soledad_test_base.py b/service/test/support/integration/soledad_test_base.py
index 60b88768..2c8bb023 100644
--- a/service/test/support/integration/soledad_test_base.py
+++ b/service/test/support/integration/soledad_test_base.py
@@ -42,10 +42,6 @@ class SoledadTestBase(unittest.TestCase):
         })
         return [ResponseMail(m) for m in res['mails']]
 
-    def post_mail(self, data):
-        res, req = self.client.post('/mails', data)
-        return ResponseMail(res)
-
     def get_attachment(self, ident, encoding):
         res, req = self.client.get("/attachment/%s" % ident, {'encoding': [encoding]}, as_json=False)
         return res
diff --git a/service/test/unit/adapter/services/__init__.py b/service/test/unit/adapter/services/__init__.py
new file mode 100644
index 00000000..e69de29b
--- /dev/null
+++ b/service/test/unit/adapter/services/__init__.py
diff --git a/service/test/unit/adapter/services/test_mail_sender.py b/service/test/unit/adapter/services/test_mail_sender.py
new file mode 100644
index 00000000..8536e5e3
--- /dev/null
+++ b/service/test/unit/adapter/services/test_mail_sender.py
@@ -0,0 +1,63 @@
+#
+# Copyright (c) 2014 ThoughtWorks, Inc.
+#
+# Pixelated is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Pixelated is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with Pixelated. If not, see <http://www.gnu.org/licenses/>.
+from twisted.trial import unittest
+
+from mockito import mock, when, verify, any, unstub
+from pixelated.adapter.services.mail_sender import MailSender
+from pixelated.adapter.model.mail import PixelatedMail, InputMail
+from test.support.test_helper import mail_dict
+from twisted.internet import reactor
+from twisted.internet.defer import Deferred
+
+
+class MailSenderTest(unittest.TestCase):
+    def test_sendmail(self):
+        when(reactor).connectTCP('localhost', 4650, any()).thenReturn(None)
+        input_mail = InputMail.from_dict(mail_dict())
+        mail_sender = MailSender('someone@somedomain.tld')
+
+        return self._succeed(mail_sender.sendmail(input_mail))
+
+    def tearDown(self):
+        unstub()
+
+    def test_sendmail_uses_twisted(self):
+        when(reactor).connectTCP('localhost', 4650, any()).thenReturn(None)
+
+        input_mail = InputMail.from_dict(mail_dict())
+        mail_sender = MailSender('someone@somedomain.tld')
+
+        sent_deferred = mail_sender.sendmail(input_mail)
+
+        verify(reactor).connectTCP('localhost', 4650, any())
+
+        return self._succeed(sent_deferred)
+
+    def test_senmail_returns_deffered(self):
+        when(reactor).connectTCP('localhost', 4650, any()).thenReturn(None)
+        input_mail = InputMail.from_dict(mail_dict())
+        mail_sender = MailSender('someone@somedomain.tld')
+
+        deferred = mail_sender.sendmail(input_mail)
+
+        self.assertIsNotNone(deferred)
+        self.assertTrue(isinstance(deferred, Deferred))
+
+        return self._succeed(deferred)
+
+    def _succeed(self, deferred):
+        deferred.callback(None)
+        return deferred
diff --git a/service/test/unit/adapter/test_mail_service.py b/service/test/unit/adapter/test_mail_service.py
index 137c17ee..98ead126 100644
--- a/service/test/unit/adapter/test_mail_service.py
+++ b/service/test/unit/adapter/test_mail_service.py
@@ -13,20 +13,24 @@
 #
 # You should have received a copy of the GNU Affero General Public License
 # along with Pixelated. If not, see <http://www.gnu.org/licenses/>.
-import unittest
+from twisted.trial import unittest
 from pixelated.adapter.model.mail import InputMail, PixelatedMail
 
 from pixelated.adapter.services.mail_service import MailService
 from test.support.test_helper import mail_dict, leap_mail
 from mockito import *
+from twisted.internet.defer import Deferred
+
+from twisted.internet import defer
 
 
 class TestMailService(unittest.TestCase):
     def setUp(self):
+        self.drafts = mock()
         self.querier = mock()
         self.mailboxes = mock()
         self.tag_service = mock()
-        self.mailboxes.drafts = lambda: mock()
+        self.mailboxes.drafts = lambda: self.drafts
         self.mailboxes.trash = lambda: mock()
         self.mailboxes.sent = lambda: mock()
 
@@ -34,13 +38,58 @@ class TestMailService(unittest.TestCase):
         self.search_engine = mock()
         self.mail_service = MailService(self.mailboxes, self.mail_sender, self.tag_service, self.querier, self.search_engine)
 
+    def tearDown(self):
+        unstub()
+
     def test_send_mail(self):
         when(InputMail).from_dict(any()).thenReturn('inputmail')
+        when(self.mail_sender).sendmail(any()).thenReturn(Deferred())
 
-        self.mail_service.send_mail(mail_dict())
+        sent_deferred = self.mail_service.send_mail(mail_dict())
 
         verify(self.mail_sender).sendmail("inputmail")
 
+        sent_deferred.callback('Assume sending mail succeeded')
+
+        return sent_deferred
+
+    def test_send_mail_removes_draft(self):
+        mail_ident = 'Some ident'
+        mail = mail_dict()
+        mail['ident'] = mail_ident
+        when(InputMail).from_dict(any()).thenReturn('inputmail')
+        deferred = Deferred()
+        when(self.mail_sender).sendmail(any()).thenReturn(deferred)
+
+        sent_deferred = self.mail_service.send_mail(mail)
+
+        verify(self.mail_sender).sendmail("inputmail")
+
+        def assert_removed_from_drafts(_):
+            verify(self.drafts).remove(any())
+
+        sent_deferred.addCallback(assert_removed_from_drafts)
+        sent_deferred.callback('Assume sending mail succeeded')
+
+        return sent_deferred
+
+    def test_send_mail_does_not_delete_draft_on_error(self):
+        when(InputMail).from_dict(any()).thenReturn('inputmail')
+        when(self.mail_sender).sendmail(any()).thenReturn(Deferred())
+
+        send_deferred = self.mail_service.send_mail(mail_dict())
+
+        verify(self.mail_sender).sendmail("inputmail")
+
+        def assert_not_removed_from_drafts(_):
+            verifyNoMoreInteractions(self.drafts)
+
+        send_deferred.addErrback(assert_not_removed_from_drafts)
+
+        send_deferred.errback(Exception('Assume sending mail failed'))
+
+        return send_deferred
+
     def test_mark_as_read(self):
         mail = mock()
         when(self.mail_service).mail(any()).thenReturn(mail)