Merge lp:~wgrant/launchpad/retarget-bugtask-model into lp:launchpad

[William Grant]

[r=stevenk][bug=80902][incr] Allow BugTask.transitionToTarget to transition between all non-series targets.

This branch extends BugTask.transitionToTarget to permit most changes. It was previously restricted to not changing the type of a task (eg. product->product, distribution->distributionsourcepackage) because of the IUpstreamBugTask, IDistroBugTask, etc. marker interfaces. But the prerequisite branches remove those, so changing task types is fine now.

transitionToTarget now uses a new bug_target_to_key function, which converts an IBugTarget to a dict of column values. This is the inverse of the existing determine_target, which has been renamed to bug_target_from_key.

I've added tests for the various cases. There are only three that are disallowed now: transition to or from a productseries; transition to or from a distroseries; and transition to or from a sourcepackage, except where it is to or from another sourcepackage in the same distroseries. Transitioning between series is forbidden, as that's what nominations are for.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-07-22 04:36:14 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-07-22 04:53:42 +0000
@@ -13,7 +13,8 @@
     'BugTask',
     'BugTaskSet',
     'bugtask_sort_key',
-    'determine_target',
+    'bug_target_from_key',
+    'bug_target_to_key',
     'get_bug_privacy_filter',
     'get_related_bugtasks_search_params',
     'search_value_to_where_condition',
@@ -266,9 +267,9 @@
     return search_params
 
 
-def determine_target(product, productseries, distribution, distroseries,
-                     sourcepackagename):
-    """Returns the IBugTarget defined by the given arguments."""
+def bug_target_from_key(product, productseries, distribution, distroseries,
+                        sourcepackagename):
+    """Returns the IBugTarget defined by the given DB column values."""
     if product:
         return product
     elif productseries:
@@ -289,6 +290,34 @@
         raise AssertionError("Unable to determine bugtask target.")
 
 
+def bug_target_to_key(target):
+    """Returns the DB column values for an IBugTarget."""
+    values = dict(
+                product=None,
+                productseries=None,
+                distribution=None,
+                distroseries=None,
+                sourcepackagename=None,
+                )
+    if IProduct.providedBy(target):
+        values['product'] = target
+    elif IProductSeries.providedBy(target):
+        values['productseries'] = target
+    elif IDistribution.providedBy(target):
+        values['distribution'] = target
+    elif IDistroSeries.providedBy(target):
+        values['distroseries'] = target
+    elif IDistributionSourcePackage.providedBy(target):
+        values['distribution'] = target.distribution
+        values['sourcepackagename'] = target.sourcepackagename
+    elif ISourcePackage.providedBy(target):
+        values['distroseries'] = target.distroseries
+        values['sourcepackagename'] = target.sourcepackagename
+    else:
+        raise AssertionError("Not an IBugTarget.")
+    return values
+
+
 class BugTaskDelta:
     """See `IBugTaskDelta`."""
 
@@ -317,7 +346,7 @@
 def validate_target_attribute(self, attr, value):
     """Update the targetnamecache."""
     # Don't update targetnamecache during _init().
-    if self._SO_creating:
+    if self._SO_creating or self._inhibit_target_check:
         return value
     # Determine the new target attributes.
     target_params = dict(
@@ -342,7 +371,7 @@
     # Update the target name cache with the potential new target. The
     # attribute changes haven't been made yet, so we need to calculate the
     # target manually.
-    self.updateTargetNameCache(determine_target(**target_params))
+    self.updateTargetNameCache(bug_target_from_key(**target_params))
 
     return value
 
@@ -424,6 +453,8 @@
         "date_left_closed")
     _NON_CONJOINED_STATUSES = (BugTaskStatus.WONTFIX, )
 
+    _inhibit_target_check = False
+
     bug = ForeignKey(dbName='bug', foreignKey='Bug', notNull=True)
     product = ForeignKey(
         dbName='product', foreignKey='Product',
@@ -523,7 +554,7 @@
     @property
     def target(self):
         """See `IBugTask`."""
-        return determine_target(
+        return bug_target_from_key(
             self.product, self.productseries, self.distribution,
             self.distroseries, self.sourcepackagename)
 
@@ -1068,22 +1099,29 @@
             # current target, or reset it to None
             self.milestone = None
 
-        if self.product:
-            if IProduct.providedBy(target):
-                self.product = target
-            else:
-                raise IllegalTarget(
-                    "Upstream bug tasks may only be re-targeted "
-                    "to another project.")
-        else:
-            if (IDistributionSourcePackage.providedBy(target) and
-                (target.distribution == self.target or
-                 target.distribution == self.target.distribution)):
-                self.sourcepackagename = target.sourcepackagename
-            else:
-                raise IllegalTarget(
-                    "Distribution bug tasks may only be re-targeted "
-                    "to a package in the same distribution.")
+        # Check if any series are involved. You can't retarget series
+        # tasks. Except for SourcePackage tasks, which can only be
+        # retargetted to another SourcePackage in the same DistroSeries.
+        interfaces = set(providedBy(target))
+        interfaces.update(providedBy(self.target))
+        if interfaces.intersection((IProductSeries, IDistroSeries)):
+            raise IllegalTarget(
+                "Series tasks may only be created by approving nominations.")
+        elif ISourcePackage in interfaces:
+            if (not ISourcePackage.providedBy(target) or
+                not ISourcePackage.providedBy(self.target) or
+                target.distroseries != self.target.distroseries):
+                raise IllegalTarget(
+                    "Series source package tasks may only be retargetted "
+                    "to another source package in the same series.")
+
+        # Inhibit validate_target_attribute, as we can't set them all
+        # atomically, but we know the final result is correct.
+        self._inhibit_target_check = True
+        for name, value in bug_target_to_key(target).iteritems():
+            setattr(self, name, value)
+        self._inhibit_target_check = False
+        self.updateTargetNameCache()
 
         # After the target has changed, we need to recalculate the maximum bug
         # heat for the new and old targets.

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-07-22 04:34:34 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-07-22 04:53:42 +0000
@@ -6,9 +6,11 @@
 from datetime import timedelta
 import unittest
 
+from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
 from testtools.matchers import Equals
 from zope.component import getUtility
+from zope.event import notify
 from zope.interface import providedBy
 from zope.security.proxy import removeSecurityProxy
 
@@ -35,7 +37,12 @@
     UNRESOLVED_BUGTASK_STATUSES,
     )
 from lp.bugs.interfaces.bugwatch import IBugWatchSet
-from lp.bugs.model.bugtask import build_tag_search_clause
+from lp.bugs.model.bugtask import (
+    bug_target_from_key,
+    bug_target_to_key,
+    build_tag_search_clause,
+    IllegalTarget,
+    )
 from lp.bugs.tests.bug import (
     create_old_bug,
     )
@@ -1638,3 +1645,189 @@
                 bug_task.maybeConfirm()
                 self.assertEqual(BugTaskStatus.CONFIRMED, bug_task.status)
                 self.assertEqual(0, len(recorder.events))
+
+
+class TestTransitionToTarget(TestCaseWithFactory):
+    """Tests for BugTask.transitionToTarget."""
+
+    layer = DatabaseFunctionalLayer
+
+    def makeAndTransition(self, old, new):
+        task = self.factory.makeBugTask(target=old)
+        p = self.factory.makePerson()
+        self.assertEqual(old, task.target)
+        old_state = Snapshot(task, providing=providedBy(task))
+        with person_logged_in(task.owner):
+            task.bug.subscribe(p, p)
+            task.transitionToTarget(new)
+            notify(ObjectModifiedEvent(task, old_state, ["target"]))
+        return task
+
+    def assertTransitionWorks(self, a, b):
+        """Check that a transition between two targets works both ways."""
+        self.assertEqual(b, self.makeAndTransition(a, b).target)
+        self.assertEqual(a, self.makeAndTransition(b, a).target)
+
+    def assertTransitionForbidden(self, a, b):
+        """Check that a transition between two targets fails both ways."""
+        self.assertRaises(IllegalTarget, self.makeAndTransition, a, b)
+        self.assertRaises(IllegalTarget, self.makeAndTransition, b, a)
+
+    def test_product_to_product_works(self):
+        self.assertTransitionWorks(
+            self.factory.makeProduct(),
+            self.factory.makeProduct())
+
+    def test_product_to_distribution_works(self):
+        self.assertTransitionWorks(
+            self.factory.makeProduct(),
+            self.factory.makeDistributionSourcePackage())
+
+    def test_product_to_package_works(self):
+        self.assertTransitionWorks(
+            self.factory.makeProduct(),
+            self.factory.makeDistributionSourcePackage())
+
+    def test_distribution_to_distribution_works(self):
+        self.assertTransitionWorks(
+            self.factory.makeDistribution(),
+            self.factory.makeDistribution())
+
+    def test_distribution_to_package_works(self):
+        distro = self.factory.makeDistribution()
+        dsp = self.factory.makeDistributionSourcePackage(distribution=distro)
+        self.assertEquals(dsp.distribution, distro)
+        self.assertTransitionWorks(distro, dsp)
+
+    def test_package_to_package_works(self):
+        distro = self.factory.makeDistribution()
+        self.assertTransitionWorks(
+            self.factory.makeDistributionSourcePackage(distribution=distro),
+            self.factory.makeDistributionSourcePackage(distribution=distro))
+
+    def test_sourcepackage_to_sourcepackage_in_same_series_works(self):
+        sp1 = self.factory.makeSourcePackage()
+        sp2 = self.factory.makeSourcePackage(distroseries=sp1.distroseries)
+        self.assertTransitionWorks(sp1, sp2)
+
+    def test_different_distros_works(self):
+        self.assertTransitionWorks(
+            self.factory.makeDistributionSourcePackage(),
+            self.factory.makeDistributionSourcePackage())
+
+    def test_cannot_transition_to_productseries(self):
+        product = self.factory.makeProduct()
+        self.assertTransitionForbidden(
+            product,
+            self.factory.makeProductSeries(product=product))
+
+    def test_cannot_transition_to_distroseries(self):
+        distro = self.factory.makeDistribution()
+        series = self.factory.makeDistroSeries(distribution=distro)
+        self.assertTransitionForbidden(distro, series)
+
+    def test_cannot_transition_to_sourcepackage(self):
+        dsp = self.factory.makeDistributionSourcePackage()
+        series = self.factory.makeDistroSeries(distribution=dsp.distribution)
+        sp = self.factory.makeSourcePackage(
+            distroseries=series, sourcepackagename=dsp.sourcepackagename)
+        self.assertTransitionForbidden(dsp, sp)
+
+    def test_cannot_transition_to_sourcepackage_in_different_series(self):
+        distro = self.factory.makeDistribution()
+        ds1 = self.factory.makeDistroSeries(distribution=distro)
+        sp1 = self.factory.makeSourcePackage(distroseries=ds1)
+        ds2 = self.factory.makeDistroSeries(distribution=distro)
+        sp2 = self.factory.makeSourcePackage(distroseries=ds2)
+        self.assertTransitionForbidden(sp1, sp2)
+
+
+class TestBugTargetKeys(TestCaseWithFactory):
+    """Tests for bug_target_to_key and bug_target_from_key."""
+
+    layer = DatabaseFunctionalLayer
+
+    def assertTargetKeyWorks(self, target, flat):
+        """Check that a target flattens to the dict and back."""
+        self.assertEqual(flat, bug_target_to_key(target))
+        self.assertEqual(target, bug_target_from_key(**flat))
+
+    def test_product(self):
+        product = self.factory.makeProduct()
+        self.assertTargetKeyWorks(
+            product,
+            dict(
+                product=product,
+                productseries=None,
+                distribution=None,
+                distroseries=None,
+                sourcepackagename=None,
+                ))
+
+    def test_productseries(self):
+        series = self.factory.makeProductSeries()
+        self.assertTargetKeyWorks(
+            series,
+            dict(
+                product=None,
+                productseries=series,
+                distribution=None,
+                distroseries=None,
+                sourcepackagename=None,
+                ))
+
+    def test_distribution(self):
+        distro = self.factory.makeDistribution()
+        self.assertTargetKeyWorks(
+            distro,
+            dict(
+                product=None,
+                productseries=None,
+                distribution=distro,
+                distroseries=None,
+                sourcepackagename=None,
+                ))
+
+    def test_distroseries(self):
+        distroseries = self.factory.makeDistroSeries()
+        self.assertTargetKeyWorks(
+            distroseries,
+            dict(
+                product=None,
+                productseries=None,
+                distribution=None,
+                distroseries=distroseries,
+                sourcepackagename=None,
+                ))
+
+    def test_distributionsourcepackage(self):
+        dsp = self.factory.makeDistributionSourcePackage()
+        self.assertTargetKeyWorks(
+            dsp,
+            dict(
+                product=None,
+                productseries=None,
+                distribution=dsp.distribution,
+                distroseries=None,
+                sourcepackagename=dsp.sourcepackagename,
+                ))
+
+    def test_sourcepackage(self):
+        sp = self.factory.makeSourcePackage()
+        self.assertTargetKeyWorks(
+            sp,
+            dict(
+                product=None,
+                productseries=None,
+                distribution=None,
+                distroseries=sp.distroseries,
+                sourcepackagename=sp.sourcepackagename,
+                ))
+
+    def test_no_key_for_non_targets(self):
+        self.assertRaises(
+            AssertionError, bug_target_to_key, self.factory.makePerson())
+
+    def test_no_target_for_bad_keys(self):
+        self.assertRaises(
+            AssertionError, bug_target_from_key, None, None, None, None, None)

=== modified file 'lib/lp/bugs/scripts/bugtasktargetnamecaches.py'
--- lib/lp/bugs/scripts/bugtasktargetnamecaches.py	2011-02-14 22:26:18 +0000
+++ lib/lp/bugs/scripts/bugtasktargetnamecaches.py	2011-07-22 04:53:42 +0000
@@ -18,7 +18,7 @@
 from canonical.launchpad.utilities.looptuner import LoopTuner
 from lp.bugs.model.bugtask import (
     BugTask,
-    determine_target,
+    bug_target_from_key,
     )
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
@@ -104,7 +104,7 @@
             target_objects = (
                 (store.get(cls, id) if id is not None else None)
                 for cls, id in zip(target_classes, target_bits))
-            target = determine_target(*target_objects)
+            target = bug_target_from_key(*target_objects)
             new_name = target.bugtargetdisplayname
             cached_names.discard(new_name)
             # If there are any outdated names cached, update them all in

=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt	2011-07-13 06:08:16 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt	2011-07-22 04:53:42 +0000
@@ -477,27 +477,6 @@
     >>> print task['target_link']
     http://api.../debian/+source/alsa-utils
 
-Just like in the web UI, we can't change a distribution task
-into an upstream task.
-
-    >>> print webservice.named_post(
-    ...     task['self_link'], 'transitionToTarget',
-    ...     target=webservice.getAbsoluteUrl('/bzr'))
-    HTTP/1.1 400 Bad Request
-    ...
-    Distribution bug tasks may only be re-targeted to
-    a package in the same distribution.
-
-We can't move a task from one distribution to another.
-
-    >>> print webservice.named_post(
-    ...     task['self_link'], 'transitionToTarget',
-    ...     target=webservice.getAbsoluteUrl('/ubuntu/+source/alsa-utils'))
-    HTTP/1.1 400 Bad Request
-    ...
-    Distribution bug tasks may only be re-targeted to
-    a package in the same distribution.
-
 We can change an upstream task to target a different project.
 
     >>> product_bugtask = webservice.get(
@@ -508,16 +487,6 @@
     HTTP/1.1 200 Ok
     ...
 
-But we can't make it into a distro task.
-
-    >>> print webservice.named_post(
-    ...     product_bugtask['self_link'].replace('jokosher', 'bzr'),
-    ...     'transitionToTarget',
-    ...     target=webservice.getAbsoluteUrl('/ubuntu/+source/alsa-utils'))
-    HTTP/1.1 400 Bad Request
-    ...
-    Upstream bug tasks may only be re-targeted to another project.
-
 If the milestone of a task is on a target other than the new
 target, we reset it in order to avoid data inconsistencies.
 

=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py	2011-07-21 07:11:09 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py	2011-07-22 04:53:42 +0000
@@ -13,7 +13,6 @@
 from lazr.lifecycle.snapshot import Snapshot
 from storm.store import Store
 from testtools.matchers import Not
-from zope.component import getUtility
 from zope.event import notify
 from zope.interface import providedBy