Merge lp:~edwin-grubbs/launchpad/bug-526001-edit-packaging-oops into lp:launchpad

[Edwin Grubbs]

Fix oops when changing the sourcepackage's productseries if the current series is obsolete.

[Edwin Grubbs (edwin-grubbs)]

Summary
-------

Fix bug where an exception is raised if you try to change the
productseries for a sourcepackage, when the current productseries
is obsolete.

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

If the current context.productseries is not in the series_list variable,
add it. The code that converts the series_list into SimpleTerm objects
was moved below that if-statement.

Tests
-----

./bin/test -vv -t 'xx-distribution-packages.txt|xx-also-affects-upstream-default-values.txt|sourcepackage-views.txt|xx-sourcepackage-packaging.txt'

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

* Open https://launchpad.dev/ubuntu/warty/+source/mozilla-firefox/+edit-packaging
    * Set the product series.
* Open the productseries page that you used.
    * Change the status to OBSOLETE.
* Open https://launchpad.dev/ubuntu/warty/+source/mozilla-firefox/+edit-packaging
    * The current project should already be entered, so just click "Continue".
        * The obsolete series should show up as a radio button option
          instead of raising an exception.

[Brad Crittenden (bac)]

Edwin as we discussed on IRC I think we need a test showing the successful working when the productseries is obsolete. Otherwise it looks good.

review: Needs Fixing (code)

[Edwin Grubbs (edwin-grubbs)]

Hi Brad,

Thanks for the review. Here are the added tests.

=== modified file 'lib/lp/registry/tests/test_sourcepackage.py'
--- lib/lp/registry/tests/test_sourcepackage.py 2009-12-13 11:55:40 +0000
+++ lib/lp/registry/tests/test_sourcepackage.py 2010-02-24 03:06:12 +0000
@@ -22,6 +22,7 @@
 from lp.code.interfaces.seriessourcepackagebranch import (
     IMakeOfficialBranchLinks)
 from lp.testing import TestCaseWithFactory
+from lp.testing.views import create_initialized_view
 from canonical.testing.layers import DatabaseFunctionalLayer

@@ -238,5 +239,79 @@
             Unauthorized, sourcepackage.setBranch, pocket, branch, registrant)

+class TestSourcePackageViews(TestCaseWithFactory):
+ """Tests for source package view classes."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ TestCaseWithFactory.setUp(self)
+ self.owner = self.factory.makePerson()
+ self.product = self.factory.makeProduct(
+ name='bonkers', displayname='Bonkers', owner=self.owner)
+
+ self.obsolete_productseries = self.factory.makeProductSeries(
+ name='obsolete', product=self.product)
+ self.obsolete_productseries.status = SeriesStatus.OBSOLETE
+
+ self.dev_productseries = self.factory.makeProductSeries(
+ name='current', product=self.product)
+ self.dev_productseries.status = SeriesStatus.DEVELOPMENT
+
+ self.distribution = self.factory.makeDistribution(
+ name='youbuntu', displayname='Youbuntu', owner=self.owner)
+ self.distroseries = self.factory.makeDistroRelease(name='busy',
+ distribution=self.distribution)
+ self.sourcepackagename = self.factory.makeSourcePackageName(
+ name='bonkers')
+ self.package = self.factory.makeSourcePackage(
+ sourcepackagename=self.sourcepackagename,
+ distroseries=self.distroseries)
+
+ def test_editpackaging_obsolete_series_in_vocabulary(self):
+ # The sourcepackage's current product series is included in
+ # the vocabulary even if it is obsolete.
+ self.package.setPackaging(self.obsolete_productseries, self.owner)
+ form = {
+ 'field.product': 'bonkers',
+ 'field.actions.continue': 'Continue',
+ 'field.__visited_steps__': 'sourcepackage_change_upstream_step1',
+ }
+ view = create_initialized_view(
+ self.package, name='+edit-packaging', form=form,
+ principal=self.owner)
+ self.assertEqual([], view.view.errors)
+ self.assertEqual(
+ self.obsolete_productseries,
+ view.view.form_fields['productseries'].field.default,
+ "The form's default productseries must be the current one.")
+ options = [term.token
+ for term in view.view.widgets['productseries'].vocabulary]
+ self.assertEqual(
+ ['trunk', 'current', 'obsolete'], options,
+ "The obsolete series must be in the vocabulary.")
+
+ def test_editpackaging_obsolete_series_not_in_vocabulary(self):
+ # Obsolete productseries are normally not in the vocabulary.
+ form = {
+ ...

[Brad Crittenden (bac)]

Thanks for adding the test.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2010-02-18 20:39:01 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2010-02-24 17:24:21 +0000
@@ -212,6 +212,22 @@
             series for series in self.product.series
             if series.status != SeriesStatus.OBSOLETE
             ]
+
+        # If the product is not being changed, then the current
+        # productseries can be the default choice. Otherwise,
+        # it will not exist in the vocabulary.
+        if (self.context.productseries is not None
+            and self.context.productseries.product == self.product):
+            series_default = self.context.productseries
+            # This only happens for obsolete series, since they aren't
+            # added to the vocabulary normally.
+            if series_default not in series_list:
+                series_list.append(series_default)
+        else:
+            series_default = None
+
+        # Put the development focus at the top of the list and create
+        # the vocabulary.
         dev_focus = self.product.development_focus
         if dev_focus in series_list:
             series_list.remove(dev_focus)
@@ -223,15 +239,6 @@
             dev_focus, dev_focus.name, "%s (Recommended)" % dev_focus.name)
         vocab_terms.insert(0, dev_focus_term)
 
-        # If the product is not being changed, then the current
-        # productseries can be the default choice. Otherwise,
-        # it will not exist in the vocabulary.
-        if (self.context.productseries is not None
-            and self.context.productseries.product == self.product):
-            series_default = self.context.productseries
-        else:
-            series_default = None
-
         productseries_choice = Choice(
             __name__='productseries',
             title=_("Series"),

=== modified file 'lib/lp/registry/tests/test_sourcepackage.py'
--- lib/lp/registry/tests/test_sourcepackage.py	2009-12-13 11:55:40 +0000
+++ lib/lp/registry/tests/test_sourcepackage.py	2010-02-24 17:24:21 +0000
@@ -22,6 +22,7 @@
 from lp.code.interfaces.seriessourcepackagebranch import (
     IMakeOfficialBranchLinks)
 from lp.testing import TestCaseWithFactory
+from lp.testing.views import create_initialized_view
 from canonical.testing.layers import DatabaseFunctionalLayer
 
 
@@ -238,5 +239,79 @@
             Unauthorized, sourcepackage.setBranch, pocket, branch, registrant)
 
 
+class TestSourcePackageViews(TestCaseWithFactory):
+    """Tests for source package view classes."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.owner = self.factory.makePerson()
+        self.product = self.factory.makeProduct(
+            name='bonkers', displayname='Bonkers', owner=self.owner)
+
+        self.obsolete_productseries = self.factory.makeProductSeries(
+            name='obsolete', product=self.product)
+        self.obsolete_productseries.status = SeriesStatus.OBSOLETE
+
+        self.dev_productseries = self.factory.makeProductSeries(
+            name='current', product=self.product)
+        self.dev_productseries.status = SeriesStatus.DEVELOPMENT
+
+        self.distribution = self.factory.makeDistribution(
+            name='youbuntu', displayname='Youbuntu', owner=self.owner)
+        self.distroseries = self.factory.makeDistroRelease(name='busy',
+            distribution=self.distribution)
+        self.sourcepackagename = self.factory.makeSourcePackageName(
+            name='bonkers')
+        self.package = self.factory.makeSourcePackage(
+            sourcepackagename=self.sourcepackagename,
+            distroseries=self.distroseries)
+
+    def test_editpackaging_obsolete_series_in_vocabulary(self):
+        # The sourcepackage's current product series is included in
+        # the vocabulary even if it is obsolete.
+        self.package.setPackaging(self.obsolete_productseries, self.owner)
+        form = {
+            'field.product': 'bonkers',
+            'field.actions.continue': 'Continue',
+            'field.__visited_steps__': 'sourcepackage_change_upstream_step1',
+            }
+        view = create_initialized_view(
+            self.package, name='+edit-packaging', form=form,
+            principal=self.owner)
+        self.assertEqual([], view.view.errors)
+        self.assertEqual(
+            self.obsolete_productseries,
+            view.view.form_fields['productseries'].field.default,
+            "The form's default productseries must be the current one.")
+        options = [term.token
+                   for term in view.view.widgets['productseries'].vocabulary]
+        self.assertEqual(
+            ['trunk', 'current', 'obsolete'], options,
+            "The obsolete series must be in the vocabulary.")
+
+    def test_editpackaging_obsolete_series_not_in_vocabulary(self):
+        # Obsolete productseries are normally not in the vocabulary.
+        form = {
+            'field.product': 'bonkers',
+            'field.actions.continue': 'Continue',
+            'field.__visited_steps__': 'sourcepackage_change_upstream_step1',
+            }
+        view = create_initialized_view(
+            self.package, name='+edit-packaging', form=form,
+            principal=self.owner)
+        self.assertEqual([], view.view.errors)
+        self.assertEqual(
+            None,
+            view.view.form_fields['productseries'].field.default,
+            "The form's default productseries must be None.")
+        options = [term.token
+                   for term in view.view.widgets['productseries'].vocabulary]
+        self.assertEqual(
+            ['trunk', 'current'], options,
+            "The obsolete series must NOT be in the vocabulary.")
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)