Merge lp:~cjwatson/launchpad/participation-query-count into lp:launchpad

[Colin Watson]

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

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.

[William Grant (wgrant)]

review: Approve (code)

=== 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 15:05:42 +0000
@@ -284,9 +284,6 @@
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
 
 
-COMMASPACE = ', '
-
-
 class PersonBreadcrumb(DisplaynameBreadcrumb):
     implements(IHeadingBreadcrumb, IMultiFacetedBreadcrumb)
 
@@ -2028,23 +2025,24 @@
     def label(self):
         return 'Team participation for ' + self.context.displayname
 
-    def _asParticipation(self, membership=None, team=None, via=None):
+    def _asParticipation(self, team=None, membership=None, via=None,
+                         mailing_list=None, subscription=None):
         """Return a dict of participation information for the membership.
 
-        Method requires membership or team, not both.
+        Method requires membership or via, not both.
         :param via: The team through which the membership in the indirect
         team is established.
         """
-        if ((membership is None and team is None) or
-            (membership is not None and team is not None)):
+        if ((membership is None and via is None) or
+            (membership is not None and via is not None)):
             raise AssertionError(
-                "The membership or team argument must be provided, not both.")
+                "The membership or via argument must be provided, not both.")
 
         if via is not None:
             # When showing the path, it's unnecessary to show the team in
             # question at the beginning of the path, or the user at the
             # end of the path.
-            via = COMMASPACE.join(
+            via = ", ".join(
                 [via_team.displayname for via_team in via[1:-1]])
 
         if membership is None:
@@ -2055,17 +2053,15 @@
             datejoined = None
         else:
             # 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:
@@ -2082,27 +2078,26 @@
         """Return the participation information for active memberships."""
         paths, memberships = self.context.getPathsToTeams()
         direct_teams = [membership.team for membership in memberships]
-        indirect_teams = [
-            team for team in paths.keys()
-            if team not in direct_teams]
+        items = []
+        for membership in memberships:
+            items.append(dict(team=membership.team, membership=membership))
+        for team, via in paths.items():
+            if team not in direct_teams:
+                items.append(dict(team=team, via=via))
+        items = [
+            item for item in items
+            if check_permission('launchpad.View', item["team"])]
         participations = []
 
-        # 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))
+        # Bulk-load mailing list subscriptions.
+        subscriptions = getUtility(IMailingListSet).getSubscriptionsForTeams(
+            self.context, [item["team"] for item in items])
 
-        # 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
-            participations.append(
-                self._asParticipation(
-                    via=paths[indirect_team],
-                    team=indirect_team))
+        # Create all the participations.
+        for item in items:
+            item["mailing_list"], item["subscription"] = subscriptions.get(
+                item["team"].id, (None, None))
+            participations.append(self._asParticipation(**item))
         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 15:05:42 +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 15:05:42 +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 15:05:42 +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 15:05:42 +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 15:05:42 +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)

=== modified file 'lib/lp/registry/tests/mailinglists_helper.py'
--- lib/lp/registry/tests/mailinglists_helper.py	2012-12-19 21:02:33 +0000
+++ lib/lp/registry/tests/mailinglists_helper.py	2015-03-09 15:05:42 +0000
@@ -29,9 +29,6 @@
 from lp.services.database.sqlbase import flush_database_updates
 
 
-COMMASPACE = ', '
-
-
 def fault_catcher(func):
     """Decorator for displaying Faults in a cross-compatible way.