Merge lp:~lifeless/launchpad/bug-279513 into lp:launchpad

Proposed by Robert Collins on 2011-02-24
Status: Work in progress
Proposed branch: lp:~lifeless/launchpad/bug-279513
Merge into: lp:launchpad
Diff against target: 427 lines (+63/-173) 11 files modified
To merge this branch: bzr merge lp:~lifeless/launchpad/bug-279513
Reviewer Review Type Date Requested Status
j.c.sackett Approve on 2011-02-24
William Grant code* 2011-02-24 Approve on 2011-02-24
Review via email: mp+51063@code.launchpad.net

Commit Message

[r=jcsackett,wgrant][bug=279513] Delete Distribution.getCurrentSourceReleases as not modelling user needs well.

Description of the Change

Distribution.getCurrentSourceReleases returns arbitrary uninteresting versions - when folk speak about 'ubuntu' they mean 'ubuntu.currentseries' (and in fact the currentseries attribute explains this). This branch just deletes the inappropriate Distribution.getCurrentSourceReleases and changes the 3 callers to indirect via the currentseries attribute.

We could alternatively make Distribution.getCurrentSourceReleases batch indirect via the relevant distro series, but I think the overall deletion of code is more of a win. We may have some tests that need updating, but I'll wait for ec2 to complain.

To post a comment you must log in.
William Grant (wgrant) wrote :

I don't like the DistributionSourcePackage.currentrelease change, but it's probably cleaner than the alternative. Otherwise this is fine.

review: Approve (code*)
j.c.sackett (jcsackett) wrote :

This looks good to land.

review: Approve
lp:~lifeless/launchpad/bug-279513 updated on 2011-03-02
12445. By Robert Collins on 2011-02-25

Merge devel.

12446. By Robert Collins on 2011-02-25

Unbreak distro source package.currentrelease property.

12447. By Robert Collins on 2011-02-26

Fix the first issue in closing-bugs-from-changelogs.

12448. By Robert Collins on 2011-03-01

Fix from wgrant for closing-bugs-from-changelogs.txt.

12449. By Robert Collins on 2011-03-01

Handle no-distroseries distributions again in bugtask labels.

12450. By Robert Collins on 2011-03-02

Merge trunk.

12451. By Robert Collins on 2011-03-02

snapshotting.

Robert Collins (lifeless) wrote :

I'm abandoning this - its a bit of a rabbit warren, and the perf wins are relatively small compared to the initial work. We are really broken right now, but this is something to do when things are fast, not right now.

Unmerged revisions

12451. By Robert Collins on 2011-03-02

snapshotting.

12450. By Robert Collins on 2011-03-02

Merge trunk.

12449. By Robert Collins on 2011-03-01

Handle no-distroseries distributions again in bugtask labels.

12448. By Robert Collins on 2011-03-01

Fix from wgrant for closing-bugs-from-changelogs.txt.

12447. By Robert Collins on 2011-02-26

Fix the first issue in closing-bugs-from-changelogs.

12446. By Robert Collins on 2011-02-25

Unbreak distro source package.currentrelease property.

12445. By Robert Collins on 2011-02-25

Merge devel.

12444. By Robert Collins on 2011-02-24

Get rid of buggy 'look up in a random distroseries' Distribution.getCurrentSourceReleases, in favour of using the Distribution.currentseries consistently.

Preview Diff

1=== modified file 'lib/lp/bugs/browser/bugtask.py'
2--- lib/lp/bugs/browser/bugtask.py 2011-03-01 05:05:26 +0000
3+++ lib/lp/bugs/browser/bugtask.py 2011-03-02 05:19:17 +0000
4@@ -250,7 +250,6 @@
5 from lp.bugs.interfaces.malone import IMaloneApplication
6 from lp.registry.interfaces.distribution import (
7 IDistribution,
8- IDistributionSet,
9 )
10 from lp.registry.interfaces.distributionsourcepackage import (
11 IDistributionSourcePackage,
12@@ -267,6 +266,7 @@
13 from lp.registry.interfaces.productseries import IProductSeries
14 from lp.registry.interfaces.projectgroup import IProjectGroup
15 from lp.registry.interfaces.sourcepackage import ISourcePackage
16+from lp.registry.model.sourcepackage import SourcePackage
17 from lp.registry.vocabularies import MilestoneVocabulary
18 from lp.services.fields import PersonChoice
19 from lp.services.propertycache import cachedproperty
20@@ -3152,32 +3152,45 @@
21 self.many_bugtasks = len(self.bugtasks) >= 10
22 self.cached_milestone_source = CachedMilestoneSourceFactory()
23 self.user_is_subscribed = self.context.isSubscribed(self.user)
24- distro_packages = defaultdict(list)
25 distro_series_packages = defaultdict(list)
26 for bugtask in self.bugtasks:
27 target = bugtask.target
28 if IDistributionSourcePackage.providedBy(target):
29- distro_packages[target.distribution].append(
30- target.sourcepackagename)
31+ distro_series = target.distribution.currentseries
32+ if distro_series is not None:
33+ distro_series_packages[distro_series].append(
34+ target.sourcepackagename)
35 if ISourcePackage.providedBy(target):
36 distro_series_packages[target.distroseries].append(
37 target.sourcepackagename)
38- distro_set = getUtility(IDistributionSet)
39- self.target_releases = dict(distro_set.getCurrentSourceReleases(
40- distro_packages))
41 distro_series_set = getUtility(IDistroSeriesSet)
42- self.target_releases.update(
43- distro_series_set.getCurrentSourceReleases(distro_series_packages))
44+ self.target_releases = distro_series_set.getCurrentSourceReleases(
45+ distro_series_packages)
46+
47+ def _get_target_release(self, target):
48+ """Get a target release for target."""
49+ error = None
50+ if not (IDistributionSourcePackage.providedBy(target) or
51+ ISourcePackage.providedBy(target)):
52+ return None, None
53+ distribution = target.distribution
54+ if IDistributionSourcePackage.providedBy(target):
55+ target = SourcePackage(target.sourcepackagename,
56+ distribution.currentseries)
57+ if target.distroseries is not None:
58+ current_release = self.target_releases.get(target)
59+ else:
60+ current_release = None
61+ if current_release is None:
62+ error = "No current release for this source package in %s" % (
63+ distribution.displayname)
64+ return current_release, error
65
66 def getTargetLinkTitle(self, target):
67 """Return text to put as the title for the link to the target."""
68- if not (IDistributionSourcePackage.providedBy(target) or
69- ISourcePackage.providedBy(target)):
70- return None
71- current_release = self.target_releases.get(target)
72+ current_release, result = self._get_target_release(target)
73 if current_release is None:
74- return "No current release for this source package in %s" % (
75- target.distribution.displayname)
76+ return result
77 uploader = current_release.creator
78 maintainer = current_release.maintainer
79 return (
80
81=== modified file 'lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt'
82--- lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt 2010-08-22 18:31:30 +0000
83+++ lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt 2011-03-02 05:19:17 +0000
84@@ -89,6 +89,8 @@
85 >>> print view.getTargetLinkTitle(published_distro)
86 None
87 >>> view = set_up_view(published_evolution)
88+ >>> from lp.registry.interfaces.series import SeriesStatus
89+ >>> published_distro.currentseries.status = SeriesStatus.FROZEN
90 >>> print view.getTargetLinkTitle(published_evolution)
91 Latest release: 1.0, uploaded to universe on 2008-07-18 09:30:15+00:00
92 by Foo Bar (name16), maintained by No Privileges Person (no-priv)
93@@ -192,7 +194,7 @@
94 >>> view = BugTasksAndNominationsView(bug, None)
95 >>> view.initialize()
96 >>> for bug_target in bug_targets:
97- ... release = view.target_releases.get(bug_target)
98+ ... release, _ = view._get_target_release(bug_target)
99 ... if release is None:
100 ... version = 'No releases'
101 ... else:
102@@ -201,7 +203,7 @@
103 product: No releases
104 product/product-series: No releases
105 published-distro: No releases
106- evolution (Published Distro): 2.0
107+ evolution (Published Distro): 1.0
108 Published-distro Published-distro-series: No releases
109 evolution (Published-distro Published-distro-series): 1.0
110 unpublished-distro: No releases
111
112=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-index.txt'
113--- lib/lp/bugs/stories/bugs/xx-bug-index.txt 2010-05-19 05:47:50 +0000
114+++ lib/lp/bugs/stories/bugs/xx-bug-index.txt 2011-03-02 05:19:17 +0000
115@@ -61,11 +61,23 @@
116 1
117
118 If the context is a distribution package, the package name has a
119-tooltip containing the package details.
120+tooltip containing the package details. The detailed tests for the tooltip
121+contents are in lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt so we
122+just need to check that one is set - that the layers are wired together.
123+
124+ >>> from zope.component import getUtility
125+ >>> from lp.registry.interfaces.distribution import IDistributionSet
126+ >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
127+ >>> mozilla = ubuntu.getSourcePackage('mozilla-firefox')
128+ >>> mozilla_release = factory.makeSourcePackageRelease(
129+ ... sourcepackagename=mozilla.sourcepackagename,
130+ ... distroseries=mozilla.distribution.currentseries,
131+ ... archive=mozilla.distribution.main_archive, version='0.9')
132+
133
134 >>> print anon_browser.getLink('mozilla-firefox (Ubuntu)').attrs['title']
135- Latest release: 0.9, uploaded to main on 2004-09-27 11:57:13+00:00...
136- by Mark Shuttleworth (mark), maintained by Mark Shuttleworth (mark)
137+ Latest release: 0.9, uploaded to main on ...
138+ by ..., maintained by ...
139
140 >>> print anon_browser.getLink('mozilla-firefox (Debian)').attrs['title']
141 No current release for this source package in Debian
142
143=== modified file 'lib/lp/registry/browser/product.py'
144--- lib/lp/registry/browser/product.py 2011-02-05 06:11:48 +0000
145+++ lib/lp/registry/browser/product.py 2011-03-02 05:19:17 +0000
146@@ -2038,9 +2038,9 @@
147 # Set the source_package_release attribute on the licenses
148 # widget, so that the source package's copyright info can be
149 # displayed.
150- ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
151+ ubuntu_dev = getUtility(ILaunchpadCelebrities).ubuntu.currentseries
152 if self.source_package_name is not None:
153- release_list = ubuntu.getCurrentSourceReleases(
154+ release_list = ubuntu_dev.getCurrentSourceReleases(
155 [self.source_package_name])
156 if len(release_list) != 0:
157 self.widgets['licenses'].source_package_release = (
158
159=== modified file 'lib/lp/registry/interfaces/distribution.py'
160--- lib/lp/registry/interfaces/distribution.py 2011-02-24 15:30:54 +0000
161+++ lib/lp/registry/interfaces/distribution.py 2011-03-02 05:19:17 +0000
162@@ -417,16 +417,6 @@
163 Receives a sourcepackagerelease.
164 """
165
166- def getCurrentSourceReleases(source_package_names):
167- """Get the current release of a list of source packages.
168-
169- :param source_package_names: a list of `ISourcePackageName`
170- instances.
171-
172- :return: a dict where the key is a `IDistributionSourcePackage`
173- and the value is a `IDistributionSourcePackageRelease`.
174- """
175-
176 def getDistroSeriesAndPocket(distroseriesname):
177 """Return a (distroseries,pocket) tuple which is the given textual
178 distroseriesname in this distribution."""
179@@ -692,15 +682,6 @@
180 members, owner, mugshot=None, logo=None, icon=None):
181 """Create a new distribution."""
182
183- def getCurrentSourceReleases(distro_to_source_packagenames):
184- """Lookup many distribution source package releases.
185-
186- :param distro_to_source_packagenames: A dictionary with
187- its keys being `IDistribution` and its values a list of
188- `ISourcePackageName`.
189- :return: A dict as per `IDistribution.getCurrentSourceReleases`
190- """
191-
192
193 class NoSuchDistribution(NameLookupFailed):
194 """Raised when we try to find a distribution that doesn't exist."""
195
196=== modified file 'lib/lp/registry/model/distribution.py'
197--- lib/lp/registry/model/distribution.py 2011-03-01 05:05:26 +0000
198+++ lib/lp/registry/model/distribution.py 2011-03-02 05:19:17 +0000
199@@ -775,11 +775,6 @@
200 """See `IDistribution`."""
201 return DistributionSourcePackageRelease(self, sourcepackagerelease)
202
203- def getCurrentSourceReleases(self, source_package_names):
204- """See `IDistribution`."""
205- return getUtility(IDistributionSet).getCurrentSourceReleases(
206- {self:source_package_names})
207-
208 @property
209 def has_any_specifications(self):
210 """See `IHasSpecifications`."""
211@@ -1900,56 +1895,3 @@
212 getUtility(IArchiveSet).new(distribution=distro,
213 owner=owner, purpose=ArchivePurpose.PRIMARY)
214 return distro
215-
216- def getCurrentSourceReleases(self, distro_source_packagenames):
217- """See `IDistributionSet`."""
218- # Builds one query for all the distro_source_packagenames.
219- # This may need tuning: its possible that grouping by the common
220- # archives may yield better efficiency: the current code is
221- # just a direct push-down of the previous in-python lookup to SQL.
222- series_clauses = []
223- distro_lookup = {}
224- for distro, package_names in distro_source_packagenames.items():
225- source_package_ids = map(attrgetter('id'), package_names)
226- # all_distro_archive_ids is just a list of ints, but it gets
227- # wrapped anyway - and sqlvalues goes boom.
228- archives = removeSecurityProxy(
229- distro.all_distro_archive_ids)
230- clause = """(spr.sourcepackagename IN %s AND
231- spph.archive IN %s AND
232- ds.distribution = %s)
233- """ % sqlvalues(source_package_ids, archives, distro.id)
234- series_clauses.append(clause)
235- distro_lookup[distro.id] = distro
236- if not len(series_clauses):
237- return {}
238- combined_clause = "(" + " OR ".join(series_clauses) + ")"
239-
240- releases = IStore(SourcePackageRelease).find(
241- (SourcePackageRelease, Distribution.id), SQL("""
242- (SourcePackageRelease.id, Distribution.id) IN (
243- SELECT DISTINCT ON (
244- spr.sourcepackagename, ds.distribution)
245- spr.id, ds.distribution
246- FROM
247- SourcePackageRelease AS spr,
248- SourcePackagePublishingHistory AS spph,
249- DistroSeries AS ds
250- WHERE
251- spph.sourcepackagerelease = spr.id
252- AND spph.distroseries = ds.id
253- AND spph.status IN %s
254- AND %s
255- ORDER BY
256- spr.sourcepackagename, ds.distribution, spph.id DESC
257- )
258- """
259- % (sqlvalues(active_publishing_status) + (combined_clause,))))
260- result = {}
261- for sp_release, distro_id in releases:
262- distro = distro_lookup[distro_id]
263- sourcepackage = distro.getSourcePackage(
264- sp_release.sourcepackagename)
265- result[sourcepackage] = DistributionSourcePackageRelease(
266- distro, sp_release)
267- return result
268
269=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
270--- lib/lp/registry/model/distributionsourcepackage.py 2011-01-21 08:12:29 +0000
271+++ lib/lp/registry/model/distributionsourcepackage.py 2011-03-02 05:19:17 +0000
272@@ -298,9 +298,11 @@
273 @property
274 def currentrelease(self):
275 """See `IDistributionSourcePackage`."""
276- releases = self.distribution.getCurrentSourceReleases(
277+ releases = self.distribution.currentseries.getCurrentSourceReleases(
278 [self.sourcepackagename])
279- return releases.get(self)
280+ if releases:
281+ return releases.values()[0]
282+ return None
283
284 def bugtasks(self, quantity=None):
285 """See `IDistributionSourcePackage`."""
286
287=== modified file 'lib/lp/registry/tests/test_distribution.py'
288--- lib/lp/registry/tests/test_distribution.py 2010-10-24 12:46:23 +0000
289+++ lib/lp/registry/tests/test_distribution.py 2011-03-02 05:19:17 +0000
290@@ -6,7 +6,6 @@
291 __metaclass__ = type
292
293 from lazr.lifecycle.snapshot import Snapshot
294-from zope.security.proxy import removeSecurityProxy
295
296 from canonical.testing.layers import (
297 DatabaseFunctionalLayer,
298@@ -15,13 +14,6 @@
299 from lp.registry.errors import NoSuchDistroSeries
300 from lp.registry.interfaces.distribution import IDistribution
301 from lp.registry.interfaces.series import SeriesStatus
302-from lp.registry.tests.test_distroseries import (
303- TestDistroSeriesCurrentSourceReleases,
304- )
305-from lp.services.propertycache import get_property_cache
306-from lp.soyuz.interfaces.distributionsourcepackagerelease import (
307- IDistributionSourcePackageRelease,
308- )
309 from lp.testing import TestCaseWithFactory
310
311
312@@ -48,64 +40,6 @@
313 self.assertEqual("'\\u0170-distro'", displayname)
314
315
316-class TestDistributionCurrentSourceReleases(
317- TestDistroSeriesCurrentSourceReleases):
318- """Test for Distribution.getCurrentSourceReleases().
319-
320- This works in the same way as
321- DistroSeries.getCurrentSourceReleases() works, except that we look
322- for the latest published source across multiple distro series.
323- """
324-
325- layer = LaunchpadFunctionalLayer
326- release_interface = IDistributionSourcePackageRelease
327-
328- @property
329- def test_target(self):
330- return self.distribution
331-
332- def test_which_distroseries_does_not_matter(self):
333- # When checking for the current release, we only care about the
334- # version numbers. We don't care whether the version is
335- # published in a earlier or later series.
336- self.current_series = self.factory.makeDistroRelease(
337- self.distribution, '1.0', status=SeriesStatus.CURRENT)
338- self.publisher.getPubSource(
339- version='0.9', distroseries=self.current_series)
340- self.publisher.getPubSource(
341- version='1.0', distroseries=self.development_series)
342- self.assertCurrentVersion('1.0')
343-
344- self.publisher.getPubSource(
345- version='1.1', distroseries=self.current_series)
346- self.assertCurrentVersion('1.1')
347-
348- def test_distribution_series_cache(self):
349- distribution = removeSecurityProxy(
350- self.factory.makeDistribution('foo'))
351-
352- cache = get_property_cache(distribution)
353-
354- # Not yet cached.
355- self.assertNotIn("series", cache)
356-
357- # Now cached.
358- series = distribution.series
359- self.assertIs(series, cache.series)
360-
361- # Cache cleared.
362- distribution.newSeries(
363- name='bar', displayname='Bar', title='Bar', summary='',
364- description='', version='1', parent_series=None,
365- owner=self.factory.makePerson())
366- self.assertNotIn("series", cache)
367-
368- # New cached value.
369- series = distribution.series
370- self.assertEqual(1, len(series))
371- self.assertIs(series, cache.series)
372-
373-
374 class SeriesByStatusTests(TestCaseWithFactory):
375 """Test IDistribution.getSeriesByStatus().
376 """
377
378=== modified file 'lib/lp/registry/tests/test_distroseries.py'
379--- lib/lp/registry/tests/test_distroseries.py 2010-10-26 15:47:24 +0000
380+++ lib/lp/registry/tests/test_distroseries.py 2011-03-02 05:19:17 +0000
381@@ -152,13 +152,13 @@
382 def test_get_multiple(self):
383 # getCurrentSourceReleases() allows you to get information about
384 # the current release for multiple packages at the same time.
385- # This is done using a single DB query, making it more efficient
386- # than using IDistributionSource.currentrelease.
387+ # This is done using a set queries, making it more efficient than
388+ # looking up each package separately.
389 self.publisher.getPubSource(version='0.9', sourcename='foo')
390 self.publisher.getPubSource(version='1.0', sourcename='bar')
391- foo_package = self.distribution.getSourcePackage('foo')
392- bar_package = self.distribution.getSourcePackage('bar')
393- releases = self.distribution.getCurrentSourceReleases(
394+ foo_package = self.publisher.distroseries.getSourcePackage('foo')
395+ bar_package = self.publisher.distroseries.getSourcePackage('bar')
396+ releases = self.publisher.distroseries.getCurrentSourceReleases(
397 [foo_package.sourcepackagename, bar_package.sourcepackagename])
398 self.assertEqual(releases[foo_package].version, '0.9')
399 self.assertEqual(releases[bar_package].version, '1.0')
400
401=== modified file 'lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt'
402--- lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt 2010-11-01 15:46:48 +0000
403+++ lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt 2011-03-02 05:19:17 +0000
404@@ -209,7 +209,10 @@
405 is "NEW".
406
407 >>> cdrkit_ubuntu = ubuntu.getSourcePackage('cdrkit')
408- >>> cdrkit_release = cdrkit_ubuntu.currentrelease.sourcepackagerelease
409+ >>> cdrkit_release = factory.makeSourcePackageRelease(
410+ ... sourcepackagename=cdrkit_ubuntu.sourcepackagename,
411+ ... distroseries=cdrkit_ubuntu.distribution['warty'],
412+ ... archive=cdrkit_ubuntu.distribution.main_archive, version='1.0')
413
414 >>> cdrkit_bug_id = cdrkit_ubuntu.createBug(bug_params).id
415
416
417=== modified file 'lib/lp/soyuz/stories/soyuz/xx-distributionsourcepackagerelease-pages.txt'
418--- lib/lp/soyuz/stories/soyuz/xx-distributionsourcepackagerelease-pages.txt 2011-01-12 23:07:40 +0000
419+++ lib/lp/soyuz/stories/soyuz/xx-distributionsourcepackagerelease-pages.txt 2011-03-02 05:19:17 +0000
420@@ -12,6 +12,7 @@
421 >>> stp = SoyuzTestPublisher()
422 >>> login('foo.bar@canonical.com')
423 >>> stp.prepareBreezyAutotest()
424+ >>> stp.distroseries = stp.ubuntutest.currentseries
425 >>> source = stp.getPubSource('testing-dspr', version='1.0')
426 >>> source.setPublished()
427 >>> binaries = stp.getPubBinaries(pub_source=source)