launchpad-reviewers team mailing list archive

[Edwin Grubbs <edwin.grubbs@xxxxxxxxxxxxx>]

Edwin Grubbs has proposed merging lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Summary
-------

This branch is dependent on the milestone_sort_key() db function that
was recently landed on db-devel. The milestones/releases for a series
are now batched. I also limited the number of milestones/releases
displayed in the timeline graph.


Implementation details
----------------------

Added batching:
    lib/lp/registry/browser/distroseries.py
    lib/lp/registry/browser/productseries.py
    lib/lp/registry/browser/tests/test_distroseries_views.py
    lib/lp/registry/browser/tests/test_productseries_views.py
    lib/lp/registry/model/milestone.py
    lib/lp/registry/templates/productseries-table-releases.pt

Limited the number of milestones/releases displayed in the timeline
graph.
    lib/lp/registry/interfaces/product.py
    lib/lp/registry/interfaces/productseries.py
    lib/lp/registry/model/productseries.py
    lib/lp/registry/tests/test_product.py

The cache TALes needs the third argument to ensure that it retrieves the
right cached value for a block in a loop.
    lib/lp/registry/templates/productseries-milestone-table-row.pt

Tests
-----

$ ./bin/test --list-tests -vv -t 'test_(distro|product)series_views|registry\.tests\.test_product\.'

Demo and Q/A
------------

* Open http://launchpad.dev/firefox/trunk
-- 
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout-part2/+merge/34159
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/distroseries.py	2010-08-31 06:01:08 +0000
@@ -360,6 +360,10 @@
 
     milestone_can_release = False
 
+    @cachedproperty
+    def milestone_batch_navigator(self):
+        return BatchNavigator(self.context.all_milestones, self.request)
+
 
 class DistroSeriesEditView(LaunchpadEditFormView, SeriesStatusMixin):
     """View class that lets you edit a DistroSeries object.

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2010-08-24 19:06:33 +0000
+++ lib/lp/registry/browser/productseries.py	2010-08-31 06:01:08 +0000
@@ -59,6 +59,7 @@
 from canonical.launchpad import _
 from canonical.launchpad.helpers import browserLanguages
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.webapp.batching import BatchNavigator
 from canonical.launchpad.webapp import (
     ApplicationMenu,
     canonical_url,
@@ -475,13 +476,17 @@
                 for status in sorted(status_counts,
                                      key=attrgetter('sortkey'))]
 
-    @property
+    @cachedproperty
     def latest_release_with_download_files(self):
         for release in self.context.releases:
             if len(list(release.files)) > 0:
                 return release
         return None
 
+    @cachedproperty
+    def milestone_batch_navigator(self):
+        return BatchNavigator(self.context.all_milestones, self.request)
+
 
 class ProductSeriesDetailedDisplayView(ProductSeriesView):
 

=== modified file 'lib/lp/registry/browser/tests/test_distroseries_views.py'
--- lib/lp/registry/browser/tests/test_distroseries_views.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/tests/test_distroseries_views.py	2010-08-31 06:01:08 +0000
@@ -3,13 +3,13 @@
 
 __metaclass__ = type
 
-import unittest
-
 from storm.zope.interfaces import IResultSet
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.config import config
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.webapp.batching import BatchNavigator
 from canonical.testing import LaunchpadZopelessLayer
 from lp.testing import TestCaseWithFactory
 from lp.testing.views import create_initialized_view
@@ -43,6 +43,26 @@
         view = create_initialized_view(distroseries, '+index')
         self.assertEqual(view.needs_linking, None)
 
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+    def test_milestone_batch_navigator(self):
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
+        for name in ('a', 'b', 'c', 'd'):
+            distroseries.newMilestone(name)
+        view = create_initialized_view(distroseries, name='+index')
+        config.push('default-batch-size', """
+        [launchpad]
+        default_batch_size: 2
+        """)
+        self.assert_(
+            isinstance(view.milestone_batch_navigator, BatchNavigator),
+            'milestone_batch_navigator is not a BatchNavigator object: %r'
+            % view.milestone_batch_navigator)
+        self.assertEqual(4, view.milestone_batch_navigator.batch.total())
+        expected = [
+            '0.9.2',
+            '0.9.1',
+            ]
+        milestone_names = [
+            item.name
+            for item in view.milestone_batch_navigator.currentBatch()]
+        config.pop('default-batch-size')

=== added file 'lib/lp/registry/browser/tests/test_productseries_views.py'
--- lib/lp/registry/browser/tests/test_productseries_views.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_productseries_views.py	2010-08-31 06:01:08 +0000
@@ -0,0 +1,40 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from canonical.config import config
+from canonical.launchpad.webapp.batching import BatchNavigator
+from canonical.testing import LaunchpadZopelessLayer
+from lp.testing import TestCaseWithFactory
+from lp.testing.views import create_initialized_view
+
+
+class TestProductSeriesView(TestCaseWithFactory):
+    """Test the product series +index view."""
+
+    layer = LaunchpadZopelessLayer
+
+    def test_milestone_batch_navigator(self):
+        product = self.factory.makeProduct()
+        for name in ('a', 'b', 'c', 'd'):
+            product.development_focus.newMilestone(name)
+        view = create_initialized_view(
+            product.development_focus, name='+index')
+        config.push('default-batch-size', """
+        [launchpad]
+        default_batch_size: 2
+        """)
+        self.assert_(
+            isinstance(view.milestone_batch_navigator, BatchNavigator),
+            'milestone_batch_navigator is not a BatchNavigator object: %r'
+            % view.milestone_batch_navigator)
+        self.assertEqual(4, view.milestone_batch_navigator.batch.total())
+        expected = [
+            '0.9.2',
+            '0.9.1',
+            ]
+        milestone_names = [
+            item.name
+            for item in view.milestone_batch_navigator.currentBatch()]
+        config.pop('default-batch-size')

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2010-08-26 23:54:09 +0000
+++ lib/lp/registry/interfaces/product.py	2010-08-31 06:01:08 +0000
@@ -786,7 +786,10 @@
     @export_read_operation()
     @export_operation_as('get_timeline')
     def getTimeline(include_inactive):
-        """Return basic timeline data useful for creating a diagram."""
+        """Return basic timeline data useful for creating a diagram.
+
+        The number of milestones returned per series is limited.
+        """
 
 
 class IProduct(

=== modified file 'lib/lp/registry/interfaces/productseries.py'
--- lib/lp/registry/interfaces/productseries.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/interfaces/productseries.py	2010-08-31 06:01:08 +0000
@@ -308,7 +308,10 @@
     @export_read_operation()
     @export_operation_as('get_timeline')
     def getTimeline(include_inactive):
-        """Return basic timeline data useful for creating a diagram."""
+        """Return basic timeline data useful for creating a diagram.
+
+        The number of milestones returned is limited.
+        """
 
 
 class IProductSeries(IProductSeriesEditRestricted, IProductSeriesPublic,

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2010-08-24 19:06:33 +0000
+++ lib/lp/registry/model/milestone.py	2010-08-31 06:01:08 +0000
@@ -97,7 +97,7 @@
 
     @property
     def has_milestones(self):
-        return self.all_milestones.any() is not None
+        return not self.all_milestones.is_empty()
 
     @property
     def all_milestones(self):
@@ -293,7 +293,10 @@
     def __init__(self, target, name, dateexpected, active):
         self.name = name
         self.code_name = None
-        self.id = None
+        # The id is necessary for generating a unique memcache key
+        # in a page template loop. The ProjectMilestone.id is passed
+        # in as the third argument to the "cache" TALes.
+        self.id = 'ProjectGroup:%s/Milestone:%s' % (target.name, name)
         self.code_name = None
         self.product = None
         self.distribution = None

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/productseries.py	2010-08-31 06:01:08 +0000
@@ -97,6 +97,9 @@
     )
 
 
+MAX_TIMELINE_MILESTONES = 20
+
+
 def landmark_key(landmark):
     """Sorts landmarks by date and name."""
     if landmark['date'] is None:
@@ -543,7 +546,7 @@
 
     def getTimeline(self, include_inactive=False):
         landmarks = []
-        for milestone in self.all_milestones:
+        for milestone in self.all_milestones[:MAX_TIMELINE_MILESTONES]:
             if milestone.product_release is None:
                 # Skip inactive milestones, but include releases,
                 # even if include_inactive is False.

=== modified file 'lib/lp/registry/templates/productseries-milestone-table-row.pt'
--- lib/lp/registry/templates/productseries-milestone-table-row.pt	2010-05-27 04:10:17 +0000
+++ lib/lp/registry/templates/productseries-milestone-table-row.pt	2010-08-31 06:01:08 +0000
@@ -5,7 +5,7 @@
   define="milestone_menu view/milestone/menu:overview;
           milestone view/milestone;
           release view/release"
-  tal:content="cache:private, 1 hour">
+  tal:content="cache:private, 1 hour, milestone/id">
   <tr>
     <td>
       <img src="/@@/milestone" alt="" />

=== modified file 'lib/lp/registry/templates/productseries-table-releases.pt'
--- lib/lp/registry/templates/productseries-table-releases.pt	2010-05-17 20:09:03 +0000
+++ lib/lp/registry/templates/productseries-table-releases.pt	2010-08-31 06:01:08 +0000
@@ -2,8 +2,10 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal";
   xmlns:metal="http://xml.zope.org/namespaces/metal";
   xmlns:i18n="http://xml.zope.org/namespaces/i18n";
-  omit-tag="">
+  tal:content="cache:anonymous, 1 hour">
 
+<tal:navigation_top
+      replace="structure view/milestone_batch_navigator/@@+navigation-links-upper" />
 <table class="listing"
   tal:attributes="id context/name/fmt:css-id/series-;
                   class view/milestone_table_class">
@@ -16,9 +18,11 @@
     </tr>
   </thead>
   <tbody id="milestone-rows">
-    <tal:row repeat="milestone context/all_milestones">
+    <tal:row repeat="milestone view/milestone_batch_navigator/currentBatch">
       <tal:milestone replace="structure milestone/@@+productseries-table-row" />
     </tal:row>
   </tbody>
 </table>
+<tal:navigation_bottom
+      replace="structure view/milestone_batch_navigator/@@+navigation-links-lower" />
 </tal:root>

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2010-08-23 16:05:36 +0000
+++ lib/lp/registry/tests/test_product.py	2010-08-31 06:01:08 +0000
@@ -92,6 +92,50 @@
             expected,
             list(product.getMilestonesAndReleases()))
 
+    def test_getTimeline_limit(self):
+        # Only 20 milestones/releases per series should be included in the
+        # getTimeline() results. The results are sorted by
+        # descending dateexpected and name, so the presumed latest
+        # milestones should be included.
+        product = self.factory.makeProduct(name='foo')
+        for i in range(25):
+            milestone_list = self.factory.makeMilestone(
+                product=product,
+                productseries=product.development_focus,
+                name=str(i))
+
+        # 0 through 4 should not be in the list.
+        expected_milestones = [
+            '/foo/+milestone/24',
+            '/foo/+milestone/23',
+            '/foo/+milestone/22',
+            '/foo/+milestone/21',
+            '/foo/+milestone/20',
+            '/foo/+milestone/19',
+            '/foo/+milestone/18',
+            '/foo/+milestone/17',
+            '/foo/+milestone/16',
+            '/foo/+milestone/15',
+            '/foo/+milestone/14',
+            '/foo/+milestone/13',
+            '/foo/+milestone/12',
+            '/foo/+milestone/11',
+            '/foo/+milestone/10',
+            '/foo/+milestone/9',
+            '/foo/+milestone/8',
+            '/foo/+milestone/7',
+            '/foo/+milestone/6',
+            '/foo/+milestone/5',
+            ]
+
+        [series] = product.getTimeline()
+        timeline_milestones = [
+            landmark['uri']
+            for landmark in series['landmarks']]
+        self.assertEqual(
+            expected_milestones,
+            timeline_milestones)
+
 
 class TestProductFiles(unittest.TestCase):
     """Tests for downloadable product files."""