Merge branch 'mailnotif'

* mailnotif:
  fix pyflakes
  MailNotifier: Simplify recipient logic.
  Simplify extraHeaders logic in MailNotifier.
  Add test for proper handling of blamelists with multiple builds.

master/buildbot/status/mail.py

@@ -591,18 +591,17 @@ def createEmail(self, msgdict, builderName, title, results, builds=None,
         # Add any extra headers that were requested, doing WithProperties
         # interpolation if only one build was given
         if self.extraHeaders:
-            for k,v in self.extraHeaders.items():
-                if len(builds) == 1:
-                    k = interfaces.IProperties(builds[0]).render(k)
+            if len(builds) == 1:
+                extraHeaders = builds[0].render(self.extraHeaders)
+            else:
+                extraHeaders = self.extraHeaders
+            for k,v in extraHeaders.items():
                 if k in m:
                     twlog.msg("Warning: Got header " + k +
                       " in self.extraHeaders "
                       "but it already exists in the Message - "
                       "not adding it.")
-                if len(builds) == 1:
-                    m[k] = interfaces.IProperties(builds[0]).render(v)
-                else:
-                    m[k] = v
+                m[k] = v
 
         return m
 
@@ -644,71 +643,76 @@ def buildMessage(self, name, builds, results):
                              results, builds, patches, logs)
 
         # now, who is this message going to?
-        self.dl = []
-        self.recipients = []
         if self.sendToInterestedUsers:
+            dl = []
             for build in builds:
-                d = defer.succeed(build)
                 if self.lookup:
-                    d.addCallback(self.useLookup)
+                    d = self.useLookup(build)
                 else:
-                    d.addCallback(self.useUsers)
+                    d = self.useUsers(build)
+                dl.append(d)
+            d = defer.gatherResults(dl)
         else:
-            d = defer.DeferredList(self.dl)
-        d.addCallback(self._gotRecipients, self.recipients, m)
+            d = defer.succeed([])
+        d.addCallback(self._gotRecipients, m)
         return d
 
     def useLookup(self, build):
+        dl = []
         for u in build.getInterestedUsers():
             d = defer.maybeDeferred(self.lookup.getAddress, u)
-            d.addCallback(self.recipients.append)
-            self.dl.append(d)
-        return defer.DeferredList(self.dl)
+            dl.append(d)
+        return defer.gatherResults(dl)
 
     def useUsers(self, build):
-        self.contacts = []
+        dl = []
         ss = build.getSourceStamp()
         for change in ss.changes:
             d = self.parent.db.changes.getChangeUids(change.number)
             def getContacts(uids):
                 def uidContactPair(contact, uid):
                     return (contact, uid)
-                d = defer.succeed(None)
+                contacts = []
                 for uid in uids:
-                    d.addCallback(lambda _ :
-                                     users.getUserContact(self.parent,
-                                                          contact_type='email',
-                                                          uid=uid))
+                    d = users.getUserContact(self.parent,
+                            contact_type='email',
+                            uid=uid)
                     d.addCallback(lambda contact: uidContactPair(contact, uid))
-                    d.addCallback(self.contacts.append)
-                return d
+                    contacts.append(d)
+                return defer.gatherResults(contacts)
             d.addCallback(getContacts)
-            def logNoMatch(_):
-                for pair in self.contacts:
+            def logNoMatch(contacts):
+                for pair in contacts:
                     contact, uid = pair
                     if contact is None:
                         twlog.msg("Unable to find email for uid: %r" % uid)
-                return [pair[0] for pair in self.contacts]
+                return [pair[0] for pair in contacts]
             d.addCallback(logNoMatch)
-            d.addCallback(self.recipients.extend)
-            def addOwners(_):
+            def addOwners(recipients):
                 owners = [e for e in build.getInterestedUsers()
                           if e not in build.getResponsibleUsers()]
-                self.recipients.extend(owners)
+                recipients.extend(owners)
+                return recipients
             d.addCallback(addOwners)
-            self.dl.append(d)
-        return defer.DeferredList(self.dl)
+            dl.append(d)
+        d = defer.gatherResults(dl)
+        @d.addCallback
+        def gatherRecipients(res):
+            recipients = []
+            map(recipients.extend, res)
+            return recipients
+        return d
 
     def _shouldAttachLog(self, logname):
         if type(self.addLogs) is bool:
             return self.addLogs
         return logname in self.addLogs
 
-    def _gotRecipients(self, res, rlist, m):
+    def _gotRecipients(self, rlist, m):
         to_recipients = set()
         cc_recipients = set()
 
-        for r in rlist:
+        for r in reduce(list.__add__, rlist, []):
             if r is None: # getAddress didn't like this address
                 continue
 

master/buildbot/test/fake/fakebuild.py

@@ -18,7 +18,7 @@
 from buildbot.process import properties
 from buildbot import interfaces
 
-class FakeBuildStatus(mock.Mock, properties.PropertiesMixin):
+class FakeBuildStatus(properties.PropertiesMixin, mock.Mock):
 
     # work around http://code.google.com/p/mock/issues/detail?id=105
     def _get_child_mock(self, **kw):

master/buildbot/test/unit/test_status_mail.py

@@ -14,14 +14,14 @@
 # Copyright Buildbot Team Members
 
 from mock import Mock
-from twisted.python import components
 from buildbot import config
-from buildbot.interfaces import IProperties
 from twisted.trial import unittest
 from buildbot.status.results import SUCCESS, FAILURE
 from buildbot.status.mail import MailNotifier
 from twisted.internet import defer
 from buildbot.test.fake import fakedb
+from buildbot.test.fake.fakebuild import FakeBuildStatus
+from buildbot.process import properties
 
 class FakeLog(object):
     def __init__(self, text):
@@ -40,27 +40,6 @@ def getText(self):
         return self.text
 
 
-class FakeBuildStatus(Mock):
-
-    def __init__(self, *args, **kwargs):
-        Mock.__init__(self, *args, **kwargs)
-        self.builder = None
-        self.properites = {}
-
-    def getBuilder(self):
-        return self.builder
-
-class FakeBuildStatusProperties(components.Adapter):
-
-    def getProperty(self, name, default):
-        return self.original.properties.get(name, default)
-
-    def render(self, value):
-        return "rndr(%s)" % (value,)
-
-components.registerAdapter(FakeBuildStatusProperties, FakeBuildStatus,
-                IProperties)
-
 class TestMailNotifier(unittest.TestCase):
     def test_createEmail_message_without_patch_and_log_contains_unicode(self):
         builds = [ FakeBuildStatus(name="build") ]
@@ -75,13 +54,15 @@ def test_createEmail_message_without_patch_and_log_contains_unicode(self):
 
     def test_createEmail_extraHeaders_one_build(self):
         builds = [ FakeBuildStatus(name="build") ]
+        builds[0].properties = properties.Properties()
+        builds[0].setProperty('hhh','vvv')
         msgdict = create_msgdict()
         mn = MailNotifier('from@example.org', extraHeaders=dict(hhh='vvv'))
         # add some Unicode to detect encoding problems
         m = mn.createEmail(msgdict, u'builder-n\u00E5me', u'project-n\u00E5me',
                            SUCCESS, builds)
         txt = m.as_string()
-        self.assertIn('rndr(hhh): rndr(vvv)', txt)
+        self.assertIn('hhh: vvv', txt)
 
     def test_createEmail_extraHeaders_two_builds(self):
         builds = [ FakeBuildStatus(name="build1"),
@@ -184,8 +165,7 @@ def fakeGetBuildRequests(self, bsid):
         build.result = FAILURE
         build.finished = True
         build.reason = "testReason"
-        build.builder = builder
-
+        build.getBuilder.return_value = builder
 
         self.db = fakedb.FakeDBConnector(self)
         self.db.insertTestData([fakedb.Buildset(id=99, sourcestampid=127,
@@ -432,6 +412,99 @@ def test_sendToInterestedUsers_False(self):
                                    exp_called_with=['marla@mayhem.net'],
                                    exp_TO="marla@mayhem.net")
 
+    def test_sendToInterestedUsers_two_builds(self):
+        from email.Message import Message
+        m = Message()
+
+        mn = MailNotifier(fromaddr="from@example.org", lookup=None)
+        mn.sendMessage = Mock()
+
+        def fakeGetBuilder(buildername):
+            if buildername == builder.name:
+                return builder
+            return None
+        def fakeGetBuildRequests(self, bsid):
+            return defer.succeed([{"buildername":"Builder", "brid":1}])
+
+        builder = Mock()
+        builder.name = "Builder"
+
+        build1 = FakeBuildStatus(name="build")
+        build1.result = FAILURE
+        build1.finished = True
+        build1.reason = "testReason"
+        build1.builder = builder
+
+        build2 = FakeBuildStatus(name="build")
+        build2.result = FAILURE
+        build2.finished = True
+        build2.reason = "testReason"
+        build2.builder = builder
+
+        def fakeCreateEmail(msgdict, builderName, title, results, builds=None,
+                            patches=None, logs=None):
+            # only concerned with m['To'] and m['CC'], which are added in
+            # _got_recipients later
+            return m
+        mn.createEmail = fakeCreateEmail
+
+        self.db = fakedb.FakeDBConnector(self)
+        self.db.insertTestData([fakedb.Buildset(id=99, sourcestampid=127,
+                                                results=SUCCESS,
+                                                reason="testReason"),
+                                fakedb.BuildRequest(id=11, buildsetid=99,
+                                                    buildername='Builder'),
+                                fakedb.Build(number=0, brid=11),
+                                fakedb.Build(number=1, brid=11),
+                                fakedb.Change(changeid=9123),
+                                fakedb.Change(changeid=9124),
+                                fakedb.ChangeUser(changeid=9123, uid=1),
+                                fakedb.ChangeUser(changeid=9124, uid=2),
+                                fakedb.User(uid=1, identifier="tdurden"),
+                                fakedb.User(uid=2, identifier="user2"),
+                                fakedb.UserInfo(uid=1, attr_type='email',
+                                            attr_data="tyler@mayhem.net"),
+                                fakedb.UserInfo(uid=2, attr_type='email',
+                                            attr_data="user2@example.net")
+                                ])
+
+        def _getInterestedUsers():
+            # 'narrator' in this case is the owner, which tests the lookup
+            return ["Big Bob <bob@mayhem.net>", "narrator"]
+        build1.getInterestedUsers = _getInterestedUsers
+        build2.getInterestedUsers = _getInterestedUsers
+
+        def _getResponsibleUsers():
+            return ["Big Bob <bob@mayhem.net>"]
+        build1.getResponsibleUsers = _getResponsibleUsers
+        build2.getResponsibleUsers = _getResponsibleUsers
+
+        # fake sourcestamp with relevant user bits
+        ss1 = Mock(name="sourcestamp")
+        fake_change1 = Mock(name="change")
+        fake_change1.number = 9123
+        ss1.changes = [fake_change1]
+        ss1.patch, ss1.addPatch = None, None
+
+        ss2 = Mock(name="sourcestamp")
+        fake_change2 = Mock(name="change")
+        fake_change2.number = 9124
+        ss2.changes = [fake_change2]
+        ss2.patch, ss1.addPatch = None, None
+
+        def fakeGetSS(ss):
+            return lambda: ss
+        build1.getSourceStamp = fakeGetSS(ss1)
+        build2.getSourceStamp = fakeGetSS(ss2)
+
+        mn.parent = self
+        self.status = mn.master_status = mn.buildMessageDict = Mock()
+        mn.master_status.getBuilder = fakeGetBuilder
+        mn.buildMessageDict.return_value = {"body": "body", "type": "text"}
+
+        mn.buildMessage(builder.name, [build1, build2], build1.result)
+        self.assertEqual(m['To'], "tyler@mayhem.net, user2@example.net")
+
 def create_msgdict():
     unibody = u'Unicode body with non-ascii (\u00E5\u00E4\u00F6).'
     msg_dict = dict(body=unibody, type='plain')