launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug711362 into lp:launchpad/db-devel with lp:~gary/launchpad/move-bugnotificationlevel-enum as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #711362 Deleting a structural subscription should cascade-delete its filters
  https://bugs.launchpad.net/bugs/711362

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug711362/+merge/48232

This branch addresses the linked bug.  It was not possible to delete structural subscriptions if they had filters, because the deletion would not cascade.  This now cascades the delete.


-- 
https://code.launchpad.net/~gary/launchpad/bug711362/+merge/48232
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug711362 into lp:launchpad/db-devel.

=== modified file 'lib/lp/bugs/interfaces/structuralsubscription.py'
--- lib/lp/bugs/interfaces/structuralsubscription.py	2011-02-01 19:42:24 +0000
+++ lib/lp/bugs/interfaces/structuralsubscription.py	2011-02-01 19:42:25 +0000
@@ -21,6 +21,7 @@
 from lazr.restful.declarations import (
     call_with,
     export_as_webservice_entry,
+    export_destructor_operation,
     export_factory_operation,
     export_read_operation,
     export_write_operation,
@@ -99,6 +100,10 @@
     def newBugFilter():
         """Returns a new `BugSubscriptionFilter` for this subscription."""
 
+    @export_destructor_operation()
+    def delete():
+        """Delete this structural subscription filter."""
+
 
 class IStructuralSubscription(
     IStructuralSubscriptionPublic, IStructuralSubscriptionRestricted):

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-02-01 19:42:24 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-02-01 19:42:25 +0000
@@ -170,6 +170,12 @@
         IStore(StructuralSubscription).flush()
         return bug_filter
 
+    def delete(self):
+        store = Store.of(self)
+        store.find(
+            BugSubscriptionFilter,
+            BugSubscriptionFilter.structural_subscription == self).remove()
+        store.remove(self)
 
 class DistroSeriesTargetHelper:
     """A helper for `IDistroSeries`s."""
@@ -419,10 +425,7 @@
             raise DeleteSubscriptionError(
                 "%s is not subscribed to %s." % (
                 subscriber.name, self.displayname))
-        # XXX gary 2011-02-01 bug=711362
-        # We should delete all associated filters.
-        store = Store.of(subscription_to_remove)
-        store.remove(subscription_to_remove)
+        subscription_to_remove.delete()
 
     def getSubscription(self, person):
         """See `IStructuralSubscriptionTarget`."""

=== modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
--- lib/lp/bugs/tests/test_structuralsubscription.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/tests/test_structuralsubscription.py	2011-02-01 19:42:25 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from storm.store import Store
 from zope.security.interfaces import Unauthorized
 
 from canonical.testing.layers import DatabaseFunctionalLayer
@@ -27,6 +28,37 @@
             self.subscription = self.product.addSubscription(
                 self.product.owner, self.product.owner)
 
+    def test_delete_requires_Edit_permission(self):
+        # delete() is only available to the subscriber.
+        with anonymous_logged_in():
+            self.assertRaises(Unauthorized, lambda: self.subscription.delete)
+        with person_logged_in(self.factory.makePerson()):
+            self.assertRaises(Unauthorized, lambda: self.subscription.delete)
+
+    def test_simple_delete(self):
+        with person_logged_in(self.product.owner):
+            self.subscription.delete()
+            self.assertEqual(
+                self.product.getSubscription(self.product.owner), None)
+
+    def test_delete_cascades_to_filters(self):
+        with person_logged_in(self.product.owner):
+            subscription_id = self.subscription.id
+            self.subscription.newBugFilter()
+            self.subscription.delete()
+            self.assertEqual(
+                self.product.getSubscription(self.product.owner), None)
+            store = Store.of(self.product)
+            # The original delete fails if it does not remove the
+            # filter first, but this is a nice double-check.
+            self.assertEqual(
+                store.find(
+                    BugSubscriptionFilter,
+                    BugSubscriptionFilter.structural_subscription_id ==
+                        subscription_id
+                    ).one(),
+                None)
+
     def test_bug_filters_empty(self):
         # The bug_filters attribute is empty to begin with.
         self.assertEqual([], list(self.subscription.bug_filters))