Merge lp:~gary/launchpad/bug811447 into lp:launchpad

[Gary Poster]

[r=jcsackett][bug=811447] drastically reduce the number of SQL needed for users who are team administrators and view a bug with many duplicates to which one or more of their teams is subscribed.

This branch fixes bug 811447 by drastically reducing the number of SQL needed for users who are team administrators and view a bug with many duplicates to which one or more of their teams is subscribed. It now takes a single query. This is done by converting the resultset of administrated teams to a list of administrated team ids, and then that list is consulted, rather than the result set.

Lint is happy.

This depends on lp:~gary/launchpad/bug838869 for a tool I used as test fixture.

Thank you.

[j.c.sackett (jcsackett)]

Nice work, Gary.

review: Approve

=== modified file 'lib/lp/bugs/model/personsubscriptioninfo.py'
--- lib/lp/bugs/model/personsubscriptioninfo.py	2011-05-17 12:49:03 +0000
+++ lib/lp/bugs/model/personsubscriptioninfo.py	2011-09-01 17:23:38 +0000
@@ -59,9 +59,9 @@
 
     implements(IAbstractSubscriptionInfoCollection)
 
-    def __init__(self, person, administrated_teams):
+    def __init__(self, person, administrated_team_ids):
         self.person = person
-        self.administrated_teams = administrated_teams
+        self.administrated_team_ids = administrated_team_ids
         self.personal = []
         self.as_team_member = []
         self.as_team_admin = []
@@ -72,7 +72,7 @@
             collection = self.personal
         else:
             assert principal.isTeam(), (principal, self.person)
-            if principal in self.administrated_teams:
+            if principal.id in self.administrated_team_ids:
                 collection = self.as_team_admin
             else:
                 collection = self.as_team_member
@@ -88,9 +88,9 @@
 
     implements(IVirtualSubscriptionInfoCollection)
 
-    def __init__(self, person, administrated_teams):
+    def __init__(self, person, administrated_team_ids):
         super(VirtualSubscriptionInfoCollection, self).__init__(
-            person, administrated_teams)
+            person, administrated_team_ids)
         self._principal_pillar_to_info = {}
 
     def _add_item_to_collection(self,
@@ -110,9 +110,9 @@
 
     implements(IRealSubscriptionInfoCollection)
 
-    def __init__(self, person, administrated_teams):
+    def __init__(self, person, administrated_team_ids):
         super(RealSubscriptionInfoCollection, self).__init__(
-            person, administrated_teams)
+            person, administrated_team_ids)
         self._principal_bug_to_infos = {}
 
     def _add_item_to_collection(self, collection, principal,
@@ -189,13 +189,13 @@
             TeamParticipation.teamID == Person.id)
 
         direct = RealSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         duplicates = RealSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         bugs = set()
         for subscription, subscribed_bug, subscriber in info:
             bugs.add(subscribed_bug)
-            if subscribed_bug != bug:
+            if subscribed_bug.id != bug.id:
                 # This is a subscription through a duplicate.
                 collection = duplicates
             else:
@@ -232,7 +232,8 @@
 
     def loadSubscriptionsFor(self, person, bug):
         self.person = person
-        self.administrated_teams = person.getAdministratedTeams()
+        self.administrated_team_ids = [
+            team.id for team in person.getAdministratedTeams()]
         self.bug = bug
 
         # First get direct and duplicate real subscriptions.
@@ -244,9 +245,9 @@
 
         # Then get owner and assignee virtual subscriptions.
         as_owner = VirtualSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         as_assignee = VirtualSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         for bugtask in bug.bugtasks:
             pillar = self._getTaskPillar(bugtask)
             owner = pillar.owner
@@ -301,7 +302,7 @@
                 }
         direct = {}
         from_duplicate = {}
-        as_owner = {} # This is an owner of a pillar with no bug supervisor.
+        as_owner = {}  # This is an owner of a pillar with no bug supervisor.
         as_assignee = {}
         subscription_data = {
             'direct': direct,

=== modified file 'lib/lp/bugs/model/tests/test_personsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2011-05-17 12:49:03 +0000
+++ lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2011-09-01 17:23:38 +0000
@@ -5,8 +5,10 @@
 
 __metaclass__ = type
 
+from testtools.matchers import LessThan
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp.adapter import SQLLogger
 from canonical.testing import DatabaseFunctionalLayer
 from lp.bugs.interfaces.personsubscriptioninfo import (
     IRealSubscriptionInfo,
@@ -33,6 +35,20 @@
         self.bug = self.factory.makeBug()
         self.subscriptions = PersonSubscriptions(self.subscriber, self.bug)
 
+    def makeDuplicates(self, count=1, subscriber=None):
+        if subscriber is None:
+            subscriber = self.subscriber
+        if subscriber.isTeam():
+            subscribed_by = subscriber.teamowner
+        else:
+            subscribed_by = subscriber
+        duplicates = [self.factory.makeBug() for i in range(count)]
+        with person_logged_in(subscribed_by):
+            for duplicate in duplicates:
+                duplicate.markAsDuplicate(self.bug)
+                duplicate.subscribe(subscriber, subscribed_by)
+        return duplicates
+
     def assertCollectionsAreEmpty(self, except_=None):
         names = ('direct', 'from_duplicate', 'as_owner', 'as_assignee')
         assert except_ is None or except_ in names
@@ -284,10 +300,7 @@
 
     def test_duplicate_direct(self):
         # Subscribed directly to the duplicate bug.
-        duplicate = self.factory.makeBug()
-        with person_logged_in(self.subscriber):
-            duplicate.markAsDuplicate(self.bug)
-            duplicate.subscribe(self.subscriber, self.subscriber)
+        [duplicate] = self.makeDuplicates(count=1)
         # Load a `PersonSubscriptionInfo`s for subscriber and a bug.
         self.subscriptions.reload()
 
@@ -487,3 +500,36 @@
         self.subscriptions.reload()
 
         self.failUnless(self.subscriptions.muted)
+
+    def test_many_duplicate_team_admin_subscriptions_few_queries(self):
+        # This is related to bug 811447. The user is subscribed to a
+        # duplicate bug through team membership in which the user is an admin.
+        team = self.factory.makeTeam()
+        with person_logged_in(team.teamowner):
+            team.addMember(self.subscriber, team.teamowner,
+                           status=TeamMembershipStatus.ADMIN)
+        self.makeDuplicates(count=1, subscriber=team)
+        logger = SQLLogger()
+        with logger:
+            self.subscriptions.reload()
+        # This should produce a very small number of queries.
+        count_with_one_subscribed_duplicate = len(logger.queries)
+        self.assertThat(count_with_one_subscribed_duplicate, LessThan(5))
+        # It should have the correct result.
+        self.assertCollectionsAreEmpty(except_='from_duplicate')
+        self.assertCollectionContents(
+            self.subscriptions.from_duplicate, as_team_admin=1)
+        # If we increase the number of duplicates subscribed via the team that
+        # the user administers...
+        self.makeDuplicates(count=4, subscriber=team)
+        with logger:
+            self.subscriptions.reload()
+        # ...then the query count should remain the same.
+        count_with_five_subscribed_duplicates = len(logger.queries)
+        self.assertEqual(
+            count_with_one_subscribed_duplicate,
+            count_with_five_subscribed_duplicates)
+        # We should still have the correct result.
+        self.assertCollectionsAreEmpty(except_='from_duplicate')
+        self.assertCollectionContents(
+            self.subscriptions.from_duplicate, as_team_admin=5)