Merge lp:~edwin-grubbs/launchpad/bug-600876-ubuntupkg-integrity-error into lp:launchpad

[Edwin Grubbs]

Summary
-------

thunderbird-locales has packaging links for:
  Maverick -> Mozilla Thunderbird 3.1 series
  Lucid -> Mozilla Thunderbird 3.0 series

https://edge.launchpad.net/ubuntu/+source/thunderbird-locales

If you go to
    https://edge.launchpad.net/thunderbird/3.1/+ubuntupkg
and try to create packaging for 'thunderbird-locales' and
the Lucid distroseries, the view class validate() stops
checking as soon as it finds out that the sourcepackagename
is the same as the existing package on the current distroseries,
which is maverick. The action handler tries to create the packaging
link, since it is only meant to ignore the links on the same
productseries.

Implementation details
----------------------

lib/lp/registry/browser/productseries.py
lib/lp/registry/browser/tests/test_packaging.py

Tests
-----

./bin/test -vv -t test_packaging

Demo and Q/A
------------

On Dev:
* Open https://launchpad.dev/netapplet/+packages
  * Remove the package for warty, since it is on the trunk distroseries.
* Open https://launchpad.dev/netapplet/+addseries
  * Create the series.
  * Click on "Link to Ubuntu package" on the new productseries page.
  * Link it to netapplet/Warty.
* Open https://launchpad.dev/netapplet/trunk/+ubuntupkg
  * Try to link it to netapplet/Warty.
  * You should get this error message instead of an oops:
    "The netapplet package in Warty is already linked to another series."

On Edge:
* https://edge.launchpad.net/thunderbird/3.1/+ubuntupkg
  * The sourcepackagename field should say "thunderbird-locales".
  * Select Lucid from the Series drop-down.
  * Click "Update".
  * You should get an error message instead of an oops.

[Curtis Hovey (sinzui)]

Thank for resolving the contradictory rules to return early when the packaging will be identical (a no change op).

I think the test step is weak (70-77). It looks like it is doing two state transitions. I think the test should directly create the package, then verify that the view return without error when the user attempts to create the identical packaging.

    self.packaging_util.createPackaging(
        productseries=productseries, sourcepackagename=sourcepackagename,
        distroseries=distroseries, packaging=Packaging.PRIMARY, self.product.owner)

review: Approve

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2010-05-11 17:43:20 +0000
+++ lib/lp/registry/browser/productseries.py	2010-07-09 02:33:44 +0000
@@ -511,33 +511,25 @@
         sourcepackagename = data.get('sourcepackagename', None)
         distroseries = self._getSubmittedSeries(data)
 
-        if sourcepackagename == self.default_sourcepackagename:
-            # The data has not changed, so nothing else needs to be done.
+        packaging_util = getUtility(IPackagingUtil)
+        if packaging_util.packagingEntryExists(
+            productseries=productseries,
+            sourcepackagename=sourcepackagename,
+            distroseries=distroseries):
+            # The package already exists. Don't display an error. The
+            # action method will let this go by.
             return
 
-        if sourcepackagename is None:
-            message = "You must choose the source package name."
-            self.setFieldError('sourcepackagename', message)
         # Do not allow users to create links to unpublished Ubuntu packages.
-        elif distroseries.distribution.full_functionality:
+        if (sourcepackagename is not None
+            and distroseries.distribution.full_functionality):
             source_package = distroseries.getSourcePackage(sourcepackagename)
             if source_package.currentrelease is None:
                 message = ("The source package is not published in %s." %
                     distroseries.displayname)
                 self.setFieldError('sourcepackagename', message)
-        else:
-            pass
-        packaging_util = getUtility(IPackagingUtil)
+
         if packaging_util.packagingEntryExists(
-            productseries=productseries,
-            sourcepackagename=sourcepackagename,
-            distroseries=distroseries):
-            # The series packaging conflicts with itself.
-            message = _(
-                "This series is already packaged in %s." %
-                distroseries.displayname)
-            self.setFieldError('sourcepackagename', message)
-        elif packaging_util.packagingEntryExists(
             sourcepackagename=sourcepackagename,
             distroseries=distroseries):
             # The series package conflicts with another series.
@@ -550,10 +542,6 @@
                  sourcepackagename.name,
                  distroseries.displayname))
             self.setFieldError('sourcepackagename', message)
-        else:
-            # The distroseries and sourcepackagename are not already linked
-            # to this series, or any other series.
-            pass
 
     @action('Update', name='continue')
     def continue_action(self, action, data):

=== modified file 'lib/lp/registry/browser/tests/test_packaging.py'
--- lib/lp/registry/browser/tests/test_packaging.py	2010-06-30 19:37:09 +0000
+++ lib/lp/registry/browser/tests/test_packaging.py	2010-07-09 02:33:44 +0000
@@ -10,7 +10,7 @@
 from zope.component import getUtility
 
 from lp.registry.interfaces.distribution import IDistributionSet
-from lp.registry.interfaces.packaging import IPackagingUtil
+from lp.registry.interfaces.packaging import IPackagingUtil, PackagingType
 from lp.registry.interfaces.product import IProductSet
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.testing import TestCaseWithFactory
@@ -41,13 +41,30 @@
             product=self.product, name='hotter')
         self.packaging_util = getUtility(IPackagingUtil)
 
+    def test_no_error_when_trying_to_readd_same_package(self):
+        # There is no reason to display an error when the user's action
+        # wouldn't cause a state change.
+        self.packaging_util.createPackaging(
+            productseries=self.productseries,
+            sourcepackagename=self.sourcepackagename,
+            distroseries=self.hoary, packaging=PackagingType.PRIME,
+            owner=self.product.owner)
+
+        form = {
+            'field.distroseries': self.hoary.name,
+            'field.sourcepackagename': self.sourcepackagename.name,
+            'field.actions.continue': 'Continue',
+            }
+        view = create_initialized_view(
+            self.productseries, '+ubuntupkg', form=form)
+        self.assertEqual([], view.errors)
+
     def test_cannot_link_to_linked_package(self):
         # Once a distro series sourcepackage is linked to a product series,
         # no other product series can link to it.
         form = {
             'field.distroseries': 'hoary',
             'field.sourcepackagename': 'hot',
-            'field.packaging': 'Primary Project',
             'field.actions.continue': 'Continue',
             }
         view = create_initialized_view(
@@ -58,7 +75,6 @@
         form = {
             'field.distroseries': 'hoary',
             'field.sourcepackagename': 'hot',
-            'field.packaging': 'Primary Project',
             'field.actions.continue': 'Continue',
             }
         view = create_initialized_view(
@@ -73,7 +89,6 @@
         form = {
             'field.distroseries': 'hoary',
             'field.sourcepackagename': '',
-            'field.packaging': 'Primary Project',
             'field.actions.continue': 'Continue',
             }
         view = create_initialized_view(
@@ -89,7 +104,6 @@
         form = {
             'field.distroseries': 'hoary',
             'field.sourcepackagename': 'vapor',
-            'field.packaging': 'Primary Project',
             'field.actions.continue': 'Continue',
             }
         view = create_initialized_view(
@@ -105,7 +119,6 @@
         form = {
             'field.distroseries': 'warty',
             'field.sourcepackagename': 'hot',
-            'field.packaging': 'Primary Project',
             'field.actions.continue': 'Continue',
             }
         view = create_initialized_view(