launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/participation-query-count into lp:launchpad.

Commit message:
Push the mailing list subscription checks on Person:+participation down to a bulk method on IMailingListSet.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1429806 in Launchpad itself: "Person:+participation times out when lots of mailing lists are involved"
  https://bugs.launchpad.net/launchpad/+bug/1429806

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/participation-query-count/+merge/252267

Push the mailing list subscription checks on Person:+participation down to a bulk method on IMailingListSet.  Also expose ITeamMembership.personID, allowing us to save an entirely unnecessary Person query for each direct team membership.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/participation-query-count into lp:launchpad.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2015-03-05 11:39:06 +0000
+++ lib/lp/registry/browser/person.py	2015-03-09 11:42:57 +0000
@@ -2028,7 +2028,8 @@
     def label(self):
         return 'Team participation for ' + self.context.displayname
 
-    def _asParticipation(self, membership=None, team=None, via=None):
+    def _asParticipation(self, membership=None, team=None, via=None,
+                         mailing_list=None, subscription=None):
         """Return a dict of participation information for the membership.
 
         Method requires membership or team, not both.
@@ -2057,15 +2058,14 @@
             # The member is a direct member; use the membership data.
             team = membership.team
             datejoined = membership.datejoined
-            if membership.person == team.teamowner:
+            if membership.personID == team.teamownerID:
                 role = 'Owner'
             elif membership.status == TeamMembershipStatus.ADMIN:
                 role = 'Admin'
             else:
                 role = 'Member'
 
-        if team.mailing_list is not None and team.mailing_list.is_usable:
-            subscription = team.mailing_list.getSubscription(self.context)
+        if mailing_list is not None:
             if subscription is None:
                 subscribed = 'Not subscribed'
             else:
@@ -2081,28 +2081,37 @@
     def active_participations(self):
         """Return the participation information for active memberships."""
         paths, memberships = self.context.getPathsToTeams()
+        memberships = [
+            membership for membership in memberships
+            if check_permission('launchpad.View', membership.team)]
         direct_teams = [membership.team for membership in memberships]
         indirect_teams = [
             team for team in paths.keys()
-            if team not in direct_teams]
+            if team not in direct_teams and
+               check_permission('launchpad.View', team)]
         participations = []
 
+        # Bulk-load mailing list subscriptions.
+        subscriptions = getUtility(IMailingListSet).getSubscriptionsForTeams(
+            self.context, direct_teams + indirect_teams)
+
         # First, create a participation for all direct memberships.
         for membership in memberships:
-            # Add a participation record for the membership if allowed.
-            if check_permission('launchpad.View', membership.team):
-                participations.append(
-                    self._asParticipation(membership=membership))
+            mailing_list, subscription = subscriptions.get(
+                membership.team.id, (None, None))
+            participations.append(self._asParticipation(
+                membership=membership, mailing_list=mailing_list,
+                subscription=subscription))
 
         # Second, create a participation for all indirect memberships,
         # using the remaining paths.
         for indirect_team in indirect_teams:
-            if not check_permission('launchpad.View', indirect_team):
-                continue
+            mailing_list, subscription = subscriptions.get(
+                indirect_team.id, (None, None))
             participations.append(
                 self._asParticipation(
-                    via=paths[indirect_team],
-                    team=indirect_team))
+                    via=paths[indirect_team], team=indirect_team,
+                    mailing_list=mailing_list, subscription=subscription))
         return sorted(participations, key=itemgetter('displayname'))
 
     @cachedproperty

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2015-03-05 15:28:11 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2015-03-09 11:42:57 +0000
@@ -47,6 +47,7 @@
 from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
 from lp.services.log.logger import FakeLogger
 from lp.services.mail import stub
+from lp.services.propertycache import clear_property_cache
 from lp.services.verification.interfaces.authtoken import LoginTokenType
 from lp.services.verification.interfaces.logintoken import ILoginTokenSet
 from lp.services.verification.tests.logintoken import get_token_url_from_email
@@ -68,6 +69,7 @@
     login_person,
     monkey_patch,
     person_logged_in,
+    record_two_runs,
     StormStatementRecorder,
     TestCaseWithFactory,
     )
@@ -878,6 +880,26 @@
         self.assertEqual(1, len(participations))
         self.assertEqual(True, self.view.has_participations)
 
+    def test_mailing_list_subscriptions_query_count(self):
+        # Additional mailing list subscriptions do not add additional queries.
+        def create_subscriptions():
+            direct_team = self.factory.makeTeam(members=[self.user])
+            direct_list = self.factory.makeMailingList(
+                direct_team, direct_team.teamowner)
+            direct_list.subscribe(self.user)
+            indirect_team = self.factory.makeTeam(members=[direct_team])
+            indirect_list = self.factory.makeMailingList(
+                indirect_team, indirect_team.teamowner)
+            indirect_list.subscribe(self.user)
+
+        def get_participations():
+            clear_property_cache(self.view)
+            return list(self.view.active_participations)
+
+        recorder1, recorder2 = record_two_runs(
+            get_participations, create_subscriptions, 5)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
 
 class TestPersonRelatedPackagesView(TestCaseWithFactory):
     """Test the related software view."""

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2015-02-26 11:34:47 +0000
+++ lib/lp/registry/configure.zcml	2015-03-09 11:42:57 +0000
@@ -37,6 +37,7 @@
                 id
                 team
                 person
+                personID
                 status
                 proposed_by
                 acknowledged_by

=== modified file 'lib/lp/registry/interfaces/mailinglist.py'
--- lib/lp/registry/interfaces/mailinglist.py	2013-01-07 02:40:55 +0000
+++ lib/lp/registry/interfaces/mailinglist.py	2015-03-09 11:42:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Mailing list interfaces."""
@@ -464,6 +464,17 @@
         :raises AssertionError: When `team_name` is not a string.
         """
 
+    def getSubscriptionsForTeams(person, teams):
+        """Return a person's subscriptions to usable lists for a set of teams.
+
+        :param person: An `IPerson`.
+        :param teams: A list of `ITeam`s.
+        :return: A dictionary mapping team IDs to tuples of `IMailingList`
+            IDs and `IMailingListSubscription` IDs; the second element will
+            be None if a team has a list to which the person is not
+            subscribed.
+        """
+
     def getSubscribedAddresses(team_names):
         """Return the set of subscribed email addresses for members.
 

=== modified file 'lib/lp/registry/interfaces/teammembership.py'
--- lib/lp/registry/interfaces/teammembership.py	2013-01-07 02:40:55 +0000
+++ lib/lp/registry/interfaces/teammembership.py	2015-03-09 11:42:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Team membership interfaces."""
@@ -131,6 +131,7 @@
         Reference(title=_("Member"), required=True, readonly=True,
                   schema=Interface),  # Specified in interfaces/person.py.
         exported_as='member')
+    personID = Int(title=_("Person ID"), required=True, readonly=True)
     proposed_by = Attribute(_('Proponent'))
     reviewed_by = Attribute(
         _("The team admin who approved/rejected the member."))

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2013-06-20 05:50:00 +0000
+++ lib/lp/registry/model/mailinglist.py	2015-03-09 11:42:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -100,6 +100,13 @@
     PostedMessageStatus.APPROVAL_PENDING)
 
 
+USABLE_STATUSES = (
+    MailingListStatus.ACTIVE,
+    MailingListStatus.MODIFIED,
+    MailingListStatus.UPDATING,
+    MailingListStatus.MOD_FAILED)
+
+
 class MessageApproval(SQLBase):
     """A held message."""
 
@@ -344,10 +351,7 @@
     @property
     def is_usable(self):
         """See `IMailingList`."""
-        return self.status in [MailingListStatus.ACTIVE,
-                               MailingListStatus.MODIFIED,
-                               MailingListStatus.UPDATING,
-                               MailingListStatus.MOD_FAILED]
+        return self.status in USABLE_STATUSES
 
     def _get_welcome_message(self):
         return self.welcome_message_
@@ -547,6 +551,24 @@
             """ % sqlvalues(team_name),
             clauseTables=['Person'])
 
+    def getSubscriptionsForTeams(self, person, teams):
+        """See `IMailingListSet`."""
+        store = IStore(MailingList)
+        team_ids = set(map(operator.attrgetter("id"), teams))
+        lists = dict(store.find(
+            (MailingList.teamID, MailingList.id),
+            MailingList.teamID.is_in(team_ids),
+            MailingList.status.is_in(USABLE_STATUSES)))
+        subscriptions = dict(store.find(
+            (MailingListSubscription.mailing_listID,
+             MailingListSubscription.id),
+            MailingListSubscription.person == person,
+            MailingListSubscription.mailing_listID.is_in(lists.values())))
+        by_team = {}
+        for team, mailing_list in lists.items():
+            by_team[team] = (mailing_list, subscriptions.get(mailing_list))
+        return by_team
+
     def _getTeamIdsAndMailingListIds(self, team_names):
         """Return a tuple of team and mailing list Ids for the team names."""
         store = IStore(MailingList)