launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/bug-805466 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #805466 in Launchpad itself: "URL hacking to +initseries for an already-initialized series OOPSes"
  https://bugs.launchpad.net/launchpad/+bug/805466

For more details, see:
https://code.launchpad.net/~rvb/launchpad/bug-805466/+merge/66925

This branch takes care of a special case where the series to initialize is in a distribution with already initialized series and has no previous_series. In this case, initialization is not possible: the +initseries page will display an error message and InitializeDistroSeries.check will raise an InitializationError.

= Tests =

./bin/test -vvc test_initialize_distroseries test_failure_when_previous_series_none
./bin/test -vvc test_distroseries test_form_hidden_when_previous_series_none

= QA =

Access +initseries on a series in a initialized distribution for which previous_series is None. The page should display an error message.

Access +initseries on a series in a initialized distribution for which previous_series is *not* None. Make previous_series None and then initialize the disto. The page should display an error message.


-- 
https://code.launchpad.net/~rvb/launchpad/bug-805466/+merge/66925
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/bug-805466 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-06-28 17:57:34 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-07-05 16:21:24 +0000
@@ -666,7 +666,8 @@
         distribution = self.context.distribution
         is_first_derivation = not distribution.has_published_sources
         cache['is_first_derivation'] = is_first_derivation
-        if not is_first_derivation:
+        if (not is_first_derivation and
+            self.context.previous_series is not None):
             cache['previous_series'] = seriesToVocab(
                 self.context.previous_series)
             previous_parents = self.context.previous_series.getParentSeries()
@@ -689,10 +690,21 @@
     def show_derivation_form(self):
         return (
             self.is_derived_series_feature_enabled and
+            not self.show_previous_series_empty_message and
             not self.context.isInitializing() and
             not self.context.isInitialized())
 
     @property
+    def show_previous_series_empty_message(self):
+        # There is a problem here:
+        # The distribution already has initialised series and this
+        # distroseries has no previous_series.
+        return (
+            self.is_derived_series_feature_enabled and
+            self.context.distribution.has_published_sources and
+            self.context.previous_series is None)
+
+    @property
     def show_already_initialized_message(self):
         return (
             self.is_derived_series_feature_enabled and

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-06-28 17:57:34 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-07-05 16:21:24 +0000
@@ -640,6 +640,30 @@
                 message.text, EqualsIgnoringWhitespace(
                     u"This series is already being initialized."))
 
+    def test_form_hidden_when_previous_series_none(self):
+        # If the distribution has an initialized series and the
+        # distroseries' previous_series is None: the form is hidden and
+        # the page contains an error message.
+        distroseries = self.factory.makeDistroSeries(
+            previous_series=None)
+        another_distroseries = self.factory.makeDistroSeries(
+            distribution=distroseries.distribution)
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=another_distroseries)
+        view = create_initialized_view(distroseries, "+initseries")
+        flags = {u"soyuz.derived_series_ui.enabled": u"true"}
+        with FeatureFixture(flags):
+            root = html.fromstring(view())
+            self.assertEqual(
+                [], root.cssselect("#initseries-form-container"))
+            # Instead an explanatory message is shown.
+            [message] = root.cssselect("p.error.message")
+            self.assertThat(
+                message.text, EqualsIgnoringWhitespace(
+                    u'Unable to initialise series: the distribution '
+                    u'already has initialised series and this distroseries '
+                    u'has no previous series.'))
+
 
 class TestDistroSeriesInitializeViewAccess(TestCaseWithFactory):
     """Test access to IDS.+initseries."""

=== modified file 'lib/lp/registry/templates/distroseries-initialize.pt'
--- lib/lp/registry/templates/distroseries-initialize.pt	2011-06-14 15:50:12 +0000
+++ lib/lp/registry/templates/distroseries-initialize.pt	2011-07-05 16:21:24 +0000
@@ -50,6 +50,14 @@
       </p>
       </tal:disabled>
 
+      <tal:previous_series_none
+        condition="view/show_previous_series_empty_message">
+      <p class="error message">
+        Unable to initialise series: the distribution already has
+        initialised series and this distroseries has no previous series.
+      </p>
+      </tal:previous_series_none>
+
       <tal:already-initialized condition="view/show_already_initialized_message">
       <p class="error message">
         This series already contains source packages and cannot be

=== modified file 'lib/lp/soyuz/scripts/initialize_distroseries.py'
--- lib/lp/soyuz/scripts/initialize_distroseries.py	2011-07-05 10:40:52 +0000
+++ lib/lp/soyuz/scripts/initialize_distroseries.py	2011-07-05 16:21:24 +0000
@@ -120,7 +120,7 @@
             # Use-case #2.
             self.derivation_parents = [self.distroseries.previous_series]
             self.derivation_parent_ids = [
-                p.id for p in self.derivation_parents]
+                p.id for p in self.derivation_parents if p is not None]
             if self.parent_ids == []:
                 self.parents = (
                     self.distroseries.previous_series.getParentSeries())
@@ -131,6 +131,13 @@
                 ("DistroSeries {child.name} has already been initialized"
                  ".").format(
                     child=self.distroseries))
+        if (self.distroseries.distribution.has_published_sources and
+            self.distroseries.previous_series is None):
+            raise InitializationError(
+                ("DistroSeries {child.name} has no previous series and "
+                 "the distribution already has initialized series"
+                 ".").format(
+                    child=self.distroseries))
         self._checkParents()
         for parent in self.derivation_parents:
             if self.distroseries.distribution.id == parent.distribution.id:

=== modified file 'lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2011-07-05 10:40:52 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py	2011-07-05 16:21:24 +0000
@@ -107,6 +107,25 @@
             "Can not copy distroarchseries from parent, there are already "
             "distroarchseries(s) initialized for this series.", ids.check)
 
+    def test_failure_when_previous_series_none(self):
+        # Initialising a distroseries with no previous_series if the
+        # distribution already has initialized series will error.
+        self.parent, self.parent_das = self.setupParent()
+        child = self.factory.makeDistroSeries(
+            previous_series=None, name='series')
+        another_distroseries = self.factory.makeDistroSeries(
+            distribution=child.distribution)
+        self.factory.makeSourcePackagePublishingHistory(
+             distroseries=another_distroseries)
+        self.factory.makeDistroArchSeries(distroseries=child)
+        ids = InitializeDistroSeries(child, [self.parent.id])
+        self.assertRaisesWithContent(
+            InitializationError,
+            ("DistroSeries series has no previous series and "
+             "the distribution already has initialized series"
+             ".").format(child=child),
+             ids.check)
+
     def test_failure_with_pending_builds(self):
         # If the parent series has pending builds, and the child is a series
         # of the same distribution (which means they share an archive), we