Launchpad itself

Merge lp:~sinzui/launchpad/package-link-validation-1 into lp:launchpad/db-devel

  1. package-link-validation-1
  2. Merge into db-devel
Proposed by Curtis Hovey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~sinzui/launchpad/package-link-validation-1
Merge into: lp:launchpad/db-devel
Diff against target: 394 lines
6 files modified
lib/lp/registry/browser/packaging.py (+35/-7)
lib/lp/registry/browser/tests/productseries-views.txt (+52/-15)
lib/lp/registry/doc/sourcepackage.txt (+0/-15)
lib/lp/registry/interfaces/packaging.py (+12/-4)
lib/lp/registry/model/packaging.py (+17/-7)
lib/lp/registry/tests/test_packaging.py (+92/-9)
To merge this branch: bzr merge lp:~sinzui/launchpad/package-link-validation-1
Reviewer Review Type Date Requested Status
Eleanor Berger (community) code Approve
Review via email: mp+13578@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

This is my second branch to ensure valid upstream package links. There
are many oopses relating to the creation and efforts to fix invalid packages.
The root cause is a bad DB constraint and two views that do not do the
required sanity checks: +addpackage and +ubuntupkg

This branch fixes +addpackage to make sane packages.
(+ubuntupkg need major refactoring and is the scope od a single branch.)

    lp:~sinzui/launchpad/package-link-validation-1
    Diff size: 372
    Launchpad bug: https://bugs.launchpad.net/bugs/344376
                   https://bugs.launchpad.net/bugs/89392
                   https://bugs.launchpad.net/bugs/196774
    Test command: ./bin/test -vv -t 'lp.reg.*(productseries|packaging)'
    Pre-implementation: flacoste
    Target release: 3.1.10

== Fixing upstream packaging links ==

Bug 196774 [It shouldn't be possible to link multiple productseries to a
    sourcepackage in a given distroseries]
    +addpackage and +ubuntupkg do not verify that the SP is unlinked for the
    distroseries.

Bug 344376 [+addpackage oopses if the "Source Package Name" is left blank]
    The most common reason users leave the field blank is that they are
    trying to remove an invalid packaging link.

Bug 89392 [+addpackage form contains nonsensical "Packaging" field]
    The final menu is labeled "Packaging" and contains options "Primary
    Product" and "SourcePackage Includes Product". This makes absolutely no
    sense to a product author.

== Rules ==

Bug 196774 [It shouldn't be possible to link multiple productseries to a
    sourcepackage in a given distroseries]
    Verify that the SP for the distroseries is available for linking.
    If not, provide a link to the current package so that the user can
    investigate it.

Bug 344376 [+addpackage oopses if the "Source Package Name" is left blank]
    Use the validate() method and report a form error if the choice is not
    sane.

Bug 89392 [+addpackage form contains nonsensical "Packaging" field]
    rewrite the form instructions

== QA ==

On staging
    * Visit a productseries
    * Choose (+) Add packaging
    * Verify the packaging field explains primary from source
    * Submit the form without a source package name
    * Verify the form error message explains that the source package must
      be provided
    * Submit the form with Ubuntu Karmic, 'gedit', primary
    * Verify the form error message explains that the gedit is already
      packaged in Ubuntu karmic. Follow the link
    * Verify the “gedit” source package in Karmic page displays.

== Lint ==

Linting changed files:
  lib/lp/registry/browser/packaging.py
  lib/lp/registry/browser/tests/productseries-views.txt
  lib/lp/registry/interfaces/packaging.py
  lib/lp/registry/model/packaging.py
  lib/lp/registry/tests/test_packaging.py

== Test ==

    * lib/lp/registry/browser/tests/productseries-views.txt
    * lib/lp/registry/tests/test_packaging.py

== Implementation ==

    * lib/lp/registry/browser/packaging.py
    * lib/lp/registry/interfaces/packaging.py
    * lib/lp/registry/model/packaging.py

Revision history for this message
Eleanor Berger (intellectronica) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/packaging.py'
2--- lib/lp/registry/browser/packaging.py 2009-10-16 15:00:55 +0000
3+++ lib/lp/registry/browser/packaging.py 2009-10-19 21:23:14 +0000
4@@ -14,6 +14,7 @@
5 IPackaging, IPackagingUtil)
6 from canonical.launchpad.webapp import canonical_url
7 from canonical.launchpad.webapp.launchpadform import action, LaunchpadFormView
8+from canonical.launchpad.webapp.menu import structured
9
10
11 class PackagingAddView(LaunchpadFormView):
12@@ -27,17 +28,45 @@
13
14 page_title = label
15
16+ @property
17+ def next_url(self):
18+ """See `LaunchpadFormView`."""
19+ return canonical_url(self.context)
20+
21+ cancel_url = next_url
22+
23 def validate(self, data):
24 productseries = self.context
25- sourcepackagename = data['sourcepackagename']
26+ sourcepackagename = data.get('sourcepackagename', None)
27 distroseries = data['distroseries']
28- packaging = data['packaging']
29-
30- if getUtility(IPackagingUtil).packagingEntryExists(
31- productseries, sourcepackagename, distroseries):
32+ if sourcepackagename is None:
33+ message = "You must choose the source package name."
34+ self.setFieldError('sourcepackagename', message)
35+ packaging_util = getUtility(IPackagingUtil)
36+ if packaging_util.packagingEntryExists(
37+ productseries=productseries,
38+ sourcepackagename=sourcepackagename,
39+ distroseries=distroseries):
40+ # The series packaging conflicts with itself.
41 self.addError(_(
42- "This series is already packaged in %s" %
43+ "This series is already packaged in %s." %
44 distroseries.displayname))
45+ elif packaging_util.packagingEntryExists(
46+ sourcepackagename=sourcepackagename,
47+ distroseries=distroseries):
48+ # The series package conflicts with another series.
49+ sourcepackage = distroseries.getSourcePackage(
50+ sourcepackagename.name)
51+ self.addError(structured(
52+ 'The <a href="%s">%s</a> package in %s is already linked to '
53+ 'another series.' %
54+ (canonical_url(sourcepackage),
55+ sourcepackagename.name,
56+ distroseries.displayname)))
57+ else:
58+ # The distroseries and sourcepackagename are not already linked
59+ # to this series, or any other series.
60+ pass
61
62 @action('Continue', name='continue')
63 def continue_action(self, action, data):
64@@ -45,4 +74,3 @@
65 getUtility(IPackagingUtil).createPackaging(
66 productseries, data['sourcepackagename'], data['distroseries'],
67 data['packaging'], owner=self.user)
68- self.next_url = canonical_url(self.context)
69
70=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
71--- lib/lp/registry/browser/tests/productseries-views.txt 2009-10-02 13:08:35 +0000
72+++ lib/lp/registry/browser/tests/productseries-views.txt 2009-10-19 21:23:14 +0000
73@@ -358,7 +358,7 @@
74 Linking packages
75 ----------------
76
77-Distrobution sourcepackages can be linked to product series using the
78+Distro series sourcepackages can be linked to product series using the
79 +addpackage named view.
80
81 >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
82@@ -385,6 +385,9 @@
83 >>> print view.field_names
84 ['distroseries', 'sourcepackagename', 'packaging']
85
86+ >>> print view.cancel_url
87+ http://launchpad.dev/hot/hotter
88+
89 >>> form = {
90 ... 'field.distroseries': 'ubuntu/hoary',
91 ... 'field.sourcepackagename': 'hot',
92@@ -399,17 +402,51 @@
93 ... print package.name
94 hot
95
96-It is an error to link a series to the same package twice.
97-
98- >>> form = {
99- ... 'field.distroseries': 'ubuntu/hoary',
100- ... 'field.sourcepackagename': 'hot',
101- ... 'field.packaging': 'Primary Product',
102- ... 'field.actions.continue': 'Continue',
103- ... }
104- >>> view = create_initialized_view(
105- ... productseries, '+addpackage', form=form)
106- >>> for error in view.errors:
107- ... print error
108- This series is already packaged in Hoary
109-
110+ >>> transaction.commit()
111+
112+It is an error to link a series to the same package and distro series twice.
113+
114+ >>> form = {
115+ ... 'field.distroseries': 'ubuntu/hoary',
116+ ... 'field.sourcepackagename': 'hot',
117+ ... 'field.packaging': 'Primary Product',
118+ ... 'field.actions.continue': 'Continue',
119+ ... }
120+ >>> view = create_initialized_view(
121+ ... productseries, '+addpackage', form=form)
122+ >>> for error in view.errors:
123+ ... print error
124+ This series is already packaged in Hoary.
125+
126+Once a distro series sourcepackage is linked to a product series, no other
127+product series can link to it.
128+
129+ >>> other_productseries = factory.makeProductSeries(
130+ ... product=product, name='hotest')
131+ >>> form = {
132+ ... 'field.distroseries': 'ubuntu/hoary',
133+ ... 'field.sourcepackagename': 'hot',
134+ ... 'field.packaging': 'Primary Product',
135+ ... 'field.actions.continue': 'Continue',
136+ ... }
137+ >>> view = create_initialized_view(
138+ ... other_productseries, '+addpackage', form=form)
139+ >>> for error in view.errors:
140+ ... print error
141+ The <a href=".../hoary/+source/hot">hot</a> package in Hoary is already
142+ linked to another series.
143+
144+A source package name must be provided.
145+
146+ >>> form = {
147+ ... 'field.distroseries': 'ubuntu/hoary',
148+ ... 'field.sourcepackagename': '',
149+ ... 'field.packaging': 'Primary Product',
150+ ... 'field.actions.continue': 'Continue',
151+ ... }
152+ >>> view = create_initialized_view(
153+ ... productseries, '+addpackage', form=form)
154+ >>> for error in view.errors:
155+ ... print error
156+ ('sourcepackagename', u'Source Package Name', RequiredMissing())
157+ You must choose the source package name.
158
159=== modified file 'lib/lp/registry/doc/sourcepackage.txt'
160--- lib/lp/registry/doc/sourcepackage.txt 2009-10-16 20:33:56 +0000
161+++ lib/lp/registry/doc/sourcepackage.txt 2009-10-19 21:23:14 +0000
162@@ -453,21 +453,6 @@
163 >>> print warty_firefox.direct_packaging.productseries.title
164 Mozilla Firefox trunk series
165
166-If multiple product series link to a sourcepackage, direct_packaging
167-returns the last packaging added:
168-
169- >>> from lp.registry.interfaces.product import IProductSet
170- >>> firefox_product = getUtility(IProductSet).getByName('firefox')
171- >>> firefox_trunk = firefox_product.getSeries('trunk')
172- >>> PackagingUtil().createPackaging(
173- ... sourcepackagename=firefox,
174- ... distroseries=hoary,
175- ... productseries=firefox_trunk,
176- ... packaging=PackagingType.PRIME,
177- ... owner=foobar)
178- >>> print hoary_firefox_one.direct_packaging.productseries.title
179- Mozilla Firefox trunk series
180-
181
182 Release History
183 ---------------
184
185=== modified file 'lib/lp/registry/interfaces/packaging.py'
186--- lib/lp/registry/interfaces/packaging.py 2009-10-16 15:00:55 +0000
187+++ lib/lp/registry/interfaces/packaging.py 2009-10-19 21:23:14 +0000
188@@ -73,7 +73,10 @@
189 vocabulary='DistroSeries')
190
191 packaging = Choice(
192- title=_('Packaging'), required=True, vocabulary=PackagingType)
193+ title=_('Packaging'), required=True, vocabulary=PackagingType,
194+ description=_(
195+ "Is the project the primary content of the source package, "
196+ "or does the source package include the work of other projects?"))
197
198 datecreated = Datetime(
199 title=_('Date Created'), required=True, readonly=True)
200@@ -92,6 +95,11 @@
201 def deletePackaging(productseries, sourcepackagename, distroseries):
202 """Delete a packaging entry."""
203
204- def packagingEntryExists(productseries, sourcepackagename,
205- distroseries):
206- """Does this packaging entry already exists?"""
207+ def packagingEntryExists(sourcepackagename, distroseries,
208+ productseries=None):
209+ """Does this packaging entry already exists?
210+
211+ A sourcepackagename is unique to a distroseries. Passing the
212+ productseries argument verifies that the packaging entry exists and
213+ that it is for the productseries
214+ """
215
216=== modified file 'lib/lp/registry/model/packaging.py'
217--- lib/lp/registry/model/packaging.py 2009-10-16 15:00:55 +0000
218+++ lib/lp/registry/model/packaging.py 2009-10-19 21:23:14 +0000
219@@ -41,7 +41,8 @@
220 datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
221 owner = ForeignKey(
222 dbName='owner', foreignKey='Person',
223- storm_validator=validate_public_person, notNull=False, default=DEFAULT)
224+ storm_validator=validate_public_person,
225+ notNull=False, default=DEFAULT)
226
227 @property
228 def sourcepackage(self):
229@@ -56,7 +57,15 @@
230
231 def createPackaging(self, productseries, sourcepackagename,
232 distroseries, packaging, owner):
233- """See `IPackaging`."""
234+ """See `IPackaging`.
235+
236+ Raises an assertion error if there is already packaging for
237+ the sourcepackagename in the distroseries.
238+ """
239+ if self.packagingEntryExists(sourcepackagename, distroseries):
240+ raise AssertionError(
241+ "A packaging entry for %s in %s already exists." %
242+ (sourcepackagename.name, distroseries.name))
243 Packaging(productseries=productseries,
244 sourcepackagename=sourcepackagename,
245 distroseries=distroseries,
246@@ -77,14 +86,15 @@
247 distroseries.parent.name, distroseries.name))
248 packaging.destroySelf()
249
250- def packagingEntryExists(self, productseries, sourcepackagename,
251- distroseries):
252+ def packagingEntryExists(self, sourcepackagename, distroseries,
253+ productseries=None):
254 """See `IPackaging`."""
255- result = Packaging.selectOneBy(
256- productseries=productseries,
257+ criteria = dict(
258 sourcepackagename=sourcepackagename,
259 distroseries=distroseries)
260+ if productseries is not None:
261+ criteria['productseries'] = productseries
262+ result = Packaging.selectOneBy(**criteria)
263 if result is None:
264 return False
265 return True
266-
267
268=== modified file 'lib/lp/registry/tests/test_packaging.py'
269--- lib/lp/registry/tests/test_packaging.py 2009-10-16 15:00:55 +0000
270+++ lib/lp/registry/tests/test_packaging.py 2009-10-19 21:23:14 +0000
271@@ -5,26 +5,110 @@
272
273 __metaclass__ = type
274
275-from unittest import TestCase, TestLoader
276+from unittest import TestLoader
277
278 from zope.component import getUtility
279
280 from lp.registry.interfaces.distribution import IDistributionSet
281-from lp.registry.interfaces.packaging import IPackagingUtil
282+from lp.registry.interfaces.packaging import IPackagingUtil, PackagingType
283 from lp.registry.interfaces.product import IProductSet
284 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
285-from canonical.launchpad.ftests import login
286-from canonical.testing import LaunchpadFunctionalLayer
287-
288-
289-class TestDeletePackaging(TestCase):
290+from lp.testing import login, TestCaseWithFactory
291+
292+from canonical.testing import DatabaseFunctionalLayer
293+
294+
295+class PackagingUtilMixin:
296+ """Common items for testing IPackagingUtil."""
297+
298+ layer = DatabaseFunctionalLayer
299+
300+ def setUp(self):
301+ TestCaseWithFactory.setUp(self)
302+ self.packaging_util = getUtility(IPackagingUtil)
303+ self.sourcepackagename = self.factory.makeSourcePackageName('sparkle')
304+ self.distroseries = self.factory.makeDistroRelease(name='dazzle')
305+ self.productseries = self.factory.makeProductSeries(name='glitter')
306+ self.owner = self.productseries.product.owner
307+
308+
309+class TestCreatePackaging(PackagingUtilMixin, TestCaseWithFactory):
310+ """Test PackagingUtil.packagingEntryExists."""
311+
312+ def test_CreatePackaging_unique(self):
313+ """Packaging is unique distroseries+sourcepackagename."""
314+ self.packaging_util.createPackaging(
315+ self.productseries, self.sourcepackagename, self.distroseries,
316+ PackagingType.PRIME, owner=self.owner)
317+ sourcepackage = self.distroseries.getSourcePackage('sparkle')
318+ packaging = sourcepackage.direct_packaging
319+ self.assertEqual(packaging.distroseries, self.distroseries)
320+ self.assertEqual(packaging.sourcepackagename, self.sourcepackagename)
321+ self.assertEqual(packaging.productseries, self.productseries)
322+
323+ def test_CreatePackaging_assert_unique(self):
324+ """Assert unique distroseries+sourcepackagename."""
325+ self.packaging_util.createPackaging(
326+ self.productseries, self.sourcepackagename, self.distroseries,
327+ PackagingType.PRIME, owner=self.owner)
328+ self.assertRaises(
329+ AssertionError, self.packaging_util.createPackaging,
330+ self.productseries, self.sourcepackagename, self.distroseries,
331+ PackagingType.PRIME, self.owner)
332+
333+
334+class TestPackagingEntryExists(PackagingUtilMixin, TestCaseWithFactory):
335+ """Test PackagingUtil.packagingEntryExists."""
336+
337+ def setUpPackaging(self):
338+ self.packaging_util.createPackaging(
339+ self.productseries, self.sourcepackagename, self.distroseries,
340+ PackagingType.PRIME, owner=self.owner)
341+
342+ def test_packagingEntryExists_false(self):
343+ """Verify that non-existent entries are false."""
344+ self.assertFalse(
345+ self.packaging_util.packagingEntryExists(
346+ sourcepackagename=self.sourcepackagename,
347+ distroseries=self.distroseries))
348+
349+ def test_packagingEntryExists_unique(self):
350+ """Packaging entries are unique to distroseries+sourcepackagename."""
351+ self.setUpPackaging()
352+ self.assertTrue(
353+ self.packaging_util.packagingEntryExists(
354+ sourcepackagename=self.sourcepackagename,
355+ distroseries=self.distroseries))
356+ other_distroseries = self.factory.makeDistroRelease(name='shimmer')
357+ self.assertFalse(
358+ self.packaging_util.packagingEntryExists(
359+ sourcepackagename=self.sourcepackagename,
360+ distroseries=other_distroseries))
361+
362+ def test_packagingEntryExists_specific(self):
363+ """Packaging entries are also specifc to both kinds of series."""
364+ self.setUpPackaging()
365+ self.assertTrue(
366+ self.packaging_util.packagingEntryExists(
367+ sourcepackagename=self.sourcepackagename,
368+ distroseries=self.distroseries,
369+ productseries=self.productseries))
370+ other_productseries = self.factory.makeProductSeries(name='flash')
371+ self.assertFalse(
372+ self.packaging_util.packagingEntryExists(
373+ sourcepackagename=self.sourcepackagename,
374+ distroseries=self.distroseries,
375+ productseries=other_productseries))
376+
377+
378+class TestDeletePackaging(TestCaseWithFactory):
379 """Test PackagingUtil.deletePackaging.
380
381 The essential functionality: deleting a Packaging record, is already
382 covered in doctests.
383 """
384
385- layer = LaunchpadFunctionalLayer
386+ layer = DatabaseFunctionalLayer
387
388 def test_deleteNonExistentPackaging(self):
389 """Deleting a non-existent Packaging fails.
390@@ -77,4 +161,3 @@
391
392 def test_suite():
393 return TestLoader().loadTestsFromName(__name__)
394-

Subscribers

People subscribed via source and target branches

to status/vote changes: