Merge lp:~wgrant/launchpad/bug-1098170 into lp:launchpad

[William Grant]

Fix MailingListSet.getSusbcribedAddresses() to not occasionally return former members when running over multiple lists.

This branch fixes a correctness bug in MailingListSet.getSubscribedAddresses() when called with more than one mailing list. This hadn't been seen on production before yesterday because mailman was until recently configured to only request information one list at a time.

The bug was a single missing line in the second (non-preferred address) query:

    MailingList.teamID == TeamParticipation.teamID,

Without that, a TeamParticipation for any of the teams in the batch qualifies the person as a participant in *all* of the teams in the batch, even if they're actually only a former member in some of them.

Rather than fix the duplicated query, I rather chose to merge the preferred and non-preferred address cases into a single query.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2013-01-07 02:40:55 +0000
+++ lib/lp/registry/model/mailinglist.py	2013-01-11 00:08:23 +0000
@@ -30,6 +30,7 @@
 from storm.expr import (
     And,
     Join,
+    Or,
     )
 from storm.info import ClassAlias
 from storm.store import Store
@@ -563,10 +564,6 @@
     def getSubscribedAddresses(self, team_names):
         """See `IMailingListSet`."""
         store = IStore(MailingList)
-        # In order to handle the case where the preferred email address is
-        # used (i.e. where MailingListSubscription.email_address is NULL), we
-        # need to UNION, those using a specific address and those using the
-        # preferred address.
         Team = ClassAlias(Person)
         tables = (
             EmailAddress,
@@ -582,16 +579,19 @@
             Join(Team, Team.id == MailingList.teamID),
             )
         team_ids, list_ids = self._getTeamIdsAndMailingListIds(team_names)
-        # Find all the people who are subscribed with their preferred address.
         preferred = store.using(*tables).find(
             (EmailAddress.email, Person.displayname, Team.name),
             And(MailingListSubscription.mailing_listID.is_in(list_ids),
                 TeamParticipation.teamID.is_in(team_ids),
                 MailingList.teamID == TeamParticipation.teamID,
                 MailingList.status != MailingListStatus.INACTIVE,
-                MailingListSubscription.email_addressID == None,
-                EmailAddress.status == EmailAddressStatus.PREFERRED,
-                Account.status == AccountStatus.ACTIVE))
+                Account.status == AccountStatus.ACTIVE,
+                Or(
+                    And(MailingListSubscription.email_addressID == None,
+                        EmailAddress.status == EmailAddressStatus.PREFERRED),
+                    EmailAddress.id ==
+                        MailingListSubscription.email_addressID),
+                ))
         # Sort by team name.
         by_team = collections.defaultdict(set)
         for email, display_name, team_name in preferred:
@@ -599,18 +599,6 @@
                 'Unexpected team name in results: %s' % team_name)
             value = (display_name, email.lower())
             by_team[team_name].add(value)
-        explicit = store.using(*tables).find(
-            (EmailAddress.email, Person.displayname, Team.name),
-            And(MailingListSubscription.mailing_listID.is_in(list_ids),
-                TeamParticipation.teamID.is_in(team_ids),
-                MailingList.status != MailingListStatus.INACTIVE,
-                EmailAddress.id == MailingListSubscription.email_addressID,
-                Account.status == AccountStatus.ACTIVE))
-        for email, display_name, team_name in explicit:
-            assert team_name in team_names, (
-                'Unexpected team name in results: %s' % team_name)
-            value = (display_name, email.lower())
-            by_team[team_name].add(value)
         # Turn the results into a mapping of lists.
         results = {}
         for team_name, address_set in by_team.items():

=== modified file 'lib/lp/registry/tests/test_mailinglist.py'
--- lib/lp/registry/tests/test_mailinglist.py	2012-12-26 01:32:19 +0000
+++ lib/lp/registry/tests/test_mailinglist.py	2013-01-11 00:08:23 +0000
@@ -640,6 +640,38 @@
         list_subscribers = [(member.displayname, member.preferredemail.email)]
         self.assertEqual(list_subscribers, result[team.name])
 
+    def test_getSubscribedAddresses_excludes_former_participants(self):
+        # getSubscribedAddresses() only includes present participants of
+        # the team, even if they still participate in another team in
+        # the batch (bug #1098170).
+        team1, member1 = self.factory.makeTeamWithMailingListSubscribers(
+            'team1')
+        team2, member2 = self.factory.makeTeamWithMailingListSubscribers(
+            'team2')
+        team1.addMember(member2, reviewer=team1.teamowner)
+        team1.mailing_list.subscribe(member2, address=member2.preferredemail)
+
+        result = self.mailing_list_set.getSubscribedAddresses(
+            ['team1', 'team2'])
+        self.assertEqual(
+            sorted([
+                (member1.displayname, member1.preferredemail.email),
+                (member2.displayname, member2.preferredemail.email)]),
+            result['team1'])
+        self.assertEqual(
+            [(member2.displayname, member2.preferredemail.email)],
+            result['team2'])
+
+        member2.retractTeamMembership(team1, member2)
+        result = self.mailing_list_set.getSubscribedAddresses(
+            ['team1', 'team2'])
+        self.assertEqual(
+            [(member1.displayname, member1.preferredemail.email)],
+            result['team1'])
+        self.assertEqual(
+            [(member2.displayname, member2.preferredemail.email)],
+            result['team2'])
+
     def test_getSubscribedAddresses_preferredemail_dict_values(self):
         # getSubscribedAddresses() dict values include users who want email to
         # go to their preferred address.