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

Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 13859
Proposed branch: lp:~gary/launchpad/bug811447
Merge into: lp:launchpad
Prerequisite: lp:~gary/launchpad/bug838869
Diff against target: 181 lines (+65/-18)
2 files modified
lib/lp/bugs/model/personsubscriptioninfo.py (+15/-14)
lib/lp/bugs/model/tests/test_personsubscriptioninfo.py (+50/-4)
To merge this branch: bzr merge lp:~gary/launchpad/bug811447
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Review via email: mp+73704@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
j.c.sackett (jcsackett) wrote :

Nice work, Gary.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
5959
60 implements(IAbstractSubscriptionInfoCollection)60 implements(IAbstractSubscriptionInfoCollection)
6161
62 def __init__(self, person, administrated_teams):62 def __init__(self, person, administrated_team_ids):
63 self.person = person63 self.person = person
64 self.administrated_teams = administrated_teams64 self.administrated_team_ids = administrated_team_ids
65 self.personal = []65 self.personal = []
66 self.as_team_member = []66 self.as_team_member = []
67 self.as_team_admin = []67 self.as_team_admin = []
@@ -72,7 +72,7 @@
72 collection = self.personal72 collection = self.personal
73 else:73 else:
74 assert principal.isTeam(), (principal, self.person)74 assert principal.isTeam(), (principal, self.person)
75 if principal in self.administrated_teams:75 if principal.id in self.administrated_team_ids:
76 collection = self.as_team_admin76 collection = self.as_team_admin
77 else:77 else:
78 collection = self.as_team_member78 collection = self.as_team_member
@@ -88,9 +88,9 @@
8888
89 implements(IVirtualSubscriptionInfoCollection)89 implements(IVirtualSubscriptionInfoCollection)
9090
91 def __init__(self, person, administrated_teams):91 def __init__(self, person, administrated_team_ids):
92 super(VirtualSubscriptionInfoCollection, self).__init__(92 super(VirtualSubscriptionInfoCollection, self).__init__(
93 person, administrated_teams)93 person, administrated_team_ids)
94 self._principal_pillar_to_info = {}94 self._principal_pillar_to_info = {}
9595
96 def _add_item_to_collection(self,96 def _add_item_to_collection(self,
@@ -110,9 +110,9 @@
110110
111 implements(IRealSubscriptionInfoCollection)111 implements(IRealSubscriptionInfoCollection)
112112
113 def __init__(self, person, administrated_teams):113 def __init__(self, person, administrated_team_ids):
114 super(RealSubscriptionInfoCollection, self).__init__(114 super(RealSubscriptionInfoCollection, self).__init__(
115 person, administrated_teams)115 person, administrated_team_ids)
116 self._principal_bug_to_infos = {}116 self._principal_bug_to_infos = {}
117117
118 def _add_item_to_collection(self, collection, principal,118 def _add_item_to_collection(self, collection, principal,
@@ -189,13 +189,13 @@
189 TeamParticipation.teamID == Person.id)189 TeamParticipation.teamID == Person.id)
190190
191 direct = RealSubscriptionInfoCollection(191 direct = RealSubscriptionInfoCollection(
192 self.person, self.administrated_teams)192 self.person, self.administrated_team_ids)
193 duplicates = RealSubscriptionInfoCollection(193 duplicates = RealSubscriptionInfoCollection(
194 self.person, self.administrated_teams)194 self.person, self.administrated_team_ids)
195 bugs = set()195 bugs = set()
196 for subscription, subscribed_bug, subscriber in info:196 for subscription, subscribed_bug, subscriber in info:
197 bugs.add(subscribed_bug)197 bugs.add(subscribed_bug)
198 if subscribed_bug != bug:198 if subscribed_bug.id != bug.id:
199 # This is a subscription through a duplicate.199 # This is a subscription through a duplicate.
200 collection = duplicates200 collection = duplicates
201 else:201 else:
@@ -232,7 +232,8 @@
232232
233 def loadSubscriptionsFor(self, person, bug):233 def loadSubscriptionsFor(self, person, bug):
234 self.person = person234 self.person = person
235 self.administrated_teams = person.getAdministratedTeams()235 self.administrated_team_ids = [
236 team.id for team in person.getAdministratedTeams()]
236 self.bug = bug237 self.bug = bug
237238
238 # First get direct and duplicate real subscriptions.239 # First get direct and duplicate real subscriptions.
@@ -244,9 +245,9 @@
244245
245 # Then get owner and assignee virtual subscriptions.246 # Then get owner and assignee virtual subscriptions.
246 as_owner = VirtualSubscriptionInfoCollection(247 as_owner = VirtualSubscriptionInfoCollection(
247 self.person, self.administrated_teams)248 self.person, self.administrated_team_ids)
248 as_assignee = VirtualSubscriptionInfoCollection(249 as_assignee = VirtualSubscriptionInfoCollection(
249 self.person, self.administrated_teams)250 self.person, self.administrated_team_ids)
250 for bugtask in bug.bugtasks:251 for bugtask in bug.bugtasks:
251 pillar = self._getTaskPillar(bugtask)252 pillar = self._getTaskPillar(bugtask)
252 owner = pillar.owner253 owner = pillar.owner
@@ -301,7 +302,7 @@
301 }302 }
302 direct = {}303 direct = {}
303 from_duplicate = {}304 from_duplicate = {}
304 as_owner = {} # This is an owner of a pillar with no bug supervisor.305 as_owner = {} # This is an owner of a pillar with no bug supervisor.
305 as_assignee = {}306 as_assignee = {}
306 subscription_data = {307 subscription_data = {
307 'direct': direct,308 'direct': direct,
308309
=== 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 @@
55
6__metaclass__ = type6__metaclass__ = type
77
8from testtools.matchers import LessThan
8from zope.security.proxy import removeSecurityProxy9from zope.security.proxy import removeSecurityProxy
910
11from canonical.launchpad.webapp.adapter import SQLLogger
10from canonical.testing import DatabaseFunctionalLayer12from canonical.testing import DatabaseFunctionalLayer
11from lp.bugs.interfaces.personsubscriptioninfo import (13from lp.bugs.interfaces.personsubscriptioninfo import (
12 IRealSubscriptionInfo,14 IRealSubscriptionInfo,
@@ -33,6 +35,20 @@
33 self.bug = self.factory.makeBug()35 self.bug = self.factory.makeBug()
34 self.subscriptions = PersonSubscriptions(self.subscriber, self.bug)36 self.subscriptions = PersonSubscriptions(self.subscriber, self.bug)
3537
38 def makeDuplicates(self, count=1, subscriber=None):
39 if subscriber is None:
40 subscriber = self.subscriber
41 if subscriber.isTeam():
42 subscribed_by = subscriber.teamowner
43 else:
44 subscribed_by = subscriber
45 duplicates = [self.factory.makeBug() for i in range(count)]
46 with person_logged_in(subscribed_by):
47 for duplicate in duplicates:
48 duplicate.markAsDuplicate(self.bug)
49 duplicate.subscribe(subscriber, subscribed_by)
50 return duplicates
51
36 def assertCollectionsAreEmpty(self, except_=None):52 def assertCollectionsAreEmpty(self, except_=None):
37 names = ('direct', 'from_duplicate', 'as_owner', 'as_assignee')53 names = ('direct', 'from_duplicate', 'as_owner', 'as_assignee')
38 assert except_ is None or except_ in names54 assert except_ is None or except_ in names
@@ -284,10 +300,7 @@
284300
285 def test_duplicate_direct(self):301 def test_duplicate_direct(self):
286 # Subscribed directly to the duplicate bug.302 # Subscribed directly to the duplicate bug.
287 duplicate = self.factory.makeBug()303 [duplicate] = self.makeDuplicates(count=1)
288 with person_logged_in(self.subscriber):
289 duplicate.markAsDuplicate(self.bug)
290 duplicate.subscribe(self.subscriber, self.subscriber)
291 # Load a `PersonSubscriptionInfo`s for subscriber and a bug.304 # Load a `PersonSubscriptionInfo`s for subscriber and a bug.
292 self.subscriptions.reload()305 self.subscriptions.reload()
293306
@@ -487,3 +500,36 @@
487 self.subscriptions.reload()500 self.subscriptions.reload()
488501
489 self.failUnless(self.subscriptions.muted)502 self.failUnless(self.subscriptions.muted)
503
504 def test_many_duplicate_team_admin_subscriptions_few_queries(self):
505 # This is related to bug 811447. The user is subscribed to a
506 # duplicate bug through team membership in which the user is an admin.
507 team = self.factory.makeTeam()
508 with person_logged_in(team.teamowner):
509 team.addMember(self.subscriber, team.teamowner,
510 status=TeamMembershipStatus.ADMIN)
511 self.makeDuplicates(count=1, subscriber=team)
512 logger = SQLLogger()
513 with logger:
514 self.subscriptions.reload()
515 # This should produce a very small number of queries.
516 count_with_one_subscribed_duplicate = len(logger.queries)
517 self.assertThat(count_with_one_subscribed_duplicate, LessThan(5))
518 # It should have the correct result.
519 self.assertCollectionsAreEmpty(except_='from_duplicate')
520 self.assertCollectionContents(
521 self.subscriptions.from_duplicate, as_team_admin=1)
522 # If we increase the number of duplicates subscribed via the team that
523 # the user administers...
524 self.makeDuplicates(count=4, subscriber=team)
525 with logger:
526 self.subscriptions.reload()
527 # ...then the query count should remain the same.
528 count_with_five_subscribed_duplicates = len(logger.queries)
529 self.assertEqual(
530 count_with_one_subscribed_duplicate,
531 count_with_five_subscribed_duplicates)
532 # We should still have the correct result.
533 self.assertCollectionsAreEmpty(except_='from_duplicate')
534 self.assertCollectionContents(
535 self.subscriptions.from_duplicate, as_team_admin=5)