launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01738
lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2 into lp:launchpad/devel
Edwin Grubbs has proposed merging lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2 into lp:launchpad/devel with lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part1 as a prerequisite.
Requested reviews:
Stuart Bishop (stub): db
Robert Collins (lifeless): db
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#663861 Product:+series timesout showing obsolete series
https://bugs.launchpad.net/bugs/663861
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.)
--
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2/+merge/39638
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-663861-product-series-timeout-part2 into lp:launchpad/devel.
=== modified file 'database/schema/trusted.sql'
--- database/schema/trusted.sql 2010-10-29 16:11:54 +0000
+++ database/schema/trusted.sql 2010-10-29 16:12:00 +0000
@@ -1826,10 +1826,13 @@
[version] = args
def substitute_filled_numbers(match):
- return match.group(0).zfill(5)
+ # Prepend "~" so that version numbers will show up first
+ # when sorted descending, i.e. [3, 2c, 2b, 1, c, b, a] instead
+ # of [c, b, a, 3, 2c, 2b, 1].
+ return '~' + match.group(0).zfill(5)
return re.sub(u'\d+', substitute_filled_numbers, version)
$$;
COMMENT ON FUNCTION version_sort_key(text) IS
-'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.';
+'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].';
=== added file 'lib/lp/code/interfaces/branchmergequeue.py'
--- lib/lp/code/interfaces/branchmergequeue.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/interfaces/branchmergequeue.py 2010-10-29 16:12:00 +0000
@@ -0,0 +1,115 @@
+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Branch merge queue interfaces."""
+
+__metaclass__ = type
+
+__all__ = [
+ 'IBranchMergeQueue',
+ 'IBranchMergeQueueSource',
+ ]
+
+from lazr.restful.declarations import (
+ export_as_webservice_entry,
+ export_write_operation,
+ exported,
+ mutator_for,
+ operation_parameters,
+ )
+from lazr.restful.fields import (
+ CollectionField,
+ Reference,
+ )
+from zope.interface import Interface
+from zope.schema import (
+ Datetime,
+ Int,
+ Text,
+ TextLine,
+ )
+
+from canonical.launchpad import _
+from lp.services.fields import (
+ PersonChoice,
+ PublicPersonChoice,
+ )
+
+
+class IBranchMergeQueue(Interface):
+ """An interface for managing branch merges."""
+
+ export_as_webservice_entry()
+
+ id = Int(title=_('ID'), readonly=True, required=True)
+
+ registrant = exported(
+ PublicPersonChoice(
+ title=_("The user that registered the branch."),
+ required=True, readonly=True,
+ vocabulary='ValidPersonOrTeam'))
+
+ owner = exported(
+ PersonChoice(
+ title=_('Owner'),
+ required=True, readonly=True,
+ vocabulary='UserTeamsParticipationPlusSelf',
+ description=_("The owner of the merge queue.")))
+
+ name = exported(
+ TextLine(
+ title=_('Name'), required=True,
+ description=_(
+ "Keep very short, unique, and descriptive, because it will "
+ "be used in URLs. "
+ "Examples: main, devel, release-1.0, gnome-vfs.")))
+
+ description = exported(
+ Text(
+ title=_('Description'), required=False,
+ description=_(
+ 'A short description of the purpose of this merge queue.')))
+
+ configuration = exported(
+ TextLine(
+ title=_('Configuration'), required=False, readonly=True,
+ description=_(
+ "A JSON string of configuration values.")))
+
+ date_created = exported(
+ Datetime(
+ title=_('Date Created'),
+ required=True,
+ readonly=True))
+
+ branches = exported(
+ CollectionField(
+ title=_('Dependent Branches'),
+ description=_(
+ 'A collection of branches that this queue manages.'),
+ readonly=True,
+ value_type=Reference(Interface)))
+
+ @mutator_for(configuration)
+ @operation_parameters(
+ config=TextLine(title=_("A JSON string of configuration values.")))
+ @export_write_operation()
+ def setMergeQueueConfig(config):
+ """Set the JSON string configuration of the merge queue.
+
+ :param config: A JSON string of configuration values.
+ """
+
+
+class IBranchMergeQueueSource(Interface):
+
+ def new(name, owner, registrant, description, configuration, branches):
+ """Create a new IBranchMergeQueue object.
+
+ :param name: The name of the branch merge queue.
+ :param description: A description of queue.
+ :param configuration: A JSON string of configuration values.
+ :param owner: The owner of the queue.
+ :param registrant: The registrant of the queue.
+ :param branches: A list of branches to add to the queue.
+ """
=== 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 16:12:00 +0000
@@ -1473,7 +1473,7 @@
template="../templates/product-portlet-packages.pt"/>
<browser:page
for="lp.registry.interfaces.product.IProduct"
- class="lp.registry.browser.product.ProductSeriesView"
+ class="lp.registry.browser.product.ProductSeriesSetView"
name="+series"
facet="overview"
permission="zope.Public"
=== 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 16:12:00 +0000
@@ -29,7 +29,7 @@
'ProductPackagesPortletView',
'ProductRdfView',
'ProductReviewLicenseView',
- 'ProductSeriesView',
+ 'ProductSeriesSetView',
'ProductSetBreadcrumb',
'ProductSetFacets',
'ProductSetNavigation',
@@ -88,6 +88,9 @@
MultiStepView,
StepView,
)
+from canonical.launchpad.components.decoratedresultset import (
+ DecoratedResultSet,
+ )
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
from canonical.launchpad.mail import (
@@ -123,7 +126,6 @@
safe_action,
)
from canonical.launchpad.webapp.menu import NavigationMenu
-from lp.app.browser.tales import MenuAPI
from canonical.widgets.date import DateWidget
from canonical.widgets.itemswidgets import (
CheckBoxMatrixWidget,
@@ -142,6 +144,7 @@
QuestionTargetFacetMixin,
QuestionTargetTraversalMixin,
)
+from lp.app.browser.tales import MenuAPI
from lp.app.enums import ServiceUsage
from lp.app.errors import NotFoundError
from lp.app.interfaces.headings import IEditableContextTitle
@@ -793,7 +796,25 @@
self.release_by_id[release.id] = release_delegate
-class SeriesWithReleases:
+class DecoratedSeries:
+ """A decorated series that includes helper attributes for templates."""
+ delegates(IProductSeries, 'series')
+
+ def __init__(self, series):
+ self.series = series
+
+ @property
+ def css_class(self):
+ """The highlighted, unhighlighted, or dimmed CSS class."""
+ if self.is_development_focus:
+ return 'highlighted'
+ elif self.status == SeriesStatus.OBSOLETE:
+ return 'dimmed'
+ else:
+ return 'unhighlighted'
+
+
+class SeriesWithReleases(DecoratedSeries):
"""A decorated series that includes releases.
The extra data is included in this class to avoid repeated
@@ -807,10 +828,9 @@
# raise an AttributeError for self.parent.
parent = None
releases = None
- delegates(IProductSeries, 'series')
def __init__(self, series, parent):
- self.series = series
+ super(SeriesWithReleases, self).__init__(series)
self.parent = parent
self.releases = []
@@ -824,16 +844,6 @@
return True
return False
- @property
- def css_class(self):
- """The highlighted, unhighlighted, or dimmed CSS class."""
- if self.is_development_focus:
- return 'highlighted'
- elif self.status == SeriesStatus.OBSOLETE:
- return 'dimmed'
- else:
- return 'unhighlighted'
-
class ReleaseWithFiles:
"""A decorated release that includes product release files.
@@ -1707,12 +1717,18 @@
return canonical_url(self.context)
-class ProductSeriesView(ProductView):
+class ProductSeriesSetView(ProductView):
"""A view for showing a product's series."""
label = 'timeline'
page_title = label
+ @cachedproperty
+ def batched_series(self):
+ decorated_result = DecoratedResultSet(
+ self.context.getVersionSortedSeries(), DecoratedSeries)
+ return BatchNavigator(decorated_result, self.request)
+
class ProductRdfView(BaseRdfView):
"""A view that sets its mime-type to application/rdf+xml"""
=== 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 16:12:00 +0000
@@ -21,7 +21,7 @@
>>> def print_series_release(sr):
... print "%s from the %s series" % (sr.release.name_with_codename,
- ... sr.series.series.name)
+ ... sr.series.name)
>>> for sr in batch:
... print_series_release(sr)
@@ -35,7 +35,8 @@
... milestone = factory.makeMilestone(productseries=productseries,
... name="%d.%d"%(i,j))
... release_file = factory.makeProductReleaseFile(
- ... product=product, productseries=productseries, milestone=milestone)
+ ... product=product, productseries=productseries,
+ ... milestone=milestone)
>>> view = create_initialized_view(product, '+download')
>>> batch = view.series_and_releases_batch.currentBatch()
>>> print len(batch)
=== 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 16:12:00 +0000
@@ -417,6 +417,27 @@
ftp://mozilla.org/firefox.*bz2
+Viewing series for a product
+============================
+
+All the product series can be viewed in batches.
+
+ >>> product = factory.makeProduct()
+ >>> for name in ('stable', 'testing', '1.1', '1.2', 'extra'):
+ ... series = factory.makeProductSeries(product=product, name=name)
+ >>> view = create_view(product, name='+series')
+ >>> batch = view.batched_series.currentBatch()
+ >>> print batch.total()
+ 6
+ >>> for series in batch:
+ ... print series.name
+ trunk
+ 1.2
+ 1.1
+ testing
+ stable
+
+
Product index view
==================
=== 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 16:12:00 +0000
@@ -733,6 +733,16 @@
"Some bug trackers host multiple projects at the same URL "
"and require an identifier for the specific project.")))
+ def getVersionSortedSeries(filter_obsolete=False):
+ """Return all the series sorted by the name field as a version.
+
+ The development focus field is an exception. It will always
+ be sorted first.
+
+ :param filter_obsolete: If true, do not include any series with
+ SeriesStatus.OBSOLETE in the results.
+ """
+
def redeemSubscriptionVoucher(voucher, registrant, purchaser,
subscription_months, whiteboard=None,
current_datetime=None):
=== 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 16:12:00 +0000
@@ -137,6 +137,7 @@
License,
LicenseStatus,
)
+from lp.registry.interfaces.series import SeriesStatus
from lp.registry.model.announcement import MakesAnnouncements
from lp.registry.model.commercialsubscription import CommercialSubscription
from lp.registry.model.distribution import Distribution
@@ -980,6 +981,30 @@
translatable_product_series,
key=operator.attrgetter('datecreated'))
+ def getVersionSortedSeries(self, filter_obsolete=False):
+ """See `IProduct`."""
+ store = Store.of(self)
+ dev_focus = store.find(
+ ProductSeries,
+ ProductSeries.id == self.development_focus.id)
+ other_series_conditions = [
+ ProductSeries.product == self,
+ ProductSeries.id != self.development_focus.id,
+ ]
+ if filter_obsolete is True:
+ other_series_conditions.append(
+ ProductSeries.status != SeriesStatus.OBSOLETE)
+ other_series = store.find(ProductSeries, other_series_conditions)
+ # The query will be much slower if the version_sort_key is not
+ # the first thing that is sorted, since it won't be able to use
+ # the productseries_name_sort index.
+ other_series.order_by(SQL('version_sort_key(name) DESC'))
+ # UNION ALL must be used to preserve the sort order from the
+ # separate queries. The sorting should not be done after
+ # unioning the two queries, because that will prevent it from
+ # being able to use the productseries_name_sort index.
+ return dev_focus.union(other_series, all=True)
+
@property
def obsolete_translatable_series(self):
"""See `IProduct`."""
=== 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 16:12:00 +0000
@@ -21,17 +21,27 @@
</li>
</ul>
- <tal:cache content="cache:public, 1 hour">
- <div tal:repeat="series view/sorted_series_list">
- <div style="margin-top: 1em;
- border-bottom: 1px solid #ccc; max-width: 60em;"
- tal:define="is_focus series/is_development_focus"
- tal:attributes="class string:${series/css_class} series;
- id series/name/fmt:css-id/series-;">
- <tal:status replace="structure series/series/@@+status" />
- </div>
- </div>
- </tal:cache>
+ <tal:series-list condition="view/batched_series/currentBatch">
+ <div class="lesser" id="active-top-navigation">
+ <tal:navigation
+ content="structure view/batched_series/@@+navigation-links-upper" />
+ </div>
+ <div tal:repeat="series view/batched_series/currentBatch">
+ <tal:cache content="cache:public, 1 hour, series/name">
+ <div style="margin-top: 1em;
+ border-bottom: 1px solid #ccc; max-width: 60em;"
+ tal:define="is_focus series/is_development_focus"
+ tal:attributes="class string:${series/css_class} series;
+ id series/name/fmt:css-id/series-;">
+ <tal:status replace="structure series/series/@@+status" />
+ </div>
+ </tal:cache>
+ </div>
+ <div class="lesser">
+ <tal:navigation
+ content="structure view/batched_series/@@+navigation-links-lower" />
+ </div>
+ </tal:series-list>
</div>
</body>
</html>
=== 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 16:12:00 +0000
@@ -27,8 +27,12 @@
IProduct,
License,
)
+from lp.registry.interfaces.series import SeriesStatus
from lp.registry.model.commercialsubscription import CommercialSubscription
-from lp.registry.model.product import Product, UnDeactivateable
+from lp.registry.model.product import (
+ Product,
+ UnDeactivateable,
+ )
from lp.registry.model.productlicense import ProductLicense
from lp.testing import TestCaseWithFactory
@@ -136,6 +140,32 @@
expected_milestones,
timeline_milestones)
+ def test_getVersionSortedSeries(self):
+ # The product series should be sorted with the development focus
+ # series first, the series starting with a number in descending
+ # order, and then the series starting with a letter in
+ # descending order.
+ product = self.factory.makeProduct()
+ for name in ('1', '2', '3', '3a', '3b', 'alpha', 'beta'):
+ self.factory.makeProductSeries(product=product, name=name)
+ self.assertEqual(
+ [u'trunk', u'3b', u'3a', u'3', u'2', u'1', u'beta', u'alpha'],
+ [series.name for series in product.getVersionSortedSeries()])
+
+ def test_getVersionSortedSeries_filter_obsolete(self):
+ # The obsolete series should not be included in the results if
+ # the filter_obsolete argument is set to True.
+ login('admin@xxxxxxxxxxxxx')
+ product = self.factory.makeProduct()
+ self.factory.makeProductSeries(product=product, name='active-series')
+ obsolete_series = self.factory.makeProductSeries(
+ product=product, name='obsolete-series')
+ obsolete_series.status = SeriesStatus.OBSOLETE
+ active_series = product.getVersionSortedSeries(filter_obsolete=True)
+ self.assertEqual(
+ [u'trunk', u'active-series'],
+ [series.name for series in active_series])
+
class TestProductFiles(unittest.TestCase):
"""Tests for downloadable product files."""
Follow ups
