launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1201984 into lp:launchpad.

Commit message:
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.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1201984 in Launchpad itself: "SourcePackageRecipe:+index and Archive:+copy-packages time out when there are many potential target PPAs"
  https://bugs.launchpad.net/launchpad/+bug/1201984

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1201984/+merge/225011

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.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1201984/+merge/225011
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1201984 into lp:launchpad.

=== 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`."""