Launchpad itself

Merge lp:~wgrant/launchpad/bug-592914-recipe-distroseries-order into lp:launchpad

  1. bug-592914-recipe-distroseries-order
  2. Merge into devel
Proposed by William Grant
Status: Merged
Approved by: Māris Fogels
Approved revision: no longer in the source branch.
Merged at revision: 11016
Proposed branch: lp:~wgrant/launchpad/bug-592914-recipe-distroseries-order
Merge into: lp:launchpad
Diff against target: 50 lines (+9/-5)
2 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+7/-3)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+2/-2)
To merge this branch: bzr merge lp:~wgrant/launchpad/bug-592914-recipe-distroseries-order
Reviewer Review Type Date Requested Status
Māris Fogels (community) Approve
Review via email: mp+27415@code.launchpad.net

Commit message

Sort by version the distroseries list when creating recipes and builds.

Description of the change

This branch fixes bug #592914 by sorting the distroseries vocabulary used when creating recipes and requesting builds. It's just reverse sorting by DistroSeries.version, which will fall apart if we ever support multiple distributions, but it will work fine for now.

I've updated the broken test, and there is no lint.

To post a comment you must log in.
Revision history for this message
Māris Fogels (mars) wrote :

Hi William,

The fix looks good, but I had two concerns:

 * Would it be better to use attrgetter instead of the lambda?: sorted(.., key=attrgetter('value.version'))

 * Just a comment, but the call to term.value.version breaks the Law of Demeter (http://en.wikipedia.org/wiki/Law_of_Demeter). That often indicates an object design problem, but since we are working with data objects, I think it can pass here.

Looks good! r=mars

Maris

review: Approve
Revision history for this message
William Grant (wgrant) wrote :

I didn't know that attrgetter could traverse multiple levels like that. I've replaced the lambda with it.

Revision history for this message
William Grant (wgrant) wrote :

It turns out that attrgetter doesn't actually work like that.

Revision history for this message
William Grant (wgrant) wrote :

I've just worked out how it passed locally: python2.6 works as you suggested, but python2.5 only works for a single level.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2010-06-15 15:08:11 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-06-16 11:49:28 +0000
@@ -37,6 +37,7 @@
37 LaunchpadView, Link, Navigation, NavigationMenu, stepthrough)37 LaunchpadView, Link, Navigation, NavigationMenu, stepthrough)
38from canonical.launchpad.webapp.authorization import check_permission38from canonical.launchpad.webapp.authorization import check_permission
39from canonical.launchpad.webapp.breadcrumb import Breadcrumb39from canonical.launchpad.webapp.breadcrumb import Breadcrumb
40from canonical.launchpad.webapp.sorting import sorted_dotted_numbers
40from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget41from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
41from lp.buildmaster.interfaces.buildbase import BuildStatus42from lp.buildmaster.interfaces.buildbase import BuildStatus
42from lp.code.errors import ForbiddenInstruction43from lp.code.errors import ForbiddenInstruction
@@ -171,9 +172,12 @@
171 ppas = getUtility(IArchiveSet).getPPAsForUser(getUtility(ILaunchBag).user)172 ppas = getUtility(IArchiveSet).getPPAsForUser(getUtility(ILaunchBag).user)
172 supported_distros = [ppa.distribution for ppa in ppas]173 supported_distros = [ppa.distribution for ppa in ppas]
173 dsset = getUtility(IDistroSeriesSet).search()174 dsset = getUtility(IDistroSeriesSet).search()
174 terms = [SimpleTerm(distro, distro.id, distro.displayname)175 terms = sorted_dotted_numbers(
175 for distro in dsset if (176 [SimpleTerm(distro, distro.id, distro.displayname)
176 distro.active and distro.distribution in supported_distros)]177 for distro in dsset if (
178 distro.active and distro.distribution in supported_distros)],
179 key=lambda term: term.value.version)
180 terms.reverse()
177 return SimpleVocabulary(terms)181 return SimpleVocabulary(terms)
178182
179def target_ppas_vocabulary(context):183def target_ppas_vocabulary(context):
180184
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-15 15:08:11 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-16 11:49:28 +0000
@@ -42,7 +42,7 @@
42 self.ppa = self.factory.makeArchive(42 self.ppa = self.factory.makeArchive(
43 displayname='Secret PPA', owner=self.chef, name='ppa')43 displayname='Secret PPA', owner=self.chef, name='ppa')
44 self.squirrel = self.factory.makeDistroSeries(44 self.squirrel = self.factory.makeDistroSeries(
45 displayname='Secret Squirrel', name='secret',45 displayname='Secret Squirrel', name='secret', version='100.04',
46 distribution=self.ppa.distribution)46 distribution=self.ppa.distribution)
47 self.squirrel.nominatedarchindep = self.squirrel.newArch(47 self.squirrel.nominatedarchindep = self.squirrel.newArch(
48 'i386', ProcessorFamily.get(1), False, self.chef,48 'i386', ProcessorFamily.get(1), False, self.chef,
@@ -516,8 +516,8 @@
516 Secret PPA (chef/ppa)516 Secret PPA (chef/ppa)
517 Distribution series:517 Distribution series:
518 Secret Squirrel518 Secret Squirrel
519 Hoary
519 Warty520 Warty
520 Hoary
521 or521 or
522 Cancel""")522 Cancel""")
523 main_text = self.getMainText(recipe, '+request-builds')523 main_text = self.getMainText(recipe, '+request-builds')