launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/prevent-dsp-series-retargeting into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/prevent-dsp-series-retargeting/+merge/71001

DistroSeries BugNominations are mildly insane; they affect the Distribution and any DistributionSourcePackage tasks on the bug. This has interesting implications when creating new tasks: sometimes we must also create additional tasks for preapproved nominations.

This also now affects task retargeting, as we're opening up inter-pillar targeting as far as practical. But retargeting to/from a distribution may require creation or deletion of tasks, which is something we're not comfortable doing during retargeting just yet.

This branch extends validateTransitionToTarget to reject pillar changes when either the origin or the target is a distribution with series tasks on this bug. sourcepackagename changes within a distribution are fine, as a Storm validator on sourcepackagename takes care of moving across all the series tasks.
-- 
https://code.launchpad.net/~wgrant/launchpad/prevent-dsp-series-retargeting/+merge/71001
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/prevent-dsp-series-retargeting into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-08-02 04:21:43 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-08-10 07:36:54 +0000
@@ -1139,6 +1139,8 @@
 
     def validateTransitionToTarget(self, target):
         """See `IBugTask`."""
+        from lp.registry.model.distroseries import DistroSeries
+
         # Check if any series are involved. You can't retarget series
         # tasks. Except for DistroSeries/SourcePackage tasks, which can
         # only be retargetted to another SourcePackage in the same
@@ -1160,8 +1162,41 @@
                     break
             if len(series) != 1:
                 raise IllegalTarget(
-                    "Distribution series tasks may only be retargetted "
+                    "Distribution series tasks may only be retargeted "
                     "to a package within the same series.")
+        # Because of the mildly insane way that DistroSeries nominations
+        # work (they affect all Distributions and
+        # DistributionSourcePackages), we can't sensibly allow
+        # pillar changes to/from distributions with series tasks on this
+        # bug. That would require us to create or delete tasks.
+        # Changing just the sourcepackagename is OK, though, as a
+        # validator on sourcepackagename will change all related tasks.
+        elif interfaces.intersection(
+            (IDistribution, IDistributionSourcePackage)):
+            # Work out the involved distros (will include None if there
+            # are product tasks).
+            distros = set()
+            for potential_target in (target, self.target):
+                if IDistribution.providedBy(potential_target):
+                    distros.add(potential_target)
+                elif IDistributionSourcePackage.providedBy(potential_target):
+                    distros.add(potential_target.distribution)
+                else:
+                    distros.add(None)
+            if len(distros) > 1:
+                # Multiple distros involved. Check that none of their
+                # series have tasks on this bug.
+                if not Store.of(self).find(
+                    BugTask,
+                    BugTask.bugID == self.bugID,
+                    BugTask.distroseriesID == DistroSeries.id,
+                    DistroSeries.distributionID.is_in(
+                        distro.id for distro in distros if distro),
+                    ).is_empty():
+                    raise IllegalTarget(
+                        "Distribution tasks with corresponding series "
+                        "tasks may only be retargeted to a different "
+                        "package.")
 
         validate_target(self.bug, target)
 

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-08-03 11:00:11 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-08-10 07:36:54 +0000
@@ -1656,20 +1656,24 @@
 
     layer = DatabaseFunctionalLayer
 
-    def makeAndCheckTransition(self, old, new):
+    def makeAndCheckTransition(self, old, new, extra=None):
         task = self.factory.makeBugTask(target=old)
+        if extra:
+            self.factory.makeBugTask(bug=task.bug, target=extra)
         with person_logged_in(task.owner):
             task.validateTransitionToTarget(new)
 
-    def assertTransitionWorks(self, a, b):
+    def assertTransitionWorks(self, a, b, extra=None):
         """Check that a transition between two targets works both ways."""
-        self.makeAndCheckTransition(a, b)
-        self.makeAndCheckTransition(b, a)
+        self.makeAndCheckTransition(a, b, extra)
+        self.makeAndCheckTransition(b, a, extra)
 
-    def assertTransitionForbidden(self, a, b):
+    def assertTransitionForbidden(self, a, b, extra=None):
         """Check that a transition between two targets fails both ways."""
-        self.assertRaises(IllegalTarget, self.makeAndCheckTransition, a, b)
-        self.assertRaises(IllegalTarget, self.makeAndCheckTransition, b, a)
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition, a, b, extra)
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition, b, a, extra)
 
     def test_product_to_product_works(self):
         self.assertTransitionWorks(
@@ -1743,6 +1747,73 @@
         sp2 = self.factory.makeSourcePackage(distroseries=ds2)
         self.assertTransitionForbidden(sp1, sp2)
 
+    # If series tasks for a distribution exist, the pillar of the
+    # non-series task cannot be changed. This is due to the strange
+    # rules around creation of DS/SP tasks.
+    def test_cannot_transition_pillar_of_distro_task_if_series_involved(self):
+        # If a Distribution task has subordinate DistroSeries tasks, its
+        # pillar cannot be changed.
+        series = self.factory.makeDistroSeries()
+        product = self.factory.makeProduct()
+        distro = self.factory.makeDistribution()
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            series.distribution, product, series)
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            series.distribution, distro, series)
+
+    def test_cannot_transition_dsp_task_if_sp_tasks_exist(self):
+        # If a DistributionSourcePackage task has subordinate
+        # SourcePackage tasks, its pillar cannot be changed.
+        sp = self.factory.makeSourcePackage(publish=True)
+        product = self.factory.makeProduct()
+        distro = self.factory.makeDistribution()
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            sp.distribution_sourcepackage, product, sp)
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            sp.distribution_sourcepackage, distro, sp)
+
+    def test_cannot_transition_to_distro_with_series_tasks(self):
+        # If there are any series (DistroSeries or SourcePackage) tasks
+        # for a distribution, you can't transition from another pillar
+        # to that distribution.
+        ds = self.factory.makeDistroSeries()
+        sp1 = self.factory.makeSourcePackage(distroseries=ds, publish=True)
+        sp2 = self.factory.makeSourcePackage(distroseries=ds, publish=True)
+        product = self.factory.makeProduct()
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            product, ds.distribution, ds)
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            product, ds.distribution, sp2)
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            product, sp1.distribution_sourcepackage, ds)
+        self.assertRaises(
+            IllegalTarget, self.makeAndCheckTransition,
+            product, sp1.distribution_sourcepackage, sp2)
+
+    def test_can_transition_dsp_task_with_sp_task_to_different_spn(self):
+        # Even if a Distribution or DistributionSourcePackage task has
+        # subordinate series tasks, the sourcepackagename can be
+        # changed, added or removed. A Storm validator on
+        # sourcepackagename changes all the related tasks.
+        ds = self.factory.makeDistroSeries()
+        sp1 = self.factory.makeSourcePackage(distroseries=ds, publish=True)
+        sp2 = self.factory.makeSourcePackage(distroseries=ds, publish=True)
+        dsp1 = sp1.distribution_sourcepackage
+        dsp2 = sp2.distribution_sourcepackage
+        # The sourcepackagename can be changed
+        self.makeAndCheckTransition(dsp1, dsp2, sp1)
+        self.makeAndCheckTransition(dsp2, dsp1, sp2)
+        # Or removed or added.
+        self.makeAndCheckTransition(dsp1, ds.distribution, sp1)
+        self.makeAndCheckTransition(ds.distribution, dsp1, ds)
+
     def test_validate_target_is_called(self):
         p = self.factory.makeProduct()
         task1 = self.factory.makeBugTask(target=p)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-10 02:42:59 +0000
+++ lib/lp/testing/factory.py	2011-08-10 07:36:54 +0000
@@ -3372,8 +3372,13 @@
             return self.makeSourcePackageName()
         return getUtility(ISourcePackageNameSet).getOrCreateByName(name)
 
-    def makeSourcePackage(self, sourcepackagename=None, distroseries=None):
-        """Make an `ISourcePackage`."""
+    def makeSourcePackage(self, sourcepackagename=None, distroseries=None,
+                          publish=False):
+        """Make an `ISourcePackage`.
+
+        :param publish: if true, create a corresponding
+            SourcePackagePublishingHistory.
+        """
         # Make sure we have a real sourcepackagename object.
         if (sourcepackagename is None or
             isinstance(sourcepackagename, basestring)):
@@ -3381,6 +3386,10 @@
                 sourcepackagename)
         if distroseries is None:
             distroseries = self.makeDistroSeries()
+        if publish:
+            self.makeSourcePackagePublishingHistory(
+                distroseries=distroseries,
+                sourcepackagename=sourcepackagename)
         return distroseries.getSourcePackage(sourcepackagename)
 
     def getAnySourcePackageUrgency(self):