launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+442903@xxxxxxxxxxxxxxxxxx>]

Simone Pelosi has proposed merging ~pelpsi/launchpad:muted-subscription-bug into launchpad:master.

Commit message:
Added new logic to filter BugNotificationRecipient
    
filters_by_person (dictionary) is the new data structure introduced:
key is the person_id and the value is a set of tuple with the following format (BugNotificationRecipe, filter_id).
Thanks to that structure we can filter the muted notifications simply by removing them from the filters_by_person[person_id] set.
The remaining notifications will be sent to the user.
    
LP: #2019428

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #2019428 in Launchpad itself: "Not receiving (proper) emails for packages I'm subscribed to"
  https://bugs.launchpad.net/launchpad/+bug/2019428

For more details, see:
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/442903
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:muted-subscription-bug into launchpad:master.

diff --git a/lib/lp/bugs/model/bugnotification.py b/lib/lp/bugs/model/bugnotification.py
index 9634c6a..1500351 100644
--- a/lib/lp/bugs/model/bugnotification.py
+++ b/lib/lp/bugs/model/bugnotification.py
@@ -10,6 +10,7 @@ __all__ = [
     "BugNotificationSet",
 ]
 
+from collections import defaultdict
 from datetime import datetime, timedelta, timezone
 
 from storm.expr import In, Join, LeftJoin
@@ -255,6 +256,8 @@ class BugNotificationSet:
         # First we get some intermediate data structures set up.
         source_person_id_map = {}
         recipient_id_map = {}
+        notifications_by_source = defaultdict(set)
+
         for recipient, sources in recipient_to_sources.items():
             if recipient.id in muted_person_ids:
                 continue
@@ -279,10 +282,11 @@ class BugNotificationSet:
                     }
                     source_person_id_map[person_id] = data
                 data["sources"].add(source)
+                notifications_by_source[person_id].add(source)
         # Now we actually look for the filters.
         store = IStore(BugSubscriptionFilter)
         source = store.using(
-            BugSubscriptionFilter,
+            (BugSubscriptionFilter,),
             Join(
                 BugNotificationFilter,
                 BugSubscriptionFilter.id
@@ -312,25 +316,49 @@ class BugNotificationSet:
                     list(source_person_id_map),
                 ),
             )
+
         filter_ids = []
+        no_filter_marker = -1
+
+        # Associate filters to each notification to facilitate search
+        filters_by_person = defaultdict(set)
         # Record the filters for each source.
+        source_filter_ids = set()
         for source_person_id, filter_id, filter_description in filter_data:
+            source_filter_ids.add(source_person_id)
             source_person_id_map[source_person_id]["filters"][
                 filter_id
             ] = filter_description
             filter_ids.append(filter_id)
 
+            filters_by_person[source_person_id] = {
+                (i, filter_id)
+                for i in notifications_by_source[source_person_id]
+            }
+
+        for id in source_filter_ids:
+            del notifications_by_source[id]
+
+        # Remaining notifications have no filters
+        for key in notifications_by_source:
+            filters_by_person[key] = {
+                (i, no_filter_marker) for i in notifications_by_source[key]
+            }
         # This is only necessary while production and sample data have
         # structural subscriptions without filters.  Assign the filters to
         # each recipient.
-        no_filter_marker = -1
-
         for recipient_data in recipient_id_map.values():
             for source_person_id in recipient_data["source person ids"]:
                 recipient_data["filters"].update(
                     source_person_id_map[source_person_id]["filters"]
                     or {no_filter_marker: None}
                 )
+
+                filters_by_person[
+                    recipient_data["principal"].id
+                ] = filters_by_person[recipient_data["principal"].id].union(
+                    filters_by_person[source_person_id]
+                )
         if filter_ids:
             # Now we get the information about subscriptions that might be
             # filtered and take that into account.
@@ -355,19 +383,37 @@ class BugNotificationSet:
                     del recipient_id_map[person_id]["filters"][
                         no_filter_marker
                     ]
+
+                # Remove notification if it's muted
+                filtered_set = set()
+                for tuple in filters_by_person[person_id]:
+                    if tuple[1] != filter_id:
+                        filtered_set.add(tuple)
+                filters_by_person[person_id] = filtered_set
+
         # Now recipient_id_map has all the information we need.  Let's
         # build the final result and return it.
         result = {}
         for recipient_data in recipient_id_map.values():
+            # Getting filtered sources
+
             if recipient_data["filters"]:
                 filter_descriptions = [
                     description
                     for description in recipient_data["filters"].values()
                     if description
                 ]
-                filter_descriptions.sort()  # This is good for tests.
+                filtered_source = []
+                for sources in filters_by_person[
+                    recipient_data["principal"].id
+                ]:
+                    filtered_source.append(sources[0])
+
+                # This is good for tests.
+                filtered_source.reverse()
+                filter_descriptions.sort()
                 result[recipient_data["principal"]] = {
-                    "sources": recipient_data["sources"],
+                    "sources": filtered_source,
                     "filter descriptions": filter_descriptions,
                 }
         return result
diff --git a/lib/lp/bugs/scripts/tests/test_bugnotification.py b/lib/lp/bugs/scripts/tests/test_bugnotification.py
index 9dc39fb..cf65f83 100644
--- a/lib/lp/bugs/scripts/tests/test_bugnotification.py
+++ b/lib/lp/bugs/scripts/tests/test_bugnotification.py
@@ -1511,3 +1511,62 @@ class TestSendBugNotifications(TestCaseWithFactory):
                     self.assertEqual(
                         BugNotificationStatus.SENT, bug_notification.status
                     )
+
+    def test_muted_team_subscription(self):
+        # If a user holds a mute on a Team subscription,
+        # having a personal subscription for that bug
+        # should receive only notifications related to that.
+
+        team_owner = self.factory.makePerson(
+            name="team-owner", email="team-owner@xxxxxxxxxxxxx"
+        )
+        team_participant = self.factory.makePerson(
+            name="team-participant", email="team-participant@xxxxxxxxxxxxx"
+        )
+
+        team = self.factory.makeTeam(
+            name="team-name", owner=team_owner, members=[team_participant]
+        )
+        bug = self.factory.makeBug()
+
+        subscription = bug.default_bugtask.target.addBugSubscription(
+            team, team_owner
+        )
+        # Take team subscription's filter
+        subscription_filter = subscription.bug_filters.one()
+        bug.default_bugtask.target.addBugSubscription(
+            team_participant, team_participant
+        )
+
+        # Mute team subscription for team_participant
+
+        message = getUtility(IMessageSet).fromText(
+            "subject",
+            "a comment.",
+            bug.owner,
+            datecreated=self.ten_minutes_ago,
+        )
+        notification = bug.addCommentNotification(message)
+
+        BugSubscriptionFilterMute(
+            person=team_participant,
+            filter=subscription_filter,
+        )
+
+        notification = self.notification_set.getNotificationsToSend()
+
+        pending_notification = get_email_notifications(notification)
+        # Since team subscription is muted for team_participant
+        # we expect two messages: one for the team member (owner)
+        # and the other for the personal subscription.
+        # team-owner@xxxxxxxxxxxxx team-name-100000
+        # team-participant@xxxxxxxxxxxxx team-participant
+
+        _, _, messages = list(pending_notification)[0]
+
+        self.assertEqual("team-owner@xxxxxxxxxxxxx", messages[0]["To"])
+        self.assertEqual("team-participant@xxxxxxxxxxxxx", messages[1]["To"])
+        self.assertEqual("team-name", messages[0]["X-Launchpad-Message-For"])
+        self.assertEqual(
+            "team-participant", messages[1]["X-Launchpad-Message-For"]
+        )