launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/code/browser/sourcepackagerecipelisting.py'
> --- lib/lp/code/browser/sourcepackagerecipelisting.py	2011-12-24 17:49:30 +0000
> +++ lib/lp/code/browser/sourcepackagerecipelisting.py	2015-02-24 13:55:06 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
> +# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Base class view for sourcepackagerecipe listings."""
> @@ -13,6 +13,7 @@
>      ]
>  
>  
> +from lp.code.browser.decorations import DecoratedBranch
>  from lp.services.feeds.browser import FeedsMixin
>  from lp.services.webapp import (
>      canonical_url,
> @@ -57,6 +58,13 @@
>  
>      branch_enabled = False
>  
> +    def initialize(self):
> +        super(BranchRecipeListingView, self).initialize()
> +        # Replace our context with a decorated branch, if it is not already
> +        # decorated.
> +        if not isinstance(self.context, DecoratedBranch):
> +            self.context = DecoratedBranch(self.context)
> +
>  
>  class PersonRecipeListingView(RecipeListingView):
>  
> 
> === modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py'
> --- lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py	2014-06-10 16:13:03 +0000
> +++ lib/lp/code/browser/tests/test_sourcepackagerecipelisting.py	2015-02-24 13:55:06 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2010-2014 Canonical Ltd.  This software is licensed under the
> +# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Tests for sourcepackagerecipe listings."""
> @@ -6,8 +6,15 @@
>  __metaclass__ = type
>  
>  
> -from lp.testing import BrowserTestCase
> +from testtools.matchers import Equals
> +
> +from lp.testing import (
> +    BrowserTestCase,
> +    person_logged_in,
> +    record_two_runs,
> +    )
>  from lp.testing.layers import DatabaseFunctionalLayer
> +from lp.testing.matchers import HasQueryCount
>  
>  
>  class TestSourcePackageRecipeListing(BrowserTestCase):
> @@ -15,7 +22,7 @@
>      layer = DatabaseFunctionalLayer
>  
>      def test_project_branch_recipe_listing(self):
> -        # We can see recipes for the product. We need to create two, since
> +        # We can see recipes for the project. We need to create two, since
>          # only one will redirect to that recipe.
>          branch = self.factory.makeProductBranch()
>          recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
> @@ -37,3 +44,49 @@
>              Source Package Recipes for lp:.*
>              Name             Owner       Registered
>              spr-name.*       Person-name""", text)
> +
> +    def test_branch_query_count(self):
> +        # The number of queries required to render the list of all recipes
> +        # for a branch is constant in the number of owners and recipes.
> +        person = self.factory.makePerson()
> +        branch = self.factory.makeProductBranch(owner=person)
> +
> +        def create_recipe():
> +            with person_logged_in(person):
> +                self.factory.makeSourcePackageRecipe(branches=[branch])
> +
> +        recorder1, recorder2 = record_two_runs(
> +            lambda: self.getMainText(branch, "+recipes"), create_recipe, 5)
> +        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
> +
> +    def test_project_query_count(self):
> +        # The number of queries required to render the list of all recipes
> +        # for a project is constant in the number of owners, branches, and
> +        # recipes.
> +        person = self.factory.makePerson()
> +        project = self.factory.makeProduct(owner=person)
> +
> +        def create_recipe():
> +            with person_logged_in(person):
> +                branch = self.factory.makeProductBranch(product=project)
> +                self.factory.makeSourcePackageRecipe(branches=[branch])
> +
> +        recorder1, recorder2 = record_two_runs(
> +            lambda: self.getMainText(project, "+recipes"), create_recipe, 5)
> +        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
> +
> +    def test_person_query_count(self):
> +        # The number of queries required to render the list of all recipes
> +        # for a person is constant in the number of projects, branches, and
> +        # recipes.
> +        person = self.factory.makePerson()
> +
> +        def create_recipe():
> +            with person_logged_in(person):
> +                branch = self.factory.makeProductBranch(owner=person)
> +                self.factory.makeSourcePackageRecipe(
> +                    owner=person, branches=[branch])
> +
> +        recorder1, recorder2 = record_two_runs(
> +            lambda: self.getMainText(person, "+recipes"), create_recipe, 5)
> +        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
> 
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py	2015-02-16 13:01:34 +0000
> +++ lib/lp/code/model/branch.py	2015-02-24 13:55:06 +0000
> @@ -1406,9 +1406,13 @@
>      @property
>      def recipes(self):
>          """See `IHasRecipes`."""
> +        from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
>          from lp.code.model.sourcepackagerecipedata import (
> -            SourcePackageRecipeData)
> -        return SourcePackageRecipeData.findRecipes(self)
> +            SourcePackageRecipeData,
> +            )
> +        hook = SourcePackageRecipe.preLoadDataForSourcePackageRecipes
> +        return DecoratedResultSet(
> +            SourcePackageRecipeData.findRecipes(self), pre_iter_hook=hook)
>  
>      merge_queue_id = Int(name='merge_queue', allow_none=True)
>      merge_queue = Reference(merge_queue_id, 'BranchMergeQueue.id')
> 
> === modified file 'lib/lp/code/model/sourcepackagerecipe.py'
> --- lib/lp/code/model/sourcepackagerecipe.py	2014-07-09 03:08:57 +0000
> +++ lib/lp/code/model/sourcepackagerecipe.py	2015-02-24 13:55:06 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Implementation of the `SourcePackageRecipe` content type."""
> @@ -12,6 +12,7 @@
>      datetime,
>      timedelta,
>      )
> +from operator import attrgetter
>  
>  from lazr.delegates import delegates
>  from pytz import utc
> @@ -49,16 +50,13 @@
>  from lp.code.interfaces.sourcepackagerecipebuild import (
>      ISourcePackageRecipeBuildSource,
>      )
> -from lp.code.model.branch import Branch
>  from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
>  from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
>  from lp.code.vocabularies.sourcepackagerecipe import BuildableDistroSeries
> +from lp.registry.interfaces.person import IPersonSet
>  from lp.registry.interfaces.pocket import PackagePublishingPocket
>  from lp.registry.model.distroseries import DistroSeries
> -from lp.services.database.bulk import (
> -    load_referencing,
> -    load_related,
> -    )
> +from lp.services.database.bulk import load_referencing
>  from lp.services.database.constants import (
>      DEFAULT,
>      UTC_NOW,
> @@ -166,14 +164,15 @@
>          spr_datas = load_referencing(
>              SourcePackageRecipeData,
>              sourcepackagerecipes, ['sourcepackage_recipe_id'])
> -        # Load the related branches.
> -        load_related(Branch, spr_datas, ['base_branch_id'])
>          # Store the SourcePackageRecipeData in the sourcepackagerecipes
>          # objects.
>          for spr_data in spr_datas:
>              cache = get_property_cache(spr_data.sourcepackage_recipe)
>              cache._recipe_data = spr_data
>          SourcePackageRecipeData.preLoadReferencedBranches(spr_datas)
> +        owner_ids = set(map(attrgetter('owner_id'), sourcepackagerecipes))
> +        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
> +            owner_ids, need_validity=True))
>  
>      def setRecipeText(self, recipe_text):
>          parsed = SourcePackageRecipeData.getParsedRecipe(recipe_text)
> 
> === modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
> --- lib/lp/code/model/sourcepackagerecipedata.py	2013-06-20 05:50:00 +0000
> +++ lib/lp/code/model/sourcepackagerecipedata.py	2015-02-24 13:55:06 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Implementation of the recipe storage.
> @@ -46,6 +46,8 @@
>      )
>  from lp.code.interfaces.branchlookup import IBranchLookup
>  from lp.code.model.branch import Branch
> +from lp.registry.model.distroseries import DistroSeries
> +from lp.registry.model.sourcepackagename import SourcePackageName
>  from lp.services.database.bulk import (
>      load_referencing,
>      load_related,
> @@ -323,14 +325,23 @@
>  
>      @staticmethod
>      def preLoadReferencedBranches(sourcepackagerecipedatas):
> +        # Circular imports.
> +        from lp.code.model.branchcollection import GenericBranchCollection
> +        from lp.registry.model.product import Product
>          # Load the related Branch, _SourcePackageRecipeDataInstruction.
> -        load_related(
> +        base_branches = load_related(
>              Branch, sourcepackagerecipedatas, ['base_branch_id'])
>          sprd_instructions = load_referencing(
>              _SourcePackageRecipeDataInstruction,
>              sourcepackagerecipedatas, ['recipe_data_id'])
>          sub_branches = load_related(
>              Branch, sprd_instructions, ['branch_id'])
> +        all_branches = base_branches + sub_branches
> +        # Pre-load branches' data.
> +        load_related(SourcePackageName, all_branches, ['sourcepackagenameID'])
> +        load_related(DistroSeries, all_branches, ['distroseriesID'])
> +        load_related(Product, all_branches, ['productID'])

Do these three perhaps belong in preloadDataForBranches?

> +        GenericBranchCollection.preloadDataForBranches(all_branches)
>          # Store the pre-fetched objects on the sourcepackagerecipedatas
>          # objects.
>          branch_to_recipe_data = dict([
> 
> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py	2015-02-06 15:29:45 +0000
> +++ lib/lp/registry/model/person.py	2015-02-24 13:55:06 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Implementation classes for a Person."""
> @@ -3158,10 +3158,11 @@
>      def recipes(self):
>          """See `IHasRecipes`."""
>          from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
> -        store = Store.of(self)
> -        return store.find(
> +        recipes = Store.of(self).find(
>              SourcePackageRecipe,
>              SourcePackageRecipe.owner == self)
> +        hook = SourcePackageRecipe.preLoadDataForSourcePackageRecipes
> +        return DecoratedResultSet(recipes, pre_iter_hook=hook)
>  
>      def canAccess(self, obj, attribute):
>          """See `IPerson.`"""
> 
> === modified file 'lib/lp/registry/model/product.py'
> --- lib/lp/registry/model/product.py	2015-02-13 17:16:22 +0000
> +++ lib/lp/registry/model/product.py	2015-02-24 13:55:06 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Database classes including and related to Product."""
> @@ -1520,12 +1520,14 @@
>      @property
>      def recipes(self):
>          """See `IHasRecipes`."""
> -        return Store.of(self).find(
> +        recipes = Store.of(self).find(
>              SourcePackageRecipe,
>              SourcePackageRecipe.id ==
>                  SourcePackageRecipeData.sourcepackage_recipe_id,
>              SourcePackageRecipeData.base_branch == Branch.id,
>              Branch.product == self)
> +        hook = SourcePackageRecipe.preLoadDataForSourcePackageRecipes
> +        return DecoratedResultSet(recipes, pre_iter_hook=hook)
>  
>      def getBugTaskWeightFunction(self):
>          """Provide a weight function to determine optimal bug task.
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/recipe-timeouts/+merge/250750
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.