From 2946b37f902873c804413a7c5312c7f4625811a3 Mon Sep 17 00:00:00 2001
From: Martin Riese <mriese@dimagi.com>
Date: Wed, 11 Dec 2024 10:45:15 -0600
Subject: [PATCH] Clean up naming and add tests

---
 .../scheduling_partitioned/models.py          | 18 +++++++------
 .../scheduling/tests/test_recipients.py       | 25 +++++++++++++++----
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/corehq/messaging/scheduling/scheduling_partitioned/models.py b/corehq/messaging/scheduling/scheduling_partitioned/models.py
index cbbb160bafdc..866de58d1075 100644
--- a/corehq/messaging/scheduling/scheduling_partitioned/models.py
+++ b/corehq/messaging/scheduling/scheduling_partitioned/models.py
@@ -275,11 +275,13 @@ def _passes_user_data_filter(self, contact):
             actual_values_set = self.convert_to_set(user_data.get(key, ""))
 
             if actual_values_set.isdisjoint(allowed_values_set):
-                return False, f"Filtered out on property {key}: exp: ({','.join(allowed_values_set)}), act: ({','.join(actual_values_set)})"
+                return False, (f"Filtered out on property {key}: "
+                               f"exp: ({','.join(allowed_values_set)}), "
+                               f"act: ({','.join(actual_values_set)})")
 
         return True, ""
 
-    def expand_recipients(self, log_filter=None):
+    def expand_recipients(self, handle_filtered_recipient=None):
         """
         Can be used as a generator to iterate over all individual contacts who
         are the recipients of this ScheduleInstance.
@@ -290,11 +292,11 @@ def expand_recipients(self, log_filter=None):
 
         for member in recipient_list:
             for contact in self._expand_recipient(member):
-                passed, error_msg = self._passes_user_data_filter(contact)
+                passed, filter_reason = self._passes_user_data_filter(contact)
                 if passed:
                     yield contact
-                elif log_filter is not None:
-                    log_filter(contact, error_msg)
+                elif handle_filtered_recipient is not None:
+                    handle_filtered_recipient(contact, filter_reason)
 
     def get_content_send_lock(self, recipient):
         if is_commcarecase(recipient):
@@ -334,15 +336,15 @@ def send_current_event_content_to_recipients(self):
 
         logged_event = MessagingEvent.create_from_schedule_instance(self, content)
 
-        def log_filter_error(filtered_recipient, msg):
+        def log_filtered_recipient(filtered_recipient, reason):
             sub_event = logged_event.create_subevent_from_contact_and_content(
                 filtered_recipient,
                 content,
             )
-            sub_event.error(MessagingEvent.ERROR_FILTER_MISMATCH, msg)
+            sub_event.error(MessagingEvent.ERROR_FILTER_MISMATCH, reason)
 
         recipient_count = 0
-        for recipient in self.expand_recipients(log_filter_error):
+        for recipient in self.expand_recipients(log_filtered_recipient):
             recipient_count += 1
 
             #   The framework will retry sending a non-processed schedule instance
diff --git a/corehq/messaging/scheduling/tests/test_recipients.py b/corehq/messaging/scheduling/tests/test_recipients.py
index e4976875780d..46786c469525 100644
--- a/corehq/messaging/scheduling/tests/test_recipients.py
+++ b/corehq/messaging/scheduling/tests/test_recipients.py
@@ -114,15 +114,19 @@ def test_fails_with_user_data_filter_because_value_does_not_match(self):
         schedule = AlertSchedule()
         schedule.use_user_case_for_filter = False
         schedule.user_data_filter = {"wants_email": ["no"]}
-        self.assertFalse(ScheduleInstance(domain=self.domain, schedule=schedule)
-                         ._passes_user_data_filter(self.mobile_user))
+        passed, msg = (ScheduleInstance(domain=self.domain, schedule=schedule).
+                       _passes_user_data_filter(self.mobile_user))
+        self.assertFalse(passed)
+        self.assertEqual(msg, "Filtered out on property wants_email: exp: (no), act: (yes)")
 
     def test_fails_with_user_data_filter_because_one_value_does_not_match(self):
         schedule = AlertSchedule()
         schedule.use_user_case_for_filter = False
         schedule.user_data_filter = {"wants_email": ["yes"], "color": ["red"]}
-        self.assertFalse(ScheduleInstance(domain=self.domain, schedule=schedule)
-                         ._passes_user_data_filter(self.mobile_user))
+        passed, msg = (ScheduleInstance(domain=self.domain, schedule=schedule).
+                       _passes_user_data_filter(self.mobile_user))
+        self.assertFalse(passed)
+        self.assertEqual(msg, "Filtered out on property color: exp: (red), act: (green)")
 
     def test_passes_with_user_case_filter(self):
         case = create_case_2(self.domain, case_type="thing", case_json={"case_color": "green"})
@@ -652,10 +656,21 @@ def test_mobile_worker_recipients_with_user_data_filter(self):
             recipient_type='Group',
             recipient_id=self.group2.get_id
         )
+        message = ""
+        filtered_count = 0
+
+        def handle_filtered_recipient(_, msg):
+            nonlocal message
+            nonlocal filtered_count
+            message = msg
+            filtered_count += 1
+
         self.assertEqual(
-            self.user_ids(instance.expand_recipients()),
+            self.user_ids(instance.expand_recipients(handle_filtered_recipient)),
             [self.mobile_user4.get_id, self.mobile_user5.get_id, self.mobile_user6.get_id]
         )
+        self.assertEqual(message, "Filtered out on property role: exp: (nurse), act: (pharmacist)")
+        self.assertEqual(2, filtered_count)
 
     def test_web_user_recipient_with_user_data_filter(self):
         schedule = TimedSchedule.create_simple_daily_schedule(