← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~allenap/launchpad/structural-subscriptions-with-filters into lp:launchpad

 

Gavin Panella has proposed merging lp:~allenap/launchpad/structural-subscriptions-with-filters into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #650991 Add getSubscriptionsForBug to IStructuralSubscriptionTarget
  https://bugs.launchpad.net/bugs/650991


In this branch I started adding getSubscriptionsForBug(), but realised
that the code was getting hairy so detoured into refactoring
StructuralSubscriptionTargetMixin. This class had a *lot* of code
like:

  if ISomeInterface.providedBy(self):
      do something
  elif IOtherInterface.providedBy(self):
      ...

This code should really be somewhere else, perhaps in an adapter or in
the classes into which this class was mixed.

Lots of call-sites rely on finding the IStructuralSubscriptionTarget
interface on the mixees (forgive me) so I don't really want to make
big changes here, like completely switching to adaption.

The mixees are all also fairly large complex classes, and this
functionality is just going to confuse them even more. It's not really
their responsibility either.

So, I've gone for a half-way approach. In most of the places where
providedBy() is used heavily it instead uses an adapter, providing a
new IStructuralSubscriptionTargetHelper interface. These adapters
could (should perhaps) grow into a replacement for the mixin class,
but it can happen a bit at a time.

There is obviously a half-working getSubscriptionsForBug() in here
too. This branch is not going to be landed as-is, so don't worry about
missing functionality.

I'm not super happy with FilteredStructuralSubscriptionTestBase, so
it's going to change. Suggestions for approaches welcome :)

-- 
https://code.launchpad.net/~allenap/launchpad/structural-subscriptions-with-filters/+merge/36968
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/structural-subscriptions-with-filters into lp:launchpad.
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2010-09-28 09:42:58 +0000
+++ lib/lp/registry/configure.zcml	2010-09-29 10:08:34 +0000
@@ -195,6 +195,9 @@
         for="lp.registry.interfaces.distroseries.IDistroSeries"
         factory="lp.registry.browser.distroseries.DistroSeriesBreadcrumb"
         permission="zope.Public"/>
+    <adapter
+        factory=".model.structuralsubscription.DistroSeriesTargetHelper"
+        permission="zope.Public"/>
 
     <facet
         facet="overview">
@@ -348,6 +351,9 @@
         for="lp.registry.interfaces.projectgroup.IProjectGroupSet"
         factory="lp.registry.browser.project.ProjectSetBreadcrumb"
         permission="zope.Public"/>
+    <adapter
+        factory=".model.structuralsubscription.ProjectGroupTargetHelper"
+        permission="zope.Public"/>
 
     <facet
         facet="answers"/>
@@ -478,6 +484,9 @@
         for="lp.registry.interfaces.distributionsourcepackage.IDistributionSourcePackage"
         factory="lp.registry.browser.distributionsourcepackage.DistributionSourcePackageBreadcrumb"
         permission="zope.Public"/>
+    <adapter
+        factory=".model.structuralsubscription.DistributionSourcePackageTargetHelper"
+        permission="zope.Public"/>
 
     <!-- CommercialSubscription -->
 
@@ -944,6 +953,9 @@
             interface="lp.registry.interfaces.structuralsubscription.IStructuralSubscriptionTargetWrite" />
 
     </class>
+    <adapter
+        factory=".model.structuralsubscription.MilestoneTargetHelper"
+        permission="zope.Public"/>
 
     <!-- IMilestoneSet -->
 
@@ -1219,6 +1231,9 @@
             attributes="
                 setBugSupervisor"/>
     </class>
+    <adapter
+        factory=".model.structuralsubscription.ProductTargetHelper"
+        permission="zope.Public"/>
 
     <!-- ProductWithLicenses
                                 This is a decorator for IProduct that eliminates an additional query
@@ -1366,6 +1381,9 @@
         for="lp.registry.interfaces.productseries.IProductSeries"
         factory="lp.registry.browser.productseries.ProductSeriesBreadcrumb"
         permission="zope.Public"/>
+    <adapter
+        factory=".model.structuralsubscription.ProductSeriesTargetHelper"
+        permission="zope.Public"/>
 
     <!-- ProductSeriesSet -->
 
@@ -1491,6 +1509,9 @@
         for="lp.registry.interfaces.distribution.IDistributionSet"
         factory="lp.registry.browser.distribution.DistributionSetBreadcrumb"
         permission="zope.Public"/>
+    <adapter
+        factory=".model.structuralsubscription.DistributionTargetHelper"
+        permission="zope.Public"/>
 
     <!-- DistributionSet -->
 

=== modified file 'lib/lp/registry/interfaces/structuralsubscription.py'
--- lib/lp/registry/interfaces/structuralsubscription.py	2010-09-24 14:31:30 +0000
+++ lib/lp/registry/interfaces/structuralsubscription.py	2010-09-29 10:08:34 +0000
@@ -14,6 +14,7 @@
     'IStructuralSubscription',
     'IStructuralSubscriptionForm',
     'IStructuralSubscriptionTarget',
+    'IStructuralSubscriptionTargetHelper',
     'UserCannotSubscribePerson',
     ]
 
@@ -191,6 +192,9 @@
     def userHasBugSubscriptions(user):
         """Is `user` subscribed, directly or via a team, to bug mail?"""
 
+    def getSubscriptionsForBug(bug, level):
+        """Return subscriptions for a given `IBug` at `level`."""
+
 
 class IStructuralSubscriptionTargetWrite(Interface):
     """A Launchpad Structure allowing users to subscribe to it.
@@ -260,6 +264,29 @@
     export_as_webservice_entry()
 
 
+class IStructuralSubscriptionTargetHelper(Interface):
+    """Provides information on subscribable objects."""
+
+    target = Attribute("The target.")
+
+    target_parent = Attribute(
+        "The target's parent, or None if one doesn't exist.")
+
+    target_type_display = Attribute(
+        "The type of the target, for display.")
+
+    target_arguments = Attribute(
+        "A dict of arguments that can be used as arguments to the "
+        "structural subscription constructor.")
+
+    pillar = Attribute(
+        "The pillar most closely corresponding to the context.")
+
+    join = Attribute(
+        "A Storm join to get the `IStructuralSubscription`s relating "
+        "to the context.")
+
+
 class IStructuralSubscriptionForm(Interface):
     """Schema for the structural subscription form."""
     subscribe_me = Bool(

=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py	2010-09-10 11:12:26 +0000
+++ lib/lp/registry/model/structuralsubscription.py	2010-09-29 10:08:34 +0000
@@ -6,7 +6,16 @@
            'StructuralSubscriptionTargetMixin']
 
 from sqlobject import ForeignKey
-from zope.component import getUtility
+from storm.expr import (
+    And,
+    LeftJoin,
+    Or,
+    )
+from storm.store import Store
+from zope.component import (
+    adapts,
+    getUtility,
+    )
 from zope.interface import implements
 
 from canonical.database.constants import UTC_NOW
@@ -17,6 +26,10 @@
     SQLBase,
     )
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
+from lp.bugs.model.bugsubscriptionfilterstatus import (
+    BugSubscriptionFilterStatus,
+    )
 from lp.registry.enum import BugNotificationLevel
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionsourcepackage import (
@@ -36,8 +49,10 @@
     DeleteSubscriptionError,
     IStructuralSubscription,
     IStructuralSubscriptionTarget,
+    IStructuralSubscriptionTargetHelper,
     UserCannotSubscribePerson,
     )
+from lp.services.propertycache import cachedproperty
 
 
 class StructuralSubscription(SQLBase):
@@ -116,9 +131,146 @@
             raise AssertionError('StructuralSubscription has no target.')
 
 
+class DistroSeriesTargetHelper:
+    """A helper for `IDistroSeries`s."""
+
+    implements(IStructuralSubscriptionTargetHelper)
+    adapts(IDistroSeries)
+
+    target_type_display = 'distribution series'
+
+    def __init__(self, target):
+        self.target = target
+        self.target_parent = target.distribution
+        self.target_arguments = {"distroseries": target}
+        self.pillar = target.distribution
+        self.join = (StructuralSubscription.distroseries == target)
+
+
+class ProjectGroupTargetHelper:
+    """A helper for `IProjectGroup`s."""
+
+    implements(IStructuralSubscriptionTargetHelper)
+    adapts(IProjectGroup)
+
+    target_type_display = 'project group'
+
+    def __init__(self, target):
+        self.target = target
+        self.target_parent = None
+        self.target_arguments = {"project": target}
+        self.pillar = target
+        self.join = (StructuralSubscription.project == target)
+
+
+class DistributionSourcePackageTargetHelper:
+    """A helper for `IDistributionSourcePackage`s."""
+
+    implements(IStructuralSubscriptionTargetHelper)
+    adapts(IDistributionSourcePackage)
+
+    target_type_display = 'package'
+
+    def __init__(self, target):
+        self.target = target
+        self.target_parent = target.distribution
+        self.target_arguments = {
+            "distribution": target.distribution,
+            "sourcepackagename": target.sourcepackagename,
+            }
+        self.pillar = target.distribution
+        self.join = And(
+            StructuralSubscription.distributionID == (
+                target.distribution.id),
+            StructuralSubscription.sourcepackagenameID == (
+                target.sourcepackagename.id))
+
+
+class MilestoneTargetHelper:
+    """A helper for `IMilestone`s."""
+
+    implements(IStructuralSubscriptionTargetHelper)
+    adapts(IMilestone)
+
+    target_type_display = 'milestone'
+
+    def __init__(self, target):
+        self.target = target
+        self.target_parent = target.target
+        self.target_arguments = {"milestone": target}
+        self.pillar = target.target
+        self.join = (StructuralSubscription.milestone == target)
+
+
+class ProductTargetHelper:
+    """A helper for `IProduct`s."""
+
+    implements(IStructuralSubscriptionTargetHelper)
+    adapts(IProduct)
+
+    target_type_display = 'project'
+
+    def __init__(self, target):
+        self.target = target
+        self.target_parent = target.project
+        self.target_arguments = {"product": target}
+        self.pillar = target
+        self.join = (StructuralSubscription.product == target)
+
+
+class ProductSeriesTargetHelper:
+    """A helper for `IProductSeries`s."""
+
+    implements(IStructuralSubscriptionTargetHelper)
+    adapts(IProductSeries)
+
+    target_type_display = 'project series'
+
+    def __init__(self, target):
+        self.target = target
+        self.target_parent = target.product
+        self.target_arguments = {"productseries": target}
+        self.pillar = target.product
+        self.join = (StructuralSubscription.productseries == target)
+
+
+class DistributionTargetHelper:
+    """A helper for `IDistribution`s."""
+
+    implements(IStructuralSubscriptionTargetHelper)
+    adapts(IDistribution)
+
+    target_type_display = 'distribution'
+
+    def __init__(self, target):
+        self.target = target
+        self.target_parent = None
+        self.target_arguments = {
+            "distribution": target,
+            "sourcepackagename": None,
+            }
+        self.pillar = target
+        self.join = And(
+            StructuralSubscription.distributionID == target.id,
+            StructuralSubscription.sourcepackagenameID == None)
+
+
 class StructuralSubscriptionTargetMixin:
     """Mixin class for implementing `IStructuralSubscriptionTarget`."""
 
+    @cachedproperty
+    def __helper(self):
+        """A `IStructuralSubscriptionTargetHelper` for this object.
+
+        Eventually this helper object could become *the* way to work with
+        structural subscriptions. For now it just provides a few bits that
+        vary with the context.
+
+        It is cached in a pseudo-private variable because this is a mixin
+        class.
+        """
+        return IStructuralSubscriptionTargetHelper(self)
+
     @property
     def _target_args(self):
         """Target Arguments.
@@ -126,27 +278,20 @@
         Return a dictionary with the arguments representing this
         target in a call to the structural subscription constructor.
         """
-        args = {}
-        if IDistributionSourcePackage.providedBy(self):
-            args['distribution'] = self.distribution
-            args['sourcepackagename'] = self.sourcepackagename
-        elif IProduct.providedBy(self):
-            args['product'] = self
-        elif IProjectGroup.providedBy(self):
-            args['project'] = self
-        elif IDistribution.providedBy(self):
-            args['distribution'] = self
-            args['sourcepackagename'] = None
-        elif IMilestone.providedBy(self):
-            args['milestone'] = self
-        elif IProductSeries.providedBy(self):
-            args['productseries'] = self
-        elif IDistroSeries.providedBy(self):
-            args['distroseries'] = self
-        else:
-            raise AssertionError(
-                '%s is not a valid structural subscription target.')
-        return args
+        return self.__helper.target_arguments
+
+    @property
+    def parent_subscription_target(self):
+        """See `IStructuralSubscriptionTarget`."""
+        parent = self.__helper.target_parent
+        assert (parent is None or
+                IStructuralSubscriptionTarget.providedBy(parent))
+        return parent
+
+    @property
+    def target_type_display(self):
+        """See `IStructuralSubscriptionTarget`."""
+        return self.__helper.target_type_display
 
     def userCanAlterSubscription(self, subscriber, subscribed_by):
         """See `IStructuralSubscriptionTarget`."""
@@ -308,54 +453,6 @@
         return self.getSubscriptions(
             min_bug_notification_level=BugNotificationLevel.METADATA)
 
-    @property
-    def parent_subscription_target(self):
-        """See `IStructuralSubscriptionTarget`."""
-        # Some structures have a related structure which can be thought
-        # of as their parent. A package is related to a distribution,
-        # a product is related to a project, etc'...
-        # This method determines whether the target has a parent,
-        # returning it if it exists.
-        if IDistributionSourcePackage.providedBy(self):
-            parent = self.distribution
-        elif IProduct.providedBy(self):
-            parent = self.project
-        elif IProductSeries.providedBy(self):
-            parent = self.product
-        elif IDistroSeries.providedBy(self):
-            parent = self.distribution
-        elif IMilestone.providedBy(self):
-            parent = self.target
-        else:
-            parent = None
-        # We only want to return the parent if it's
-        # an `IStructuralSubscriptionTarget`.
-        if IStructuralSubscriptionTarget.providedBy(parent):
-            return parent
-        else:
-            return None
-
-    @property
-    def target_type_display(self):
-        """See `IStructuralSubscriptionTarget`."""
-        if IDistributionSourcePackage.providedBy(self):
-            return 'package'
-        elif IProduct.providedBy(self):
-            return 'project'
-        elif IProjectGroup.providedBy(self):
-            return 'project group'
-        elif IDistribution.providedBy(self):
-            return 'distribution'
-        elif IMilestone.providedBy(self):
-            return 'milestone'
-        elif IProductSeries.providedBy(self):
-            return 'project series'
-        elif IDistroSeries.providedBy(self):
-            return 'distribution series'
-        else:
-            raise AssertionError(
-                '%s is not a valid structural subscription target.', self)
-
     def userHasBugSubscriptions(self, user):
         """See `IStructuralSubscriptionTarget`."""
         bug_subscriptions = self.getSubscriptions(
@@ -367,3 +464,40 @@
                     # The user has a bug subscription
                     return True
         return False
+
+    def getSubscriptionsForBug(self, bug, level):
+        """See `IStructuralSubscriptionTarget`."""
+        if IProjectGroup.providedBy(self):
+            targets = set(self.products)
+        elif IMilestone.providedBy(self):
+            targets = [self.series_target]
+        else:
+            targets = [self]
+
+        bugtasks = [
+            bugtask for bugtask in bug.bugtasks
+            if bugtask.target in targets
+            ]
+
+        assert len(bugtasks) != 0, repr(self)
+
+        origin = [
+            StructuralSubscription,
+            LeftJoin(
+                BugSubscriptionFilter,
+                BugSubscriptionFilter.structural_subscription_id == (
+                    StructuralSubscription.id)),
+            LeftJoin(
+                BugSubscriptionFilterStatus,
+                BugSubscriptionFilterStatus.filter_id == (
+                    BugSubscriptionFilter.id)),
+            ]
+
+        conditions = [
+            Or(BugSubscriptionFilter.id == None,
+               BugSubscriptionFilterStatus.status.is_in(
+                    bugtask.status for bugtask in bugtasks))
+            ]
+
+        return Store.of(self.__helper.pillar).using(*origin).find(
+            StructuralSubscription, self.__helper.join, *conditions)

=== modified file 'lib/lp/registry/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-09-29 10:08:34 +0000
@@ -8,6 +8,7 @@
 
 import unittest
 
+from storm.expr import compile as compile_storm
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import (
@@ -22,25 +23,36 @@
     )
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.testing import verifyObject
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.testing import (
+    LaunchpadFunctionalLayer,
+    LaunchpadZopelessLayer,
+    )
 from lp.bugs.interfaces.bug import CreateBugParams
+from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
+from lp.bugs.model.bugsubscriptionfilterstatus import (
+    BugSubscriptionFilterStatus,
+    )
 from lp.bugs.tests.test_bugtarget import bugtarget_filebug
+from lp.registry.enum import BugNotificationLevel
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.product import IProductSet
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.registry.interfaces.structuralsubscription import (
     DeleteSubscriptionError,
     IStructuralSubscriptionTarget,
+    IStructuralSubscriptionTargetHelper,
     UserCannotSubscribePerson,
     )
 from lp.registry.model.structuralsubscription import StructuralSubscription
 from lp.testing import (
     ANONYMOUS,
     login,
+    login_celebrity,
     login_person,
     TestCaseWithFactory,
     )
-from lp.testing._login import login_celebrity
+from lp.testing.matchers import Provides
 
 
 class StructuralSubscriptionTestBase:
@@ -153,8 +165,59 @@
             None)
 
 
-class TestStructuralSubscriptionForDistro(StructuralSubscriptionTestBase,
-    TestCaseWithFactory):
+class FilteredStructuralSubscriptionTestBase(StructuralSubscriptionTestBase):
+    """Tests for filtered structural subscriptions."""
+
+    def makeBugTask(self):
+        return self.factory.makeBugTask(target=self.target)
+
+    def test_getSubscriptionsForBug(self):
+        # If no one has a filtered subscription for the given bug, the result
+        # of getSubscriptionsForBug() is the same as for getSubscriptions().
+        bugtask = self.makeBugTask()
+        subscriptions = self.target.getSubscriptions(
+            min_bug_notification_level=BugNotificationLevel.NOTHING)
+        subscriptions_for_bug = self.target.getSubscriptionsForBug(
+            bugtask.bug, BugNotificationLevel.NOTHING)
+        self.assertEqual(list(subscriptions), list(subscriptions_for_bug))
+
+    def test_getSubscriptionsForBug_with_filter_on_status(self):
+        # If a status filter exists for a subscription, the result of
+        # getSubscriptionsForBug() may be a subset of getSubscriptions().
+        bugtask = self.makeBugTask()
+
+        # Create a new subscription on self.target.
+        login_person(self.ordinary_subscriber)
+        subscription = self.target.addSubscription(
+            self.ordinary_subscriber, self.ordinary_subscriber)
+        subscription.bug_notification_level = BugNotificationLevel.COMMENTS
+
+        # Without any filters the subscription is found.
+        subscriptions_for_bug = self.target.getSubscriptionsForBug(
+            bugtask.bug, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bug))
+
+        # Filter the subscription to bugs in the CONFIRMED state.
+        subscription_filter = BugSubscriptionFilter()
+        subscription_filter.structural_subscription = subscription
+        subscription_filter_status = BugSubscriptionFilterStatus()
+        subscription_filter_status.filter = subscription_filter
+        subscription_filter_status.status = BugTaskStatus.CONFIRMED
+
+        # With the filter the subscription is not found.
+        subscriptions_for_bug = self.target.getSubscriptionsForBug(
+            bugtask.bug, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bug))
+
+        # If the filter is adjusted, the subscription is found again.
+        subscription_filter_status.status = bugtask.status
+        subscriptions_for_bug = self.target.getSubscriptionsForBug(
+            bugtask.bug, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bug))
+
+
+class TestStructuralSubscriptionForDistro(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -218,7 +281,9 @@
 
 
 class TestStructuralSubscriptionForProduct(
-    UnrestrictedStructuralSubscription, TestCaseWithFactory):
+    UnrestrictedStructuralSubscription,
+    FilteredStructuralSubscriptionTestBase,
+    TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -228,7 +293,9 @@
 
 
 class TestStructuralSubscriptionForDistroSourcePackage(
-    UnrestrictedStructuralSubscription, TestCaseWithFactory):
+    UnrestrictedStructuralSubscription,
+    FilteredStructuralSubscriptionTestBase,
+    TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -239,7 +306,9 @@
 
 
 class TestStructuralSubscriptionForMilestone(
-    UnrestrictedStructuralSubscription, TestCaseWithFactory):
+    UnrestrictedStructuralSubscription,
+    FilteredStructuralSubscriptionTestBase,
+    TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -248,9 +317,15 @@
         self.target = self.factory.makeMilestone()
         self.target = ProxyFactory(self.target)
 
+    def makeBugTask(self):
+        # XXX Should test with target *and* series_target.
+        return self.factory.makeBugTask(target=self.target.series_target)
+
 
 class TestStructuralSubscriptionForDistroSeries(
-    UnrestrictedStructuralSubscription, TestCaseWithFactory):
+    UnrestrictedStructuralSubscription,
+    FilteredStructuralSubscriptionTestBase,
+    TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -260,6 +335,166 @@
         self.target = ProxyFactory(self.target)
 
 
+class TestStructuralSubscriptionForProjectGroup(
+    UnrestrictedStructuralSubscription,
+    FilteredStructuralSubscriptionTestBase,
+    TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestStructuralSubscriptionForProjectGroup, self).setUp()
+        self.target = self.factory.makeProject()
+        self.target = ProxyFactory(self.target)
+
+    def makeBugTask(self):
+        return self.factory.makeBugTask(
+            target=self.factory.makeProduct(project=self.target))
+
+
+class TestStructuralSubscriptionForProductSeries(
+    UnrestrictedStructuralSubscription,
+    FilteredStructuralSubscriptionTestBase,
+    TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestStructuralSubscriptionForProductSeries, self).setUp()
+        self.target = self.factory.makeProductSeries()
+        self.target = ProxyFactory(self.target)
+
+
+class TestStructuralSubscriptionTargetHelper(TestCaseWithFactory):
+    """Tests for implementations of `IStructuralSubscriptionTargetHelper`."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestStructuralSubscriptionTargetHelper, self).setUp()
+        self.person = self.factory.makePerson()
+        login_person(self.person)
+
+    def test_distribution_series(self):
+        target = self.factory.makeDistroSeries()
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual("distribution series", helper.target_type_display)
+        self.assertEqual(target, helper.target)
+        self.assertEqual(target.distribution, helper.target_parent)
+        self.assertEqual({"distroseries": target}, helper.target_arguments)
+        self.assertEqual(target.distribution, helper.pillar)
+        self.assertEqual(
+            u"StructuralSubscription.distroseries = ?",
+            compile_storm(helper.join))
+
+    def test_project_group(self):
+        target = self.factory.makeProject(owner=self.person)
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual("project group", helper.target_type_display)
+        self.assertEqual(target, helper.target)
+        self.assertEqual(None, helper.target_parent)
+        self.assertEqual(target, helper.pillar)
+        self.assertEqual({"project": target}, helper.target_arguments)
+        self.assertEqual(
+            u"StructuralSubscription.project = ?",
+            compile_storm(helper.join))
+
+    def test_distribution_source_package(self):
+        target = self.factory.makeDistributionSourcePackage()
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual("package", helper.target_type_display)
+        self.assertEqual(target, helper.target)
+        self.assertEqual(target.distribution, helper.target_parent)
+        self.assertThat(
+            helper.target_parent, Provides(IStructuralSubscriptionTarget))
+        self.assertEqual(target.distribution, helper.pillar)
+        self.assertEqual(
+            {"distribution": target.distribution,
+             "sourcepackagename": target.sourcepackagename},
+            helper.target_arguments)
+        self.assertEqual(
+            u"StructuralSubscription.distribution = ? AND "
+            u"StructuralSubscription.sourcepackagename = ?",
+            compile_storm(helper.join))
+
+    def test_milestone(self):
+        target = self.factory.makeMilestone()
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual("milestone", helper.target_type_display)
+        self.assertEqual(target, helper.target)
+        self.assertEqual(target.target, helper.target_parent)
+        self.assertThat(
+            helper.target_parent, Provides(IStructuralSubscriptionTarget))
+        self.assertEqual(target.target, helper.pillar)
+        self.assertEqual({"milestone": target}, helper.target_arguments)
+        self.assertEqual(
+            u"StructuralSubscription.milestone = ?",
+            compile_storm(helper.join))
+
+    def test_product(self):
+        target = self.factory.makeProduct(owner=self.person)
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual("project", helper.target_type_display)
+        self.assertEqual(target, helper.target)
+        self.assertEqual(None, helper.target_parent)
+        self.assertEqual(target, helper.pillar)
+        self.assertEqual({"product": target}, helper.target_arguments)
+        self.assertEqual(
+            u"StructuralSubscription.product = ?",
+            compile_storm(helper.join))
+
+    def test_product_in_group(self):
+        project = self.factory.makeProject(owner=self.person)
+        target = self.factory.makeProduct(project=project)
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual("project", helper.target_type_display)
+        self.assertEqual(target, helper.target)
+        self.assertEqual(project, helper.target_parent)
+        self.assertEqual(target, helper.pillar)
+        self.assertEqual({"product": target}, helper.target_arguments)
+        self.assertEqual(
+            u"StructuralSubscription.product = ?",
+            compile_storm(helper.join))
+
+    def test_product_series(self):
+        target = self.factory.makeProductSeries(owner=self.person)
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual("project series", helper.target_type_display)
+        self.assertEqual(target, helper.target)
+        self.assertEqual(target.product, helper.target_parent)
+        self.assertThat(
+            helper.target_parent, Provides(IStructuralSubscriptionTarget))
+        self.assertEqual(target.product, helper.pillar)
+        self.assertEqual({"productseries": target}, helper.target_arguments)
+        self.assertEqual(
+            u"StructuralSubscription.productseries = ?",
+            compile_storm(helper.join))
+
+    def test_distribution(self):
+        target = self.factory.makeDistribution(owner=self.person)
+        helper = IStructuralSubscriptionTargetHelper(target)
+        self.assertThat(helper, Provides(IStructuralSubscriptionTargetHelper))
+        self.assertEqual(target, helper.target)
+        self.assertEqual("distribution", helper.target_type_display)
+        self.assertEqual(None, helper.target_parent)
+        self.assertEqual(target, helper.pillar)
+        self.assertEqual(
+            {"distribution": target,
+             "sourcepackagename": None},
+            helper.target_arguments)
+        self.assertEqual(
+            u"StructuralSubscription.distribution = ? AND "
+            u"StructuralSubscription.sourcepackagename IS NULL",
+            compile_storm(helper.join))
+
+
 def distributionSourcePackageSetUp(test):
     setUp(test)
     ubuntu = getUtility(IDistributionSet).getByName('ubuntu')