Launchpad itself

Merge lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2 into lp:launchpad/db-devel

  1. bug-663861-product-series-timeout-part2
  2. Merge into db-devel
Proposed by Edwin Grubbs
Status: Merged
Approved by: Edwin Grubbs
Approved revision: no longer in the source branch.
Merged at revision: 9939
Proposed branch: lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part1
Diff against target: 361 lines (+150/-33)
9 files modified
database/schema/trusted.sql (+6/-2)
lib/lp/registry/browser/configure.zcml (+1/-1)
lib/lp/registry/browser/product.py (+32/-16)
lib/lp/registry/browser/tests/product-files-views.txt (+3/-2)
lib/lp/registry/browser/tests/product-views.txt (+21/-0)
lib/lp/registry/interfaces/product.py (+10/-0)
lib/lp/registry/model/product.py (+25/-0)
lib/lp/registry/templates/product-series.pt (+21/-11)
lib/lp/registry/tests/test_product.py (+31/-1)
To merge this branch: bzr merge lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2
Reviewer Review Type Date Requested Status
Stuart Bishop (community) db Approve
Abel Deuring (community) code Approve
Robert Collins db Pending
Review via email: mp+39643@code.launchpad.net

Commit message

Batch results on $product/+series page to fix timeout.

Description of the change

Summary
-------

In a prerequisite branch, I added the functional index, but I
had to make a slight change to it to move the numbered versions
to the top, so I will also need a db review again.
The python sort method orders them as
[devfocus, 3, 2a, 2b, 1, a, b, c], but it makes more sense to order it
as [devfocus, 3, 2b, 2a, 1, c, b, a].

The branch fixes a timeout on the $product/+series page by batching the
results.

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

When sorting descending, move the numbered versions above the letters.
    database/schema/trusted.sql

Changed the product.ProductSeriesView to ProductSeriesSetView in order
to avoid confusion with productseries.ProductSeriesView.
    lib/lp/registry/browser/configure.zcml

Added the getVersionSortedSeries() method in the model and added the
ProductSeriesSetView.batched_series attribute that batches the results.
The template needs the css_class attribute in the decorated series, but
I didn't want to worry about SeriesWithReleases.releases not being
populated, even though I don't think the template uses it. Therefore, I
split css_class into the DecoratedSeries class.
    lib/lp/registry/browser/product.py
    lib/lp/registry/templates/product-series.pt
    lib/lp/registry/browser/tests/product-views.txt
    lib/lp/registry/interfaces/product.py
    lib/lp/registry/model/product.py
    lib/lp/registry/tests/test_product.py

Fixed lint error and removed reference to the series attribute inside
the decorated series, since the `name` can be retrieved from the
decorated series.
    lib/lp/registry/browser/tests/product-files-views.txt

Tests
-----

./bin/test -vv -t 'test_product|product-views|product-files-views'

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

* Open http://qastaging.launchpad.net/obsolete-junk/+series
  * It should not timeout, and the results should be batched.
    (The timeline graph might still timeout.)

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Ediwn,

a nice branch! I just wondered if it makes sense to add comment to substitue_filled_numbers() explaining that '~' > c is always true, where c is any character that can appear in a series name.

review: Approve (code)
Revision history for this message
Stuart Bishop (stub) wrote :

Fine.

There will be no problems with the change provided it is landed at the same time as the main database patch.

review: Approve (db)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'database/schema/trusted.sql'
--- database/schema/trusted.sql 2010-10-29 17:28:51 +0000
+++ database/schema/trusted.sql 2010-10-29 17:28:52 +0000
@@ -1826,10 +1826,14 @@
1826 [version] = args1826 [version] = args
18271827
1828 def substitute_filled_numbers(match):1828 def substitute_filled_numbers(match):
1829 return match.group(0).zfill(5)1829 # Prepend "~" so that version numbers will show up first
1830 # when sorted descending, i.e. [3, 2c, 2b, 1, c, b, a] instead
1831 # of [c, b, a, 3, 2c, 2b, 1]. "~" has the highest ASCII value
1832 # of visible ASCII characters.
1833 return '~' + match.group(0).zfill(5)
18301834
1831 return re.sub(u'\d+', substitute_filled_numbers, version)1835 return re.sub(u'\d+', substitute_filled_numbers, version)
1832$$;1836$$;
18331837
1834COMMENT ON FUNCTION version_sort_key(text) IS1838COMMENT ON FUNCTION version_sort_key(text) IS
1835'Sort a field as version numbers that do not necessarily conform to debian package versions (For example, when "2-2" should be considered greater than "1:1"). debversion_sort_key() should be used for debian versions.';1839'Sort a field as version numbers that do not necessarily conform to debian package versions (For example, when "2-2" should be considered greater than "1:1"). debversion_sort_key() should be used for debian versions. Numbers will be sorted after letters unlike typical ASCII, so that a descending sort will put the latest version number that starts with a number instead of a letter will be at the top. E.g. ascending is [a, z, 1, 9] and descending is [9, 1, z, a].';
18361840
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml 2010-10-21 03:22:06 +0000
+++ lib/lp/registry/browser/configure.zcml 2010-10-29 17:28:52 +0000
@@ -1473,7 +1473,7 @@
1473 template="../templates/product-portlet-packages.pt"/>1473 template="../templates/product-portlet-packages.pt"/>
1474 <browser:page1474 <browser:page
1475 for="lp.registry.interfaces.product.IProduct"1475 for="lp.registry.interfaces.product.IProduct"
1476 class="lp.registry.browser.product.ProductSeriesView"1476 class="lp.registry.browser.product.ProductSeriesSetView"
1477 name="+series"1477 name="+series"
1478 facet="overview"1478 facet="overview"
1479 permission="zope.Public"1479 permission="zope.Public"
14801480
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py 2010-10-20 14:58:01 +0000
+++ lib/lp/registry/browser/product.py 2010-10-29 17:28:52 +0000
@@ -29,7 +29,7 @@
29 'ProductPackagesPortletView',29 'ProductPackagesPortletView',
30 'ProductRdfView',30 'ProductRdfView',
31 'ProductReviewLicenseView',31 'ProductReviewLicenseView',
32 'ProductSeriesView',32 'ProductSeriesSetView',
33 'ProductSetBreadcrumb',33 'ProductSetBreadcrumb',
34 'ProductSetFacets',34 'ProductSetFacets',
35 'ProductSetNavigation',35 'ProductSetNavigation',
@@ -88,6 +88,9 @@
88 MultiStepView,88 MultiStepView,
89 StepView,89 StepView,
90 )90 )
91from canonical.launchpad.components.decoratedresultset import (
92 DecoratedResultSet,
93 )
91from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities94from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
92from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet95from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
93from canonical.launchpad.mail import (96from canonical.launchpad.mail import (
@@ -123,7 +126,6 @@
123 safe_action,126 safe_action,
124 )127 )
125from canonical.launchpad.webapp.menu import NavigationMenu128from canonical.launchpad.webapp.menu import NavigationMenu
126from lp.app.browser.tales import MenuAPI
127from canonical.widgets.date import DateWidget129from canonical.widgets.date import DateWidget
128from canonical.widgets.itemswidgets import (130from canonical.widgets.itemswidgets import (
129 CheckBoxMatrixWidget,131 CheckBoxMatrixWidget,
@@ -142,6 +144,7 @@
142 QuestionTargetFacetMixin,144 QuestionTargetFacetMixin,
143 QuestionTargetTraversalMixin,145 QuestionTargetTraversalMixin,
144 )146 )
147from lp.app.browser.tales import MenuAPI
145from lp.app.enums import ServiceUsage148from lp.app.enums import ServiceUsage
146from lp.app.errors import NotFoundError149from lp.app.errors import NotFoundError
147from lp.app.interfaces.headings import IEditableContextTitle150from lp.app.interfaces.headings import IEditableContextTitle
@@ -793,7 +796,25 @@
793 self.release_by_id[release.id] = release_delegate796 self.release_by_id[release.id] = release_delegate
794797
795798
796class SeriesWithReleases:799class DecoratedSeries:
800 """A decorated series that includes helper attributes for templates."""
801 delegates(IProductSeries, 'series')
802
803 def __init__(self, series):
804 self.series = series
805
806 @property
807 def css_class(self):
808 """The highlighted, unhighlighted, or dimmed CSS class."""
809 if self.is_development_focus:
810 return 'highlighted'
811 elif self.status == SeriesStatus.OBSOLETE:
812 return 'dimmed'
813 else:
814 return 'unhighlighted'
815
816
817class SeriesWithReleases(DecoratedSeries):
797 """A decorated series that includes releases.818 """A decorated series that includes releases.
798819
799 The extra data is included in this class to avoid repeated820 The extra data is included in this class to avoid repeated
@@ -807,10 +828,9 @@
807 # raise an AttributeError for self.parent.828 # raise an AttributeError for self.parent.
808 parent = None829 parent = None
809 releases = None830 releases = None
810 delegates(IProductSeries, 'series')
811831
812 def __init__(self, series, parent):832 def __init__(self, series, parent):
813 self.series = series833 super(SeriesWithReleases, self).__init__(series)
814 self.parent = parent834 self.parent = parent
815 self.releases = []835 self.releases = []
816836
@@ -824,16 +844,6 @@
824 return True844 return True
825 return False845 return False
826846
827 @property
828 def css_class(self):
829 """The highlighted, unhighlighted, or dimmed CSS class."""
830 if self.is_development_focus:
831 return 'highlighted'
832 elif self.status == SeriesStatus.OBSOLETE:
833 return 'dimmed'
834 else:
835 return 'unhighlighted'
836
837847
838class ReleaseWithFiles:848class ReleaseWithFiles:
839 """A decorated release that includes product release files.849 """A decorated release that includes product release files.
@@ -1707,12 +1717,18 @@
1707 return canonical_url(self.context)1717 return canonical_url(self.context)
17081718
17091719
1710class ProductSeriesView(ProductView):1720class ProductSeriesSetView(ProductView):
1711 """A view for showing a product's series."""1721 """A view for showing a product's series."""
17121722
1713 label = 'timeline'1723 label = 'timeline'
1714 page_title = label1724 page_title = label
17151725
1726 @cachedproperty
1727 def batched_series(self):
1728 decorated_result = DecoratedResultSet(
1729 self.context.getVersionSortedSeries(), DecoratedSeries)
1730 return BatchNavigator(decorated_result, self.request)
1731
17161732
1717class ProductRdfView(BaseRdfView):1733class ProductRdfView(BaseRdfView):
1718 """A view that sets its mime-type to application/rdf+xml"""1734 """A view that sets its mime-type to application/rdf+xml"""
17191735
=== modified file 'lib/lp/registry/browser/tests/product-files-views.txt'
--- lib/lp/registry/browser/tests/product-files-views.txt 2010-08-03 14:51:31 +0000
+++ lib/lp/registry/browser/tests/product-files-views.txt 2010-10-29 17:28:52 +0000
@@ -21,7 +21,7 @@
2121
22 >>> def print_series_release(sr):22 >>> def print_series_release(sr):
23 ... print "%s from the %s series" % (sr.release.name_with_codename,23 ... print "%s from the %s series" % (sr.release.name_with_codename,
24 ... sr.series.series.name)24 ... sr.series.name)
2525
26 >>> for sr in batch:26 >>> for sr in batch:
27 ... print_series_release(sr)27 ... print_series_release(sr)
@@ -35,7 +35,8 @@
35 ... milestone = factory.makeMilestone(productseries=productseries,35 ... milestone = factory.makeMilestone(productseries=productseries,
36 ... name="%d.%d"%(i,j))36 ... name="%d.%d"%(i,j))
37 ... release_file = factory.makeProductReleaseFile(37 ... release_file = factory.makeProductReleaseFile(
38 ... product=product, productseries=productseries, milestone=milestone)38 ... product=product, productseries=productseries,
39 ... milestone=milestone)
39 >>> view = create_initialized_view(product, '+download')40 >>> view = create_initialized_view(product, '+download')
40 >>> batch = view.series_and_releases_batch.currentBatch()41 >>> batch = view.series_and_releases_batch.currentBatch()
41 >>> print len(batch)42 >>> print len(batch)
4243
=== modified file 'lib/lp/registry/browser/tests/product-views.txt'
--- lib/lp/registry/browser/tests/product-views.txt 2010-10-18 22:24:59 +0000
+++ lib/lp/registry/browser/tests/product-views.txt 2010-10-29 17:28:52 +0000
@@ -417,6 +417,27 @@
417 ftp://mozilla.org/firefox.*bz2417 ftp://mozilla.org/firefox.*bz2
418418
419419
420Viewing series for a product
421============================
422
423All the product series can be viewed in batches.
424
425 >>> product = factory.makeProduct()
426 >>> for name in ('stable', 'testing', '1.1', '1.2', 'extra'):
427 ... series = factory.makeProductSeries(product=product, name=name)
428 >>> view = create_view(product, name='+series')
429 >>> batch = view.batched_series.currentBatch()
430 >>> print batch.total()
431 6
432 >>> for series in batch:
433 ... print series.name
434 trunk
435 1.2
436 1.1
437 testing
438 stable
439
440
420Product index view441Product index view
421==================442==================
422443
423444
=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py 2010-09-21 09:37:06 +0000
+++ lib/lp/registry/interfaces/product.py 2010-10-29 17:28:52 +0000
@@ -733,6 +733,16 @@
733 "Some bug trackers host multiple projects at the same URL "733 "Some bug trackers host multiple projects at the same URL "
734 "and require an identifier for the specific project.")))734 "and require an identifier for the specific project.")))
735735
736 def getVersionSortedSeries(filter_obsolete=False):
737 """Return all the series sorted by the name field as a version.
738
739 The development focus field is an exception. It will always
740 be sorted first.
741
742 :param filter_obsolete: If true, do not include any series with
743 SeriesStatus.OBSOLETE in the results.
744 """
745
736 def redeemSubscriptionVoucher(voucher, registrant, purchaser,746 def redeemSubscriptionVoucher(voucher, registrant, purchaser,
737 subscription_months, whiteboard=None,747 subscription_months, whiteboard=None,
738 current_datetime=None):748 current_datetime=None):
739749
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py 2010-10-24 13:02:07 +0000
+++ lib/lp/registry/model/product.py 2010-10-29 17:28:52 +0000
@@ -137,6 +137,7 @@
137 License,137 License,
138 LicenseStatus,138 LicenseStatus,
139 )139 )
140from lp.registry.interfaces.series import SeriesStatus
140from lp.registry.model.announcement import MakesAnnouncements141from lp.registry.model.announcement import MakesAnnouncements
141from lp.registry.model.commercialsubscription import CommercialSubscription142from lp.registry.model.commercialsubscription import CommercialSubscription
142from lp.registry.model.distribution import Distribution143from lp.registry.model.distribution import Distribution
@@ -980,6 +981,30 @@
980 translatable_product_series,981 translatable_product_series,
981 key=operator.attrgetter('datecreated'))982 key=operator.attrgetter('datecreated'))
982983
984 def getVersionSortedSeries(self, filter_obsolete=False):
985 """See `IProduct`."""
986 store = Store.of(self)
987 dev_focus = store.find(
988 ProductSeries,
989 ProductSeries.id == self.development_focus.id)
990 other_series_conditions = [
991 ProductSeries.product == self,
992 ProductSeries.id != self.development_focus.id,
993 ]
994 if filter_obsolete is True:
995 other_series_conditions.append(
996 ProductSeries.status != SeriesStatus.OBSOLETE)
997 other_series = store.find(ProductSeries, other_series_conditions)
998 # The query will be much slower if the version_sort_key is not
999 # the first thing that is sorted, since it won't be able to use
1000 # the productseries_name_sort index.
1001 other_series.order_by(SQL('version_sort_key(name) DESC'))
1002 # UNION ALL must be used to preserve the sort order from the
1003 # separate queries. The sorting should not be done after
1004 # unioning the two queries, because that will prevent it from
1005 # being able to use the productseries_name_sort index.
1006 return dev_focus.union(other_series, all=True)
1007
983 @property1008 @property
984 def obsolete_translatable_series(self):1009 def obsolete_translatable_series(self):
985 """See `IProduct`."""1010 """See `IProduct`."""
9861011
=== modified file 'lib/lp/registry/templates/product-series.pt'
--- lib/lp/registry/templates/product-series.pt 2010-05-17 17:29:08 +0000
+++ lib/lp/registry/templates/product-series.pt 2010-10-29 17:28:52 +0000
@@ -21,17 +21,27 @@
21 </li>21 </li>
22 </ul>22 </ul>
2323
24 <tal:cache content="cache:public, 1 hour">24 <tal:series-list condition="view/batched_series/currentBatch">
25 <div tal:repeat="series view/sorted_series_list">25 <div class="lesser" id="active-top-navigation">
26 <div style="margin-top: 1em;26 <tal:navigation
27 border-bottom: 1px solid #ccc; max-width: 60em;"27 content="structure view/batched_series/@@+navigation-links-upper" />
28 tal:define="is_focus series/is_development_focus"28 </div>
29 tal:attributes="class string:${series/css_class} series;29 <div tal:repeat="series view/batched_series/currentBatch">
30 id series/name/fmt:css-id/series-;">30 <tal:cache content="cache:public, 1 hour, series/name">
31 <tal:status replace="structure series/series/@@+status" />31 <div style="margin-top: 1em;
32 </div>32 border-bottom: 1px solid #ccc; max-width: 60em;"
33 </div>33 tal:define="is_focus series/is_development_focus"
34 </tal:cache>34 tal:attributes="class string:${series/css_class} series;
35 id series/name/fmt:css-id/series-;">
36 <tal:status replace="structure series/series/@@+status" />
37 </div>
38 </tal:cache>
39 </div>
40 <div class="lesser">
41 <tal:navigation
42 content="structure view/batched_series/@@+navigation-links-lower" />
43 </div>
44 </tal:series-list>
35 </div>45 </div>
36</body>46</body>
37</html>47</html>
3848
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py 2010-10-04 19:50:45 +0000
+++ lib/lp/registry/tests/test_product.py 2010-10-29 17:28:52 +0000
@@ -27,8 +27,12 @@
27 IProduct,27 IProduct,
28 License,28 License,
29 )29 )
30from lp.registry.interfaces.series import SeriesStatus
30from lp.registry.model.commercialsubscription import CommercialSubscription31from lp.registry.model.commercialsubscription import CommercialSubscription
31from lp.registry.model.product import Product, UnDeactivateable32from lp.registry.model.product import (
33 Product,
34 UnDeactivateable,
35 )
32from lp.registry.model.productlicense import ProductLicense36from lp.registry.model.productlicense import ProductLicense
33from lp.testing import TestCaseWithFactory37from lp.testing import TestCaseWithFactory
3438
@@ -136,6 +140,32 @@
136 expected_milestones,140 expected_milestones,
137 timeline_milestones)141 timeline_milestones)
138142
143 def test_getVersionSortedSeries(self):
144 # The product series should be sorted with the development focus
145 # series first, the series starting with a number in descending
146 # order, and then the series starting with a letter in
147 # descending order.
148 product = self.factory.makeProduct()
149 for name in ('1', '2', '3', '3a', '3b', 'alpha', 'beta'):
150 self.factory.makeProductSeries(product=product, name=name)
151 self.assertEqual(
152 [u'trunk', u'3b', u'3a', u'3', u'2', u'1', u'beta', u'alpha'],
153 [series.name for series in product.getVersionSortedSeries()])
154
155 def test_getVersionSortedSeries_filter_obsolete(self):
156 # The obsolete series should not be included in the results if
157 # the filter_obsolete argument is set to True.
158 login('admin@canonical.com')
159 product = self.factory.makeProduct()
160 self.factory.makeProductSeries(product=product, name='active-series')
161 obsolete_series = self.factory.makeProductSeries(
162 product=product, name='obsolete-series')
163 obsolete_series.status = SeriesStatus.OBSOLETE
164 active_series = product.getVersionSortedSeries(filter_obsolete=True)
165 self.assertEqual(
166 [u'trunk', u'active-series'],
167 [series.name for series in active_series])
168
139169
140class TestProductFiles(unittest.TestCase):170class TestProductFiles(unittest.TestCase):
141 """Tests for downloadable product files."""171 """Tests for downloadable product files."""

Subscribers

People subscribed via source and target branches

to status/vote changes: