launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00817
[Merge] lp:~lifeless/launchpad/bug-607935 into lp:launchpad/devel
Robert Collins has proposed merging lp:~lifeless/launchpad/bug-607935 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Remove the 2-queries-per-team-membership scaling from BugTask:+index via a new helper on IBug.
Also enhances some test support to allow non-request-based query count assertThat uses.
This was motivated by looking at the queries in the OOPSes on bug 607935.
I don't expect this to fix the timeouts (though I could always be pleasantly surprised), but it should make most bugtask pages about a second faster for users in many teams.
--
https://code.launchpad.net/~lifeless/launchpad/bug-607935/+merge/34143
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-607935 into lp:launchpad/devel.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2010-08-27 05:34:32 +0000
+++ lib/lp/bugs/browser/bugtask.py 2010-08-30 23:53:41 +0000
@@ -668,23 +668,26 @@
return
# Set up widgets in order to handle subscription requests.
- if bug.isSubscribed(self.user) or bug.isSubscribedToDupes(self.user):
- subscription_terms = [
- SimpleTerm(
- self.user, self.user.name,
- 'Unsubscribe me from this bug')]
- else:
- subscription_terms = [
- SimpleTerm(
- self.user, self.user.name, 'Subscribe me to this bug')]
- for team in self.user.teams_participated_in:
- if (bug.isSubscribed(team) or bug.isSubscribedToDupes(team)):
- subscription_terms.append(
- SimpleTerm(
- team, team.name,
+ subscription_terms = []
+ self_subscribed = False
+ for person in bug.getSubscribersForPerson(self.user):
+ if person.id == self.user.id:
+ subscription_terms.append(
+ SimpleTerm(
+ person, person,
+ 'Unsubscribe me from this bug'))
+ self_subscribed = True
+ else:
+ subscription_terms.append(
+ SimpleTerm(
+ person, person.name,
'Unsubscribe <a href="%s">%s</a> from this bug' % (
- canonical_url(team),
- cgi.escape(team.displayname))))
+ canonical_url(person),
+ cgi.escape(person.displayname))))
+ if not self_subscribed:
+ subscription_terms.insert(0,
+ SimpleTerm(
+ self.user, self.user.name, 'Subscribe me to this bug'))
subscription_vocabulary = SimpleVocabulary(subscription_terms)
person_field = Choice(
__name__='subscription',
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2010-08-30 23:53:41 +0000
@@ -1,12 +1,21 @@
# Copyright 2009 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
+from __future__ import with_statement
+
__metaclass__ = type
+from contextlib import nested
from doctest import DocTestSuite
import unittest
+from storm.store import Store
+from testtools.matchers import (
+ Equals,
+ LessThan,
+ MatchesAny,
+ )
from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.ftests import (
@@ -23,11 +32,66 @@
from canonical.testing import LaunchpadFunctionalLayer
from lp.bugs.browser import bugtask
from lp.bugs.browser.bugtask import (
+ BugTaskView,
BugTaskEditView,
BugTasksAndNominationsView,
)
from lp.bugs.interfaces.bugtask import BugTaskStatus
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ person_logged_in,
+ StormStatementRecorder,
+ TestCaseWithFactory,
+ )
+from lp.testing.matchers import HasQueryCount
+from lp.testing.sampledata import (
+ ADMIN_EMAIL,
+ NO_PRIVILEGE_EMAIL,
+ USER_EMAIL,
+ )
+
+
+class TestBugTaskView(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def initialize_view(self, bugtask, person):
+ self.invalidate_caches(bugtask)
+ # Login first because logging in triggers queries.
+ with nested(person_logged_in(person), StormStatementRecorder()) as (
+ _,
+ recorder):
+ view = BugTaskView(bugtask, LaunchpadTestRequest())
+ view.initialize()
+ return recorder
+
+ def invalidate_caches(self, obj):
+ store = Store.of(obj)
+ # Make sure everything is in the database.
+ store.flush()
+ # And invalidate the cache (not a reset, because that stops us using
+ # the domain objects)
+ store.invalidate()
+
+ def test_query_counts_constant_with_team_memberships(self):
+ login(ADMIN_EMAIL)
+ bugtask = self.factory.makeBugTask()
+ person_no_teams = self.factory.makePerson()
+ person_with_teams = self.factory.makePerson()
+ for _ in range(4):
+ team = self.factory.makeTeam()
+ team.addMember(person_with_teams, team.teamowner)
+ # count with no teams
+ recorder = self.initialize_view(bugtask, person_no_teams)
+ self.assertThat(recorder, HasQueryCount(LessThan(14)))
+ count_with_no_teams = recorder.count
+ # count with 2 teams
+ recorder2 = self.initialize_view(bugtask, person_with_teams)
+ # Allow an increase of one because storm bug 619017 causes additional
+ # queries, revalidating things unnecessarily. An increase of 1 from
+ # four new teams shows it is definitely not growing per-team.
+ self.assertThat(recorder2, HasQueryCount(
+ LessThan(count_with_no_teams + 2),
+ ))
class TestBugTasksAndNominationsView(TestCaseWithFactory):
@@ -36,7 +100,7 @@
def setUp(self):
super(TestBugTasksAndNominationsView, self).setUp()
- login('foo.bar@xxxxxxxxxxxxx')
+ login(ADMIN_EMAIL)
self.bug = self.factory.makeBug()
self.view = BugTasksAndNominationsView(
self.bug, LaunchpadTestRequest())
@@ -248,7 +312,7 @@
def test_status_field_items_for_ordinary_users(self):
# Ordinary users can set the status to all values except Won't fix,
# Expired, Triaged, Unknown.
- login('no-priv@xxxxxxxxxxxxx')
+ login(NO_PRIVILEGE_EMAIL)
view = BugTaskEditView(
self.bug.default_bugtask, LaunchpadTestRequest())
view.initialize()
@@ -282,7 +346,7 @@
login_person(owner)
self.bug.default_bugtask.transitionToStatus(
BugTaskStatus.UNKNOWN, owner)
- login('no-priv@xxxxxxxxxxxxx')
+ login(NO_PRIVILEGE_EMAIL)
view = BugTaskEditView(
self.bug.default_bugtask, LaunchpadTestRequest())
view.initialize()
@@ -296,7 +360,7 @@
# in the options.
removeSecurityProxy(self.bug.default_bugtask).status = (
BugTaskStatus.EXPIRED)
- login('no-priv@xxxxxxxxxxxxx')
+ login(NO_PRIVILEGE_EMAIL)
view = BugTaskEditView(
self.bug.default_bugtask, LaunchpadTestRequest())
view.initialize()
@@ -317,7 +381,7 @@
def test_assignee_field_vocabulary_regular_user(self):
# For regular users, the assignee vocabulary is
# AllUserTeamsParticipation.
- login('test@xxxxxxxxxxxxx')
+ login(USER_EMAIL)
view = BugTaskEditView(self.bugtask, LaunchpadTestRequest())
view.initialize()
self.assertEqual(
@@ -336,10 +400,7 @@
def test_suite():
- suite = unittest.TestSuite()
- suite.addTest(unittest.makeSuite(TestBugTasksAndNominationsView))
- suite.addTest(unittest.makeSuite(TestBugTaskEditViewStatusField))
- suite.addTest(unittest.makeSuite(TestBugTaskEditViewAssigneeField))
+ suite = unittest.TestLoader().loadTestsFromName(__name__)
suite.addTest(DocTestSuite(bugtask))
suite.addTest(LayeredDocFileSuite(
'bugtask-target-link-titles.txt', setUp=setUp, tearDown=tearDown,
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2010-08-29 23:35:09 +0000
+++ lib/lp/bugs/configure.zcml 2010-08-30 23:53:41 +0000
@@ -640,6 +640,7 @@
isSubscribedToDupes
getSubscribersFromDuplicates
getSubscriptionsFromDuplicates
+ getSubscribersForPerson
indexed_messages
getAlsoNotifiedSubscribers
getBugWatch
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2010-08-29 22:13:26 +0000
+++ lib/lp/bugs/interfaces/bug.py 2010-08-30 23:53:41 +0000
@@ -489,6 +489,14 @@
def getSubscribersFromDuplicates():
"""Return IPersons subscribed from dupes of this bug."""
+ def getSubscribersForPerson(person):
+ """Find the persons or teams by which person is subscribed.
+
+ This call should be quite cheap to make and performs a single query.
+
+ :return: An IResultSet.
+ """
+
def getBugNotificationRecipients(duplicateof=None, old_bug=None,
include_master_dupe_subscribers=False):
"""Return a complete INotificationRecipientSet instance.
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-08-29 22:05:15 +0000
+++ lib/lp/bugs/model/bug.py 2010-08-30 23:53:41 +0000
@@ -187,6 +187,7 @@
ValidPersonCache,
)
from lp.registry.model.pillar import pillar_sort_key
+from lp.registry.model.teammembership import TeamParticipation
from lp.services.fields import DuplicateBug
@@ -781,6 +782,33 @@
return sorted(
dupe_subscribers, key=operator.attrgetter("displayname"))
+ def getSubscribersForPerson(self, person):
+ """See `IBug."""
+ assert person is not None
+ return Store.of(self).find(
+ # return people
+ Person,
+ # For this bug or its duplicates
+ Or(
+ Bug.id == self.id,
+ Bug.duplicateof == self.id),
+ # Get subscriptions for these bugs
+ BugSubscription.bug == Bug.id,
+ # Filter by subscriptions to any team person is in.
+ # Note that teamparticipation includes self-participation entries
+ # (person X is in the team X)
+ TeamParticipation.person == person.id,
+ # XXX: Storm fails to compile this, so manually done.
+ # bug=https://bugs.edge.launchpad.net/storm/+bug/627137
+ # RBC 20100831
+ SQL("""TeamParticipation.team = BugSubscription.person"""),
+ # Join in the Person rows we want
+ # XXX: Storm fails to compile this, so manually done.
+ # bug=https://bugs.edge.launchpad.net/storm/+bug/627137
+ # RBC 20100831
+ SQL("""Person.id = TeamParticipation.team"""),
+ )
+
def getAlsoNotifiedSubscribers(self, recipients=None, level=None):
"""See `IBug`.
=== added file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bug.py 2010-08-30 23:53:41 +0000
@@ -0,0 +1,79 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import with_statement
+
+__metaclass__ = type
+
+from canonical.testing import DatabaseFunctionalLayer
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+
+
+class TestBug(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_get_subscribers_for_person_unsubscribed(self):
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ self.assertTrue(bug.getSubscribersForPerson(person).is_empty())
+
+ def test_get_subscribers_for_person_direct_subscription(self):
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ with person_logged_in(person):
+ bug.subscribe(person, person)
+ self.assertEqual([person], list(bug.getSubscribersForPerson(person)))
+
+ def test_get_subscribers_for_person_indirect_subscription(self):
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ team1 = self.factory.makeTeam()
+ with person_logged_in(team1.teamowner):
+ team1.addMember(person, team1.teamowner)
+ team2 = self.factory.makeTeam()
+ with person_logged_in(team2.teamowner):
+ team2.addMember(person, team2.teamowner)
+ with person_logged_in(person):
+ bug.subscribe(team1, person)
+ self.assertEqual([team1], list(bug.getSubscribersForPerson(person)))
+
+ def test_get_subscribers_for_person_many_subscriptions(self):
+ bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ team1 = self.factory.makeTeam()
+ with person_logged_in(team1.teamowner):
+ team1.addMember(person, team1.teamowner)
+ team2 = self.factory.makeTeam()
+ with person_logged_in(team2.teamowner):
+ team2.addMember(person, team2.teamowner)
+ with person_logged_in(person):
+ bug.subscribe(team1, person)
+ bug.subscribe(team2, person)
+ bug.subscribe(person, person)
+ self.assertEqual(
+ set([person, team1, team2]),
+ set(bug.getSubscribersForPerson(person)))
+
+ def test_get_subscribers_for_person_from_duplicates_too(self):
+ bug = self.factory.makeBug()
+ real_bug = self.factory.makeBug()
+ person = self.factory.makePerson()
+ team1 = self.factory.makeTeam()
+ with person_logged_in(team1.teamowner):
+ team1.addMember(person, team1.teamowner)
+ team2 = self.factory.makeTeam()
+ with person_logged_in(team2.teamowner):
+ team2.addMember(person, team2.teamowner)
+ with person_logged_in(person):
+ bug.subscribe(team1, person)
+ bug.subscribe(team2, person)
+ bug.subscribe(person, person)
+ bug.markAsDuplicate(real_bug)
+ self.assertEqual(
+ set([person, team1, team2]),
+ set(real_bug.getSubscribersForPerson(person)))
+
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2010-08-27 11:20:55 +0000
+++ lib/lp/testing/__init__.py 2010-08-30 23:53:41 +0000
@@ -33,6 +33,7 @@
'run_with_login',
'run_with_storm_debug',
'run_script',
+ 'StormStatementRecorder',
'TestCase',
'TestCaseWithFactory',
'test_tales',
@@ -220,11 +221,53 @@
class StormStatementRecorder:
- """A storm tracer to count queries."""
+ """A storm tracer to count queries.
+
+ This exposes the count and queries as lp.testing._webservice.QueryCollector
+ does permitting its use with the HasQueryCount matcher.
+
+ It also meets the context manager protocol, so you can gather queries
+ easily:
+ with StormStatementRecorder() as recorder:
+ do somestuff
+ self.assertThat(recorder, HasQueryCount(LessThan(42)))
+
+ Note that due to the storm API used, only one of these recorders may be in
+ place at a time: all will be removed when the first one is removed (by
+ calling __exit__ or leaving the scope of a with statement).
+ """
def __init__(self):
self.statements = []
+ @property
+ def count(self):
+ return len(self.statements)
+
+ @property
+ def queries(self):
+ """The statements executed as per get_request_statements."""
+ # Perhaps we could just consolidate this code with the request tracer
+ # code and not need a custom tracer at all - if we provided a context
+ # factory to the tracer, which in the production tracers would
+ # use the adapter magic, and in test created ones would log to a list.
+ # We would need to be able to remove just one tracer though, which I
+ # haven't looked into yet. RBC 20100831
+ result = []
+ for statement in self.statements:
+ result.append((0, 0, 'unknown', statement))
+ return result
+
+ def __enter__(self):
+ """Context manager protocol - return this object as the context."""
+ install_tracer(self)
+ return self
+
+ def __exit__(self, _ignored, _ignored2, _ignored3):
+ """Content manager protocol - do not swallow exceptions."""
+ remove_tracer_type(StormStatementRecorder)
+ return False
+
def connection_raw_execute(self, ignored, raw_cursor, statement, params):
"""Increment the counter. We don't care about the args."""
@@ -244,12 +287,8 @@
:return: a tuple containing the return value of the function,
and a list of sql statements.
"""
- recorder = StormStatementRecorder()
- try:
- install_tracer(recorder)
+ with StormStatementRecorder() as recorder:
ret = function(*args, **kwargs)
- finally:
- remove_tracer_type(StormStatementRecorder)
return (ret, recorder.statements)
