Merge lp:~edwin-grubbs/launchpad/bug-590840-getMembershipInformation-timeout into lp:launchpad

[Edwin Grubbs]

Summary
-------

The MailingListAPIView.getMembershipInformation() method in
canonical.launchpad.xmlrpc.mailinglist was causing soft timeouts
(See OOPS-1612S995).

This was caused by the MailingListSet.getSenderAddresses(team_names)
retrieving all the columns from the tables being joined instead of
just the three columns actually needed.

Implementation details
----------------------

Improved performance of getSenderAddresses(team_names):
    lib/lp/registry/model/mailinglist.py

Drive-by testfix. I'm not sure if this test is being run by buildbot,
since it didn't look like it could possibly work otherwise.
    lib/lp/services/mailman/doc/staging.txt

Tests
-----

./bin/test -vv -t mailinglist
./bin/test -vv --layer=Mailman

[Michael Nelson (michael.nelson)]

Great, thanks Edwin. Small note below that you can choose to ignore.

> === modified file 'lib/lp/registry/model/mailinglist.py'
> --- lib/lp/registry/model/mailinglist.py 2010-04-23 15:19:10 +0000
> +++ lib/lp/registry/model/mailinglist.py 2010-06-10 13:10:37 +0000
> @@ -729,20 +726,13 @@
> Person.teamowner != None))
> )
> team_members = store.using(*tables).find(
> - (EmailAddress, MailingList, Person, Team),
> + (Team.name, Person.displayname, EmailAddress.email),
> And(TeamParticipation.teamID.is_in(team_ids),

I only realised that it wasn't part of your change after testing it, but the And expression is not needed here. Up to you (other examples follow too.

review: Approve (code)

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2010-04-23 15:19:10 +0000
+++ lib/lp/registry/model/mailinglist.py	2010-06-09 05:07:29 +0000
@@ -22,7 +22,7 @@
 from string import Template
 
 from storm.info import ClassAlias
-from storm.expr import And, LeftJoin
+from storm.expr import And, Join, LeftJoin
 from storm.store import Store
 
 from sqlobject import ForeignKey, StringCol
@@ -713,14 +713,11 @@
         Team = ClassAlias(Person)
         tables = (
             Person,
-            LeftJoin(Account, Account.id == Person.accountID),
-            LeftJoin(EmailAddress, EmailAddress.personID == Person.id),
-            LeftJoin(TeamParticipation,
-                     TeamParticipation.personID == Person.id),
-            LeftJoin(MailingList,
-                     MailingList.teamID == TeamParticipation.teamID),
-            LeftJoin(Team,
-                     Team.id == MailingList.teamID),
+            Join(Account, Account.id == Person.accountID),
+            Join(EmailAddress, EmailAddress.personID == Person.id),
+            Join(TeamParticipation, TeamParticipation.personID == Person.id),
+            Join(MailingList, MailingList.teamID == TeamParticipation.teamID),
+            Join(Team, Team.id == MailingList.teamID),
             )
         team_ids = set(
             team.id for team in store.find(
@@ -729,20 +726,13 @@
                     Person.teamowner != None))
             )
         team_members = store.using(*tables).find(
-            (EmailAddress, MailingList, Person, Team),
+            (Team.name, Person.displayname, EmailAddress.email),
             And(TeamParticipation.teamID.is_in(team_ids),
                 MailingList.status != MailingListStatus.INACTIVE,
                 Person.teamowner == None,
                 EmailAddress.status.is_in(EMAIL_ADDRESS_STATUSES),
                 Account.status == AccountStatus.ACTIVE,
                 ))
-        # Sort allowed posters by team/mailing list.
-        by_team = {}
-        for address, mailing_list, person, team in team_members:
-            assert team.name in team_names, (
-                'Unexpected team name in results: %s' % team.name)
-            value = (person.displayname, address.email)
-            by_team.setdefault(team.name, set()).add(value)
         # Second, find all of the email addresses for all of the people who
         # have been explicitly approved for posting to the team mailing lists.
         # This occurs as part of first post moderation, but since they've
@@ -750,31 +740,32 @@
         # for three global approvals.
         tables = (
             Person,
-            LeftJoin(Account, Account.id == Person.accountID),
-            LeftJoin(EmailAddress, EmailAddress.personID == Person.id),
-            LeftJoin(MessageApproval,
-                     MessageApproval.posted_byID == Person.id),
-            LeftJoin(MailingList,
+            Join(Account, Account.id == Person.accountID),
+            Join(EmailAddress, EmailAddress.personID == Person.id),
+            Join(MessageApproval, MessageApproval.posted_byID == Person.id),
+            Join(MailingList,
                      MailingList.id == MessageApproval.mailing_listID),
-            LeftJoin(Team,
-                     Team.id == MailingList.teamID),
+            Join(Team, Team.id == MailingList.teamID),
             )
         list_ids = set(
             mailing_list.id for mailing_list in store.find(
                 MailingList,
                 MailingList.teamID.is_in(team_ids)))
         approved_posters = store.using(*tables).find(
-            (EmailAddress, MessageApproval, Person, Team),
+            (Team.name, Person.displayname, EmailAddress.email),
             And(MessageApproval.mailing_listID.is_in(list_ids),
                 MessageApproval.status.is_in(MESSAGE_APPROVAL_STATUSES),
                 EmailAddress.status.is_in(EMAIL_ADDRESS_STATUSES),
                 Account.status == AccountStatus.ACTIVE,
                 ))
-        for address, message_approval, person, team in approved_posters:
-            assert team.name in team_names, (
-                'Unexpected team name in results: %s' % team.name)
-            value = (person.displayname, address.email)
-            by_team.setdefault(team.name, set()).add(value)
+        # Sort allowed posters by team/mailing list.
+        by_team = {}
+        all_posters = team_members.union(approved_posters)
+        for team_name, person_displayname, email in all_posters:
+            assert team_name in team_names, (
+                'Unexpected team name in results: %s' % team_name)
+            value = (person_displayname, email)
+            by_team.setdefault(team_name, set()).add(value)
         # Turn the results into a mapping of lists.
         results = {}
         for team_name, address_set in by_team.items():

=== modified file 'lib/lp/services/mailman/doc/staging.txt'
--- lib/lp/services/mailman/doc/staging.txt	2010-04-15 11:44:04 +0000
+++ lib/lp/services/mailman/doc/staging.txt	2010-06-09 05:07:29 +0000
@@ -29,7 +29,8 @@
     >>> transaction.commit()
     >>> logout()
 
-    >>> browser = Browser('%s:%s' %
+    >>> from canonical.launchpad.testing.pages import setupBrowser
+    >>> browser = setupBrowser('Basic %s:%s' %
     ...     (owner._preferredemail_cached.email,
     ...      owner._password_cleartext_cached))