launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/sprint-attendees into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #735996 in Launchpad itself: "Sprint:+index timeouts"
  https://bugs.launchpad.net/launchpad/+bug/735996

For more details, see:
https://code.launchpad.net/~thumper/launchpad/sprint-attendees/+merge/53941

Addresses Sprint:+index timeouts, bug 735996

As part of this work I Stormified the SprintAttendance class.
I'd been wondering how hard it would be, as we started using
Storm ages ago, but I'd guess that most model classes are still
using SQLBase.

As a part of that I had to change how the SprintAttendance
objects were being retrieved.  Which led me to a few very
poor implementations.
-- 
https://code.launchpad.net/~thumper/launchpad/sprint-attendees/+merge/53941
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/sprint-attendees into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/sprint.py'
--- lib/lp/blueprints/browser/sprint.py	2011-02-02 15:43:31 +0000
+++ lib/lp/blueprints/browser/sprint.py	2011-03-18 04:16:34 +0000
@@ -158,7 +158,7 @@
     enable_only = ['overview', ]
 
 
-class SprintView(HasSpecificationsView, LaunchpadView):
+class SprintView(HasSpecificationsView):
 
     implements(IMajorHeadingView)
 

=== added file 'lib/lp/blueprints/browser/tests/test_sprint.py'
--- lib/lp/blueprints/browser/tests/test_sprint.py	1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/browser/tests/test_sprint.py	2011-03-18 04:16:34 +0000
@@ -0,0 +1,37 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for Sprint pages and views."""
+
+__metaclass__ = type
+
+from storm.locals import Store
+from testtools.matchers import LessThan
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
+from lp.testing.views import create_initialized_view
+
+
+class TestSprintIndex(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_query_count(self):
+        sprint = self.factory.makeSprint()
+        for x in range(30):
+            sprint.attend(
+                self.factory.makePerson(),
+                sprint.time_starts,
+                sprint.time_ends,
+                True)
+        Store.of(sprint).flush()
+        Store.of(sprint).invalidate()
+        view = create_initialized_view(sprint, '+index')
+        with StormStatementRecorder() as recorder:
+            view.render()
+        self.assertThat(recorder, HasQueryCount(LessThan(10)))

=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2010-11-26 13:43:34 +0000
+++ lib/lp/blueprints/model/sprint.py	2011-03-18 04:16:34 +0000
@@ -13,9 +13,9 @@
 
 from sqlobject import (
     ForeignKey,
-    SQLRelatedJoin,
     StringCol,
     )
+from storm.locals import Store
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -45,7 +45,10 @@
 from lp.blueprints.model.specification import HasSpecificationsMixin
 from lp.blueprints.model.sprintattendance import SprintAttendance
 from lp.blueprints.model.sprintspecification import SprintSpecification
-from lp.registry.interfaces.person import validate_public_person
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    validate_public_person,
+    )
 from lp.registry.model.hasdrivers import HasDriversMixin
 
 
@@ -96,10 +99,10 @@
             return [self.driver, self.owner]
         return [self.owner]
 
-    # useful joins
-    attendees = SQLRelatedJoin('Person',
-        joinColumn='sprint', otherColumn='attendee',
-        intermediateTable='SprintAttendance', orderBy='name')
+    @property
+    def attendees(self):
+        # Only really used in tests.
+        return [a.attendee for a in self.attendances]
 
     def spec_filter_clause(self, filter=None):
         """Figure out the appropriate query for specifications on a sprint.
@@ -281,30 +284,39 @@
     # attendance
     def attend(self, person, time_starts, time_ends, is_physical):
         """See `ISprint`."""
-        # first see if a relevant attendance exists, and if so, update it
-        for attendance in self.attendances:
-            if attendance.attendee.id == person.id:
-                attendance.time_starts = time_starts
-                attendance.time_ends = time_ends
-                attendance.is_physical = is_physical
-                return attendance
-        # since no previous attendance existed, create a new one
+        # First see if a relevant attendance exists, and if so, update it.
+        attendance = Store.of(self).find(
+            SprintAttendance,
+            SprintAttendance.sprint == self,
+            SprintAttendance.attendee == person).one()
+        if attendance is not None:
+            attendance.time_starts = time_starts
+            attendance.time_ends = time_ends
+            attendance.is_physical = is_physical
+            return attendance
+        # Since no previous attendance existed, create a new one.
         return SprintAttendance(
             sprint=self, attendee=person, is_physical=is_physical,
             time_starts=time_starts, time_ends=time_ends)
 
     def removeAttendance(self, person):
         """See `ISprint`."""
-        for attendance in self.attendances:
-            if attendance.attendee.id == person.id:
-                attendance.destroySelf()
-                return
+        Store.of(self).find(
+            SprintAttendance,
+            SprintAttendance.sprint == self,
+            SprintAttendance.attendee == person).remove()
 
     @property
     def attendances(self):
-        ret = SprintAttendance.selectBy(sprint=self)
-        return sorted(ret.prejoin(['attendee']),
-                      key=lambda a: a.attendee.name)
+        result = list(Store.of(self).find(
+            SprintAttendance,
+            SprintAttendance.sprint == self))
+        people = [a.attendee_id for a in result]
+        # In order to populate the person cache we need to materialize the
+        # result set.  Listification should do.
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                people, need_validity=True))
+        return sorted(result, key=lambda a: a.attendee.name)
 
     # linking to specifications
     def linkSpecification(self, spec):

=== modified file 'lib/lp/blueprints/model/sprintattendance.py'
--- lib/lp/blueprints/model/sprintattendance.py	2010-08-20 20:31:18 +0000
+++ lib/lp/blueprints/model/sprintattendance.py	2011-03-18 04:16:34 +0000
@@ -7,30 +7,43 @@
 
 __all__ = ['SprintAttendance']
 
-from sqlobject import (
-    BoolCol,
-    ForeignKey,
+from storm.locals import (
+    Bool,
+    Int,
+    Reference,
+    Store,
     )
 from zope.interface import implements
 
 from canonical.database.datetimecol import UtcDateTimeCol
-from canonical.database.sqlbase import SQLBase
 from lp.blueprints.interfaces.sprintattendance import ISprintAttendance
 from lp.registry.interfaces.person import validate_public_person
-
-
-class SprintAttendance(SQLBase):
+from lp.services.database.stormbase import StormBase
+
+
+class SprintAttendance(StormBase):
     """A record of the attendance of a person at a sprint."""
 
     implements(ISprintAttendance)
 
-    _table = 'SprintAttendance'
-
-    sprint = ForeignKey(dbName='sprint', foreignKey='Sprint',
-        notNull=True)
-    attendee = ForeignKey(
-        dbName='attendee', foreignKey='Person',
-        storm_validator=validate_public_person, notNull=True)
+    __storm_table__ = 'SprintAttendance'
+
+    id = Int(primary=True)
+
+    sprint_id = Int(name='sprint')
+    sprint = Reference(sprint_id, 'Sprint.id')
+
+    attendee_id = Int(name='attendee', validator=validate_public_person)
+    attendee = Reference(attendee_id, 'Person.id')
+
     time_starts = UtcDateTimeCol(notNull=True)
     time_ends = UtcDateTimeCol(notNull=True)
-    is_physical = BoolCol(dbName='is_physical', notNull=True, default=True)
+    is_physical = Bool(default=True)
+
+    def __init__(self, sprint, attendee, is_physical,
+                 time_starts, time_ends):
+        self.sprint = sprint
+        self.attendee = attendee
+        self.is_physical = is_physical
+        self.time_starts = time_starts
+        self.time_ends = time_ends

=== modified file 'lib/lp/blueprints/templates/sprint-portlet-attendees.pt'
--- lib/lp/blueprints/templates/sprint-portlet-attendees.pt	2009-11-10 16:18:54 +0000
+++ lib/lp/blueprints/templates/sprint-portlet-attendees.pt	2011-03-18 04:16:34 +0000
@@ -8,10 +8,11 @@
 
   <h2>Attendees</h2>
 
-  <div class="portletBody portletContent">
+  <div class="portletBody portletContent"
+       tal:define="attendances context/attendances">
 
     <ul>
-      <li tal:repeat="attendance context/attendances">
+      <li tal:repeat="attendance attendances">
         <a tal:replace="structure attendance/attendee/fmt:link">Foo Bar</a><br />
         <div class="discreet">
            <span tal:replace="python:view.formatDate(attendance.time_starts)">
@@ -21,7 +22,7 @@
         </div>
       </li>
     </ul>
-    <p tal:condition="not: context/attendees">
+    <p tal:condition="not: attendances">
       <img src="/@@/info" />
       <i>No attendees yet registered.</i>
     </p>