launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/subscribe-to-tag-bug-151129 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #151129 Can't subscribe to a tag
  https://bugs.launchpad.net/bugs/151129


This adds the ability to subscribe to tags as part of a structural
subscription. It was already possible to subscribe to the presence or
absense of tags, just not specific tags (presence or absense thereof).

The implementation is perhaps interesting. Because set operations need
to be performed, it's not enough to simply left join to the subscribed
tags table (BugSubscriptionFilterTag). We need set operations! So, in
this implementation I've used PostgreSQL ARRAY types.

-- 
https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129/+merge/40324
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/subscribe-to-tag-bug-151129 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py	2010-10-25 13:24:39 +0000
+++ lib/lp/registry/model/structuralsubscription.py	2010-11-08 13:14:20 +0000
@@ -10,6 +10,7 @@
     And,
     LeftJoin,
     Or,
+    SQL,
     )
 from storm.store import Store
 from zope.component import (
@@ -484,13 +485,55 @@
                     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[] = ARRAY(%s)" % tags_include_expr)
+
+        # 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[] = ARRAY(%s)" % tags_exclude_expr)
+
+        # 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.
+                SQL("%s = %s" % (tags_array, tags_include_array)),
+                # The subscription's excluded tags can be anything.
                 ]
         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 = [

=== modified file 'lib/lp/registry/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-10-07 10:07:51 +0000
+++ lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-11-08 13:14:20 +0000
@@ -320,6 +320,128 @@
             bugtask, BugNotificationLevel.NOTHING)
         self.assertEqual([], list(subscriptions_for_bugtask))
 
+    def test_getSubscriptionsForBugTask_with_filter_for_any_tag(self):
+        # If a subscription filter specifies that any of one or more specific
+        # tags must be present, bugs with any of those tags are matched.
+        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
+        subscription_filter = subscription.newBugFilter()
+
+        # Looking for either the "foo" or the "bar" tag.
+        subscription_filter.tags = [u"foo", u"bar"]
+        subscription_filter.find_all_tags = False
+
+        # Without either tag the subscription is not found.
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bugtask))
+
+        # With either tag the subscription is found.
+        bugtask.bug.tags = ["bar", "baz"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+
+    def test_getSubscriptionsForBugTask_with_filter_for_all_tags(self):
+        # If a subscription filter specifies that all of one or more specific
+        # tags must be present, bugs with all of those tags are matched.
+        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
+        subscription_filter = subscription.newBugFilter()
+
+        # Looking for both the "foo" and the "bar" tag.
+        subscription_filter.tags = [u"foo", u"bar"]
+        subscription_filter.find_all_tags = True
+
+        # Without either tag the subscription is not found.
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bugtask))
+
+        # Without only one of the required tags the subscription is not found.
+        bugtask.bug.tags = ["foo"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bugtask))
+
+        # With both required tags the subscription is found.
+        bugtask.bug.tags = ["foo", "bar"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+
+    def test_getSubscriptionsForBugTask_with_filter_for_not_any_tag(self):
+        # If a subscription filter specifies that any of one or more specific
+        # tags must not be present, bugs without any of those tags are
+        # matched.
+        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
+        subscription_filter = subscription.newBugFilter()
+
+        # Looking to exclude the "foo" or "bar" tags.
+        subscription_filter.tags = [u"-foo", u"-bar"]
+        subscription_filter.find_all_tags = False
+
+        # Without either tag the subscription is found.
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+
+        # With either tag the subscription is no longer found.
+        bugtask.bug.tags = ["foo"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bugtask))
+
+    def test_getSubscriptionsForBugTask_with_filter_for_not_all_tags(self):
+        # If a subscription filter specifies that all of one or more specific
+        # tags must not be present, bugs without all of those tags are
+        # matched.
+        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
+        subscription_filter = subscription.newBugFilter()
+
+        # Looking to exclude the "foo" and "bar" tags.
+        subscription_filter.tags = [u"-foo", u"-bar"]
+        subscription_filter.find_all_tags = True
+
+        # Without either tag the subscription is found.
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+
+        # With only one of the excluded tags the subscription is found.
+        bugtask.bug.tags = ["foo"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+
+        # With both tags the subscription is no longer found.
+        bugtask.bug.tags = ["foo", "bar"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bugtask))
+
     def test_getSubscriptionsForBugTask_with_multiple_filters(self):
         # If multiple filters exist for a subscription, all filters must
         # match.
@@ -331,6 +453,9 @@
             self.ordinary_subscriber, self.ordinary_subscriber)
         subscription.bug_notification_level = BugNotificationLevel.COMMENTS
 
+        # Add the "foo" tag to the bug.
+        bugtask.bug.tags = ["foo"]
+
         # Filter the subscription to bugs in the CRITICAL state.
         subscription_filter = BugSubscriptionFilter()
         subscription_filter.structural_subscription = subscription
@@ -356,6 +481,34 @@
             bugtask, BugNotificationLevel.NOTHING)
         self.assertEqual([subscription], list(subscriptions_for_bugtask))
 
+        # If the filter is given some tag criteria, the subscription is not
+        # found.
+        subscription_filter.tags = [u"-foo", u"bar", u"baz"]
+        subscription_filter.find_all_tags = False
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bugtask))
+
+        # After removing the "foo" tag and adding the "bar" tag, the
+        # subscription is found.
+        bugtask.bug.tags = ["bar"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+
+        # Requiring that all tag criteria are fulfilled causes the
+        # subscription to no longer be found.
+        subscription_filter.find_all_tags = True
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([], list(subscriptions_for_bugtask))
+
+        # After adding the "baz" tag, the subscription is found again.
+        bugtask.bug.tags = ["bar", "baz"]
+        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
+            bugtask, BugNotificationLevel.NOTHING)
+        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+
 
 class TestStructuralSubscriptionForDistro(
     FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):