launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/workitems-delete-series into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1163097 in Launchpad itself: "Can not delete a series that contains a milestone that has a deleted workitem targetted to it"
  https://bugs.launchpad.net/launchpad/+bug/1163097

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/workitems-delete-series/+merge/156457

The Milestone deletion code does not respect workitems, which will lead to OOPSes when attempting to delete series that contain a milestone which contains workitems.

I have performed a little bit of clean-up, but not enough to force this branch to be net-negative.
-- 
https://code.launchpad.net/~stevenk/launchpad/workitems-delete-series/+merge/156457
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/workitems-delete-series into lp:launchpad.

=== modified file 'lib/lp/registry/browser/__init__.py'
--- lib/lp/registry/browser/__init__.py	2012-10-19 14:22:36 +0000
+++ lib/lp/registry/browser/__init__.py	2013-04-02 04:35:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Common registry browser helpers and mixins."""
@@ -30,6 +30,7 @@
     LaunchpadEditFormView,
     )
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
 from lp.registry.interfaces.productseries import IProductSeries
@@ -248,6 +249,9 @@
                 nb.milestone = None
         for spec in self._getSpecifications(milestone):
             spec.milestone = None
+        Store.of(milestone).find(
+            SpecificationWorkItem, milestone_id=milestone.id).set(
+                milestone_id=None)
         self._deleteRelease(milestone.product_release)
         milestone.destroySelf()
 

=== modified file 'lib/lp/registry/browser/tests/test_productseries_views.py'
--- lib/lp/registry/browser/tests/test_productseries_views.py	2012-10-18 17:16:49 +0000
+++ lib/lp/registry/browser/tests/test_productseries_views.py	2013-04-02 04:35:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """View tests for ProductSeries pages."""
@@ -104,9 +104,9 @@
         """The displayed branch name should include the unique name."""
         branch = self.factory.makeProductBranch()
         series = self.factory.makeProductSeries(branch=branch)
-        tag = soupmatchers.Tag('series-branch', 'a',
-                               attrs={'id': 'series-branch'},
-                               text='lp://dev/' + branch.unique_name)
+        tag = soupmatchers.Tag(
+            'series-branch', 'a', attrs={'id': 'series-branch'},
+            text='lp://dev/' + branch.unique_name)
         browser = self.getViewBrowser(series)
         self.assertThat(browser.contents, soupmatchers.HTMLContains(tag))
 
@@ -121,8 +121,8 @@
             information_type=InformationType.PROPRIETARY)
         productseries = self.factory.makeProductSeries(product=product)
         ubuntu_series = self.factory.makeUbuntuDistroSeries()
-        sp = self.factory.makeSourcePackage(distroseries=ubuntu_series,
-                                            publish=True)
+        sp = self.factory.makeSourcePackage(
+            distroseries=ubuntu_series, publish=True)
         browser = self.getBrowser(productseries, '+ubuntupkg')
         browser.getControl('Source Package Name').value = (
             sp.sourcepackagename.name)
@@ -156,11 +156,9 @@
         series = self.factory.makeProductSeries(product=product)
         for status in BugTaskStatusSearch.items:
             self.factory.makeBug(
-                series=series, status=status,
-                owner=product.owner)
+                series=series, status=status, owner=product.owner)
         self.factory.makeBug(
-            series=series, status=BugTaskStatus.UNKNOWN,
-            owner=product.owner)
+            series=series, status=BugTaskStatus.UNKNOWN, owner=product.owner)
         expected = [
             (BugTaskStatus.NEW, 1),
             (BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE, 1),
@@ -177,11 +175,28 @@
             (BugTaskStatus.INPROGRESS, 1),
             (BugTaskStatus.FIXCOMMITTED, 1),
             (BugTaskStatus.FIXRELEASED, 1),
-            (BugTaskStatus.UNKNOWN, 1),
-            ]
+            (BugTaskStatus.UNKNOWN, 1)]
         with person_logged_in(product.owner):
             view = create_initialized_view(series, '+status')
             observed = [
                 (status_count.status, status_count.count)
                 for status_count in view.bugtask_status_counts]
         self.assertEqual(expected, observed)
+
+
+class TestProductSeriesDeleteView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_delete_series_with_deleted_workitems(self):
+        series = self.factory.makeProductSeries()
+        milestone = self.factory.makeMilestone(productseries=series)
+        specification = self.factory.makeSpecification(
+            product=milestone.product)
+        workitem = self.factory.makeSpecificationWorkItem(
+            specification=specification, milestone=milestone, deleted=True)
+        form = {'field.actions.delete': 'Delete Series'}
+        with person_logged_in(milestone.product.owner):
+            view = create_initialized_view(series, '+delete', form=form)
+        self.assertEqual([], view.errors)
+        self.assertIs(None, workitem.milestone)

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2013-03-25 05:53:38 +0000
+++ lib/lp/registry/model/milestone.py	2013-04-02 04:35:27 +0000
@@ -15,6 +15,7 @@
 
 import datetime
 import httplib
+from operator import itemgetter
 
 from lazr.restful.declarations import error_status
 from sqlobject import (
@@ -186,13 +187,11 @@
                             SpecificationWorkItem.deleted == False)),
                     all=True)),
             *clauses)
-        ordered_results = results.order_by(Desc(Specification.priority),
-                                           Specification.definition_status,
-                                           Specification.implementation_status,
-                                           Specification.title)
+        ordered_results = results.order_by(
+            Desc(Specification.priority), Specification.definition_status,
+            Specification.implementation_status, Specification.title)
         ordered_results.config(distinct=True)
-        mapper = lambda row: row[0]
-        return DecoratedResultSet(ordered_results, mapper)
+        return DecoratedResultSet(ordered_results, itemgetter(0))
 
     def bugtasks(self, user):
         """The list of non-conjoined bugtasks targeted to this milestone."""