launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1098170 into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1098170/+merge/142816

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.

-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1098170/+merge/142816
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1098170 into lp:launchpad.

=== 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.