launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/delete-structsub-filters into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/delete-structsub-filters/+merge/164666

Currently, the structural subscription deletion code deletes the filters before it removes itself from the store, but it naively uses ResultSet.remove(), and doesn't delete the underlying parts of the filters, leading to IntegrityErrors when a subscription is removed without manually unsetting the filters first.

Fix the delete method to call delete() on the filters, ensuring that it works correctly.
-- 
https://code.launchpad.net/~stevenk/launchpad/delete-structsub-filters/+merge/164666
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/delete-structsub-filters into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2012-09-28 06:15:58 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2013-05-20 03:17:28 +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).
 
 __metaclass__ = type
@@ -227,10 +227,6 @@
 
         if self._has_other_filters():
             Store.of(self).remove(self)
-        else:
-            # There are no other filters.  We can delete the parent
-            # subscription.
-            self.structural_subscription.delete()
 
     def isMuteAllowed(self, person):
         """See `IBugSubscriptionFilter`."""

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2012-09-07 20:25:51 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2013-05-20 03:17:28 +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).
 
 __metaclass__ = type
@@ -186,9 +186,8 @@
         return bug_filter
 
     def delete(self):
-        store = Store.of(self)
-        self.bug_filters.remove()
-        store.remove(self)
+        [bugfilter.delete() for bugfilter in self.bug_filters]
+        Store.of(self).remove(self)
 
 
 class DistroSeriesTargetHelper:

=== modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
--- lib/lp/bugs/tests/test_structuralsubscription.py	2012-09-19 13:22:42 +0000
+++ lib/lp/bugs/tests/test_structuralsubscription.py	2013-05-20 03:17:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `StructuralSubscription`."""
@@ -81,27 +81,26 @@
     def test_delete_cascades_to_filters(self):
         with person_logged_in(self.product.owner):
             subscription_id = self.subscription.id
-            self.subscription.newBugFilter()
+            bugfilter = self.subscription.newBugFilter()
+            bugfilter.information_types = [InformationType.USERDATA]
             self.subscription.delete()
             self.assertEqual(
                 self.product.getSubscription(self.product.owner), None)
-            store = Store.of(self.product)
             # We know that the filter is gone, because we know the
             # subscription is gone, and the database would have
             # prevented the deletion of a subscription without first
             # deleting the filters.  We'll double-check, to be sure.
-            self.assertEqual(
-                store.find(
-                    BugSubscriptionFilter,
-                    BugSubscriptionFilter.structural_subscription_id ==
-                        subscription_id).one(),
-                None)
+            bugfilter = Store.of(self.product).find(
+                BugSubscriptionFilter,
+                BugSubscriptionFilter.structural_subscription_id ==
+                    subscription_id).one()
+            self.assertIsNone(bugfilter)
 
     def test_bug_filters_default(self):
         # The bug_filters attribute has a default non-filtering bug filter
         # to begin with.
-        self.assertEqual([self.original_filter],
-                         list(self.subscription.bug_filters))
+        self.assertEqual(
+            [self.original_filter], list(self.subscription.bug_filters))
 
     def test_bug_filters(self):
         # The bug_filters attribute returns the BugSubscriptionFilter records