← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] ~cjwatson/launchpad:tidy-bugsubscription-filter into launchpad:master

 

Colin Watson has proposed merging ~cjwatson/launchpad:tidy-bugsubscription-filter into launchpad:master.

Commit message:
Tidy up BugSubscriptionFilter creation

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/397168

`BugSubscriptionFilter` now has a constructor that takes a reasonable set of arguments, and the factory has a `makeBugSubscriptionFilter` method which ensures that new filters have a structural subscription.  With this, it is possible to run the test suite with the `BugSubscriptionFilter.structuralsubscription` DB column marked as non-nullable.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:tidy-bugsubscription-filter into launchpad:master.
diff --git a/lib/lp/bugs/adapters/tests/test_bugsubscriptionfilter.py b/lib/lp/bugs/adapters/tests/test_bugsubscriptionfilter.py
index a893bce..fe6585e 100644
--- a/lib/lp/bugs/adapters/tests/test_bugsubscriptionfilter.py
+++ b/lib/lp/bugs/adapters/tests/test_bugsubscriptionfilter.py
@@ -9,13 +9,9 @@ from lp.bugs.adapters.bugsubscriptionfilter import (
     bugsubscriptionfilter_to_distribution,
     bugsubscriptionfilter_to_product,
     )
-from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.product import IProduct
-from lp.testing import (
-    login_person,
-    TestCaseWithFactory,
-    )
+from lp.testing import TestCaseWithFactory
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
@@ -23,17 +19,10 @@ class BugSubscriptionFilterTestCase(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
-    def makeBugSubscriptionFilter(self, target):
-        subscriber = self.factory.makePerson()
-        login_person(subscriber)
-        subscription = target.addBugSubscription(subscriber, subscriber)
-        subscription_filter = BugSubscriptionFilter()
-        subscription_filter.structural_subscription = subscription
-        return subscription_filter
-
     def test_bugsubscriptionfilter_to_product_with_product(self):
         product = self.factory.makeProduct()
-        subscription_filter = self.makeBugSubscriptionFilter(product)
+        subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=product)
         self.assertEqual(
             product, bugsubscriptionfilter_to_product(subscription_filter))
         self.assertEqual(product, IProduct(subscription_filter))
@@ -41,14 +30,16 @@ class BugSubscriptionFilterTestCase(TestCaseWithFactory):
     def test_bugsubscriptionfilter_to_product_with_productseries(self):
         product = self.factory.makeProduct()
         series = product.development_focus
-        subscription_filter = self.makeBugSubscriptionFilter(series)
+        subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=series)
         self.assertEqual(
             product, bugsubscriptionfilter_to_product(subscription_filter))
         self.assertEqual(product, IProduct(subscription_filter))
 
     def test_bugsubscriptionfilter_to_distribution_with_distribution(self):
         distribution = self.factory.makeDistribution()
-        subscription_filter = self.makeBugSubscriptionFilter(distribution)
+        subscription_filter = self.factory.makeBugSubscriptionFilter(
+            distribution)
         self.assertEqual(
             distribution,
             bugsubscriptionfilter_to_distribution(subscription_filter))
@@ -57,7 +48,8 @@ class BugSubscriptionFilterTestCase(TestCaseWithFactory):
     def test_bugsubscriptionfilter_to_distroseries_with_distribution(self):
         distribution = self.factory.makeDistribution()
         series = self.factory.makeDistroSeries(distribution=distribution)
-        subscription_filter = self.makeBugSubscriptionFilter(series)
+        subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=series)
         self.assertEqual(
             distribution,
             bugsubscriptionfilter_to_distribution(subscription_filter))
diff --git a/lib/lp/bugs/model/bugsubscriptionfilter.py b/lib/lp/bugs/model/bugsubscriptionfilter.py
index 712a3c6..3e83aa3 100644
--- a/lib/lp/bugs/model/bugsubscriptionfilter.py
+++ b/lib/lp/bugs/model/bugsubscriptionfilter.py
@@ -81,6 +81,20 @@ class BugSubscriptionFilter(StormBase):
 
     description = Unicode('description')
 
+    def __init__(self, structural_subscription,
+                 bug_notification_level=BugNotificationLevel.COMMENTS,
+                 find_all_tags=False,
+                 include_any_tags=False, exclude_any_tags=False,
+                 other_parameters=None, description=None):
+        super(BugSubscriptionFilter, self).__init__()
+        self.structural_subscription = structural_subscription
+        self.bug_notification_level = bug_notification_level
+        self.find_all_tags = find_all_tags
+        self.include_any_tags = include_any_tags
+        self.exclude_any_tags = exclude_any_tags
+        self.other_parameters = other_parameters
+        self.description = description
+
     def _get_collection(self, cls, attribute):
         kind = getattr(cls, attribute)
         return frozenset(
diff --git a/lib/lp/bugs/model/structuralsubscription.py b/lib/lp/bugs/model/structuralsubscription.py
index c2834e3..59955d3 100644
--- a/lib/lp/bugs/model/structuralsubscription.py
+++ b/lib/lp/bugs/model/structuralsubscription.py
@@ -175,8 +175,7 @@ class StructuralSubscription(Storm):
 
     def newBugFilter(self):
         """See `IStructuralSubscription`."""
-        bug_filter = BugSubscriptionFilter()
-        bug_filter.structural_subscription = self
+        bug_filter = BugSubscriptionFilter(structural_subscription=self)
         # This flush is needed for the web service API.
         IStore(StructuralSubscription).flush()
         return bug_filter
diff --git a/lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py b/lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
index 6125dfc..57cfd6d 100644
--- a/lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
+++ b/lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
@@ -49,15 +49,14 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
     def test_basics(self):
         """Test the basic operation of `BugSubscriptionFilter` objects."""
         # Create.
-        bug_subscription_filter = BugSubscriptionFilter()
-        bug_subscription_filter.structural_subscription = self.subscription
-        bug_subscription_filter.bug_notification_level = (
-            BugNotificationLevel.METADATA)
-        bug_subscription_filter.find_all_tags = True
-        bug_subscription_filter.include_any_tags = True
-        bug_subscription_filter.exclude_any_tags = True
-        bug_subscription_filter.other_parameters = u"foo"
-        bug_subscription_filter.description = u"bar"
+        bug_subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription,
+            bug_notification_level=BugNotificationLevel.METADATA,
+            find_all_tags=True,
+            include_any_tags=True,
+            exclude_any_tags=True,
+            other_parameters=u"foo",
+            description=u"bar")
         # Flush and reload.
         IStore(bug_subscription_filter).flush()
         IStore(bug_subscription_filter).reload(bug_subscription_filter)
@@ -80,15 +79,16 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
 
     def test_description(self):
         """Test the description property."""
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.description = u"foo"
         self.assertEqual(u"foo", bug_subscription_filter.description)
 
     def test_defaults(self):
         """Test the default values of `BugSubscriptionFilter` objects."""
         # Create.
-        bug_subscription_filter = BugSubscriptionFilter()
-        bug_subscription_filter.structural_subscription = self.subscription
+        bug_subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
         # Check.
         self.assertEqual(
             BugNotificationLevel.COMMENTS,
@@ -106,8 +106,8 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
         deleted.
         """
         # This is a second filter for the subscription.
-        bug_subscription_filter = BugSubscriptionFilter()
-        bug_subscription_filter.structural_subscription = self.subscription
+        bug_subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
         bug_subscription_filter.importances = [BugTaskImportance.LOW]
         bug_subscription_filter.statuses = [BugTaskStatus.NEW]
         bug_subscription_filter.tags = [u"foo"]
@@ -157,12 +157,14 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
     def test_statuses(self):
         # The statuses property is a frozenset of the statuses that are
         # filtered upon.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         self.assertEqual(frozenset(), bug_subscription_filter.statuses)
 
     def test_statuses_set(self):
         # Assigning any iterable to statuses updates the database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.statuses = [
             BugTaskStatus.NEW, BugTaskStatus.INCOMPLETE]
         self.assertEqual(
@@ -176,25 +178,29 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
 
     def test_statuses_set_all(self):
         # Setting all statuses is normalized into setting no statuses.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.statuses = list(BugTaskStatus.items)
         self.assertEqual(frozenset(), bug_subscription_filter.statuses)
 
     def test_statuses_set_empty(self):
         # Assigning an empty iterable to statuses updates the database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.statuses = []
         self.assertEqual(frozenset(), bug_subscription_filter.statuses)
 
     def test_importances(self):
         # The importances property is a frozenset of the importances that are
         # filtered upon.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         self.assertEqual(frozenset(), bug_subscription_filter.importances)
 
     def test_importances_set(self):
         # Assigning any iterable to importances updates the database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.importances = [
             BugTaskImportance.HIGH, BugTaskImportance.LOW]
         self.assertEqual(
@@ -209,26 +215,30 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
 
     def test_importances_set_all(self):
         # Setting all importances is normalized into setting no importances.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.importances = list(BugTaskImportance.items)
         self.assertEqual(frozenset(), bug_subscription_filter.importances)
 
     def test_importances_set_empty(self):
         # Assigning an empty iterable to importances updates the database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.importances = []
         self.assertEqual(frozenset(), bug_subscription_filter.importances)
 
     def test_information_types(self):
         # The information_types property is a frozenset of the
         # information_types that are filtered upon.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         self.assertEqual(
             frozenset(), bug_subscription_filter.information_types)
 
     def test_information_types_set(self):
         # Assigning any iterable to information_types updates the database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.information_types = [
             InformationType.PRIVATESECURITY, InformationType.USERDATA]
         self.assertEqual(
@@ -245,7 +255,8 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
     def test_information_types_set_all(self):
         # Setting all information_types is normalized into setting no
         # information_types.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.information_types = list(
             InformationType.items)
         self.assertEqual(
@@ -254,19 +265,22 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
     def test_information_types_set_empty(self):
         # Assigning an empty iterable to information_types updates the
         # database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.information_types = []
         self.assertEqual(
             frozenset(), bug_subscription_filter.information_types)
 
     def test_tags(self):
         # The tags property is a frozenset of the tags that are filtered upon.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         self.assertEqual(frozenset(), bug_subscription_filter.tags)
 
     def test_tags_set(self):
         # Assigning any iterable to tags updates the database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.tags = [u"foo", u"-bar"]
         self.assertEqual(
             frozenset((u"foo", u"-bar")),
@@ -279,14 +293,16 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
 
     def test_tags_set_empty(self):
         # Assigning an empty iterable to tags updates the database.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         bug_subscription_filter.tags = []
         self.assertEqual(frozenset(), bug_subscription_filter.tags)
 
     def test_tags_set_wildcard(self):
         # Setting one or more wildcard tags may update include_any_tags or
         # exclude_any_tags.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         self.assertEqual(frozenset(), bug_subscription_filter.tags)
         self.assertFalse(bug_subscription_filter.include_any_tags)
         self.assertFalse(bug_subscription_filter.exclude_any_tags)
@@ -315,7 +331,8 @@ class TestBugSubscriptionFilter(TestCaseWithFactory):
     def test_tags_with_any_and_all(self):
         # If the tags are bundled in a c.l.searchbuilder.any or .all, the
         # find_any_tags attribute will also be updated.
-        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter = self.factory.makeBugSubscriptionFilter(
+            target=self.target)
         self.assertEqual(frozenset(), bug_subscription_filter.tags)
         self.assertFalse(bug_subscription_filter.find_all_tags)
 
@@ -348,8 +365,8 @@ class TestBugSubscriptionFilterPermissions(TestCaseWithFactory):
 
     def test_read_to_all(self):
         """`BugSubscriptionFilter`s can be read by anyone."""
-        bug_subscription_filter = BugSubscriptionFilter()
-        bug_subscription_filter.structural_subscription = self.subscription
+        bug_subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
         bug_subscription_filter = ProxyFactory(bug_subscription_filter)
         with person_logged_in(self.subscriber):
             bug_subscription_filter.find_all_tags
@@ -360,8 +377,8 @@ class TestBugSubscriptionFilterPermissions(TestCaseWithFactory):
 
     def test_write_to_subscribers(self):
         """`BugSubscriptionFilter`s can only be modifed by subscribers."""
-        bug_subscription_filter = BugSubscriptionFilter()
-        bug_subscription_filter.structural_subscription = self.subscription
+        bug_subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
         bug_subscription_filter = ProxyFactory(bug_subscription_filter)
         # The subscriber can edit the filter.
         with person_logged_in(self.subscriber):
@@ -379,8 +396,8 @@ class TestBugSubscriptionFilterPermissions(TestCaseWithFactory):
 
     def test_delete_by_subscribers(self):
         """`BugSubscriptionFilter`s can only be deleted by subscribers."""
-        bug_subscription_filter = BugSubscriptionFilter()
-        bug_subscription_filter.structural_subscription = self.subscription
+        bug_subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
         bug_subscription_filter = ProxyFactory(bug_subscription_filter)
         # Anonymous users are denied rights to delete the filter.
         with anonymous_logged_in():
@@ -406,8 +423,8 @@ class TestBugSubscriptionFilterImportance(TestCaseWithFactory):
         login_person(self.subscriber)
         self.subscription = self.target.addBugSubscription(
             self.subscriber, self.subscriber)
-        self.subscription_filter = BugSubscriptionFilter()
-        self.subscription_filter.structural_subscription = self.subscription
+        self.subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
 
     def test_basics(self):
         """Test the basics of `BugSubscriptionFilterImportance` objects."""
@@ -438,8 +455,8 @@ class TestBugSubscriptionFilterStatus(TestCaseWithFactory):
         login_person(self.subscriber)
         self.subscription = self.target.addBugSubscription(
             self.subscriber, self.subscriber)
-        self.subscription_filter = BugSubscriptionFilter()
-        self.subscription_filter.structural_subscription = self.subscription
+        self.subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
 
     def test_basics(self):
         """Test the basics of `BugSubscriptionFilterStatus` objects."""
@@ -469,8 +486,8 @@ class TestBugSubscriptionFilterTag(TestCaseWithFactory):
         login_person(self.subscriber)
         self.subscription = self.target.addBugSubscription(
             self.subscriber, self.subscriber)
-        self.subscription_filter = BugSubscriptionFilter()
-        self.subscription_filter.structural_subscription = self.subscription
+        self.subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
 
     def test_basics(self):
         """Test the basics of `BugSubscriptionFilterTag` objects."""
@@ -517,8 +534,8 @@ class TestBugSubscriptionFilterInformationType(TestCaseWithFactory):
         login_person(self.subscriber)
         self.subscription = self.target.addBugSubscription(
             self.subscriber, self.subscriber)
-        self.subscription_filter = BugSubscriptionFilter()
-        self.subscription_filter.structural_subscription = self.subscription
+        self.subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
 
     def test_basics(self):
         # Test the basics of `BugSubscriptionFilterInformationType` objects.
diff --git a/lib/lp/bugs/tests/test_structuralsubscription.py b/lib/lp/bugs/tests/test_structuralsubscription.py
index 6ca025b..8678532 100644
--- a/lib/lp/bugs/tests/test_structuralsubscription.py
+++ b/lib/lp/bugs/tests/test_structuralsubscription.py
@@ -105,8 +105,8 @@ class TestStructuralSubscription(TestCaseWithFactory):
     def test_bug_filters(self):
         # The bug_filters attribute returns the BugSubscriptionFilter records
         # associated with this subscription.
-        subscription_filter = BugSubscriptionFilter()
-        subscription_filter.structural_subscription = self.subscription
+        subscription_filter = BugSubscriptionFilter(
+            structural_subscription=self.subscription)
         self.assertContentEqual(
             [subscription_filter, self.original_filter],
             list(self.subscription.bug_filters))
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 0bd8f83..0047f80 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -2161,6 +2161,25 @@ class BareLaunchpadObjectFactory(ObjectFactory):
             owner, data, comment, filename, content_type=content_type,
             description=description, **other_params)
 
+    def makeBugSubscriptionFilter(self, target=None, subscriber=None,
+                                  subscribed_by=None):
+        """Create and return a new bug subscription filter.
+
+        :param target: An `IStructuralSubscriptionTarget`.  Defaults to a
+            new `Product`.
+        :param subscriber: An `IPerson`.  Defaults to a new `Person`.
+        :param subscribed_by: An `IPerson`.  Defaults to `subscriber`.
+        :return: An `IBugSubscriptionFilter`.
+        """
+        if target is None:
+            target = self.makeProduct()
+        if subscriber is None:
+            subscriber = self.makePerson()
+        if subscribed_by is None:
+            subscribed_by = subscriber
+        return removeSecurityProxy(target).addBugSubscriptionFilter(
+            subscriber, subscribed_by)
+
     def makeSignedMessage(self, msgid=None, body=None, subject=None,
             attachment_contents=None, force_transfer_encoding=False,
             email_address=None, signing_context=None, to_address=None):

Follow ups