launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/repeated-subscription-queries into lp:launchpad.

Commit message:
Avoid unnecessarily-duplicated BranchSubscription/GitSubscription queries.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/repeated-subscription-queries/+merge/354614
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/repeated-subscription-queries into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py	2015-09-28 17:38:45 +0000
+++ lib/lp/code/browser/branchsubscription.py	2018-09-10 14:22:33 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -42,18 +42,18 @@
         # Cache permissions so private subscribers can be rendered.
         # The security adaptor will do the job also but we don't want or need
         # the expense of running several complex SQL queries.
-        person_ids = [sub.personID for sub in self.context.subscriptions]
+        subscriptions = list(self.context.subscriptions)
+        person_ids = [sub.personID for sub in subscriptions]
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
             person_ids, need_validity=True))
         if self.user is not None:
             subscribers = [
-                subscription.person
-                for subscription in self.context.subscriptions]
+                subscription.person for subscription in subscriptions]
             precache_permission_for_objects(
                 self.request, "launchpad.LimitedView", subscribers)
 
         visible_subscriptions = [
-            subscription for subscription in self.context.subscriptions
+            subscription for subscription in subscriptions
             if check_permission('launchpad.LimitedView', subscription.person)]
         return sorted(
             visible_subscriptions,

=== modified file 'lib/lp/code/browser/gitsubscription.py'
--- lib/lp/code/browser/gitsubscription.py	2015-09-28 17:38:45 +0000
+++ lib/lp/code/browser/gitsubscription.py	2018-09-10 14:22:33 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -42,18 +42,18 @@
         # Cache permissions so private subscribers can be rendered.
         # The security adaptor will do the job also but we don't want or
         # need the expense of running several complex SQL queries.
-        person_ids = [sub.person_id for sub in self.context.subscriptions]
+        subscriptions = list(self.context.subscriptions)
+        person_ids = [sub.person_id for sub in subscriptions]
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
             person_ids, need_validity=True))
         if self.user is not None:
             subscribers = [
-                subscription.person
-                for subscription in self.context.subscriptions]
+                subscription.person for subscription in subscriptions]
             precache_permission_for_objects(
                 self.request, "launchpad.LimitedView", subscribers)
 
         visible_subscriptions = [
-            subscription for subscription in self.context.subscriptions
+            subscription for subscription in subscriptions
             if check_permission("launchpad.LimitedView", subscription.person)]
         return sorted(
             visible_subscriptions,

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2018-09-07 13:43:50 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2018-09-10 14:22:33 +0000
@@ -598,7 +598,7 @@
             branch, '+branch-portlet-subscriber-content')
         with StormStatementRecorder() as recorder:
             view.render()
-        self.assertThat(recorder, HasQueryCount(Equals(9)))
+        self.assertThat(recorder, HasQueryCount(Equals(6)))
 
     def test_query_count_index_with_subscribers(self):
         branch = self.factory.makeBranch()

=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
--- lib/lp/code/browser/tests/test_gitrepository.py	2018-09-06 16:00:45 +0000
+++ lib/lp/code/browser/tests/test_gitrepository.py	2018-09-10 14:22:33 +0000
@@ -13,7 +13,11 @@
 
 from fixtures import FakeLogger
 import pytz
-from testtools.matchers import DocTestMatches
+from storm.store import Store
+from testtools.matchers import (
+    DocTestMatches,
+    Equals,
+    )
 import transaction
 from zope.component import getUtility
 from zope.formlib.itemswidgets import ItemDisplayWidget
@@ -48,6 +52,7 @@
     logout,
     person_logged_in,
     record_two_runs,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
@@ -375,6 +380,18 @@
                 self.assertIsNone(
                     find_tag_by_id(browser.contents, 'landing-targets'))
 
+    def test_query_count_subscriber_content(self):
+        repository = self.factory.makeGitRepository()
+        for _ in range(10):
+            self.factory.makeGitSubscription(repository=repository)
+        Store.of(repository).flush()
+        Store.of(repository).invalidate()
+        view = create_initialized_view(
+            repository, "+repository-portlet-subscriber-content")
+        with StormStatementRecorder() as recorder:
+            view.render()
+        self.assertThat(recorder, HasQueryCount(Equals(6)))
+
 
 class TestGitRepositoryViewPrivateArtifacts(BrowserTestCase):
     """Tests that Git repositories with private team artifacts can be viewed.