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
1=== modified file 'database/schema/trusted.sql'
2--- database/schema/trusted.sql 2010-10-29 17:28:51 +0000
3+++ database/schema/trusted.sql 2010-10-29 17:28:52 +0000
4@@ -1826,10 +1826,14 @@
5 [version] = args
6
7 def substitute_filled_numbers(match):
8- return match.group(0).zfill(5)
9+ # Prepend "~" so that version numbers will show up first
10+ # when sorted descending, i.e. [3, 2c, 2b, 1, c, b, a] instead
11+ # of [c, b, a, 3, 2c, 2b, 1]. "~" has the highest ASCII value
12+ # of visible ASCII characters.
13+ return '~' + match.group(0).zfill(5)
14
15 return re.sub(u'\d+', substitute_filled_numbers, version)
16 $$;
17
18 COMMENT ON FUNCTION version_sort_key(text) IS
19-'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.';
20+'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].';
21
22=== modified file 'lib/lp/registry/browser/configure.zcml'
23--- lib/lp/registry/browser/configure.zcml 2010-10-21 03:22:06 +0000
24+++ lib/lp/registry/browser/configure.zcml 2010-10-29 17:28:52 +0000
25@@ -1473,7 +1473,7 @@
26 template="../templates/product-portlet-packages.pt"/>
27 <browser:page
28 for="lp.registry.interfaces.product.IProduct"
29- class="lp.registry.browser.product.ProductSeriesView"
30+ class="lp.registry.browser.product.ProductSeriesSetView"
31 name="+series"
32 facet="overview"
33 permission="zope.Public"
34
35=== modified file 'lib/lp/registry/browser/product.py'
36--- lib/lp/registry/browser/product.py 2010-10-20 14:58:01 +0000
37+++ lib/lp/registry/browser/product.py 2010-10-29 17:28:52 +0000
38@@ -29,7 +29,7 @@
39 'ProductPackagesPortletView',
40 'ProductRdfView',
41 'ProductReviewLicenseView',
42- 'ProductSeriesView',
43+ 'ProductSeriesSetView',
44 'ProductSetBreadcrumb',
45 'ProductSetFacets',
46 'ProductSetNavigation',
47@@ -88,6 +88,9 @@
48 MultiStepView,
49 StepView,
50 )
51+from canonical.launchpad.components.decoratedresultset import (
52+ DecoratedResultSet,
53+ )
54 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
55 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
56 from canonical.launchpad.mail import (
57@@ -123,7 +126,6 @@
58 safe_action,
59 )
60 from canonical.launchpad.webapp.menu import NavigationMenu
61-from lp.app.browser.tales import MenuAPI
62 from canonical.widgets.date import DateWidget
63 from canonical.widgets.itemswidgets import (
64 CheckBoxMatrixWidget,
65@@ -142,6 +144,7 @@
66 QuestionTargetFacetMixin,
67 QuestionTargetTraversalMixin,
68 )
69+from lp.app.browser.tales import MenuAPI
70 from lp.app.enums import ServiceUsage
71 from lp.app.errors import NotFoundError
72 from lp.app.interfaces.headings import IEditableContextTitle
73@@ -793,7 +796,25 @@
74 self.release_by_id[release.id] = release_delegate
75
76
77-class SeriesWithReleases:
78+class DecoratedSeries:
79+ """A decorated series that includes helper attributes for templates."""
80+ delegates(IProductSeries, 'series')
81+
82+ def __init__(self, series):
83+ self.series = series
84+
85+ @property
86+ def css_class(self):
87+ """The highlighted, unhighlighted, or dimmed CSS class."""
88+ if self.is_development_focus:
89+ return 'highlighted'
90+ elif self.status == SeriesStatus.OBSOLETE:
91+ return 'dimmed'
92+ else:
93+ return 'unhighlighted'
94+
95+
96+class SeriesWithReleases(DecoratedSeries):
97 """A decorated series that includes releases.
98
99 The extra data is included in this class to avoid repeated
100@@ -807,10 +828,9 @@
101 # raise an AttributeError for self.parent.
102 parent = None
103 releases = None
104- delegates(IProductSeries, 'series')
105
106 def __init__(self, series, parent):
107- self.series = series
108+ super(SeriesWithReleases, self).__init__(series)
109 self.parent = parent
110 self.releases = []
111
112@@ -824,16 +844,6 @@
113 return True
114 return False
115
116- @property
117- def css_class(self):
118- """The highlighted, unhighlighted, or dimmed CSS class."""
119- if self.is_development_focus:
120- return 'highlighted'
121- elif self.status == SeriesStatus.OBSOLETE:
122- return 'dimmed'
123- else:
124- return 'unhighlighted'
125-
126
127 class ReleaseWithFiles:
128 """A decorated release that includes product release files.
129@@ -1707,12 +1717,18 @@
130 return canonical_url(self.context)
131
132
133-class ProductSeriesView(ProductView):
134+class ProductSeriesSetView(ProductView):
135 """A view for showing a product's series."""
136
137 label = 'timeline'
138 page_title = label
139
140+ @cachedproperty
141+ def batched_series(self):
142+ decorated_result = DecoratedResultSet(
143+ self.context.getVersionSortedSeries(), DecoratedSeries)
144+ return BatchNavigator(decorated_result, self.request)
145+
146
147 class ProductRdfView(BaseRdfView):
148 """A view that sets its mime-type to application/rdf+xml"""
149
150=== modified file 'lib/lp/registry/browser/tests/product-files-views.txt'
151--- lib/lp/registry/browser/tests/product-files-views.txt 2010-08-03 14:51:31 +0000
152+++ lib/lp/registry/browser/tests/product-files-views.txt 2010-10-29 17:28:52 +0000
153@@ -21,7 +21,7 @@
154
155 >>> def print_series_release(sr):
156 ... print "%s from the %s series" % (sr.release.name_with_codename,
157- ... sr.series.series.name)
158+ ... sr.series.name)
159
160 >>> for sr in batch:
161 ... print_series_release(sr)
162@@ -35,7 +35,8 @@
163 ... milestone = factory.makeMilestone(productseries=productseries,
164 ... name="%d.%d"%(i,j))
165 ... release_file = factory.makeProductReleaseFile(
166- ... product=product, productseries=productseries, milestone=milestone)
167+ ... product=product, productseries=productseries,
168+ ... milestone=milestone)
169 >>> view = create_initialized_view(product, '+download')
170 >>> batch = view.series_and_releases_batch.currentBatch()
171 >>> print len(batch)
172
173=== modified file 'lib/lp/registry/browser/tests/product-views.txt'
174--- lib/lp/registry/browser/tests/product-views.txt 2010-10-18 22:24:59 +0000
175+++ lib/lp/registry/browser/tests/product-views.txt 2010-10-29 17:28:52 +0000
176@@ -417,6 +417,27 @@
177 ftp://mozilla.org/firefox.*bz2
178
179
180+Viewing series for a product
181+============================
182+
183+All the product series can be viewed in batches.
184+
185+ >>> product = factory.makeProduct()
186+ >>> for name in ('stable', 'testing', '1.1', '1.2', 'extra'):
187+ ... series = factory.makeProductSeries(product=product, name=name)
188+ >>> view = create_view(product, name='+series')
189+ >>> batch = view.batched_series.currentBatch()
190+ >>> print batch.total()
191+ 6
192+ >>> for series in batch:
193+ ... print series.name
194+ trunk
195+ 1.2
196+ 1.1
197+ testing
198+ stable
199+
200+
201 Product index view
202 ==================
203
204
205=== modified file 'lib/lp/registry/interfaces/product.py'
206--- lib/lp/registry/interfaces/product.py 2010-09-21 09:37:06 +0000
207+++ lib/lp/registry/interfaces/product.py 2010-10-29 17:28:52 +0000
208@@ -733,6 +733,16 @@
209 "Some bug trackers host multiple projects at the same URL "
210 "and require an identifier for the specific project.")))
211
212+ def getVersionSortedSeries(filter_obsolete=False):
213+ """Return all the series sorted by the name field as a version.
214+
215+ The development focus field is an exception. It will always
216+ be sorted first.
217+
218+ :param filter_obsolete: If true, do not include any series with
219+ SeriesStatus.OBSOLETE in the results.
220+ """
221+
222 def redeemSubscriptionVoucher(voucher, registrant, purchaser,
223 subscription_months, whiteboard=None,
224 current_datetime=None):
225
226=== modified file 'lib/lp/registry/model/product.py'
227--- lib/lp/registry/model/product.py 2010-10-24 13:02:07 +0000
228+++ lib/lp/registry/model/product.py 2010-10-29 17:28:52 +0000
229@@ -137,6 +137,7 @@
230 License,
231 LicenseStatus,
232 )
233+from lp.registry.interfaces.series import SeriesStatus
234 from lp.registry.model.announcement import MakesAnnouncements
235 from lp.registry.model.commercialsubscription import CommercialSubscription
236 from lp.registry.model.distribution import Distribution
237@@ -980,6 +981,30 @@
238 translatable_product_series,
239 key=operator.attrgetter('datecreated'))
240
241+ def getVersionSortedSeries(self, filter_obsolete=False):
242+ """See `IProduct`."""
243+ store = Store.of(self)
244+ dev_focus = store.find(
245+ ProductSeries,
246+ ProductSeries.id == self.development_focus.id)
247+ other_series_conditions = [
248+ ProductSeries.product == self,
249+ ProductSeries.id != self.development_focus.id,
250+ ]
251+ if filter_obsolete is True:
252+ other_series_conditions.append(
253+ ProductSeries.status != SeriesStatus.OBSOLETE)
254+ other_series = store.find(ProductSeries, other_series_conditions)
255+ # The query will be much slower if the version_sort_key is not
256+ # the first thing that is sorted, since it won't be able to use
257+ # the productseries_name_sort index.
258+ other_series.order_by(SQL('version_sort_key(name) DESC'))
259+ # UNION ALL must be used to preserve the sort order from the
260+ # separate queries. The sorting should not be done after
261+ # unioning the two queries, because that will prevent it from
262+ # being able to use the productseries_name_sort index.
263+ return dev_focus.union(other_series, all=True)
264+
265 @property
266 def obsolete_translatable_series(self):
267 """See `IProduct`."""
268
269=== modified file 'lib/lp/registry/templates/product-series.pt'
270--- lib/lp/registry/templates/product-series.pt 2010-05-17 17:29:08 +0000
271+++ lib/lp/registry/templates/product-series.pt 2010-10-29 17:28:52 +0000
272@@ -21,17 +21,27 @@
273 </li>
274 </ul>
275
276- <tal:cache content="cache:public, 1 hour">
277- <div tal:repeat="series view/sorted_series_list">
278- <div style="margin-top: 1em;
279- border-bottom: 1px solid #ccc; max-width: 60em;"
280- tal:define="is_focus series/is_development_focus"
281- tal:attributes="class string:${series/css_class} series;
282- id series/name/fmt:css-id/series-;">
283- <tal:status replace="structure series/series/@@+status" />
284- </div>
285- </div>
286- </tal:cache>
287+ <tal:series-list condition="view/batched_series/currentBatch">
288+ <div class="lesser" id="active-top-navigation">
289+ <tal:navigation
290+ content="structure view/batched_series/@@+navigation-links-upper" />
291+ </div>
292+ <div tal:repeat="series view/batched_series/currentBatch">
293+ <tal:cache content="cache:public, 1 hour, series/name">
294+ <div style="margin-top: 1em;
295+ border-bottom: 1px solid #ccc; max-width: 60em;"
296+ tal:define="is_focus series/is_development_focus"
297+ tal:attributes="class string:${series/css_class} series;
298+ id series/name/fmt:css-id/series-;">
299+ <tal:status replace="structure series/series/@@+status" />
300+ </div>
301+ </tal:cache>
302+ </div>
303+ <div class="lesser">
304+ <tal:navigation
305+ content="structure view/batched_series/@@+navigation-links-lower" />
306+ </div>
307+ </tal:series-list>
308 </div>
309 </body>
310 </html>
311
312=== modified file 'lib/lp/registry/tests/test_product.py'
313--- lib/lp/registry/tests/test_product.py 2010-10-04 19:50:45 +0000
314+++ lib/lp/registry/tests/test_product.py 2010-10-29 17:28:52 +0000
315@@ -27,8 +27,12 @@
316 IProduct,
317 License,
318 )
319+from lp.registry.interfaces.series import SeriesStatus
320 from lp.registry.model.commercialsubscription import CommercialSubscription
321-from lp.registry.model.product import Product, UnDeactivateable
322+from lp.registry.model.product import (
323+ Product,
324+ UnDeactivateable,
325+ )
326 from lp.registry.model.productlicense import ProductLicense
327 from lp.testing import TestCaseWithFactory
328
329@@ -136,6 +140,32 @@
330 expected_milestones,
331 timeline_milestones)
332
333+ def test_getVersionSortedSeries(self):
334+ # The product series should be sorted with the development focus
335+ # series first, the series starting with a number in descending
336+ # order, and then the series starting with a letter in
337+ # descending order.
338+ product = self.factory.makeProduct()
339+ for name in ('1', '2', '3', '3a', '3b', 'alpha', 'beta'):
340+ self.factory.makeProductSeries(product=product, name=name)
341+ self.assertEqual(
342+ [u'trunk', u'3b', u'3a', u'3', u'2', u'1', u'beta', u'alpha'],
343+ [series.name for series in product.getVersionSortedSeries()])
344+
345+ def test_getVersionSortedSeries_filter_obsolete(self):
346+ # The obsolete series should not be included in the results if
347+ # the filter_obsolete argument is set to True.
348+ login('admin@canonical.com')
349+ product = self.factory.makeProduct()
350+ self.factory.makeProductSeries(product=product, name='active-series')
351+ obsolete_series = self.factory.makeProductSeries(
352+ product=product, name='obsolete-series')
353+ obsolete_series.status = SeriesStatus.OBSOLETE
354+ active_series = product.getVersionSortedSeries(filter_obsolete=True)
355+ self.assertEqual(
356+ [u'trunk', u'active-series'],
357+ [series.name for series in active_series])
358+
359
360 class TestProductFiles(unittest.TestCase):
361 """Tests for downloadable product files."""

Subscribers

People subscribed via source and target branches

to status/vote changes: