launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/bugtask-activity-preload into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1037532 in Launchpad itself: "BugTask:+activity Timeout looping over 100s of people"
  https://bugs.launchpad.net/launchpad/+bug/1037532

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/bugtask-activity-preload/+merge/123484

Change Bug:+activity to bulk load the people from ValidPersonCache. With 10 BugActivity rows, the query count drops from 13 to 5, and with 50 rows the query count drops from 53 to 5. This also required adding a personID attribute onto IBugActivity.
-- 
https://code.launchpad.net/~stevenk/launchpad/bugtask-activity-preload/+merge/123484
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/bugtask-activity-preload into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2012-08-26 22:50:37 +0000
+++ lib/lp/bugs/browser/bug.py	2012-09-10 06:08:19 +0000
@@ -103,7 +103,9 @@
     get_structural_subscriptions_for_bug,
     )
 from lp.registry.enums import PRIVATE_INFORMATION_TYPES
+from lp.registry.model.person import ValidPersonCache
 from lp.registry.vocabularies import InformationTypeVocabulary
+from lp.services.database.bulk import load_related
 from lp.services.fields import DuplicateBug
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.mail.mailwrapper import MailWrapper
@@ -605,6 +607,12 @@
 
     page_title = 'Activity log'
 
+    @property
+    def activity(self):
+        activity = IBug(self.context).activity
+        load_related(ValidPersonCache, activity, ['personID'])
+        return activity
+
 
 class BugSubscriptionPortletDetails:
     """A mixin used to collate bug subscription details for a view."""

=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py	2012-09-06 00:01:38 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py	2012-09-10 06:08:19 +0000
@@ -5,20 +5,29 @@
 
 __metaclass__ = type
 
+from datetime import (
+    datetime,
+    timedelta,
+    )
+
 from BeautifulSoup import BeautifulSoup
+import pytz
 import simplejson
 from soupmatchers import (
     HTMLContains,
     Tag,
     )
+from storm.store import Store
 from testtools.matchers import (
     Contains,
+    Equals,
     MatchesAll,
     Not,
     )
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.bugs.adapters.bugchange import BugAttachmentChange
 from lp.registry.enums import (
     BugSharingPolicy,
     InformationType,
@@ -35,12 +44,14 @@
     BrowserTestCase,
     login_person,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import find_tag_by_id
 from lp.testing.views import (
     create_initialized_view,
@@ -673,3 +684,26 @@
             'table',
             {'id': 'affected-software', 'class': 'listing'})
         self.assertIsNotNone(table)
+
+
+class TestBugActivityView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_bug_activity_query_count(self):
+        # Bug:+activity doesn't make O(n) queries based on the amount of
+        # activity.
+        bug = self.factory.makeBug()
+        ten_minutes_ago = datetime.now(pytz.UTC) - timedelta(minutes=10)
+        with person_logged_in(bug.owner):
+            attachment = self.factory.makeBugAttachment(bug=bug)
+            for i in range(10):
+                bug.addChange(BugAttachmentChange(
+                    ten_minutes_ago, self.factory.makePerson(), 'attachment',
+                    None, attachment))
+        Store.of(bug).flush()
+        with StormStatementRecorder() as recorder:
+            view = create_initialized_view(
+                bug.default_bugtask, name='+activity')
+            view.render()
+        self.assertThat(recorder, HasQueryCount(Equals(5)))

=== modified file 'lib/lp/bugs/interfaces/bugactivity.py'
--- lib/lp/bugs/interfaces/bugactivity.py	2011-12-24 16:54:44 +0000
+++ lib/lp/bugs/interfaces/bugactivity.py	2012-09-10 06:08:19 +0000
@@ -16,7 +16,10 @@
     export_as_webservice_entry,
     exported,
     )
-from zope.interface import Interface
+from zope.interface import (
+    Attribute,
+    Interface,
+    )
 from zope.schema import (
     Datetime,
     Text,
@@ -42,6 +45,7 @@
                  description=_("The date on which this activity occurred."),
                  readonly=True))
 
+    personID = Attribute('DB ID for Person')
     person = exported(PersonChoice(
         title=_('Person'), required=True, vocabulary='ValidPersonOrTeam',
         readonly=True, description=_("The person's Launchpad ID or "

=== modified file 'lib/lp/bugs/templates/bug-activity.pt'
--- lib/lp/bugs/templates/bug-activity.pt	2010-10-10 21:54:16 +0000
+++ lib/lp/bugs/templates/bug-activity.pt	2012-09-10 06:08:19 +0000
@@ -20,7 +20,7 @@
             </tr>
           </thead>
           <tbody>
-            <tr tal:repeat="log context/bug/activity">
+            <tr tal:repeat="log view/activity">
               <tal:comment condition="nothing">
                 XXX: Gavin Panella 2009-08-12 bug=412963: Using strftime()
                 here because fmt:datetime changes timezone, even though we