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
=== modified file 'lib/lp/registry/browser/packaging.py'
--- lib/lp/registry/browser/packaging.py 2009-10-16 15:00:55 +0000
+++ lib/lp/registry/browser/packaging.py 2009-10-19 21:23:14 +0000
@@ -14,6 +14,7 @@
14 IPackaging, IPackagingUtil)14 IPackaging, IPackagingUtil)
15from canonical.launchpad.webapp import canonical_url15from canonical.launchpad.webapp import canonical_url
16from canonical.launchpad.webapp.launchpadform import action, LaunchpadFormView16from canonical.launchpad.webapp.launchpadform import action, LaunchpadFormView
17from canonical.launchpad.webapp.menu import structured
1718
1819
19class PackagingAddView(LaunchpadFormView):20class PackagingAddView(LaunchpadFormView):
@@ -27,17 +28,45 @@
2728
28 page_title = label29 page_title = label
2930
31 @property
32 def next_url(self):
33 """See `LaunchpadFormView`."""
34 return canonical_url(self.context)
35
36 cancel_url = next_url
37
30 def validate(self, data):38 def validate(self, data):
31 productseries = self.context39 productseries = self.context
32 sourcepackagename = data['sourcepackagename']40 sourcepackagename = data.get('sourcepackagename', None)
33 distroseries = data['distroseries']41 distroseries = data['distroseries']
34 packaging = data['packaging']42 if sourcepackagename is None:
3543 message = "You must choose the source package name."
36 if getUtility(IPackagingUtil).packagingEntryExists(44 self.setFieldError('sourcepackagename', message)
37 productseries, sourcepackagename, distroseries):45 packaging_util = getUtility(IPackagingUtil)
46 if packaging_util.packagingEntryExists(
47 productseries=productseries,
48 sourcepackagename=sourcepackagename,
49 distroseries=distroseries):
50 # The series packaging conflicts with itself.
38 self.addError(_(51 self.addError(_(
39 "This series is already packaged in %s" %52 "This series is already packaged in %s." %
40 distroseries.displayname))53 distroseries.displayname))
54 elif packaging_util.packagingEntryExists(
55 sourcepackagename=sourcepackagename,
56 distroseries=distroseries):
57 # The series package conflicts with another series.
58 sourcepackage = distroseries.getSourcePackage(
59 sourcepackagename.name)
60 self.addError(structured(
61 'The <a href="%s">%s</a> package in %s is already linked to '
62 'another series.' %
63 (canonical_url(sourcepackage),
64 sourcepackagename.name,
65 distroseries.displayname)))
66 else:
67 # The distroseries and sourcepackagename are not already linked
68 # to this series, or any other series.
69 pass
4170
42 @action('Continue', name='continue')71 @action('Continue', name='continue')
43 def continue_action(self, action, data):72 def continue_action(self, action, data):
@@ -45,4 +74,3 @@
45 getUtility(IPackagingUtil).createPackaging(74 getUtility(IPackagingUtil).createPackaging(
46 productseries, data['sourcepackagename'], data['distroseries'],75 productseries, data['sourcepackagename'], data['distroseries'],
47 data['packaging'], owner=self.user)76 data['packaging'], owner=self.user)
48 self.next_url = canonical_url(self.context)
4977
=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
--- lib/lp/registry/browser/tests/productseries-views.txt 2009-10-02 13:08:35 +0000
+++ lib/lp/registry/browser/tests/productseries-views.txt 2009-10-19 21:23:14 +0000
@@ -358,7 +358,7 @@
358Linking packages358Linking packages
359----------------359----------------
360360
361Distrobution sourcepackages can be linked to product series using the361Distro series sourcepackages can be linked to product series using the
362+addpackage named view.362+addpackage named view.
363363
364 >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu364 >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
@@ -385,6 +385,9 @@
385 >>> print view.field_names385 >>> print view.field_names
386 ['distroseries', 'sourcepackagename', 'packaging']386 ['distroseries', 'sourcepackagename', 'packaging']
387387
388 >>> print view.cancel_url
389 http://launchpad.dev/hot/hotter
390
388 >>> form = {391 >>> form = {
389 ... 'field.distroseries': 'ubuntu/hoary',392 ... 'field.distroseries': 'ubuntu/hoary',
390 ... 'field.sourcepackagename': 'hot',393 ... 'field.sourcepackagename': 'hot',
@@ -399,17 +402,51 @@
399 ... print package.name402 ... print package.name
400 hot403 hot
401404
402It is an error to link a series to the same package twice.405 >>> transaction.commit()
403406
404 >>> form = {407It is an error to link a series to the same package and distro series twice.
405 ... 'field.distroseries': 'ubuntu/hoary',408
406 ... 'field.sourcepackagename': 'hot',409 >>> form = {
407 ... 'field.packaging': 'Primary Product',410 ... 'field.distroseries': 'ubuntu/hoary',
408 ... 'field.actions.continue': 'Continue',411 ... 'field.sourcepackagename': 'hot',
409 ... }412 ... 'field.packaging': 'Primary Product',
410 >>> view = create_initialized_view(413 ... 'field.actions.continue': 'Continue',
411 ... productseries, '+addpackage', form=form)414 ... }
412 >>> for error in view.errors:415 >>> view = create_initialized_view(
413 ... print error416 ... productseries, '+addpackage', form=form)
414 This series is already packaged in Hoary417 >>> for error in view.errors:
415418 ... print error
419 This series is already packaged in Hoary.
420
421Once a distro series sourcepackage is linked to a product series, no other
422product series can link to it.
423
424 >>> other_productseries = factory.makeProductSeries(
425 ... product=product, name='hotest')
426 >>> form = {
427 ... 'field.distroseries': 'ubuntu/hoary',
428 ... 'field.sourcepackagename': 'hot',
429 ... 'field.packaging': 'Primary Product',
430 ... 'field.actions.continue': 'Continue',
431 ... }
432 >>> view = create_initialized_view(
433 ... other_productseries, '+addpackage', form=form)
434 >>> for error in view.errors:
435 ... print error
436 The <a href=".../hoary/+source/hot">hot</a> package in Hoary is already
437 linked to another series.
438
439A source package name must be provided.
440
441 >>> form = {
442 ... 'field.distroseries': 'ubuntu/hoary',
443 ... 'field.sourcepackagename': '',
444 ... 'field.packaging': 'Primary Product',
445 ... 'field.actions.continue': 'Continue',
446 ... }
447 >>> view = create_initialized_view(
448 ... productseries, '+addpackage', form=form)
449 >>> for error in view.errors:
450 ... print error
451 ('sourcepackagename', u'Source Package Name', RequiredMissing())
452 You must choose the source package name.
416453
=== modified file 'lib/lp/registry/doc/sourcepackage.txt'
--- lib/lp/registry/doc/sourcepackage.txt 2009-10-16 20:33:56 +0000
+++ lib/lp/registry/doc/sourcepackage.txt 2009-10-19 21:23:14 +0000
@@ -453,21 +453,6 @@
453 >>> print warty_firefox.direct_packaging.productseries.title453 >>> print warty_firefox.direct_packaging.productseries.title
454 Mozilla Firefox trunk series454 Mozilla Firefox trunk series
455455
456If multiple product series link to a sourcepackage, direct_packaging
457returns the last packaging added:
458
459 >>> from lp.registry.interfaces.product import IProductSet
460 >>> firefox_product = getUtility(IProductSet).getByName('firefox')
461 >>> firefox_trunk = firefox_product.getSeries('trunk')
462 >>> PackagingUtil().createPackaging(
463 ... sourcepackagename=firefox,
464 ... distroseries=hoary,
465 ... productseries=firefox_trunk,
466 ... packaging=PackagingType.PRIME,
467 ... owner=foobar)
468 >>> print hoary_firefox_one.direct_packaging.productseries.title
469 Mozilla Firefox trunk series
470
471456
472Release History457Release History
473---------------458---------------
474459
=== modified file 'lib/lp/registry/interfaces/packaging.py'
--- lib/lp/registry/interfaces/packaging.py 2009-10-16 15:00:55 +0000
+++ lib/lp/registry/interfaces/packaging.py 2009-10-19 21:23:14 +0000
@@ -73,7 +73,10 @@
73 vocabulary='DistroSeries')73 vocabulary='DistroSeries')
7474
75 packaging = Choice(75 packaging = Choice(
76 title=_('Packaging'), required=True, vocabulary=PackagingType)76 title=_('Packaging'), required=True, vocabulary=PackagingType,
77 description=_(
78 "Is the project the primary content of the source package, "
79 "or does the source package include the work of other projects?"))
7780
78 datecreated = Datetime(81 datecreated = Datetime(
79 title=_('Date Created'), required=True, readonly=True)82 title=_('Date Created'), required=True, readonly=True)
@@ -92,6 +95,11 @@
92 def deletePackaging(productseries, sourcepackagename, distroseries):95 def deletePackaging(productseries, sourcepackagename, distroseries):
93 """Delete a packaging entry."""96 """Delete a packaging entry."""
9497
95 def packagingEntryExists(productseries, sourcepackagename,98 def packagingEntryExists(sourcepackagename, distroseries,
96 distroseries):99 productseries=None):
97 """Does this packaging entry already exists?"""100 """Does this packaging entry already exists?
101
102 A sourcepackagename is unique to a distroseries. Passing the
103 productseries argument verifies that the packaging entry exists and
104 that it is for the productseries
105 """
98106
=== modified file 'lib/lp/registry/model/packaging.py'
--- lib/lp/registry/model/packaging.py 2009-10-16 15:00:55 +0000
+++ lib/lp/registry/model/packaging.py 2009-10-19 21:23:14 +0000
@@ -41,7 +41,8 @@
41 datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)41 datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
42 owner = ForeignKey(42 owner = ForeignKey(
43 dbName='owner', foreignKey='Person',43 dbName='owner', foreignKey='Person',
44 storm_validator=validate_public_person, notNull=False, default=DEFAULT)44 storm_validator=validate_public_person,
45 notNull=False, default=DEFAULT)
4546
46 @property47 @property
47 def sourcepackage(self):48 def sourcepackage(self):
@@ -56,7 +57,15 @@
5657
57 def createPackaging(self, productseries, sourcepackagename,58 def createPackaging(self, productseries, sourcepackagename,
58 distroseries, packaging, owner):59 distroseries, packaging, owner):
59 """See `IPackaging`."""60 """See `IPackaging`.
61
62 Raises an assertion error if there is already packaging for
63 the sourcepackagename in the distroseries.
64 """
65 if self.packagingEntryExists(sourcepackagename, distroseries):
66 raise AssertionError(
67 "A packaging entry for %s in %s already exists." %
68 (sourcepackagename.name, distroseries.name))
60 Packaging(productseries=productseries,69 Packaging(productseries=productseries,
61 sourcepackagename=sourcepackagename,70 sourcepackagename=sourcepackagename,
62 distroseries=distroseries,71 distroseries=distroseries,
@@ -77,14 +86,15 @@
77 distroseries.parent.name, distroseries.name))86 distroseries.parent.name, distroseries.name))
78 packaging.destroySelf()87 packaging.destroySelf()
7988
80 def packagingEntryExists(self, productseries, sourcepackagename,89 def packagingEntryExists(self, sourcepackagename, distroseries,
81 distroseries):90 productseries=None):
82 """See `IPackaging`."""91 """See `IPackaging`."""
83 result = Packaging.selectOneBy(92 criteria = dict(
84 productseries=productseries,
85 sourcepackagename=sourcepackagename,93 sourcepackagename=sourcepackagename,
86 distroseries=distroseries)94 distroseries=distroseries)
95 if productseries is not None:
96 criteria['productseries'] = productseries
97 result = Packaging.selectOneBy(**criteria)
87 if result is None:98 if result is None:
88 return False99 return False
89 return True100 return True
90
91101
=== modified file 'lib/lp/registry/tests/test_packaging.py'
--- lib/lp/registry/tests/test_packaging.py 2009-10-16 15:00:55 +0000
+++ lib/lp/registry/tests/test_packaging.py 2009-10-19 21:23:14 +0000
@@ -5,26 +5,110 @@
55
6__metaclass__ = type6__metaclass__ = type
77
8from unittest import TestCase, TestLoader8from unittest import TestLoader
99
10from zope.component import getUtility10from zope.component import getUtility
1111
12from lp.registry.interfaces.distribution import IDistributionSet12from lp.registry.interfaces.distribution import IDistributionSet
13from lp.registry.interfaces.packaging import IPackagingUtil13from lp.registry.interfaces.packaging import IPackagingUtil, PackagingType
14from lp.registry.interfaces.product import IProductSet14from lp.registry.interfaces.product import IProductSet
15from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet15from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
16from canonical.launchpad.ftests import login16from lp.testing import login, TestCaseWithFactory
17from canonical.testing import LaunchpadFunctionalLayer17
1818from canonical.testing import DatabaseFunctionalLayer
1919
20class TestDeletePackaging(TestCase):20
21class PackagingUtilMixin:
22 """Common items for testing IPackagingUtil."""
23
24 layer = DatabaseFunctionalLayer
25
26 def setUp(self):
27 TestCaseWithFactory.setUp(self)
28 self.packaging_util = getUtility(IPackagingUtil)
29 self.sourcepackagename = self.factory.makeSourcePackageName('sparkle')
30 self.distroseries = self.factory.makeDistroRelease(name='dazzle')
31 self.productseries = self.factory.makeProductSeries(name='glitter')
32 self.owner = self.productseries.product.owner
33
34
35class TestCreatePackaging(PackagingUtilMixin, TestCaseWithFactory):
36 """Test PackagingUtil.packagingEntryExists."""
37
38 def test_CreatePackaging_unique(self):
39 """Packaging is unique distroseries+sourcepackagename."""
40 self.packaging_util.createPackaging(
41 self.productseries, self.sourcepackagename, self.distroseries,
42 PackagingType.PRIME, owner=self.owner)
43 sourcepackage = self.distroseries.getSourcePackage('sparkle')
44 packaging = sourcepackage.direct_packaging
45 self.assertEqual(packaging.distroseries, self.distroseries)
46 self.assertEqual(packaging.sourcepackagename, self.sourcepackagename)
47 self.assertEqual(packaging.productseries, self.productseries)
48
49 def test_CreatePackaging_assert_unique(self):
50 """Assert unique distroseries+sourcepackagename."""
51 self.packaging_util.createPackaging(
52 self.productseries, self.sourcepackagename, self.distroseries,
53 PackagingType.PRIME, owner=self.owner)
54 self.assertRaises(
55 AssertionError, self.packaging_util.createPackaging,
56 self.productseries, self.sourcepackagename, self.distroseries,
57 PackagingType.PRIME, self.owner)
58
59
60class TestPackagingEntryExists(PackagingUtilMixin, TestCaseWithFactory):
61 """Test PackagingUtil.packagingEntryExists."""
62
63 def setUpPackaging(self):
64 self.packaging_util.createPackaging(
65 self.productseries, self.sourcepackagename, self.distroseries,
66 PackagingType.PRIME, owner=self.owner)
67
68 def test_packagingEntryExists_false(self):
69 """Verify that non-existent entries are false."""
70 self.assertFalse(
71 self.packaging_util.packagingEntryExists(
72 sourcepackagename=self.sourcepackagename,
73 distroseries=self.distroseries))
74
75 def test_packagingEntryExists_unique(self):
76 """Packaging entries are unique to distroseries+sourcepackagename."""
77 self.setUpPackaging()
78 self.assertTrue(
79 self.packaging_util.packagingEntryExists(
80 sourcepackagename=self.sourcepackagename,
81 distroseries=self.distroseries))
82 other_distroseries = self.factory.makeDistroRelease(name='shimmer')
83 self.assertFalse(
84 self.packaging_util.packagingEntryExists(
85 sourcepackagename=self.sourcepackagename,
86 distroseries=other_distroseries))
87
88 def test_packagingEntryExists_specific(self):
89 """Packaging entries are also specifc to both kinds of series."""
90 self.setUpPackaging()
91 self.assertTrue(
92 self.packaging_util.packagingEntryExists(
93 sourcepackagename=self.sourcepackagename,
94 distroseries=self.distroseries,
95 productseries=self.productseries))
96 other_productseries = self.factory.makeProductSeries(name='flash')
97 self.assertFalse(
98 self.packaging_util.packagingEntryExists(
99 sourcepackagename=self.sourcepackagename,
100 distroseries=self.distroseries,
101 productseries=other_productseries))
102
103
104class TestDeletePackaging(TestCaseWithFactory):
21 """Test PackagingUtil.deletePackaging.105 """Test PackagingUtil.deletePackaging.
22106
23 The essential functionality: deleting a Packaging record, is already107 The essential functionality: deleting a Packaging record, is already
24 covered in doctests.108 covered in doctests.
25 """109 """
26110
27 layer = LaunchpadFunctionalLayer111 layer = DatabaseFunctionalLayer
28112
29 def test_deleteNonExistentPackaging(self):113 def test_deleteNonExistentPackaging(self):
30 """Deleting a non-existent Packaging fails.114 """Deleting a non-existent Packaging fails.
@@ -77,4 +161,3 @@
77161
78def test_suite():162def test_suite():
79 return TestLoader().loadTestsFromName(__name__)163 return TestLoader().loadTestsFromName(__name__)
80

Subscribers

People subscribed via source and target branches

to status/vote changes: