Launchpad itself

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

  1. bug-663861-product-series-timeout-part2
  2. Merge into devel
Proposed by Edwin Grubbs
Status: Superseded
Proposed branch: lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2
Merge into: lp:launchpad
Prerequisite: lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part1
Diff against target: 480 lines (+264/-33)
10 files modified
database/schema/trusted.sql (+5/-2)
lib/lp/code/interfaces/branchmergequeue.py (+115/-0)
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 db Pending
Robert Collins db Pending
Launchpad code reviewers Pending
Review via email: mp+39638@code.launchpad.net

Commit message

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

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
Edwin Grubbs (edwin-grubbs) wrote :

I resubmitted this mp, since I accidentally selected devel instead of db-devel.

https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2/+merge/39643

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 16:11:54 +0000
3+++ database/schema/trusted.sql 2010-10-29 16:12:00 +0000
4@@ -1826,10 +1826,13 @@
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].
12+ return '~' + match.group(0).zfill(5)
13
14 return re.sub(u'\d+', substitute_filled_numbers, version)
15 $$;
16
17 COMMENT ON FUNCTION version_sort_key(text) IS
18-'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.';
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. 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].';
20
21=== added file 'lib/lp/code/interfaces/branchmergequeue.py'
22--- lib/lp/code/interfaces/branchmergequeue.py 1970-01-01 00:00:00 +0000
23+++ lib/lp/code/interfaces/branchmergequeue.py 2010-10-29 16:12:00 +0000
24@@ -0,0 +1,115 @@
25+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
26+# GNU Affero General Public License version 3 (see the file LICENSE).
27+
28+"""Branch merge queue interfaces."""
29+
30+__metaclass__ = type
31+
32+__all__ = [
33+ 'IBranchMergeQueue',
34+ 'IBranchMergeQueueSource',
35+ ]
36+
37+from lazr.restful.declarations import (
38+ export_as_webservice_entry,
39+ export_write_operation,
40+ exported,
41+ mutator_for,
42+ operation_parameters,
43+ )
44+from lazr.restful.fields import (
45+ CollectionField,
46+ Reference,
47+ )
48+from zope.interface import Interface
49+from zope.schema import (
50+ Datetime,
51+ Int,
52+ Text,
53+ TextLine,
54+ )
55+
56+from canonical.launchpad import _
57+from lp.services.fields import (
58+ PersonChoice,
59+ PublicPersonChoice,
60+ )
61+
62+
63+class IBranchMergeQueue(Interface):
64+ """An interface for managing branch merges."""
65+
66+ export_as_webservice_entry()
67+
68+ id = Int(title=_('ID'), readonly=True, required=True)
69+
70+ registrant = exported(
71+ PublicPersonChoice(
72+ title=_("The user that registered the branch."),
73+ required=True, readonly=True,
74+ vocabulary='ValidPersonOrTeam'))
75+
76+ owner = exported(
77+ PersonChoice(
78+ title=_('Owner'),
79+ required=True, readonly=True,
80+ vocabulary='UserTeamsParticipationPlusSelf',
81+ description=_("The owner of the merge queue.")))
82+
83+ name = exported(
84+ TextLine(
85+ title=_('Name'), required=True,
86+ description=_(
87+ "Keep very short, unique, and descriptive, because it will "
88+ "be used in URLs. "
89+ "Examples: main, devel, release-1.0, gnome-vfs.")))
90+
91+ description = exported(
92+ Text(
93+ title=_('Description'), required=False,
94+ description=_(
95+ 'A short description of the purpose of this merge queue.')))
96+
97+ configuration = exported(
98+ TextLine(
99+ title=_('Configuration'), required=False, readonly=True,
100+ description=_(
101+ "A JSON string of configuration values.")))
102+
103+ date_created = exported(
104+ Datetime(
105+ title=_('Date Created'),
106+ required=True,
107+ readonly=True))
108+
109+ branches = exported(
110+ CollectionField(
111+ title=_('Dependent Branches'),
112+ description=_(
113+ 'A collection of branches that this queue manages.'),
114+ readonly=True,
115+ value_type=Reference(Interface)))
116+
117+ @mutator_for(configuration)
118+ @operation_parameters(
119+ config=TextLine(title=_("A JSON string of configuration values.")))
120+ @export_write_operation()
121+ def setMergeQueueConfig(config):
122+ """Set the JSON string configuration of the merge queue.
123+
124+ :param config: A JSON string of configuration values.
125+ """
126+
127+
128+class IBranchMergeQueueSource(Interface):
129+
130+ def new(name, owner, registrant, description, configuration, branches):
131+ """Create a new IBranchMergeQueue object.
132+
133+ :param name: The name of the branch merge queue.
134+ :param description: A description of queue.
135+ :param configuration: A JSON string of configuration values.
136+ :param owner: The owner of the queue.
137+ :param registrant: The registrant of the queue.
138+ :param branches: A list of branches to add to the queue.
139+ """
140
141=== modified file 'lib/lp/registry/browser/configure.zcml'
142--- lib/lp/registry/browser/configure.zcml 2010-10-21 03:22:06 +0000
143+++ lib/lp/registry/browser/configure.zcml 2010-10-29 16:12:00 +0000
144@@ -1473,7 +1473,7 @@
145 template="../templates/product-portlet-packages.pt"/>
146 <browser:page
147 for="lp.registry.interfaces.product.IProduct"
148- class="lp.registry.browser.product.ProductSeriesView"
149+ class="lp.registry.browser.product.ProductSeriesSetView"
150 name="+series"
151 facet="overview"
152 permission="zope.Public"
153
154=== modified file 'lib/lp/registry/browser/product.py'
155--- lib/lp/registry/browser/product.py 2010-10-20 14:58:01 +0000
156+++ lib/lp/registry/browser/product.py 2010-10-29 16:12:00 +0000
157@@ -29,7 +29,7 @@
158 'ProductPackagesPortletView',
159 'ProductRdfView',
160 'ProductReviewLicenseView',
161- 'ProductSeriesView',
162+ 'ProductSeriesSetView',
163 'ProductSetBreadcrumb',
164 'ProductSetFacets',
165 'ProductSetNavigation',
166@@ -88,6 +88,9 @@
167 MultiStepView,
168 StepView,
169 )
170+from canonical.launchpad.components.decoratedresultset import (
171+ DecoratedResultSet,
172+ )
173 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
174 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
175 from canonical.launchpad.mail import (
176@@ -123,7 +126,6 @@
177 safe_action,
178 )
179 from canonical.launchpad.webapp.menu import NavigationMenu
180-from lp.app.browser.tales import MenuAPI
181 from canonical.widgets.date import DateWidget
182 from canonical.widgets.itemswidgets import (
183 CheckBoxMatrixWidget,
184@@ -142,6 +144,7 @@
185 QuestionTargetFacetMixin,
186 QuestionTargetTraversalMixin,
187 )
188+from lp.app.browser.tales import MenuAPI
189 from lp.app.enums import ServiceUsage
190 from lp.app.errors import NotFoundError
191 from lp.app.interfaces.headings import IEditableContextTitle
192@@ -793,7 +796,25 @@
193 self.release_by_id[release.id] = release_delegate
194
195
196-class SeriesWithReleases:
197+class DecoratedSeries:
198+ """A decorated series that includes helper attributes for templates."""
199+ delegates(IProductSeries, 'series')
200+
201+ def __init__(self, series):
202+ self.series = series
203+
204+ @property
205+ def css_class(self):
206+ """The highlighted, unhighlighted, or dimmed CSS class."""
207+ if self.is_development_focus:
208+ return 'highlighted'
209+ elif self.status == SeriesStatus.OBSOLETE:
210+ return 'dimmed'
211+ else:
212+ return 'unhighlighted'
213+
214+
215+class SeriesWithReleases(DecoratedSeries):
216 """A decorated series that includes releases.
217
218 The extra data is included in this class to avoid repeated
219@@ -807,10 +828,9 @@
220 # raise an AttributeError for self.parent.
221 parent = None
222 releases = None
223- delegates(IProductSeries, 'series')
224
225 def __init__(self, series, parent):
226- self.series = series
227+ super(SeriesWithReleases, self).__init__(series)
228 self.parent = parent
229 self.releases = []
230
231@@ -824,16 +844,6 @@
232 return True
233 return False
234
235- @property
236- def css_class(self):
237- """The highlighted, unhighlighted, or dimmed CSS class."""
238- if self.is_development_focus:
239- return 'highlighted'
240- elif self.status == SeriesStatus.OBSOLETE:
241- return 'dimmed'
242- else:
243- return 'unhighlighted'
244-
245
246 class ReleaseWithFiles:
247 """A decorated release that includes product release files.
248@@ -1707,12 +1717,18 @@
249 return canonical_url(self.context)
250
251
252-class ProductSeriesView(ProductView):
253+class ProductSeriesSetView(ProductView):
254 """A view for showing a product's series."""
255
256 label = 'timeline'
257 page_title = label
258
259+ @cachedproperty
260+ def batched_series(self):
261+ decorated_result = DecoratedResultSet(
262+ self.context.getVersionSortedSeries(), DecoratedSeries)
263+ return BatchNavigator(decorated_result, self.request)
264+
265
266 class ProductRdfView(BaseRdfView):
267 """A view that sets its mime-type to application/rdf+xml"""
268
269=== modified file 'lib/lp/registry/browser/tests/product-files-views.txt'
270--- lib/lp/registry/browser/tests/product-files-views.txt 2010-08-03 14:51:31 +0000
271+++ lib/lp/registry/browser/tests/product-files-views.txt 2010-10-29 16:12:00 +0000
272@@ -21,7 +21,7 @@
273
274 >>> def print_series_release(sr):
275 ... print "%s from the %s series" % (sr.release.name_with_codename,
276- ... sr.series.series.name)
277+ ... sr.series.name)
278
279 >>> for sr in batch:
280 ... print_series_release(sr)
281@@ -35,7 +35,8 @@
282 ... milestone = factory.makeMilestone(productseries=productseries,
283 ... name="%d.%d"%(i,j))
284 ... release_file = factory.makeProductReleaseFile(
285- ... product=product, productseries=productseries, milestone=milestone)
286+ ... product=product, productseries=productseries,
287+ ... milestone=milestone)
288 >>> view = create_initialized_view(product, '+download')
289 >>> batch = view.series_and_releases_batch.currentBatch()
290 >>> print len(batch)
291
292=== modified file 'lib/lp/registry/browser/tests/product-views.txt'
293--- lib/lp/registry/browser/tests/product-views.txt 2010-10-18 22:24:59 +0000
294+++ lib/lp/registry/browser/tests/product-views.txt 2010-10-29 16:12:00 +0000
295@@ -417,6 +417,27 @@
296 ftp://mozilla.org/firefox.*bz2
297
298
299+Viewing series for a product
300+============================
301+
302+All the product series can be viewed in batches.
303+
304+ >>> product = factory.makeProduct()
305+ >>> for name in ('stable', 'testing', '1.1', '1.2', 'extra'):
306+ ... series = factory.makeProductSeries(product=product, name=name)
307+ >>> view = create_view(product, name='+series')
308+ >>> batch = view.batched_series.currentBatch()
309+ >>> print batch.total()
310+ 6
311+ >>> for series in batch:
312+ ... print series.name
313+ trunk
314+ 1.2
315+ 1.1
316+ testing
317+ stable
318+
319+
320 Product index view
321 ==================
322
323
324=== modified file 'lib/lp/registry/interfaces/product.py'
325--- lib/lp/registry/interfaces/product.py 2010-09-21 09:37:06 +0000
326+++ lib/lp/registry/interfaces/product.py 2010-10-29 16:12:00 +0000
327@@ -733,6 +733,16 @@
328 "Some bug trackers host multiple projects at the same URL "
329 "and require an identifier for the specific project.")))
330
331+ def getVersionSortedSeries(filter_obsolete=False):
332+ """Return all the series sorted by the name field as a version.
333+
334+ The development focus field is an exception. It will always
335+ be sorted first.
336+
337+ :param filter_obsolete: If true, do not include any series with
338+ SeriesStatus.OBSOLETE in the results.
339+ """
340+
341 def redeemSubscriptionVoucher(voucher, registrant, purchaser,
342 subscription_months, whiteboard=None,
343 current_datetime=None):
344
345=== modified file 'lib/lp/registry/model/product.py'
346--- lib/lp/registry/model/product.py 2010-10-24 13:02:07 +0000
347+++ lib/lp/registry/model/product.py 2010-10-29 16:12:00 +0000
348@@ -137,6 +137,7 @@
349 License,
350 LicenseStatus,
351 )
352+from lp.registry.interfaces.series import SeriesStatus
353 from lp.registry.model.announcement import MakesAnnouncements
354 from lp.registry.model.commercialsubscription import CommercialSubscription
355 from lp.registry.model.distribution import Distribution
356@@ -980,6 +981,30 @@
357 translatable_product_series,
358 key=operator.attrgetter('datecreated'))
359
360+ def getVersionSortedSeries(self, filter_obsolete=False):
361+ """See `IProduct`."""
362+ store = Store.of(self)
363+ dev_focus = store.find(
364+ ProductSeries,
365+ ProductSeries.id == self.development_focus.id)
366+ other_series_conditions = [
367+ ProductSeries.product == self,
368+ ProductSeries.id != self.development_focus.id,
369+ ]
370+ if filter_obsolete is True:
371+ other_series_conditions.append(
372+ ProductSeries.status != SeriesStatus.OBSOLETE)
373+ other_series = store.find(ProductSeries, other_series_conditions)
374+ # The query will be much slower if the version_sort_key is not
375+ # the first thing that is sorted, since it won't be able to use
376+ # the productseries_name_sort index.
377+ other_series.order_by(SQL('version_sort_key(name) DESC'))
378+ # UNION ALL must be used to preserve the sort order from the
379+ # separate queries. The sorting should not be done after
380+ # unioning the two queries, because that will prevent it from
381+ # being able to use the productseries_name_sort index.
382+ return dev_focus.union(other_series, all=True)
383+
384 @property
385 def obsolete_translatable_series(self):
386 """See `IProduct`."""
387
388=== modified file 'lib/lp/registry/templates/product-series.pt'
389--- lib/lp/registry/templates/product-series.pt 2010-05-17 17:29:08 +0000
390+++ lib/lp/registry/templates/product-series.pt 2010-10-29 16:12:00 +0000
391@@ -21,17 +21,27 @@
392 </li>
393 </ul>
394
395- <tal:cache content="cache:public, 1 hour">
396- <div tal:repeat="series view/sorted_series_list">
397- <div style="margin-top: 1em;
398- border-bottom: 1px solid #ccc; max-width: 60em;"
399- tal:define="is_focus series/is_development_focus"
400- tal:attributes="class string:${series/css_class} series;
401- id series/name/fmt:css-id/series-;">
402- <tal:status replace="structure series/series/@@+status" />
403- </div>
404- </div>
405- </tal:cache>
406+ <tal:series-list condition="view/batched_series/currentBatch">
407+ <div class="lesser" id="active-top-navigation">
408+ <tal:navigation
409+ content="structure view/batched_series/@@+navigation-links-upper" />
410+ </div>
411+ <div tal:repeat="series view/batched_series/currentBatch">
412+ <tal:cache content="cache:public, 1 hour, series/name">
413+ <div style="margin-top: 1em;
414+ border-bottom: 1px solid #ccc; max-width: 60em;"
415+ tal:define="is_focus series/is_development_focus"
416+ tal:attributes="class string:${series/css_class} series;
417+ id series/name/fmt:css-id/series-;">
418+ <tal:status replace="structure series/series/@@+status" />
419+ </div>
420+ </tal:cache>
421+ </div>
422+ <div class="lesser">
423+ <tal:navigation
424+ content="structure view/batched_series/@@+navigation-links-lower" />
425+ </div>
426+ </tal:series-list>
427 </div>
428 </body>
429 </html>
430
431=== modified file 'lib/lp/registry/tests/test_product.py'
432--- lib/lp/registry/tests/test_product.py 2010-10-04 19:50:45 +0000
433+++ lib/lp/registry/tests/test_product.py 2010-10-29 16:12:00 +0000
434@@ -27,8 +27,12 @@
435 IProduct,
436 License,
437 )
438+from lp.registry.interfaces.series import SeriesStatus
439 from lp.registry.model.commercialsubscription import CommercialSubscription
440-from lp.registry.model.product import Product, UnDeactivateable
441+from lp.registry.model.product import (
442+ Product,
443+ UnDeactivateable,
444+ )
445 from lp.registry.model.productlicense import ProductLicense
446 from lp.testing import TestCaseWithFactory
447
448@@ -136,6 +140,32 @@
449 expected_milestones,
450 timeline_milestones)
451
452+ def test_getVersionSortedSeries(self):
453+ # The product series should be sorted with the development focus
454+ # series first, the series starting with a number in descending
455+ # order, and then the series starting with a letter in
456+ # descending order.
457+ product = self.factory.makeProduct()
458+ for name in ('1', '2', '3', '3a', '3b', 'alpha', 'beta'):
459+ self.factory.makeProductSeries(product=product, name=name)
460+ self.assertEqual(
461+ [u'trunk', u'3b', u'3a', u'3', u'2', u'1', u'beta', u'alpha'],
462+ [series.name for series in product.getVersionSortedSeries()])
463+
464+ def test_getVersionSortedSeries_filter_obsolete(self):
465+ # The obsolete series should not be included in the results if
466+ # the filter_obsolete argument is set to True.
467+ login('admin@canonical.com')
468+ product = self.factory.makeProduct()
469+ self.factory.makeProductSeries(product=product, name='active-series')
470+ obsolete_series = self.factory.makeProductSeries(
471+ product=product, name='obsolete-series')
472+ obsolete_series.status = SeriesStatus.OBSOLETE
473+ active_series = product.getVersionSortedSeries(filter_obsolete=True)
474+ self.assertEqual(
475+ [u'trunk', u'active-series'],
476+ [series.name for series in active_series])
477+
478
479 class TestProductFiles(unittest.TestCase):
480 """Tests for downloadable product files."""