Gavin--
 
This is a really nice branch! I like cutting out all of that logic into the utility.
 
I am a little weirded out by it living alone in models; the logic is a file in models should have a model in it--this one is just utilities for another model file. Since it doesn't have use outside of structuralsubscription.py, I think it might be better added to that file. I don't think that hurts the readability of that file, since the logic is still nicely contained in the class you created.
 
Additional comments are in the diff below.
 
> === modified file 'lib/lp/registry/model/structuralsubscription.py'
> --- lib/lp/registry/model/structuralsubscription.py	2010-11-18 16:11:43 +0000
> +++ lib/lp/registry/model/structuralsubscription.py	2010-11-18 16:11:51 +0000
>      def getSubscriptionsForBugTask(self, bugtask, level):
>          """See `IStructuralSubscriptionTarget`."""
> -        origin = [
> -            StructuralSubscription,
> -            LeftJoin(
> -                BugSubscriptionFilter,
> -                BugSubscriptionFilter.structural_subscription_id == (
> -                    StructuralSubscription.id)),
> -            LeftJoin(
> -                BugSubscriptionFilterStatus,
> -                BugSubscriptionFilterStatus.filter_id == (
> -                    BugSubscriptionFilter.id)),
> -            LeftJoin(
> -                BugSubscriptionFilterImportance,
> -                BugSubscriptionFilterImportance.filter_id == (
> -                    BugSubscriptionFilter.id)),
> -            ]
> -
> -        # An ARRAY[] expression for the given bug's tags.
> -        tags_array = "ARRAY[%s]::TEXT[]" % ",".join(
> -            quote(tag) for tag in bugtask.bug.tags)
> -
> -        # The tags a subscription requests for inclusion.
> -        tags_include_expr = (
> -            "SELECT tag FROM BugSubscriptionFilterTag "
> -            "WHERE filter = BugSubscriptionFilter.id AND include")
> -        tags_include_array = "ARRAY(%s)" % tags_include_expr
> -        tags_include_is_empty = SQL(
> -            "ARRAY[]::TEXT[] = %s" % tags_include_array)
> -
> -        # The tags a subscription requests for exclusion.
> -        tags_exclude_expr = (
> -            "SELECT tag FROM BugSubscriptionFilterTag "
> -            "WHERE filter = BugSubscriptionFilter.id AND NOT include")
> -        tags_exclude_array = "ARRAY(%s)" % tags_exclude_expr
> -        tags_exclude_is_empty = SQL(
> -            "ARRAY[]::TEXT[] = %s" % tags_exclude_array)
> -
> -        # Choose the correct expression depending on the find_all_tags flag.
> -        def tags_find_all_combinator(find_all_expr, find_any_expr):
> -            return SQL(
> -                "CASE WHEN BugSubscriptionFilter.find_all_tags "
> -                "THEN (%s) ELSE (%s) END" % (find_all_expr, find_any_expr))
> -
> -        if len(bugtask.bug.tags) == 0:
> -            tag_conditions = [
> -                BugSubscriptionFilter.include_any_tags == False,
> -                # The subscription's required tags must be an empty set.
> -                tags_include_is_empty,
> -                # The subscription's excluded tags can be anything so no
> -                # condition is needed.
> -                ]
> -        else:
> -            tag_conditions = [
> -                BugSubscriptionFilter.exclude_any_tags == False,
> -                # The bug's tags must contain the subscription's required tags
> -                # if find_all_tags is set, else there must be an intersection.
> -                Or(tags_include_is_empty,
> -                   tags_find_all_combinator(
> -                        "%s @> %s" % (tags_array, tags_include_array),
> -                        "%s && %s" % (tags_array, tags_include_array))),
> -                # The bug's tags must not contain the subscription's excluded
> -                # tags if find_all_tags is set, else there must not be an
> -                # intersection.
> -                Or(tags_exclude_is_empty,
> -                   tags_find_all_combinator(
> -                        "NOT (%s @> %s)" % (tags_array, tags_exclude_array),
> -                        "NOT (%s && %s)" % (tags_array, tags_exclude_array))),
> -                ]
> -
> -        conditions = [
> -            StructuralSubscription.bug_notification_level >= level,
> -            Or(
> -                # There's no filter or ...
> -                BugSubscriptionFilter.id == None,
> -                # There is a filter and ...
> -                And(
> -                    # There's no status filter, or there is a status filter
> -                    # and and it matches.
> -                    Or(BugSubscriptionFilterStatus.id == None,
> -                       BugSubscriptionFilterStatus.status == bugtask.status),
> -                    # There's no importance filter, or there is an importance
> -                    # filter and it matches.
> -                    Or(BugSubscriptionFilterImportance.id == None,
> -                       BugSubscriptionFilterImportance.importance == (
> -                            bugtask.importance)),
> -                    # Any number of conditions relating to tags.
> -                    *tag_conditions)),
> -            ]
> -
> -        return Store.of(self.__helper.pillar).using(*origin).find(
> -            StructuralSubscription, self.__helper.join, *conditions)
> +        from lp.registry.model import structuralsubscriptionfilter
> +        set_builder = structuralsubscriptionfilter.BugFilterSetBuilder(
> +            bugtask, level, self.__helper.join)
> +        return Store.of(self.__helper.pillar).find(
> +            StructuralSubscription, In(
> +                StructuralSubscription.id,
> +                set_builder.subscriptions))
 
Again: I love how much this cuts away.
 
Is there any reason to not import BugFilterSetBuilder instead of the module that owns it? It may just be a personal thing, but I think something like the following might parse better:
 
>>> from lp.registry.model.structuralsubscriptionfilter import (
...     BugFilterSetBuilder,
...     )
>>> set_builder = BugFilterSetBuilder(bugtask, level, self.__helper.join)
 
 
> === added file 'lib/lp/registry/model/structuralsubscriptionfilter.py'
> --- lib/lp/registry/model/structuralsubscriptionfilter.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/model/structuralsubscriptionfilter.py	2010-11-18 16:11:51 +0000
> @@ -0,0 +1,246 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +__all__ = [
> +    'BugFilterSetBuilder',
> +    ]
> +
> +from storm.expr import (
> +    Alias,
> +    And,
> +    CompoundOper,
> +    Except,
> +    In,
> +    Intersect,
> +    LeftJoin,
> +    NamedFunc,
> +    Not,
> +    Or,
> +    Select,
> +    SQL,
> +    Union,
> +    )
> +
> +from canonical.database.sqlbase import quote
> +from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
> +from lp.bugs.model.bugsubscriptionfilterimportance import (
> +    BugSubscriptionFilterImportance,
> +    )
> +from lp.bugs.model.bugsubscriptionfilterstatus import (
> +    BugSubscriptionFilterStatus,
> +    )
> +from lp.bugs.model.bugsubscriptionfiltertag import BugSubscriptionFilterTag
> +from lp.registry.model.structuralsubscription import StructuralSubscription
> +
> +
> +class ArrayAgg(NamedFunc):
> +    __slots__ = ()
> +    name = "ARRAY_AGG"
> +
> +
> +class ArrayContains(CompoundOper):
> +    __slots__ = ()
> +    oper = "@>"
> +
> +
> +class BugFilterSetBuilder:
> +    """A convenience class to build queries for getSubscriptionsForBugTask."""
> +
> +    def __init__(self, bugtask, level, join_condition):
> +        self.status = bugtask.status
> +        self.importance = bugtask.importance
> +        # The list() gets around some weirdness with security proxies; Storm
> +        # does not know how to compile an expression with a proxied list.
> +        self.tags = list(bugtask.bug.tags)
> +        # Set up common conditions.
> +        self.base_conditions = And(
> +            StructuralSubscription.bug_notification_level >= level,
> +            join_condition)
> +        # Set up common filter conditions.
> +        if len(self.tags) == 0:
> +            self.filter_conditions = And(
> +                BugSubscriptionFilter.include_any_tags == False,
> +                self.base_conditions)
> +        else:
> +            self.filter_conditions = And(
> +                BugSubscriptionFilter.exclude_any_tags == False,
> +                self.base_conditions)
> +
 
A quick comment here explaining the logic of setting up exclude_any vs include_any would be good.
 
> +    @property
> +    def subscriptions_without_filters(self):
> +        """Subscriptions without filters."""
> +        return Select(
> +            StructuralSubscription.id,
> +            tables=(
> +                StructuralSubscription,
> +                LeftJoin(
> +                    BugSubscriptionFilter,
> +                    BugSubscriptionFilter.structural_subscription_id == (
> +                        StructuralSubscription.id))),
> +            where=And(
> +                BugSubscriptionFilter.id == None,
> +                self.base_conditions))
> +
> +    def _filters_matching_x(self, join, where_condition, **extra):
> +        # The expressions returned by this function are used in set (union,
> +        # intersect, except) operations at the *filter* level. However, the
> +        # interesting result of these set operations is the structural
> +        # subscription, hence both columns are included in the expressions
> +        # generated. Since a structural subscription can have zero or more
> +        # filters, and a filter can never be associated with more than one
> +        # subscription, the set operations are unaffected.
> +        return Select(
> +            columns=(
> +                # Alias this column so it can be selected in
> +                # subscriptions_matching.
> +                Alias(
> +                    BugSubscriptionFilter.structural_subscription_id,
> +                    "structural_subscription_id"),
> +                BugSubscriptionFilter.id),
> +            tables=(
> +                StructuralSubscription, BugSubscriptionFilter, join),
> +            where=And(
> +                BugSubscriptionFilter.structural_subscription_id == (
> +                    StructuralSubscription.id),
> +                self.filter_conditions,
> +                where_condition),
> +            **extra)
> +
 
I would prefer some sort of docstring for this in place of in addition to the comment block at the top.
 
> +    @property
> +    def filters_matching_status(self):
> +        """Filters with the given bugtask's status."""
> +        join = LeftJoin(
> +            BugSubscriptionFilterStatus,
> +            BugSubscriptionFilterStatus.filter_id == (
> +                BugSubscriptionFilter.id))
> +        condition = Or(
> +            BugSubscriptionFilterStatus.id == None,
> +            BugSubscriptionFilterStatus.status == self.status)
> +        return self._filters_matching_x(join, condition)
> +
> +    @property
> +    def filters_matching_importance(self):
> +        """Filters with the given bugtask's importance."""
> +        join = LeftJoin(
> +            BugSubscriptionFilterImportance,
> +            BugSubscriptionFilterImportance.filter_id == (
> +                BugSubscriptionFilter.id))
> +        condition = Or(
> +            BugSubscriptionFilterImportance.id == None,
> +            BugSubscriptionFilterImportance.importance == self.importance)
> +        return self._filters_matching_x(join, condition)
> +
> +    @property
> +    def filters_without_include_tags(self):
> +        """Filters with no tags required."""
> +        join = LeftJoin(
> +            BugSubscriptionFilterTag,
> +            And(BugSubscriptionFilterTag.filter_id == (
> +                    BugSubscriptionFilter.id),
> +                BugSubscriptionFilterTag.include))
> +        return self._filters_matching_x(
> +            join, BugSubscriptionFilterTag.id == None)
> +
> +    @property
> +    def filters_matching_any_include_tags(self):
> +        """Filters including any of the bug's tags."""
> +        condition = And(
> +            BugSubscriptionFilterTag.filter_id == (
> +                BugSubscriptionFilter.id),
> +            BugSubscriptionFilterTag.include,
> +            Not(BugSubscriptionFilter.find_all_tags),
> +            In(BugSubscriptionFilterTag.tag, self.tags))
> +        return self._filters_matching_x(
> +            BugSubscriptionFilterTag, condition)
> +
> +    @property
> +    def filters_matching_any_exclude_tags(self):
> +        """Filters excluding any of the bug's tags."""
> +        condition = And(
> +            BugSubscriptionFilterTag.filter_id == (
> +                BugSubscriptionFilter.id),
> +            Not(BugSubscriptionFilterTag.include),
> +            Not(BugSubscriptionFilter.find_all_tags),
> +            In(BugSubscriptionFilterTag.tag, self.tags))
> +        return self._filters_matching_x(
> +            BugSubscriptionFilterTag, condition)
> +
> +    def _filters_matching_all_x_tags(self, where_condition):
> +        tags_array = "ARRAY[%s]::TEXT[]" % ",".join(
> +            quote(tag) for tag in self.tags)
> +        return self._filters_matching_x(
> +            BugSubscriptionFilterTag,
> +            And(
> +                BugSubscriptionFilterTag.filter_id == (
> +                    BugSubscriptionFilter.id),
> +                BugSubscriptionFilter.find_all_tags,
> +                self.filter_conditions,
> +                where_condition),
> +            group_by=(
> +                BugSubscriptionFilter.structural_subscription_id,
> +                BugSubscriptionFilter.id),
> +            having=ArrayContains(
> +                SQL(tags_array), ArrayAgg(
> +                    BugSubscriptionFilterTag.tag)))
> +
 
Given how much this is used in the following methods, a docstring explaining it as a convenience for the next methods would be nice.
 
> +    @property
> +    def filters_matching_all_include_tags(self):
> +        """Filters including the bug's tags."""
> +        return self._filters_matching_all_x_tags(
> +            BugSubscriptionFilterTag.include)
> +
> +    @property
> +    def filters_matching_all_exclude_tags(self):
> +        """Filters excluding the bug's tags."""
> +        return self._filters_matching_all_x_tags(
> +            Not(BugSubscriptionFilterTag.include))
> +
> +    @property
> +    def filters_matching_include_tags(self):
> +        """Filters with tag filters including the bug."""
> +        return Union(
> +            self.filters_matching_any_include_tags,
> +            self.filters_matching_all_include_tags)
> +
> +    @property
> +    def filters_matching_exclude_tags(self):
> +        """Filters with tag filters excluding the bug."""
> +        return Union(
> +            self.filters_matching_any_exclude_tags,
> +            self.filters_matching_all_exclude_tags)
> +
> +    @property
> +    def filters_matching_tags(self):
> +        """Filters with tag filters matching the bug."""
> +        if len(self.tags) == 0:
> +            # The filter's required tags must be an empty set. The filter's
> +            # excluded tags can be anything so no condition is needed.
> +            return self.filters_without_include_tags
> +        else:
> +            return Except(
> +                Union(self.filters_without_include_tags,
> +                      self.filters_matching_include_tags),
> +                self.filters_matching_exclude_tags)
> +
> +    @property
> +    def filters_matching(self):
> +        """Filters matching the bug."""
> +        return Intersect(
> +            self.filters_matching_status,
> +            self.filters_matching_importance,
> +            self.filters_matching_tags)
> +
> +    @property
> +    def subscriptions_with_matching_filters(self):
> +        """Subscriptions with one or more filters matching the bug."""
> +        return Select(
> +            # I don't know of a more Storm-like way of doing this.
> +            SQL("filters_matching.structural_subscription_id"),
> +            tables=Alias(self.filters_matching, "filters_matching"))
> +
> +    @property
> +    def subscriptions(self):
> +        return Union(
> +            self.subscriptions_without_filters,
> +            self.subscriptions_with_matching_filters)
> 
> === modified file 'lib/lp/registry/tests/test_structuralsubscriptiontarget.py'
> --- lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-11-18 16:11:43 +0000
> +++ lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-11-18 16:11:51 +0000
> @@ -67,7 +67,7 @@
>  
>  
>  class RestrictedStructuralSubscription(StructuralSubscriptionTestBase):
> -    # Tests suitable for a target that restricts structural subscriptions.
> +    """Tests suitable for a target that restricts structural subscriptions."""
>  
>      def test_target_implements_structural_subscription_target(self):
>          self.assertTrue(verifyObject(IStructuralSubscriptionTarget,
> @@ -128,8 +128,10 @@
>  
>  
>  class UnrestrictedStructuralSubscription(RestrictedStructuralSubscription):
> -    # Tests suitable for a target that does not restrict structural
> -    # subscriptions.
> +    """
> +    Tests suitable for a target that does not restrict structural
> +    subscriptions.
> +    """
>  
>      def test_structural_subscription_by_ordinary_user(self):
>          # ordinary users can subscribe themselves
 
Thanks for adding those docstrings.