launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/precache-specification-linked-branches into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/precache-specification-linked-branches/+merge/145544

Turn ISpecification.linked_branches into a cachedproperty list, and preload it inside search_specifications().

Clean up some whitespace and other such gubbins to try and claw back some LoC.
-- 
https://code.launchpad.net/~stevenk/launchpad/precache-specification-linked-branches/+merge/145544
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/precache-specification-linked-branches into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2012-12-11 01:53:50 +0000
+++ lib/lp/app/browser/tales.py	2013-01-30 05:39:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation of the lp: htmlform: fmt: namespaces in TALES."""
@@ -1052,7 +1052,7 @@
 
         badges = ''
 
-        if self._context.linked_branches.count() > 0:
+        if len(self._context.linked_branches) > 0:
             badges += self.icon_template % (
                 "branch", "Branch is available", "sprite branch")
 

=== modified file 'lib/lp/blueprints/browser/specification.py'
--- lib/lp/blueprints/browser/specification.py	2013-01-25 03:30:08 +0000
+++ lib/lp/blueprints/browser/specification.py	2013-01-30 05:39:21 +0000
@@ -625,7 +625,7 @@
 
     @enabled_with_permission('launchpad.AnyPerson')
     def linkbranch(self):
-        if self.context.linked_branches.count() > 0:
+        if len(self.context.linked_branches) > 0:
             text = 'Link to another branch'
         else:
             text = 'Link a related branch'

=== modified file 'lib/lp/blueprints/browser/tests/test_specification.py'
--- lib/lp/blueprints/browser/tests/test_specification.py	2012-12-10 13:43:47 +0000
+++ lib/lp/blueprints/browser/tests/test_specification.py	2013-01-30 05:39:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -12,6 +12,7 @@
 from lazr.restful.interfaces import IJSONRequestCache
 import pytz
 import soupmatchers
+from storm.store import Store
 from testtools.matchers import (
     Equals,
     Not,
@@ -35,10 +36,7 @@
     )
 from lp.registry.enums import SpecificationSharingPolicy
 from lp.registry.interfaces.person import PersonVisibility
-from lp.registry.interfaces.product import (
-    IProduct,
-    IProductSeries,
-    )
+from lp.registry.interfaces.product import IProductSeries
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.escaping import html_escape
 from lp.services.webapp.interaction import ANONYMOUS
@@ -50,10 +48,14 @@
     login_person,
     logout,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.matchers import DocTestMatches
+from lp.testing.matchers import (
+    DocTestMatches,
+    HasQueryCount,
+    )
 from lp.testing.pages import (
     extract_text,
     find_tag_by_id,
@@ -243,19 +245,29 @@
         with person_logged_in(spec_owner):
             removeSecurityProxy(spec.target)._ensurePolicies(
                 [InformationType.PROPRIETARY])
-            spec.transitionToInformationType(InformationType.PROPRIETARY,
-                                             spec.owner)
+            spec.transitionToInformationType(
+                InformationType.PROPRIETARY, spec.owner)
         browser = self.getViewBrowser(specs)
         self.assertNotIn('Not allowed', browser.contents)
         self.assertNotIn(spec_name, browser.contents)
 
+    def test_query_count(self):
+        product = self.factory.makeProduct()
+        removeSecurityProxy(product).official_blueprints = True
+        specs = [
+            self.factory.makeSpecification(product=product) for i in range(5)]
+        Store.of(specs[0]).invalidate()
+        with StormStatementRecorder() as recorder:
+            self.getViewBrowser(product, rootsite='blueprints')
+        self.assertThat(recorder, HasQueryCount(Equals(35)))
+        
 
 class TestSpecificationInformationType(BrowserTestCase):
 
     layer = DatabaseFunctionalLayer
 
-    portlet_tag = soupmatchers.Tag('info-type-portlet', True,
-                                   attrs=dict(id='information-type-summary'))
+    portlet_tag = soupmatchers.Tag(
+        'info-type-portlet', True, attrs=dict(id='information-type-summary'))
 
     def setUp(self):
         super(TestSpecificationInformationType, self).setUp()
@@ -290,8 +302,8 @@
             browser = self.getViewBrowser(spec, user=owner)
         privacy_banner = soupmatchers.Tag('privacy-banner', True,
                 attrs={'class': 'private_banner_container'})
-        self.assertThat(browser.contents,
-                        soupmatchers.HTMLContains(privacy_banner))
+        self.assertThat(
+            browser.contents, soupmatchers.HTMLContains(privacy_banner))
 
     def set_secrecy(self, spec, owner, information_type='PROPRIETARY'):
         form = {
@@ -317,8 +329,8 @@
             [InformationType.PROPRIETARY])
         self.set_secrecy(spec, owner)
         with person_logged_in(owner):
-            self.assertEqual(InformationType.PROPRIETARY,
-                             spec.information_type)
+            self.assertEqual(
+                InformationType.PROPRIETARY, spec.information_type)
 
     def test_secrecy_change_nonsense(self):
         """Invalid values produce sane errors."""
@@ -363,11 +375,11 @@
               ignore_permissions=True)
 
         browser = self.getViewBrowser(spec, '+index', user=owner)
-        self.assertThat(browser.contents,
-                        soupmatchers.HTMLContains(privacy_banner))
+        self.assertThat(
+            browser.contents, soupmatchers.HTMLContains(privacy_banner))
         browser = self.getViewBrowser(spec, '+subscribe', user=owner)
-        self.assertThat(browser.contents,
-                        soupmatchers.HTMLContains(privacy_banner))
+        self.assertThat(
+            browser.contents, soupmatchers.HTMLContains(privacy_banner))
 
 
 # canonical_url erroneously returns http://blueprints.launchpad.dev/+new
@@ -408,8 +420,7 @@
         # specifications.
         view = self.createInitializedView()
         self.assertEqual(
-            InformationType.PUBLIC,
-            view.initial_values['information_type'])
+            InformationType.PUBLIC, view.initial_values['information_type'])
 
 
 class TestNewSpecificationFromRootView(TestCaseWithFactory,
@@ -450,9 +461,7 @@
         form = self._create_form_data(product.name)
         form['field.information_type'] = 'PROPRIETARY'
         self._assert_information_type_validation_error(
-            sprint,
-            form,
-            sprint.owner)
+            sprint, form, sprint.owner)
 
 
 class TestNewSpecificationFromProjectView(TestCaseWithFactory,
@@ -472,9 +481,7 @@
         form = self._create_form_data(product.name)
         form['field.information_type'] = 'PROPRIETARY'
         self._assert_information_type_validation_error(
-            project,
-            form,
-            project.owner)
+            project, form, project.owner)
 
 
 class TestNewSpecificationFromProductView(TestCaseWithFactory,
@@ -495,8 +502,7 @@
         # among the allowed types.
         view = self.createInitializedView()
         self.assertEqual(
-            InformationType.EMBARGOED,
-            view.initial_values['information_type'])
+            InformationType.EMBARGOED, view.initial_values['information_type'])
 
 
 class TestNewSpecificationFromDistributionView(TestCaseWithFactory,
@@ -518,8 +524,8 @@
     def setUp(self):
         super(TestNewSpecificationInformationType, self).setUp()
         set_blueprint_information_type(self, True)
-        it_field = soupmatchers.Tag('it-field', True,
-                                    attrs=dict(name='field.information_type'))
+        it_field = soupmatchers.Tag(
+            'it-field', True, attrs=dict(name='field.information_type'))
         self.match_it = soupmatchers.HTMLContains(it_field)
 
     def test_from_root(self):
@@ -629,8 +635,8 @@
 
     def _setUp(self):
         set_blueprint_information_type(self, True)
-        it_field = soupmatchers.Tag('it-field', True,
-                                    attrs=dict(name='field.information_type'))
+        it_field = soupmatchers.Tag(
+            'it-field', True, attrs=dict(name='field.information_type'))
         self.match_it = soupmatchers.HTMLContains(it_field)
 
     def makeTarget(self, policy, owner=None):
@@ -642,11 +648,9 @@
         Useful because we need to follow to product from
         ProductSeries to get to _ensurePolicies.
         """
-        if IProduct.providedBy(target):
-            removeSecurityProxy(target)._ensurePolicies(information_type)
-        elif IProductSeries.providedBy(target):
-            removeSecurityProxy(target.product)._ensurePolicies(
-                information_type)
+        if IProductSeries.providedBy(target):
+            target = target.product
+        removeSecurityProxy(target)._ensurePolicies(information_type)
 
     def getSpecification(self, target, name):
         """Helper to get the specification.

=== modified file 'lib/lp/blueprints/doc/specification-branch.txt'
--- lib/lp/blueprints/doc/specification-branch.txt	2012-12-26 01:32:19 +0000
+++ lib/lp/blueprints/doc/specification-branch.txt	2013-01-30 05:39:21 +0000
@@ -56,10 +56,10 @@
     >>> branchlink.branch == branch
     True
 
-Finally, the branch link can be removed using its destroySelf()
+Finally, the branch link can be removed using the unlinkBranch()
 method:
 
-    >>> branchlink.destroySelf()
+    >>> spec.unlinkBranch(branch, user)
     >>> for branchlink in spec.linked_branches:
     ...     print branchlink.branch.unique_name
     ~name12/+junk/junk.dev

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2013-01-29 02:16:13 +0000
+++ lib/lp/blueprints/model/specification.py	2013-01-30 05:39:21 +0000
@@ -244,9 +244,6 @@
     bugs = SQLRelatedJoin('Bug',
         joinColumn='specification', otherColumn='bug',
         intermediateTable='SpecificationBug', orderBy='id')
-    linked_branches = SQLMultipleJoin('SpecificationBranch',
-        joinColumn='specification',
-        orderBy='id')
     spec_dependency_links = SQLMultipleJoin('SpecificationDependency',
         joinColumn='specification', orderBy='id')
 
@@ -256,6 +253,13 @@
     information_type = EnumCol(
         enum=InformationType, notNull=True, default=InformationType.PUBLIC)
 
+    @cachedproperty
+    def linked_branches(self):
+        return list(Store.of(self).find(
+            SpecificationBranch,
+            SpecificationBranch.specificationID == self.id).order_by(
+                SpecificationBranch.id))
+
     def _fetch_children_or_parents(self, join_cond, cond, user):
         from lp.blueprints.model.specificationsearch import (
             get_specification_privacy_filter)
@@ -857,12 +861,16 @@
             return branch_link
         branch_link = SpecificationBranch(
             specification=self, branch=branch, registrant=registrant)
+        Store.of(self).flush()
+        del get_property_cache(self).linked_branches
         notify(ObjectCreatedEvent(branch_link))
         return branch_link
 
     def unlinkBranch(self, branch, user):
         spec_branch = self.getBranchLink(branch)
         spec_branch.destroySelf()
+        Store.of(self).flush()
+        del get_property_cache(self).linked_branches
 
     def getLinkedBugTasks(self, user):
         """See `ISpecification`."""

=== modified file 'lib/lp/blueprints/model/specificationsearch.py'
--- lib/lp/blueprints/model/specificationsearch.py	2013-01-29 06:29:03 +0000
+++ lib/lp/blueprints/model/specificationsearch.py	2013-01-30 05:39:21 +0000
@@ -35,6 +35,7 @@
     SpecificationSort,
     )
 from lp.blueprints.model.specification import Specification
+from lp.blueprints.model.specificationbranch import SpecificationBranch
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import IPersonSet
@@ -42,6 +43,7 @@
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.model.teammembership import TeamParticipation
+from lp.services.database.bulk import load_referencing
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
 from lp.services.database.stormexpr import (
@@ -103,13 +105,13 @@
     else:
         order = [sort]
     # Set the _known_viewers property for each specification, as well as
-    # preloading the people involved, if asked.
+    # preloading the objects involved, if asked.
     decorators = []
     preload_hook = None
     if user is not None and not IPersonRoles(user).in_admin:
         decorators.append(_make_cache_user_can_view_spec(user))
     if prejoin_people:
-        preload_hook = _preload_specifications_related_people
+        preload_hook = _preload_specifications_related_objects
     results = store.using(*tables).find(
         Specification, *clauses).order_by(*order).config(limit=quantity)
     return DecoratedResultSet(
@@ -225,14 +227,20 @@
     return cache_user_can_view_spec
 
 
-def _preload_specifications_related_people(rows):
+def _preload_specifications_related_objects(rows):
     person_ids = set()
     for spec in rows:
         person_ids |= set(
             [spec._assigneeID, spec._approverID, spec._drafterID])
+        get_property_cache(spec).linked_branches = []
     person_ids -= set([None])
     list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
         person_ids, need_validity=True))
+    spec_branches = load_referencing(
+        SpecificationBranch, rows, ['specificationID'])
+    for sbranch in spec_branches:
+        get_property_cache(sbranch.specification).linked_branches.append(
+            sbranch)
 
 
 def get_specification_started_clause():