launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01954
[Merge] lp:~allenap/launchpad/subscribe-to-tag-bug-151129-2 into lp:launchpad/devel
Gavin Panella has proposed merging lp:~allenap/launchpad/subscribe-to-tag-bug-151129-2 into lp:launchpad/devel with lp:~allenap/launchpad/subscribe-to-tag-bug-151129 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
This changes the way subscription filters are found. Instead of some
left joins and some array gymnastics there is a convenience class,
BugFilterSetBuilder, that combines sets of filters to figure out which
match. From that it can figure out which subscriptions match.
--
https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-2/+merge/41186
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/subscribe-to-tag-bug-151129-2 into lp:launchpad/devel.
=== 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
@@ -8,9 +8,7 @@
from sqlobject import ForeignKey
from storm.expr import (
And,
- LeftJoin,
- Or,
- SQL,
+ In,
)
from storm.store import Store
from zope.component import (
@@ -29,12 +27,6 @@
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.interfaces.lpstorm import IStore
from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
-from lp.bugs.model.bugsubscriptionfilterimportance import (
- BugSubscriptionFilterImportance,
- )
-from lp.bugs.model.bugsubscriptionfilterstatus import (
- BugSubscriptionFilterStatus,
- )
from lp.registry.enum import BugNotificationLevel
from lp.registry.errors import (
DeleteSubscriptionError,
@@ -472,93 +464,10 @@
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))
=== 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)
+
+ @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)
+
+ @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)))
+
+ @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
@@ -409,6 +411,35 @@
self.bug.tags = ["bar", "baz"]
self.assertSubscriptions([self.subscription])
+ def test_getSubscriptionsForBugTask_any_filter_is_a_match(self):
+ # If a subscription has multiple filters, the subscription is selected
+ # when any filter is found to match. Put another way, the filters are
+ # ORed together.
+ subscription_filter1 = self.subscription.newBugFilter()
+ subscription_filter1.statuses = [BugTaskStatus.CONFIRMED]
+ subscription_filter2 = self.subscription.newBugFilter()
+ subscription_filter2.tags = [u"foo"]
+
+ # With the filter the subscription is not found.
+ self.assertSubscriptions([])
+
+ # If the bugtask is adjusted to match the criteria of the first filter
+ # but not those of the second, the subscription is found.
+ self.bugtask.transitionToStatus(
+ BugTaskStatus.CONFIRMED, self.ordinary_subscriber)
+ self.assertSubscriptions([self.subscription])
+
+ # If the filter is adjusted to also match the criteria of the second
+ # filter, the subscription is still found.
+ self.bugtask.bug.tags = [u"foo"]
+ self.assertSubscriptions([self.subscription])
+
+ # If the bugtask is adjusted to no longer match the criteria of the
+ # first filter, the subscription is still found.
+ self.bugtask.transitionToStatus(
+ BugTaskStatus.INPROGRESS, self.ordinary_subscriber)
+ self.assertSubscriptions([self.subscription])
+
class TestStructuralSubscriptionForDistro(
RestrictedStructuralSubscription, TestCaseWithFactory):
