Merge lp:~wgrant/launchpad/bug-1201984 into lp:launchpad

[William Grant]

Filter out disabled PPAs in ArchiveSet.getPPAsForUser, so we don't have to check launchpad.Append on destination archive vocabs. This gets the archive widgets down to constant queries.

The target archive widget on +copy-packages had a launchpad.Append check added as part of the fix for bug #367796. This was inherited by the SourcePackageRecipe destination archive widget, so both widgets check launchpad.Append on every archive the user might be able to upload to.

But calculating launchpad.Append on an Archive directly is expensive, and can't be easily preloaded. This causes pages with a target archive widget to timeout for users who can upload to lots of PPAs. So I've fixed ArchiveSet.getPPAsForUser to directly exclude disabled archives, making it a sufficiently close approximation of launchpad.Append that we no longer need to filter on that permission. getPPAsForUser is only used by target widgets, so this won't change anything else.

[Celso Providelo (cprov)]

Okay, as discussed on IRC, the definitive permission check is already performed in the asynchronous copy job. We will filter out disabled PPAs just to rule out *impossible* destination PPAs.

It's seems fair to avoid multiple O(n) queries when rendering the widget.

review: Approve

=== modified file 'lib/lp/code/vocabularies/sourcepackagerecipe.py'
--- lib/lp/code/vocabularies/sourcepackagerecipe.py	2013-08-22 01:26:23 +0000
+++ lib/lp/code/vocabularies/sourcepackagerecipe.py	2014-06-30 14:11:01 +0000
@@ -16,7 +16,6 @@
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.model.distroseries import DistroSeries
-from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.sorting import sorted_dotted_numbers
 from lp.services.webapp.vocabulary import (
@@ -59,6 +58,5 @@
 
 def target_ppas_vocabulary(context):
     """Return a vocabulary of ppas that the current user can target."""
-    ppas = getUtility(IArchiveSet).getPPAsForUser(getUtility(ILaunchBag).user)
     return make_archive_vocabulary(
-        ppa for ppa in ppas if check_permission('launchpad.Append', ppa))
+        getUtility(IArchiveSet).getPPAsForUser(getUtility(ILaunchBag).user))

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2014-06-16 10:01:59 +0000
+++ lib/lp/soyuz/browser/archive.py	2014-06-30 14:11:01 +0000
@@ -1433,10 +1433,7 @@
     @cachedproperty
     def ppas_for_user(self):
         """Return all PPAs for which the user accessing the page can copy."""
-        return list(
-            ppa
-            for ppa in getUtility(IArchiveSet).getPPAsForUser(self.user)
-            if check_permission('launchpad.Append', ppa))
+        return list(getUtility(IArchiveSet).getPPAsForUser(self.user))
 
     @cachedproperty
     def can_copy(self):

=== added file 'lib/lp/soyuz/browser/tests/test_archive.py'
--- lib/lp/soyuz/browser/tests/test_archive.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/browser/tests/test_archive.py	2014-06-30 14:11:01 +0000
@@ -0,0 +1,39 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from testtools.matchers import Equals
+
+from lp.testing import (
+    admin_logged_in,
+    login_person,
+    record_two_runs,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
+from lp.testing.views import create_initialized_view
+
+
+class TestArchiveCopyPackagesView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_query_count(self):
+        person = self.factory.makePerson()
+        source = self.factory.makeArchive()
+
+        def create_targets():
+            self.factory.makeArchive(
+                owner=self.factory.makeTeam(members=[person]))
+            archive = self.factory.makeArchive()
+            with admin_logged_in():
+                archive.newComponentUploader(person, 'main')
+        nb_objects = 2
+        login_person(person)
+        recorder1, recorder2 = record_two_runs(
+            lambda: create_initialized_view(
+                source, '+copy-packages', user=person),
+            create_targets, nb_objects)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2014-04-23 14:24:15 +0000
+++ lib/lp/soyuz/model/archive.py	2014-06-30 14:11:01 +0000
@@ -2301,12 +2301,14 @@
 
     def getPPAsForUser(self, user):
         """See `IArchiveSet`."""
+        from lp.registry.model.person import Person
         # If there's no user logged in, then there's no archives.
         if user is None:
             return []
         store = Store.of(user)
         direct_membership = store.find(
             Archive,
+            Archive._enabled == True,
             Archive.purpose == ArchivePurpose.PPA,
             TeamParticipation.team == Archive.ownerID,
             TeamParticipation.person == user,
@@ -2322,7 +2324,10 @@
         result = direct_membership.union(third_party_upload_acl)
         result.order_by(Archive.displayname)
 
-        return result
+        def preload_owners(rows):
+            load_related(Person, rows, ['ownerID'])
+
+        return DecoratedResultSet(result, pre_iter_hook=preload_owners)
 
     def getPPAsPendingSigningKey(self):
         """See `IArchiveSet`."""