From f37eedc577a965b622a8dfe472c442bfcf5b776a Mon Sep 17 00:00:00 2001 From: "adeel.tajamul" Date: Wed, 4 Oct 2023 16:08:00 +0500 Subject: [PATCH] feat: notifications and preferences will be created in batches --- lms/envs/common.py | 1 + .../notifications/base_notification.py | 13 +++ .../core/djangoapps/notifications/models.py | 10 +- .../core/djangoapps/notifications/tasks.py | 72 +++++++----- .../notifications/tests/test_tasks.py | 108 ++++++++++++++++++ .../core/djangoapps/notifications/utils.py | 9 ++ 6 files changed, 181 insertions(+), 32 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index 6c78327d07e8..3a6f8d95d0ad 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -5380,6 +5380,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring ############## NOTIFICATIONS EXPIRY ############## NOTIFICATIONS_EXPIRY = 60 EXPIRED_NOTIFICATIONS_DELETE_BATCH_SIZE = 10000 +NOTIFICATION_CREATION_BATCH_SIZE = 99 #### django-simple-history## # disable indexing on date field its coming from django-simple-history. diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index ec3018c0079e..d7d5f58574bc 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -322,3 +322,16 @@ def get_notification_content(notification_type, context): if notification_type_content_template: return notification_type_content_template.format(**context, **html_tags_context) return '' + + +def get_default_values_of_preference(notification_app, notification_type): + """ + Returns default preference for notification_type + """ + default_prefs = NotificationAppManager().get_notification_app_preferences() + app_prefs = default_prefs.get(notification_app, {}) + core_notification_types = app_prefs.get('core_notification_types', []) + notification_types = app_prefs.get('notification_types', {}) + if notification_type in core_notification_types: + return notification_types.get('core', {}) + return notification_types.get(notification_type, {}) diff --git a/openedx/core/djangoapps/notifications/models.py b/openedx/core/djangoapps/notifications/models.py index d0f8daf0e553..66bd3a109b80 100644 --- a/openedx/core/djangoapps/notifications/models.py +++ b/openedx/core/djangoapps/notifications/models.py @@ -130,12 +130,12 @@ def __str__(self): return f'{self.user.username} - {self.course_id}' @staticmethod - def get_updated_user_course_preferences(user, course_id): + def get_user_course_preference(user_id, course_id): """ Returns updated courses preferences for a user """ preferences, _ = CourseNotificationPreference.objects.get_or_create( - user=user, + user_id=user_id, course_id=course_id, is_active=True, ) @@ -149,9 +149,13 @@ def get_updated_user_course_preferences(user, course_id): preferences.save() # pylint: disable-next=broad-except except Exception as e: - log.error(f'Unable to update notification preference for {user.username} to new config. {e}') + log.error(f'Unable to update notification preference to new config. {e}') return preferences + @staticmethod + def get_updated_user_course_preferences(user, course_id): + return CourseNotificationPreference.get_user_course_preference(user.id, course_id) + def get_app_config(self, app_name) -> Dict: """ Returns the app config for the given app name. diff --git a/openedx/core/djangoapps/notifications/tasks.py b/openedx/core/djangoapps/notifications/tasks.py index e59ff77eb16b..1401a2d9ab17 100644 --- a/openedx/core/djangoapps/notifications/tasks.py +++ b/openedx/core/djangoapps/notifications/tasks.py @@ -13,6 +13,7 @@ from pytz import UTC from common.djangoapps.student.models import CourseEnrollment +from openedx.core.djangoapps.notifications.base_notification import get_default_values_of_preference from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS from openedx.core.djangoapps.notifications.events import notification_generated_event from openedx.core.djangoapps.notifications.models import ( @@ -20,6 +21,7 @@ Notification, get_course_notification_preference_config_version ) +from openedx.core.djangoapps.notifications.utils import get_list_in_batches logger = get_task_logger(__name__) @@ -90,49 +92,61 @@ def send_notifications(user_ids, course_key: str, app_name, notification_type, c if not ENABLE_NOTIFICATIONS.is_enabled(course_key): return user_ids = list(set(user_ids)) + batch_size = settings.NOTIFICATION_CREATION_BATCH_SIZE - # check if what is preferences of user and make decision to send notification or not - preferences = CourseNotificationPreference.objects.filter( - user_id__in=user_ids, - course_id=course_key, - ) - preferences = create_notification_pref_if_not_exists(user_ids, list(preferences), course_key) - notifications = [] audience = [] - for preference in preferences: - preference = update_user_preference(preference, preference.user, course_key) - if ( - preference and - preference.get_web_config(app_name, notification_type) and - preference.get_app_config(app_name).get('enabled', False) - ): - notifications.append( - Notification( - user_id=preference.user_id, - app_name=app_name, - notification_type=notification_type, - content_context=context, - content_url=content_url, - course_id=course_key, + notifications_generated = False + notification_content = '' + default_web_config = get_default_values_of_preference(app_name, notification_type).get('web', True) + for batch_user_ids in get_list_in_batches(user_ids, batch_size): + # check if what is preferences of user and make decision to send notification or not + preferences = CourseNotificationPreference.objects.filter( + user_id__in=batch_user_ids, + course_id=course_key, + ) + preferences = list(preferences) + + if default_web_config: + preferences = create_notification_pref_if_not_exists(batch_user_ids, preferences, course_key) + + notifications = [] + for preference in preferences: + preference = update_user_preference(preference, preference.user_id, course_key) + if ( + preference and + preference.get_web_config(app_name, notification_type) and + preference.get_app_config(app_name).get('enabled', False) + ): + notifications.append( + Notification( + user_id=preference.user_id, + app_name=app_name, + notification_type=notification_type, + content_context=context, + content_url=content_url, + course_id=course_key, + ) ) - ) - audience.append(preference.user_id) - # send notification to users but use bulk_create - notifications_generated = Notification.objects.bulk_create(notifications) + audience.append(preference.user_id) + # send notification to users but use bulk_create + notification_objects = Notification.objects.bulk_create(notifications) + if notification_objects and not notifications_generated: + notifications_generated = True + notification_content = notification_objects[0].content + if notifications_generated: - notification_content = notifications_generated[0].content notification_generated_event( audience, app_name, notification_type, course_key, content_url, notification_content, ) -def update_user_preference(preference: CourseNotificationPreference, user, course_id): +def update_user_preference(preference: CourseNotificationPreference, user_id, course_id): """ Update user preference if config version is changed. """ current_version = get_course_notification_preference_config_version() if preference.config_version != current_version: - return preference.get_updated_user_course_preferences(user, course_id) + return preference.get_user_course_preference(user_id, course_id) return preference diff --git a/openedx/core/djangoapps/notifications/tests/test_tasks.py b/openedx/core/djangoapps/notifications/tests/test_tasks.py index 51cdc741d6ef..3164f9a9bae2 100644 --- a/openedx/core/djangoapps/notifications/tests/test_tasks.py +++ b/openedx/core/djangoapps/notifications/tests/test_tasks.py @@ -5,8 +5,10 @@ from unittest.mock import patch import ddt +from django.conf import settings from edx_toggles.toggles.testutils import override_waffle_flag +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 @@ -159,3 +161,109 @@ def test_send_with_app_disabled_notifications(self, app_name, notification_type) # Assert that `Notification` objects are not created for the users. notification = Notification.objects.filter(user_id=self.user.id).first() self.assertIsNone(notification) + + +@ddt.ddt +class SendBatchNotificationsTest(ModuleStoreTestCase): + """ + Test that notification and notification preferences are created in batches + """ + def setUp(self): + """ + Setups test case + """ + super().setUp() + self.course = CourseFactory.create( + org='test_org', + number='test_course', + run='test_run' + ) + + def _create_users(self, num_of_users): + """ + Create users and enroll them in course + """ + users = [ + UserFactory.create(username=f'user{i}', email=f'user{i}@example.com') + for i in range(num_of_users) + ] + for user in users: + CourseEnrollment.enroll(user=user, course_key=self.course.id) + return users + + @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) + @ddt.data( + (settings.NOTIFICATION_CREATION_BATCH_SIZE, 1, 2), + (settings.NOTIFICATION_CREATION_BATCH_SIZE + 10, 2, 4), + (settings.NOTIFICATION_CREATION_BATCH_SIZE - 10, 1, 2), + ) + @ddt.unpack + def test_notification_is_send_in_batch(self, creation_size, prefs_query_count, notifications_query_count): + """ + Tests notifications and notification preferences are created in batches + """ + notification_app = "discussion" + notification_type = "new_discussion_post" + users = self._create_users(creation_size) + user_ids = [user.id for user in users] + context = { + "post_title": "Test Post", + "username": "Test Author" + } + + # Creating preferences and asserting query count + with self.assertNumQueries(prefs_query_count): + send_notifications(user_ids, str(self.course.id), notification_app, notification_type, + context, "http://test.url") + + # Updating preferences for notification creation + preferences = CourseNotificationPreference.objects.filter( + user_id__in=user_ids, + course_id=self.course.id + ) + for preference in preferences: + discussion_config = preference.notification_preference_config['discussion'] + discussion_config['notification_types'][notification_type]['web'] = True + preference.save() + + # Creating notifications and asserting query count + with self.assertNumQueries(notifications_query_count): + send_notifications(user_ids, str(self.course.id), notification_app, notification_type, + context, "http://test.url") + + def test_preference_not_created_for_default_off_preference(self): + """ + Tests if new preferences are NOT created when default preference for + notification type is False + """ + notification_app = "discussion" + notification_type = "new_discussion_post" + users = self._create_users(20) + user_ids = [user.id for user in users] + context = { + "post_title": "Test Post", + "username": "Test Author" + } + with override_waffle_flag(ENABLE_NOTIFICATIONS, active=True): + with self.assertNumQueries(1): + send_notifications(user_ids, str(self.course.id), notification_app, notification_type, + context, "http://test.url") + + def test_preference_created_for_default_on_preference(self): + """ + Tests if new preferences are created when default preference for + notification type is True + """ + notification_app = "discussion" + notification_type = "new_comment" + users = self._create_users(20) + user_ids = [user.id for user in users] + context = { + "post_title": "Test Post", + "author_name": "Test Author", + "replier_name": "Replier Name" + } + with override_waffle_flag(ENABLE_NOTIFICATIONS, active=True): + with self.assertNumQueries(3): + send_notifications(user_ids, str(self.course.id), notification_app, notification_type, + context, "http://test.url") diff --git a/openedx/core/djangoapps/notifications/utils.py b/openedx/core/djangoapps/notifications/utils.py index 4ee3a614a7b3..39bf8755da8e 100644 --- a/openedx/core/djangoapps/notifications/utils.py +++ b/openedx/core/djangoapps/notifications/utils.py @@ -42,3 +42,12 @@ def get_show_notifications_tray(user): break return show_notifications_tray + + +def get_list_in_batches(input_list, batch_size): + """ + Divides the list of objects into list of list of objects each of length batch_size. + """ + list_length = len(input_list) + for index in range(0, list_length, batch_size): + yield input_list[index: index + batch_size]