fix: Added review suggestions

Author: jawad-khan

openedx/core/djangoapps/notifications/config/waffle.py

@@ -50,23 +50,13 @@
 # .. toggle_tickets: INF-1472
 ENABLE_NOTIFICATION_GROUPING = CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.enable_notification_grouping', __name__)
 
-# .. toggle_name: notifications.enable_new_notification_view
-# .. toggle_implementation: WaffleFlag
-# .. toggle_default: False
-# .. toggle_description: Waffle flag to enable new notification view
-# .. toggle_use_cases: temporary, open_edx
-# .. toggle_creation_date: 2024-09-30
-# .. toggle_target_removal_date: 2025-10-10
-# .. toggle_tickets: INF-1603
-ENABLE_NEW_NOTIFICATION_VIEW = WaffleFlag(f"{WAFFLE_NAMESPACE}.enable_new_notification_view", __name__)
-
 # .. toggle_name: notifications.enable_push_notifications
-# .. toggle_implementation: WaffleFlag
+# .. toggle_implementation: CourseWaffleFlag
 # .. toggle_default: False
 # .. toggle_description: Waffle flag to enable push Notifications feature on mobile devices
 # .. toggle_use_cases: temporary, open_edx
 # .. toggle_creation_date: 2025-02-11
 # .. toggle_target_removal_date: 2026-02-11
 # .. toggle_warning: When the flag is ON, Notifications will go through using braze on mobile devices.
 # .. toggle_tickets: LEARNER-10298
-ENABLE_PUSH_NOTIFICATIONS = WaffleFlag(f'{WAFFLE_NAMESPACE}.enable_push_notifications', __name__)
+ENABLE_PUSH_NOTIFICATIONS = CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.enable_push_notifications', __name__)

openedx/core/djangoapps/notifications/push/tasks.py

@@ -31,15 +31,18 @@ def send_ace_msg_to_braze_push_channel(audience_ids, notification_object, sender
     }
     emails = list(User.objects.filter(id__in=audience_ids).values_list('email', flat=True))
     context = {'post_data': post_data}
-
-    sender = User.objects.get(id=sender_id)
-    recipient = Recipient(sender.id, sender.email)
+    try:
+        sender = User.objects.get(id=sender_id)
+        recipient = Recipient(sender.id, sender.email)
+    except User.DoesNotExist:
+        recipient = None
 
     message = PushNotificationMessageType(
         app_label="notifications", name="braze_push"
     ).personalize(recipient, 'en', context)
     message.options['emails'] = emails
     message.options['braze_campaign'] = notification_type
+    message.options['skip_disable_user_policy'] = True
 
     ace.send(message, limit_to_channels=[ChannelType.BRAZE_PUSH])
     logger.info('Sent mobile notification for %s to ace channel', notification_type)

openedx/core/djangoapps/notifications/push/tests/__init__.py

@@ -0,0 +1,73 @@
+"""
+Tests for braze push notifications tasks.
+"""
+from unittest import mock
+
+from common.djangoapps.student.tests.factories import UserFactory
+from openedx.core.djangoapps.notifications.push.tasks import send_ace_msg_to_braze_push_channel
+from openedx.core.djangoapps.notifications.tests.utils import create_notification
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
+
+
+class SendNotificationsTest(ModuleStoreTestCase):
+    """
+    Tests for send_notifications.
+    """
+
+    def setUp(self):
+        """
+        Create a course and users for the course.
+        """
+
+        super().setUp()
+        self.user_1 = UserFactory()
+        self.user_2 = UserFactory()
+        self.course_1 = CourseFactory.create(
+            org='testorg',
+            number='testcourse',
+            run='testrun'
+        )
+
+        self.notification = create_notification(
+            self.user, self.course_1.id, app_name='discussion', notification_type='new_comment'
+        )
+
+    @mock.patch('openedx.core.djangoapps.notifications.push.tasks.ace.send')
+    def test_send_ace_msg_success(self, mock_ace_send):
+        """ Test send_ace_msg_success """
+        send_ace_msg_to_braze_push_channel(
+            [self.user_1.id, self.user_2.id],
+            self.notification,
+            sender_id=self.user_1.id
+        )
+
+        mock_ace_send.assert_called_once()
+        message_sent = mock_ace_send.call_args[0][0]
+        assert message_sent.options['emails'] == [self.user_1.email, self.user_2.email]
+        assert message_sent.options['braze_campaign'] == 'new_comment'
+
+    @mock.patch('openedx.core.djangoapps.notifications.push.tasks.ace.send')
+    def test_send_ace_msg_no_sender(self, mock_ace_send):
+        """ Test when sender is not valid """
+        send_ace_msg_to_braze_push_channel(
+            [self.user_1.id, self.user_2.id],
+            self.notification,
+            sender_id=999
+        )
+
+        mock_ace_send.assert_called_once()
+
+    @mock.patch('openedx.core.djangoapps.notifications.push.tasks.ace.send')
+    def test_send_ace_msg_empty_audience(self, mock_ace_send):
+        """ Test send_ace_msg_success with empty audience """
+        send_ace_msg_to_braze_push_channel([], self.notification, sender_id=self.user_1.id)
+        mock_ace_send.assert_not_called()
+
+    @mock.patch('openedx.core.djangoapps.notifications.push.tasks.ace.send')
+    def test_send_ace_msg_non_discussion_app(self, mock_ace_send):
+        """ Test send_ace_msg_success with non-discussion app """
+        self.notification.app_name = 'ecommerce'
+        self.notification.save()
+        send_ace_msg_to_braze_push_channel([1], self.notification, sender_id=self.user_1.id)
+        mock_ace_send.assert_not_called()

openedx/core/djangoapps/notifications/push/tests/test_tasks.py

@@ -26,8 +26,9 @@
 )
 from openedx.core.djangoapps.notifications.events import notification_generated_event
 from openedx.core.djangoapps.notifications.grouping_notifications import (
+    NotificationRegistry,
     get_user_existing_notifications,
-    group_user_notifications, NotificationRegistry,
+    group_user_notifications
 )
 from openedx.core.djangoapps.notifications.models import (
     CourseNotificationPreference,
@@ -37,7 +38,6 @@
 from openedx.core.djangoapps.notifications.push.tasks import send_ace_msg_to_braze_push_channel
 from openedx.core.djangoapps.notifications.utils import clean_arguments, get_list_in_batches
 
-
 logger = get_task_logger(__name__)
 
 
@@ -125,6 +125,7 @@ def send_notifications(user_ids, course_key: str, app_name, notification_type, c
     """
     Send notifications to the users.
     """
+    # pylint: disable=too-many-statements
     course_key = CourseKey.from_string(course_key)
     if not ENABLE_NOTIFICATIONS.is_enabled(course_key):
         return
@@ -143,7 +144,7 @@ def send_notifications(user_ids, course_key: str, app_name, notification_type, c
     default_web_config = get_default_values_of_preference(app_name, notification_type).get('web', False)
     generated_notification_audience = []
     push_notification_audience = []
-    is_push_notification_enabled = ENABLE_PUSH_NOTIFICATIONS.is_enabled()
+    is_push_notification_enabled = ENABLE_PUSH_NOTIFICATIONS.is_enabled(course_key)
 
     if group_by_id and not grouping_enabled:
         logger.info(

openedx/core/djangoapps/notifications/tasks.py

@@ -9,15 +9,14 @@
 from django.conf import settings
 from django.core.exceptions import ValidationError
 from edx_toggles.toggles.testutils import override_waffle_flag
-from testfixtures import LogCapture
 
 from common.djangoapps.student.models import CourseEnrollment
 from common.djangoapps.student.tests.factories import UserFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
-from ..config.waffle import ENABLE_NOTIFICATIONS, ENABLE_NOTIFICATION_GROUPING, ENABLE_PUSH_NOTIFICATIONS
-from ..models import CourseNotificationPreference, Notification, NotificationBrazeCampaigns
+from ..config.waffle import ENABLE_NOTIFICATION_GROUPING, ENABLE_NOTIFICATIONS, ENABLE_PUSH_NOTIFICATIONS
+from ..models import CourseNotificationPreference, Notification
 from ..tasks import (
     create_notification_pref_if_not_exists,
     delete_notifications,
@@ -26,8 +25,6 @@
 )
 from .utils import create_notification
 
-LOGGER_NAME = 'openedx.core.djangoapps.notifications.tasks'
-
 
 @patch('openedx.core.djangoapps.notifications.models.COURSE_NOTIFICATION_CONFIG_VERSION', 1)
 class TestNotificationsTasks(ModuleStoreTestCase):
@@ -234,7 +231,6 @@ def test_send_notification_with_grouping_enabled(self):
             user_notifications_mock.assert_called_once()
 
     @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True)
-    @override_waffle_flag(ENABLE_PUSH_NOTIFICATIONS, active=True)
     @ddt.data(
         ('discussion', 'new_comment_on_response'),  # core notification
         ('discussion', 'new_response'),  # non core notification
@@ -261,7 +257,6 @@ def test_send_with_app_disabled_notifications(self, app_name, notification_type)
         self.assertIsNone(notification)
 
     @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True)
-    @override_waffle_flag(ENABLE_PUSH_NOTIFICATIONS, active=True)
     def test_notification_not_created_when_context_is_incomplete(self):
         try:
             send_notifications([self.user.id], str(self.course_1.id), "discussion", "new_comment", {}, "")
@@ -299,11 +294,10 @@ def _create_users(self, num_of_users):
         return users
 
     @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True)
-    @override_waffle_flag(ENABLE_PUSH_NOTIFICATIONS, active=True)
     @ddt.data(
-        (settings.NOTIFICATION_CREATION_BATCH_SIZE, 10, 6),
-        (settings.NOTIFICATION_CREATION_BATCH_SIZE + 10, 12, 9),
-        (settings.NOTIFICATION_CREATION_BATCH_SIZE - 10, 10, 5),
+        (settings.NOTIFICATION_CREATION_BATCH_SIZE, 13, 6),
+        (settings.NOTIFICATION_CREATION_BATCH_SIZE + 10, 15, 9),
+        (settings.NOTIFICATION_CREATION_BATCH_SIZE - 10, 13, 5),
     )
     @ddt.unpack
     def test_notification_is_send_in_batch(self, creation_size, prefs_query_count, notifications_query_count):
@@ -354,7 +348,7 @@ def test_preference_not_created_for_default_off_preference(self):
             "username": "Test Author"
         }
         with override_waffle_flag(ENABLE_NOTIFICATIONS, active=True):
-            with self.assertNumQueries(11):
+            with self.assertNumQueries(13):
                 send_notifications(user_ids, str(self.course.id), notification_app, notification_type,
                                    context, "http://test.url")
 
@@ -374,7 +368,7 @@ def test_preference_created_for_default_on_preference(self):
         }
         with override_waffle_flag(ENABLE_NOTIFICATIONS, active=True):
             with override_waffle_flag(ENABLE_PUSH_NOTIFICATIONS, active=True):
-                with self.assertNumQueries(13):
+                with self.assertNumQueries(16):
                     send_notifications(user_ids, str(self.course.id), notification_app, notification_type,
                                        context, "http://test.url")
 
@@ -419,79 +413,6 @@ def test_preference_enabled_in_batch_audience(self, notification_type,
         send_notifications(user_ids, str(self.course.id), app_name, notification_type, context, content_url)
         self.assertEqual(len(Notification.objects.all()), generated_count)
 
-    @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True)
-    @override_waffle_flag(ENABLE_PUSH_NOTIFICATIONS, active=True)
-    @patch('openedx.core.djangoapps.notifications.tasks.get_braze_client')
-    def test_mobile_notification_send(self, mock_braze_client):
-        mock_braze_client = mock_braze_client.return_value
-        mock_braze_client.send_campaign_message.return_value = None
-
-        notification_app = "discussion"
-        notification_type = "new_response"
-        users = self._create_users(5)
-        user_ids = [user.id for user in users]
-        context = {
-            "post_title": "Test Post",
-            "author_name": "Test Author",
-            "replier_name": "Replier Name"
-        }
-        with LogCapture() as log:
-            send_notifications(user_ids, str(self.course.id), notification_app,
-                               notification_type, context, "https://test.url")
-
-            self.assertFalse(mock_braze_client.send_campaign_message.called)
-            log.check_present(
-                (
-                    'openedx.core.djangoapps.notifications.models',
-                    'INFO',
-                    'No campaign id found for notification type new_response',
-                )
-            )
-
-            NotificationBrazeCampaigns.objects.create(notification_type=notification_type, braze_campaign_id="123")
-            send_notifications(user_ids, str(self.course.id), notification_app,
-                               notification_type, context, "https://test.url")
-
-            self.assertTrue(mock_braze_client.send_campaign_message.called)
-            log.check_present(
-                (
-                    LOGGER_NAME,
-                    'INFO',
-                    'Sent mobile notification for new_response with Braze',
-                )
-            )
-
-    @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True)
-    @override_waffle_flag(ENABLE_PUSH_NOTIFICATIONS, active=True)
-    @patch('openedx.core.djangoapps.notifications.tasks.get_braze_client')
-    def test_mobile_notification_braze_error(self, mock_braze_client):
-        mock_braze_client = mock_braze_client.return_value
-        mock_braze_client.send_campaign_message.side_effect = Exception("Error")
-
-        notification_app = "discussion"
-        notification_type = "new_response"
-        users = self._create_users(5)
-        user_ids = [user.id for user in users]
-        NotificationBrazeCampaigns.objects.create(notification_type=notification_type, braze_campaign_id="123")
-
-        context = {
-            "post_title": "Test Post",
-            "author_name": "Test Author",
-            "replier_name": "Replier Name"
-        }
-        with LogCapture() as log:
-            send_notifications(user_ids, str(self.course.id), notification_app,
-                               notification_type, context, "https://test.url")
-
-            self.assertTrue(mock_braze_client.send_campaign_message.called)
-            log.check_present(
-                (
-                    LOGGER_NAME,
-                    'ERROR',
-                    'Unable to send mobile notification for new_response with Braze. Reason: Error',
-                )
-            )
-
 
 class TestDeleteNotificationTask(ModuleStoreTestCase):
     """

openedx/core/djangoapps/notifications/tests/test_tasks.py

@@ -398,7 +398,7 @@ drf-yasg==1.21.10
     # via
     #   django-user-tasks
     #   edx-api-doc-tools
-edx-ace==1.12.1
+edx-ace==1.14.0
     # via -r requirements/edx/kernel.in
 edx-api-doc-tools==2.0.0
     # via

requirements/edx/base.txt

@@ -658,7 +658,7 @@ drf-yasg==1.21.10
     #   -r requirements/edx/testing.txt
     #   django-user-tasks
     #   edx-api-doc-tools
-edx-ace==1.12.1
+edx-ace==1.14.0
     # via
     #   -r requirements/edx/doc.txt
     #   -r requirements/edx/testing.txt

requirements/edx/development.txt

@@ -482,7 +482,7 @@ drf-yasg==1.21.10
     #   -r requirements/edx/base.txt
     #   django-user-tasks
     #   edx-api-doc-tools
-edx-ace==1.12.1
+edx-ace==1.14.0
     # via -r requirements/edx/base.txt
 edx-api-doc-tools==2.0.0
     # via

requirements/edx/doc.txt

@@ -507,7 +507,7 @@ drf-yasg==1.21.10
     #   -r requirements/edx/base.txt
     #   django-user-tasks
     #   edx-api-doc-tools
-edx-ace==1.12.1
+edx-ace==1.14.0
     # via -r requirements/edx/base.txt
 edx-api-doc-tools==2.0.0
     # via