diff --git a/openedx/core/djangoapps/notifications/email/tasks.py b/openedx/core/djangoapps/notifications/email/tasks.py index 9e9367153365..966b51c1e917 100644 --- a/openedx/core/djangoapps/notifications/email/tasks.py +++ b/openedx/core/djangoapps/notifications/email/tasks.py @@ -15,7 +15,6 @@ from edx_ace import ace from edx_ace.recipient import Recipient from edx_django_utils.monitoring import set_code_owner_attribute -from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.notifications.email_notifications import EmailCadence from openedx.core.djangoapps.notifications.models import ( @@ -530,6 +529,13 @@ def decide_email_action(user: User, course_key: str, notification: Notification) """ Decide what to do with this notification. + The immediate-email buffer is scoped per user (not per user + course): a + learner who just received an immediate email for any enrolled course has + subsequent immediate notifications — regardless of course — grouped into a + single buffered digest. ``course_key`` is accepted for signature stability + and is used by the caller for the immediate-email content, but it no longer + affects the batching decision. + Logic: - No recent email? → send_immediate (1st) - Recent email + no buffer? → schedule_buffer (2nd) @@ -541,10 +547,10 @@ def decide_email_action(user: User, course_key: str, notification: Notification) buffer_minutes = get_buffer_minutes() buffer_threshold = datetime.now() - timedelta(minutes=buffer_minutes) - # Use select_for_update to prevent race conditions + # Use select_for_update to prevent race conditions. + # Scoped by user only so all of the user's courses share one buffer window. recent_notifications = Notification.objects.select_for_update().filter( user=user, - course_id=course_key, created__gte=buffer_threshold ) @@ -647,10 +653,10 @@ def schedule_digest_buffer( """ buffer_minutes = get_buffer_minutes() - # Find when we last sent an email + # Find when we last sent an email to this user (across all courses — the + # buffer is per user, so the digest window starts at the user's most recent send). last_sent = Notification.objects.filter( user=user, - course_id=course_key, email_sent_on__isnull=False ).order_by('-email_sent_on').first() @@ -711,7 +717,11 @@ def send_buffered_digest( Send digest email with all buffered notifications. This collects ALL notifications where email_scheduled=True - for this user+course within the buffer period. + for this user within the buffer period, regardless of course. + + ``course_key`` is retained in the signature for compatibility with tasks + that were enqueued before the per-user batching change; it is no longer + used to filter notifications. Simple! No task ID tracking needed. """ @@ -729,11 +739,10 @@ def send_buffered_digest( end_date = datetime.now() - # Get ALL scheduled notifications + # Get ALL scheduled notifications for the user (across every course). # Simple query: just find where email_scheduled=True scheduled_notifications = Notification.objects.filter( user=user, - course_id=course_key, email_scheduled=True, # This is all we need! created__gte=start_date, created__lte=end_date, @@ -764,10 +773,9 @@ def send_buffered_digest( scheduled_notifications.update(email_scheduled=False) return - # Build digest email + # Build digest email. The digest may span multiple courses; per-notification + # course names are resolved on demand inside create_email_digest_context. apps_dict = create_app_notifications_dict(notifications_list) - course_key = CourseKey.from_string(course_key) - course_name = get_course_info(course_key).get("name", course_key) message_context = create_email_digest_context( apps_dict, @@ -775,7 +783,7 @@ def send_buffered_digest( start_date, end_date, EmailCadence.IMMEDIATELY, - courses_data={course_key: {'name': course_name}} + courses_data={} ) # Send digest diff --git a/openedx/core/djangoapps/notifications/email/tests/test_tasks.py b/openedx/core/djangoapps/notifications/email/tests/test_tasks.py index f97f63f64ac1..5acc0dcfead9 100644 --- a/openedx/core/djangoapps/notifications/email/tests/test_tasks.py +++ b/openedx/core/djangoapps/notifications/email/tests/test_tasks.py @@ -457,22 +457,24 @@ def test_old_email_triggers_new_immediate_send(self): assert decision == 'send_immediate' @freeze_time("2025-12-15 10:00:00") - def test_different_course_doesnt_affect_decision(self): - """Test that notifications from different courses are independent.""" + @override_settings(NOTIFICATION_IMMEDIATE_EMAIL_BUFFER_MINUTES=15) + def test_recent_email_in_different_course_schedules_buffer(self): + """A recent email in another course counts toward the user-level buffer.""" other_course = CourseFactory.create() - # Notification from different course + # Email recently sent for a different course self._create_notification( course_id=str(other_course.id), email_sent_on=timezone.now() - timedelta(minutes=5) ) - # This course should still send immediate + # New notification in this course should join the shared (per-user) buffer, + # not send immediately, because the user already got a recent email. notification = self._create_notification() decision = decide_email_action(self.user, self.course_key, notification) - assert decision == 'send_immediate' + assert decision == 'schedule_buffer' @freeze_time("2025-12-15 10:00:00") def test_race_condition_protection(self): @@ -760,6 +762,44 @@ def test_digest_collects_all_scheduled_notifications(self, mock_ace_send): assert notif.email_sent_on is not None assert notif.email_scheduled is False + @freeze_time("2025-12-15 10:15:00") + @patch('openedx.core.djangoapps.notifications.email.tasks.ace.send') + def test_digest_collects_notifications_across_courses(self, mock_ace_send): + """Buffered digest pools scheduled notifications from all of the user's courses.""" + course2 = CourseFactory.create(display_name='Second Course') + start_time = timezone.now() - timedelta(minutes=15) + + for course_id in (self.course_key, str(course2.id)): + Notification.objects.create( + user=self.user, + course_id=course_id, + app_name='discussion', + notification_type='new_discussion_post', + content_url='http://example.com', + content_context=get_new_post_notification_content_context(), + email=True, + email_scheduled=True, + created=start_time + timedelta(minutes=5) + ) + + send_buffered_digest( # pylint: disable=no-value-for-parameter + user_id=self.user.id, + course_key=self.course_key, + start_date=start_time, + user_language='en' + ) + + # A single digest email is sent... + assert mock_ace_send.call_count == 1 + # ...covering the notifications from BOTH courses. + sent = Notification.objects.filter( + user=self.user, + email_sent_on__isnull=False, + email_scheduled=False, + ) + assert sent.count() == 2 + assert {str(n.course_id) for n in sent} == {self.course_key, str(course2.id)} + @patch('openedx.core.djangoapps.notifications.email.tasks.ace.send') def test_digest_skips_non_scheduled_notifications(self, mock_ace_send): """Test that digest only includes scheduled notifications.""" @@ -1050,12 +1090,14 @@ def test_notification_after_buffer_expires_sends_immediate(self, mock_ace_send): assert notif2.email_sent_on is not None assert notif2.email_scheduled is False - def test_multiple_courses_independent_buffers(self): - """Test that different courses maintain independent buffers.""" + @freeze_time("2025-12-15 10:00:00") + @override_settings(NOTIFICATION_IMMEDIATE_EMAIL_BUFFER_MINUTES=15) + def test_multiple_courses_share_buffer(self): + """Notifications from different courses share a single per-user buffer.""" course2 = CourseFactory.create() - # Notifications in course 1 - notif1 = Notification.objects.create( # noqa: F841 + # Recent email sent for course 1 + Notification.objects.create( user=self.user, course_id=self.course.id, app_name='discussion', @@ -1065,7 +1107,7 @@ def test_multiple_courses_independent_buffers(self): email_sent_on=timezone.now() - timedelta(minutes=5) ) - # Notification in course 2 should be independent + # Notification in course 2 should join the shared buffer (not send immediately) notif2 = Notification.objects.create( user=self.user, course_id=str(course2.id), @@ -1076,7 +1118,7 @@ def test_multiple_courses_independent_buffers(self): ) decision = decide_email_action(self.user, str(course2.id), notif2) - assert decision == 'send_immediate' + assert decision == 'schedule_buffer' def get_new_post_notification_content_context(**kwargs):