launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-834303 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #834303 in Launchpad itself: "Archive:+subscriptions times out with many subscribers"
  https://bugs.launchpad.net/launchpad/+bug/834303

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-834303/+merge/90304

This branch fixes bug 834303: Archive:+subscriptions times out with
many subscribers.

The cause of the timeout is Storm needing really much time to load
15000 or 20000 ArchiveSubscriber records.

As lifeless noted in a bug comment, the fix is obvious: To show the
data in batches.

This leads to another problem, noted y Anthony Lenton in another comment
on the bug: The data was sorted by the creation time of a subscription,
while an important use case is to change the subscription status of
a subscriber. Looking them up by sifting through more than 100 pages
to look up a subscriber by name would be really painful.

The fix is obvious: Sort by name. Anthony suggested to use the display
name; after an IRC chat with him I switched instead to sorting by LP
user name. This allows us to use StormRangeFactory, which uses the
sort value of the first/last displayed result row als the "batch value"
in URLs. This allows to manually edit a URL to quickly look up the
subscription status of a given user. Not as nice as a decent search
form, but at least a convenient work-around, as long as we don't have
a search form and the related result page...

In theory, we could use Person.displayname for batching with
StormRangeFactory too, but the related URLs would look even more
insane than the URLs for sorting by LP user name: We would need
Person.id as an additional sort parameter, and the sort value
-- which appears in the URL -- may contain spaces, and when you
change a URL manually, you should be aware of the meaning of the
second sort parameter, Person.id. To compare the URLs:

sort by Person.name:

https://launchpad.dev/~cprov/+archive/p3a/+subscriptions?batch=5&memo=[%22stevea%22]&start=5

sort by Person.displayname, Person.id

https://launchpad.dev/~cprov/+archive/p3a/+subscriptions?batch=5&memo=[%22Steve+Alexander%22,1234]&start=5

tests:

./bin/test soyuz -vvt test_archivesubscriptionview
./bin/test soyuz -vvt archivesubscriber.txt

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/bug-834303/+merge/90304
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-834303 into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/archivesubscription.py'
--- lib/lp/soyuz/browser/archivesubscription.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/browser/archivesubscription.py	2012-01-26 18:22:19 +0000
@@ -43,6 +43,10 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.services.fields import PersonChoice
 from lp.services.propertycache import cachedproperty
+from lp.services.webapp.batching import (
+    BatchNavigator,
+    StormRangeFactory,
+    )
 from lp.services.webapp.publisher import (
     canonical_url,
     LaunchpadView,
@@ -147,21 +151,28 @@
             return
 
         super(ArchiveSubscribersView, self).initialize()
+        subscription_set = getUtility(IArchiveSubscriberSet)
+        self.subscriptions = subscription_set.getByArchive(self.context)
+        self.batchnav = BatchNavigator(
+            self.subscriptions, self.request,
+            range_factory=StormRangeFactory(self.subscriptions))
 
     @cachedproperty
-    def subscriptions(self):
-        """Return all the subscriptions for this archive."""
-        result = list(getUtility(IArchiveSubscriberSet
-            ).getByArchive(self.context))
-        ids = set(map(attrgetter('subscriber_id'), result))
+    def current_subscriptions_batch(self):
+        """Return the subscriptions of the current batch.
+
+        Bulk loads the related Person records.
+        """
+        batch = list(self.batchnav.currentBatch())
+        ids = set(map(attrgetter('subscriber_id'), batch))
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(ids,
-            need_validity=True))
-        return result
+           need_validity=True))
+        return batch
 
     @cachedproperty
     def has_subscriptions(self):
         """Return whether this archive has any subscribers."""
-        return bool(self.subscriptions)
+        return self.subscriptions.any() is not None
 
     def validate_new_subscription(self, action, data):
         """Ensure the subscriber isn't already subscribed.

=== modified file 'lib/lp/soyuz/doc/archivesubscriber.txt'
--- lib/lp/soyuz/doc/archivesubscriber.txt	2012-01-15 13:32:27 +0000
+++ lib/lp/soyuz/doc/archivesubscriber.txt	2012-01-26 18:22:19 +0000
@@ -277,11 +277,14 @@
     PPA named p3a for Celso Providelo      2009-02-26
     PPA named p3a for Mark Shuttleworth    2009-02-22
 
+getByArchive() sorts by subscriber name.
+
     >>> for subscription in sub_set.getByArchive(mark_private_ppa):
+    ...     print subscription.subscriber.name
     ...     print subscription.subscriber.displayname
     ...     print subscription.date_created.date()
-    Team Cprov      2009-02-24
-    Joe Smith       2009-02-22
+    joesmith            Joe Smith       2009-02-22
+    team-name-100000    Team Cprov      2009-02-24
 
 If we cancel one of the subscriptions:
 

=== modified file 'lib/lp/soyuz/model/archivesubscriber.py'
--- lib/lp/soyuz/model/archivesubscriber.py	2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/model/archivesubscriber.py	2012-01-26 18:22:19 +0000
@@ -9,6 +9,7 @@
     'ArchiveSubscriber',
     ]
 
+from operator import itemgetter
 import pytz
 from storm.expr import (
     And,
@@ -31,6 +32,7 @@
 from lp.registry.interfaces.person import validate_person
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.constants import UTC_NOW
+from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.enumcol import DBEnum
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.identity.model.emailaddress import EmailAddress
@@ -209,13 +211,17 @@
         """See `IArchiveSubscriberSet`."""
         # Grab the extra Storm expressions, for this query,
         # depending on the params:
+        # avoid circular imports.
+        from lp.registry.model.person import Person
         extra_exprs = self._getExprsForSubscriptionQueries(
             archive, current_only)
 
         store = Store.of(archive)
-        return store.find(
-            ArchiveSubscriber,
-            *extra_exprs).order_by(Desc(ArchiveSubscriber.date_created))
+        result = store.using(ArchiveSubscriber,
+             Join(Person, ArchiveSubscriber.subscriber_id == Person.id)).find(
+            (ArchiveSubscriber, Person),
+            *extra_exprs).order_by(Person.name)
+        return DecoratedResultSet(result, itemgetter(0))
 
     def _getExprsForSubscriptionQueries(self, archive=None,
                                         current_only=True):

=== modified file 'lib/lp/soyuz/templates/archive-subscribers.pt'
--- lib/lp/soyuz/templates/archive-subscribers.pt	2012-01-19 22:14:01 +0000
+++ lib/lp/soyuz/templates/archive-subscribers.pt	2012-01-26 18:22:19 +0000
@@ -37,6 +37,8 @@
           enctype="multipart/form-data"
           accept-charset="UTF-8">
 
+        <tal:navigation_top
+           replace="structure view/batchnav/@@+navigation-links-upper" />
         <table tal:attributes="summary string:Subscribers for ${context/displayname};"
                id="archive-subscribers" class="listing">
           <thead>
@@ -78,7 +80,7 @@
               </td>
             </tr>
             <tr class="archive_subscriber_row"
-                tal:repeat="subscription view/subscriptions">
+                tal:repeat="subscription view/current_subscriptions_batch">
               <td tal:content="structure subscription/subscriber/fmt:link" />
               <td tal:content="subscription/date_expires/fmt:date" />
               <td tal:content="subscription/description" />
@@ -90,6 +92,8 @@
            </tr>
           </tbody>
         </table>
+        <tal:navigation_bottom
+          replace="structure view/batchnav/@@+navigation-links-lower" />
         </form>
       </div><!-- class="portlet" -->
       <script type="text/javascript" id="setup-archivesubscribers-index">

=== added file 'lib/lp/soyuz/tests/test_archivesubscriptionview.py'
--- lib/lp/soyuz/tests/test_archivesubscriptionview.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_archivesubscriptionview.py	2012-01-26 18:22:19 +0000
@@ -0,0 +1,49 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for ArchiveSubscribersView."""
+
+__metaclass__ = type
+
+from soupmatchers import (
+    HTMLContains,
+    Tag,
+    )
+from zope.component import getUtility
+
+from lp.registry.interfaces.person import IPersonSet
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.views import create_initialized_view
+
+
+class TestArchiveSubscribersView(TestCaseWithFactory):
+    """Tests for ArchiveSubscribersView."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestArchiveSubscribersView, self).setUp()
+        self.p3a_owner = self.factory.makePerson()
+        admin = getUtility(IPersonSet).getByEmail('admin@xxxxxxxxxxxxx')
+        with person_logged_in(admin):
+            self.private_ppa = self.factory.makeArchive(
+                owner=self.p3a_owner, private=True, name='p3a')
+        with person_logged_in(self.p3a_owner):
+            for count in range(3):
+                subscriber = self.factory.makePerson()
+                self.private_ppa.newSubscription(subscriber, self.p3a_owner)
+
+    def test_has_batch_navigation(self):
+        # The page has the usual batch navigation links.
+        with person_logged_in(self.p3a_owner):
+            view = create_initialized_view(
+                self.private_ppa, '+subscriptions', principal=self.p3a_owner)
+            html = view.render()
+        has_batch_navigation = HTMLContains(
+            Tag('batch navigation links', 'td',
+                attrs={'class': 'batch-navigation-links'}, count=2))
+        self.assertThat(html, has_batch_navigation)