launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04331
[Merge] lp:~wgrant/launchpad/retarget-bugtask-model into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/retarget-bugtask-model into lp:launchpad with lp:~wgrant/launchpad/destroy-bugtask-markers as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/retarget-bugtask-model/+merge/68761
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.
--
https://code.launchpad.net/~wgrant/launchpad/retarget-bugtask-model/+merge/68761
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/retarget-bugtask-model into lp:launchpad.
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-07-22 00:17:11 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-07-22 00:17:15 +0000
@@ -14,7 +14,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',
@@ -267,9 +268,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:
@@ -290,6 +291,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`."""
@@ -332,7 +361,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)
@@ -377,7 +406,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(
@@ -402,7 +431,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
@@ -484,6 +513,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',
@@ -1078,22 +1109,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 00:17:11 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2011-07-22 00:17:15 +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,
)
@@ -1650,3 +1657,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 00:17:15 +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 00:17:15 +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-22 00:17:11 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py 2011-07-22 00:17:15 +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
