diff --git a/lms/djangoapps/discussion/rest_api/tasks.py b/lms/djangoapps/discussion/rest_api/tasks.py index 628d93d15b..5f0bf8ed40 100644 --- a/lms/djangoapps/discussion/rest_api/tasks.py +++ b/lms/djangoapps/discussion/rest_api/tasks.py @@ -5,12 +5,12 @@ from celery import shared_task from django.contrib.auth import get_user_model from edx_django_utils.monitoring import set_code_owner_attribute from opaque_keys.edx.locator import CourseKey + from lms.djangoapps.courseware.courses import get_course_with_access +from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender from openedx.core.djangoapps.django_comment_common.comment_client import Comment from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread -from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS, ENABLE_COURSEWIDE_NOTIFICATIONS -from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender - +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS User = get_user_model() @@ -22,7 +22,7 @@ def send_thread_created_notification(thread_id, course_key_str, user_id): Send notification when a new thread is created """ course_key = CourseKey.from_string(course_key_str) - if not (ENABLE_NOTIFICATIONS.is_enabled(course_key) and ENABLE_COURSEWIDE_NOTIFICATIONS.is_enabled(course_key)): + if not ENABLE_NOTIFICATIONS.is_enabled(course_key): return thread = Thread(id=thread_id).retrieve() user = User.objects.get(id=user_id) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py index adc8235d43..eeed292037 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py @@ -6,10 +6,8 @@ import unittest from unittest.mock import MagicMock, patch import pytest -from edx_toggles.toggles.testutils import override_waffle_flag from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender -from lms.djangoapps.discussion.toggles import ENABLE_REPORTED_CONTENT_NOTIFICATIONS @patch('lms.djangoapps.discussion.rest_api.discussions_notifications.DiscussionNotificationSender' @@ -22,7 +20,6 @@ class TestDiscussionNotificationSender(unittest.TestCase): Tests for the DiscussionNotificationSender class """ - @override_waffle_flag(ENABLE_REPORTED_CONTENT_NOTIFICATIONS, True) def setUp(self): self.thread = MagicMock() self.course = MagicMock() diff --git a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py index 29e63524b2..2614685736 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py @@ -8,15 +8,16 @@ import ddt import httpretty from django.conf import settings from edx_toggles.toggles.testutils import override_waffle_flag -from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED, COURSE_NOTIFICATION_REQUESTED +from openedx_events.learning.signals import COURSE_NOTIFICATION_REQUESTED, USER_NOTIFICATION_REQUESTED from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.tests.factories import StaffFactory, UserFactory from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory from lms.djangoapps.discussion.rest_api.tasks import ( + send_response_endorsed_notifications, send_response_notifications, - send_thread_created_notification, - send_response_endorsed_notifications) + send_thread_created_notification +) from lms.djangoapps.discussion.rest_api.tests.utils import ThreadMock, make_minimal_cs_thread from openedx.core.djangoapps.course_groups.models import CohortMembership, CourseCohortsSettings from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory @@ -28,7 +29,7 @@ from openedx.core.djangoapps.django_comment_common.models import ( FORUM_ROLE_STUDENT, CourseDiscussionSettings ) -from openedx.core.djangoapps.notifications.config.waffle import ENABLE_COURSEWIDE_NOTIFICATIONS, ENABLE_NOTIFICATIONS +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -47,7 +48,6 @@ def _get_mfe_url(course_id, post_id): @httpretty.activate @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) -@override_waffle_flag(ENABLE_COURSEWIDE_NOTIFICATIONS, active=True) class TestNewThreadCreatedNotification(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """ Test cases related to new_discussion_post and new_question_post notification types diff --git a/lms/djangoapps/discussion/signals/handlers.py b/lms/djangoapps/discussion/signals/handlers.py index 8c4991cd02..35288cdbd9 100644 --- a/lms/djangoapps/discussion/signals/handlers.py +++ b/lms/djangoapps/discussion/signals/handlers.py @@ -10,19 +10,17 @@ from django.utils.html import strip_tags from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocator -from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender -from lms.djangoapps.discussion.toggles import ENABLE_REPORTED_CONTENT_NOTIFICATIONS -from xmodule.modulestore.django import SignalHandler, modulestore - from lms.djangoapps.discussion import tasks +from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender from lms.djangoapps.discussion.rest_api.tasks import ( + send_response_endorsed_notifications, send_response_notifications, - send_thread_created_notification, - send_response_endorsed_notifications + send_thread_created_notification ) from openedx.core.djangoapps.django_comment_common import signals from openedx.core.djangoapps.site_configuration.models import SiteConfiguration from openedx.core.djangoapps.theming.helpers import get_current_site +from xmodule.modulestore.django import SignalHandler, modulestore log = logging.getLogger(__name__) @@ -101,8 +99,6 @@ def send_reported_content_notification(sender, user, post, **kwargs): Sends notification for reported content. """ course_key = CourseKey.from_string(post.course_id) - if not ENABLE_REPORTED_CONTENT_NOTIFICATIONS.is_enabled(course_key): - return course = modulestore().get_course(course_key) DiscussionNotificationSender(post, course, user).send_reported_content_notification() diff --git a/lms/djangoapps/discussion/toggles.py b/lms/djangoapps/discussion/toggles.py index 29918c45a9..a1c292a473 100644 --- a/lms/djangoapps/discussion/toggles.py +++ b/lms/djangoapps/discussion/toggles.py @@ -12,15 +12,3 @@ from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag # .. toggle_creation_date: 2021-11-05 # .. toggle_target_removal_date: 2022-12-05 ENABLE_DISCUSSIONS_MFE = CourseWaffleFlag(f'{WAFFLE_FLAG_NAMESPACE}.enable_discussions_mfe', __name__) - -# .. toggle_name: discussions.enable_reported_content_notifications -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Waffle flag to enable reported content notifications. -# .. toggle_use_cases: temporary, open_edx -# .. toggle_creation_date: 18-Jan-2024 -# .. toggle_target_removal_date: 18-Feb-2024 -ENABLE_REPORTED_CONTENT_NOTIFICATIONS = CourseWaffleFlag( - f'{WAFFLE_FLAG_NAMESPACE}.enable_reported_content_notifications', - __name__ -) diff --git a/openedx/core/djangoapps/notifications/config/waffle.py b/openedx/core/djangoapps/notifications/config/waffle.py index fa1d02adf1..af89bb6857 100644 --- a/openedx/core/djangoapps/notifications/config/waffle.py +++ b/openedx/core/djangoapps/notifications/config/waffle.py @@ -18,46 +18,6 @@ WAFFLE_NAMESPACE = 'notifications' # .. toggle_tickets: INF-866 ENABLE_NOTIFICATIONS = CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.enable_notifications', __name__) -# .. toggle_name: notifications.show_notifications_tray -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Waffle flag to show notifications tray -# .. toggle_use_cases: temporary, open_edx -# .. toggle_creation_date: 2023-06-07 -# .. toggle_target_removal_date: 2023-12-07 -# .. toggle_tickets: INF-902 -SHOW_NOTIFICATIONS_TRAY = CourseWaffleFlag(f"{WAFFLE_NAMESPACE}.show_notifications_tray", __name__) - -# .. toggle_name: notifications.enable_notifications_filters -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Waffle flag to enable filters in notifications task -# .. toggle_use_cases: temporary, open_edx -# .. toggle_creation_date: 2023-06-07 -# .. toggle_target_removal_date: 2024-06-01 -# .. toggle_tickets: INF-902 -ENABLE_NOTIFICATIONS_FILTERS = CourseWaffleFlag(f"{WAFFLE_NAMESPACE}.enable_notifications_filters", __name__) - -# .. toggle_name: notifications.enable_coursewide_notifications -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Waffle flag to enable coursewide notifications -# .. toggle_use_cases: temporary, open_edx -# .. toggle_creation_date: 2023-10-25 -# .. toggle_target_removal_date: 2024-06-01 -# .. toggle_tickets: INF-1145 -ENABLE_COURSEWIDE_NOTIFICATIONS = CourseWaffleFlag(f"{WAFFLE_NAMESPACE}.enable_coursewide_notifications", __name__) - -# .. toggle_name: notifications.enable_ora_staff_notifications -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Waffle flag to enable ORA staff notifications -# .. toggle_use_cases: temporary, open_edx -# .. toggle_creation_date: 2024-04-04 -# .. toggle_target_removal_date: 2024-06-04 -# .. toggle_tickets: INF-1304 -ENABLE_ORA_STAFF_NOTIFICATION = CourseWaffleFlag(f"{WAFFLE_NAMESPACE}.enable_ora_staff_notifications", __name__) - # .. toggle_name: notifications.enable_email_notifications # .. toggle_implementation: WaffleFlag # .. toggle_default: False diff --git a/openedx/core/djangoapps/notifications/handlers.py b/openedx/core/djangoapps/notifications/handlers.py index 6d7e7a7714..505f4b5e70 100644 --- a/openedx/core/djangoapps/notifications/handlers.py +++ b/openedx/core/djangoapps/notifications/handlers.py @@ -8,20 +8,20 @@ from django.db import IntegrityError, transaction from django.dispatch import receiver from openedx_events.learning.signals import ( COURSE_ENROLLMENT_CREATED, - COURSE_UNENROLLMENT_COMPLETED, - USER_NOTIFICATION_REQUESTED, COURSE_NOTIFICATION_REQUESTED, + COURSE_UNENROLLMENT_COMPLETED, + USER_NOTIFICATION_REQUESTED ) from common.djangoapps.student.models import CourseEnrollment from openedx.core.djangoapps.notifications.audience_filters import ( - ForumRoleAudienceFilter, - EnrollmentAudienceFilter, - TeamAudienceFilter, CohortAudienceFilter, CourseRoleAudienceFilter, + EnrollmentAudienceFilter, + ForumRoleAudienceFilter, + TeamAudienceFilter ) -from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS, ENABLE_ORA_STAFF_NOTIFICATION +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS from openedx.core.djangoapps.notifications.models import CourseNotificationPreference log = logging.getLogger(__name__) @@ -108,11 +108,6 @@ def generate_course_notifications(signal, sender, course_notification_data, meta """ Watches for COURSE_NOTIFICATION_REQUESTED signal and calls send_notifications task """ - if ( - course_notification_data.notification_type == 'ora_staff_notification' - and not ENABLE_ORA_STAFF_NOTIFICATION.is_enabled(course_notification_data.course_key) - ): - return from openedx.core.djangoapps.notifications.tasks import send_notifications course_notification_data = course_notification_data.__dict__ diff --git a/openedx/core/djangoapps/notifications/serializers.py b/openedx/core/djangoapps/notifications/serializers.py index 3b6877cc29..d56fb820f6 100644 --- a/openedx/core/djangoapps/notifications/serializers.py +++ b/openedx/core/djangoapps/notifications/serializers.py @@ -12,7 +12,7 @@ from openedx.core.djangoapps.notifications.models import ( get_notification_channels, get_additional_notification_channel_settings ) from .base_notification import COURSE_NOTIFICATION_APPS, COURSE_NOTIFICATION_TYPES, EmailCadence -from .utils import filter_course_wide_preferences, remove_preferences_with_no_access +from .utils import remove_preferences_with_no_access def add_info_to_notification_config(config_obj): @@ -73,7 +73,6 @@ class UserCourseNotificationPreferenceSerializer(serializers.ModelSerializer): course_id = self.context['course_id'] user = self.context['user'] preferences = add_info_to_notification_config(preferences) - preferences = filter_course_wide_preferences(course_id, preferences) preferences = remove_preferences_with_no_access(preferences, user) return preferences diff --git a/openedx/core/djangoapps/notifications/tasks.py b/openedx/core/djangoapps/notifications/tasks.py index a876b513cb..4fcec36214 100644 --- a/openedx/core/djangoapps/notifications/tasks.py +++ b/openedx/core/djangoapps/notifications/tasks.py @@ -18,13 +18,13 @@ from openedx.core.djangoapps.notifications.base_notification import ( get_default_values_of_preference, get_notification_content ) -from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS, ENABLE_NOTIFICATIONS_FILTERS +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.filters import NotificationFilter from openedx.core.djangoapps.notifications.models import ( CourseNotificationPreference, Notification, - get_course_notification_preference_config_version, + get_course_notification_preference_config_version ) from openedx.core.djangoapps.notifications.utils import clean_arguments, get_list_in_batches @@ -137,10 +137,9 @@ def send_notifications(user_ids, course_key: str, app_name, notification_type, c generated_notification_audience = [] for batch_user_ids in get_list_in_batches(user_ids, batch_size): - if ENABLE_NOTIFICATIONS_FILTERS.is_enabled(course_key): - logger.info(f'Sending notifications to {len(batch_user_ids)} users in {course_key}') - batch_user_ids = NotificationFilter().apply_filters(batch_user_ids, course_key, notification_type) - logger.info(f'After applying filters, sending notifications to {len(batch_user_ids)} users in {course_key}') + logger.info(f'Sending notifications to {len(batch_user_ids)} users in {course_key}') + batch_user_ids = NotificationFilter().apply_filters(batch_user_ids, course_key, notification_type) + logger.info(f'After applying filters, sending notifications to {len(batch_user_ids)} users in {course_key}') # check if what is preferences of user and make decision to send notification or not preferences = CourseNotificationPreference.objects.filter( diff --git a/openedx/core/djangoapps/notifications/tests/test_tasks.py b/openedx/core/djangoapps/notifications/tests/test_tasks.py index 036d4d3261..5058c0f492 100644 --- a/openedx/core/djangoapps/notifications/tests/test_tasks.py +++ b/openedx/core/djangoapps/notifications/tests/test_tasks.py @@ -2,12 +2,12 @@ Tests for notifications tasks. """ +import datetime from unittest.mock import patch -import datetime import ddt -from django.core.exceptions import ValidationError from django.conf import settings +from django.core.exceptions import ValidationError from edx_toggles.toggles.testutils import override_waffle_flag from common.djangoapps.student.models import CourseEnrollment @@ -15,7 +15,6 @@ from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from .utils import create_notification from ..config.waffle import ENABLE_NOTIFICATIONS from ..models import CourseNotificationPreference, Notification from ..tasks import ( @@ -24,6 +23,7 @@ from ..tasks import ( send_notifications, update_user_preference ) +from .utils import create_notification @patch('openedx.core.djangoapps.notifications.models.COURSE_NOTIFICATION_CONFIG_VERSION', 1) @@ -225,7 +225,6 @@ class SendNotificationsTest(ModuleStoreTestCase): @ddt.ddt -@patch('openedx.core.djangoapps.notifications.tasks.ENABLE_NOTIFICATIONS_FILTERS.is_enabled', lambda x: False) class SendBatchNotificationsTest(ModuleStoreTestCase): """ Test that notification and notification preferences are created in batches @@ -255,9 +254,9 @@ class SendBatchNotificationsTest(ModuleStoreTestCase): @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), + (settings.NOTIFICATION_CREATION_BATCH_SIZE, 7, 3), + (settings.NOTIFICATION_CREATION_BATCH_SIZE + 10, 9, 6), + (settings.NOTIFICATION_CREATION_BATCH_SIZE - 10, 7, 3), ) @ddt.unpack def test_notification_is_send_in_batch(self, creation_size, prefs_query_count, notifications_query_count): @@ -307,7 +306,7 @@ class SendBatchNotificationsTest(ModuleStoreTestCase): "username": "Test Author" } with override_waffle_flag(ENABLE_NOTIFICATIONS, active=True): - with self.assertNumQueries(1): + with self.assertNumQueries(7): send_notifications(user_ids, str(self.course.id), notification_app, notification_type, context, "http://test.url") @@ -326,7 +325,7 @@ class SendBatchNotificationsTest(ModuleStoreTestCase): "replier_name": "Replier Name" } with override_waffle_flag(ENABLE_NOTIFICATIONS, active=True): - with self.assertNumQueries(3): + with self.assertNumQueries(9): send_notifications(user_ids, str(self.course.id), notification_app, notification_type, context, "http://test.url") diff --git a/openedx/core/djangoapps/notifications/tests/test_views.py b/openedx/core/djangoapps/notifications/tests/test_views.py index ca5572ed9f..b90d220638 100644 --- a/openedx/core/djangoapps/notifications/tests/test_views.py +++ b/openedx/core/djangoapps/notifications/tests/test_views.py @@ -19,18 +19,13 @@ from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import CourseStaffRole from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory -from lms.djangoapps.discussion.toggles import ENABLE_REPORTED_CONTENT_NOTIFICATIONS from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.django_comment_common.models import ( FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_MODERATOR ) -from openedx.core.djangoapps.notifications.config.waffle import ( - ENABLE_COURSEWIDE_NOTIFICATIONS, - ENABLE_NOTIFICATIONS, - SHOW_NOTIFICATIONS_TRAY -) +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS from openedx.core.djangoapps.notifications.models import CourseNotificationPreference, Notification from openedx.core.djangoapps.notifications.serializers import NotificationCourseEnrollmentSerializer from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -80,26 +75,23 @@ class CourseEnrollmentListViewTest(ModuleStoreTestCase): ) @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) - @ddt.data((False,), (True,)) @ddt.unpack - def test_course_enrollment_list_view(self, show_notifications_tray): + def test_course_enrollment_list_view(self): """ Test the CourseEnrollmentListView. """ self.client.login(username=self.user.username, password=self.TEST_PASSWORD) - # Enable or disable the waffle flag based on the test case data - with override_waffle_flag(SHOW_NOTIFICATIONS_TRAY, active=show_notifications_tray): - url = reverse('enrollment-list') - response = self.client.get(url) + url = reverse('enrollment-list') + response = self.client.get(url) - self.assertEqual(response.status_code, status.HTTP_200_OK) - data = response.data['results'] - enrollments = CourseEnrollment.objects.filter(user=self.user, is_active=True) - expected_data = NotificationCourseEnrollmentSerializer(enrollments, many=True).data + self.assertEqual(response.status_code, status.HTTP_200_OK) + data = response.data['results'] + enrollments = CourseEnrollment.objects.filter(user=self.user, is_active=True) + expected_data = NotificationCourseEnrollmentSerializer(enrollments, many=True).data - self.assertEqual(len(data), 1) - self.assertEqual(data, expected_data) - self.assertEqual(response.data['show_preferences'], show_notifications_tray) + self.assertEqual(len(data), 1) + self.assertEqual(data, expected_data) + self.assertEqual(response.data['show_preferences'], True) def test_course_enrollment_api_permission(self): """ @@ -172,7 +164,6 @@ class CourseEnrollmentPostSaveTest(ModuleStoreTestCase): @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) -@override_waffle_flag(ENABLE_REPORTED_CONTENT_NOTIFICATIONS, active=True) @ddt.ddt class UserNotificationPreferenceAPITest(ModuleStoreTestCase): """ @@ -321,11 +312,6 @@ class UserNotificationPreferenceAPITest(ModuleStoreTestCase): } } } - if not ENABLE_COURSEWIDE_NOTIFICATIONS.is_enabled(course.id): - app_prefs = response['notification_preference_config']['discussion'] - notification_types = app_prefs['notification_types'] - for notification_type in ['new_discussion_post', 'new_question_post']: - notification_types.pop(notification_type) return response def test_get_user_notification_preference_without_login(self): @@ -336,7 +322,6 @@ class UserNotificationPreferenceAPITest(ModuleStoreTestCase): self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) @mock.patch("eventtracking.tracker.emit") - @override_waffle_flag(ENABLE_COURSEWIDE_NOTIFICATIONS, active=True) def test_get_user_notification_preference(self, mock_emit): """ Test get user notification preference. @@ -351,7 +336,6 @@ class UserNotificationPreferenceAPITest(ModuleStoreTestCase): self.assertEqual(event_name, 'edx.notifications.preferences.viewed') @mock.patch("eventtracking.tracker.emit") - @override_waffle_flag(ENABLE_COURSEWIDE_NOTIFICATIONS, active=True) @mock.patch.dict(COURSE_NOTIFICATION_TYPES, { **COURSE_NOTIFICATION_TYPES, **{ @@ -473,7 +457,6 @@ class UserNotificationPreferenceAPITest(ModuleStoreTestCase): @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) -@override_waffle_flag(ENABLE_REPORTED_CONTENT_NOTIFICATIONS, active=True) @ddt.ddt class UserNotificationChannelPreferenceAPITest(ModuleStoreTestCase): """ @@ -624,11 +607,6 @@ class UserNotificationChannelPreferenceAPITest(ModuleStoreTestCase): } } } - if not ENABLE_COURSEWIDE_NOTIFICATIONS.is_enabled(course.id): - app_prefs = response['notification_preference_config']['discussion'] - notification_types = app_prefs['notification_types'] - for notification_type in ['new_discussion_post', 'new_question_post']: - notification_types.pop(notification_type) return response @ddt.data( @@ -910,24 +888,21 @@ class NotificationCountViewSetTestCase(ModuleStoreTestCase): Notification.objects.create(user=self.user, app_name='App Name 2', notification_type='Type A') Notification.objects.create(user=self.user, app_name='App Name 3', notification_type='Type C') - @ddt.data((False,), (True,)) + @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) @ddt.unpack - def test_get_unseen_notifications_count_with_show_notifications_tray(self, show_notifications_tray_enabled): + def test_get_unseen_notifications_count_with_show_notifications_tray(self): """ Test that the endpoint returns the correct count of unseen notifications and show_notifications_tray value. """ self.client.login(username=self.user.username, password=self.TEST_PASSWORD) + # Make a request to the view + response = self.client.get(self.url) - # Enable or disable the waffle flag based on the test case data - with override_waffle_flag(SHOW_NOTIFICATIONS_TRAY, active=show_notifications_tray_enabled): - # Make a request to the view - response = self.client.get(self.url) - - self.assertEqual(response.status_code, 200) - self.assertEqual(response.data['count'], 4) - self.assertEqual(response.data['count_by_app_name'], { - 'App Name 1': 2, 'App Name 2': 1, 'App Name 3': 1, 'discussion': 0, 'updates': 0, 'grading': 0}) - self.assertEqual(response.data['show_notifications_tray'], show_notifications_tray_enabled) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['count'], 4) + self.assertEqual(response.data['count_by_app_name'], { + 'App Name 1': 2, 'App Name 2': 1, 'App Name 3': 1, 'discussion': 0, 'updates': 0, 'grading': 0}) + self.assertEqual(response.data['show_notifications_tray'], True) def test_get_unseen_notifications_count_for_unauthenticated_user(self): """ diff --git a/openedx/core/djangoapps/notifications/utils.py b/openedx/core/djangoapps/notifications/utils.py index 5ce592bb41..249eaf8749 100644 --- a/openedx/core/djangoapps/notifications/utils.py +++ b/openedx/core/djangoapps/notifications/utils.py @@ -3,13 +3,11 @@ Utils function for notifications app """ from typing import Dict, List -from common.djangoapps.student.models import CourseEnrollment, CourseAccessRole -from lms.djangoapps.discussion.toggles import ENABLE_REPORTED_CONTENT_NOTIFICATIONS +from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment from openedx.core.djangoapps.django_comment_common.models import Role +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS from openedx.core.lib.cache_utils import request_cached -from .config.waffle import ENABLE_COURSEWIDE_NOTIFICATIONS, SHOW_NOTIFICATIONS_TRAY - def find_app_in_normalized_apps(app_name, apps_list): """ @@ -42,7 +40,7 @@ def get_show_notifications_tray(user): ).values_list('course_id', flat=True) for course_id in learner_enrollments_course_ids: - if SHOW_NOTIFICATIONS_TRAY.is_enabled(course_id): + if ENABLE_NOTIFICATIONS.is_enabled(course_id): show_notifications_tray = True break @@ -58,27 +56,6 @@ def get_list_in_batches(input_list, batch_size): yield input_list[index: index + batch_size] -def filter_course_wide_preferences(course_key, preferences): - """ - If course wide notifications is disabled for course, it filters course_wide - preferences from response - """ - if ENABLE_COURSEWIDE_NOTIFICATIONS.is_enabled(course_key): - return preferences - course_wide_notification_types = ['new_discussion_post', 'new_question_post'] - - if not ENABLE_REPORTED_CONTENT_NOTIFICATIONS.is_enabled(course_key): - course_wide_notification_types.append('content_reported') - - config = preferences['notification_preference_config'] - for app_prefs in config.values(): - notification_types = app_prefs['notification_types'] - for course_wide_type in course_wide_notification_types: - if course_wide_type in notification_types.keys(): - notification_types.pop(course_wide_type) - return preferences - - def get_user_forum_roles(user_id: int, course_id: str) -> List[str]: """ Get forum roles for the given user in the specified course. diff --git a/openedx/core/djangoapps/notifications/views.py b/openedx/core/djangoapps/notifications/views.py index cc830dbb8d..547847f55e 100644 --- a/openedx/core/djangoapps/notifications/views.py +++ b/openedx/core/djangoapps/notifications/views.py @@ -375,7 +375,7 @@ class NotificationCountView(APIView): .annotate(count=Count('*')) ) count_total = 0 - show_notifications_tray = get_show_notifications_tray(request.user) + show_notifications_tray = get_show_notifications_tray(self.request.user) count_by_app_name_dict = { app_name: 0 for app_name in COURSE_NOTIFICATION_APPS diff --git a/openedx/core/djangoapps/programs/tasks.py b/openedx/core/djangoapps/programs/tasks.py index 72ccc79bdd..3ea5638578 100644 --- a/openedx/core/djangoapps/programs/tasks.py +++ b/openedx/core/djangoapps/programs/tasks.py @@ -2,6 +2,7 @@ This file contains celery tasks and utility functions responsible for syncing course and program certificate metadata between the monolith and the Credentials IDA. """ + from typing import Dict, List from urllib.parse import urljoin @@ -13,6 +14,7 @@ from django.contrib.auth import get_user_model from django.contrib.sites.models import Site from django.core.exceptions import ObjectDoesNotExist from edx_django_utils.monitoring import set_code_owner_attribute +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from requests.exceptions import HTTPError @@ -250,7 +252,16 @@ def post_course_certificate_configuration(client, cert_config, certificate_avail response.raise_for_status() -@shared_task(bind=True, ignore_result=True) +# pylint: disable=unused-argument +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def award_program_certificates(self, username): # lint-amnesty, pylint: disable=too-many-statements """ @@ -272,23 +283,14 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable Returns: None """ - def _retry_with_custom_exception(username, reason, countdown): - exception = MaxRetriesExceededError( - f"Failed to award a program certificate to user {username}. Reason: {reason}" - ) - return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - - countdown = 2**self.request.retries - # If the credentials config model is disabled for this feature, it may indicate a condition where processing of such - # tasks has been temporarily disabled. Since this is a recoverable situation, mark this task for retry instead of - # failing it altogether. + # tasks has been temporarily disabled. This is a recoverable situation, so let celery retry. if not is_credentials_enabled(): error_msg = ( "Task award_program_certificates cannot be executed, use of the Credentials service is disabled by config" ) LOGGER.warning(error_msg) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) + raise MaxRetriesExceededError(f"Failed to award a program certificate. Reason: {error_msg}") try: student = User.objects.get(username=username) @@ -323,7 +325,9 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable except Exception as exc: error_msg = f"Failed to determine program certificates to be awarded for user {student}: {exc}" LOGGER.exception(error_msg) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) from exc + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" + ) from exc # For each completed program for which the student doesn't already have a certificate, award one now. # @@ -339,8 +343,10 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable except Exception as exc: error_msg = "Failed to create a credentials API client to award program certificates" LOGGER.exception(error_msg) - # Retry because a misconfiguration could be fixed - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) from exc + # A misconfiguration could be fixed; let celery retry. + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" + ) from exc failed_program_certificate_award_attempts = [] for program_uuid in new_program_uuids: @@ -360,17 +366,13 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable "not be configured correctly in Credentials" ) elif exc.response.status_code == 429: - rate_limit_countdown = 60 + # Let celery handle retry attempts and backoff error_msg = ( - f"Rate limited. Retrying task to award certificate to user {student} in program " - f"{program_uuid} in {rate_limit_countdown} seconds" + f"Rate limited. Attempting to award certificate to user {student} in program {program_uuid}." ) LOGGER.warning(error_msg) - # Retry after 60 seconds, when we should be in a new throttling window - raise _retry_with_custom_exception( - username=username, - reason=error_msg, - countdown=rate_limit_countdown, + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" ) from exc else: LOGGER.warning( @@ -378,7 +380,7 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable "might not be configured correctly in Credentials" ) except Exception as exc: # pylint: disable=broad-except - # keep trying to award other certs, but retry the whole task to fix any missing entries + # keep trying to award other certs, but let celery retry the whole task to fix any missing entries LOGGER.exception( f"Failed to award program certificate to user {student} in program {program_uuid}: {exc}" ) @@ -394,7 +396,9 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable f"Failed to award program certificate(s) for user {student} in programs " f"{failed_program_certificate_award_attempts}" ) - raise _retry_with_custom_exception(username=username, reason=error_msg, countdown=countdown) + raise MaxRetriesExceededError( + f"Failed to award a program certificate to user {student.id}. Reason: {error_msg}" + ) else: LOGGER.warning(f"User {student} is not eligible for any new program certificates") @@ -402,7 +406,15 @@ def award_program_certificates(self, username): # lint-amnesty, pylint: disable # pylint: disable=W0613 -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def update_credentials_course_certificate_configuration_available_date( self, course_key, certificate_available_date=None @@ -427,7 +439,8 @@ def update_credentials_course_certificate_configuration_available_date( course_modes = CourseMode.objects.filter(course_id=course_key) # There should only ever be one certificate relevant mode per course run modes = [ - mode.slug for mode in course_modes + mode.slug + for mode in course_modes if mode.slug in CourseMode.CERTIFICATE_RELEVANT_MODES or CourseMode.is_eligible_for_certificate(mode.slug) ] if len(modes) != 1: @@ -448,7 +461,15 @@ def update_credentials_course_certificate_configuration_available_date( ) -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def award_course_certificate(self, username, course_run_key): """ @@ -457,35 +478,21 @@ def award_course_certificate(self, username, course_run_key): It can be called independently for a username and a course_run, but is invoked on each GeneratedCertificate.save. - If this function is moved, make sure to update it's entry in EXPLICIT_QUEUES in the settings files so it runs in the + If this function is moved, make sure to update its entry in EXPLICIT_QUEUES in the settings files so it runs in the correct queue. Arguments: username (str): The user to award the Credentials course cert to course_run_key (str): The course run key to award the certificate for """ - def _retry_with_custom_exception(username, course_run_key, reason, countdown): - exception = MaxRetriesExceededError( - f"Failed to award course certificate for user {username} for course {course_run_key}. Reason: {reason}" - ) - return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - - countdown = 2**self.request.retries - # If the credentials config model is disabled for this feature, it may indicate a condition where processing of such - # tasks has been temporarily disabled. Since this is a recoverable situation, - # mark this task for retry instead of failing it altogether. + # tasks has been temporarily disabled. This is a recoverable situation, let celery retry. if not is_credentials_enabled(): error_msg = ( "Task award_course_certificate cannot be executed when credentials issuance is disabled in API config" ) LOGGER.warning(error_msg) - raise _retry_with_custom_exception( - username=username, - course_run_key=course_run_key, - reason=error_msg, - countdown=countdown, - ) + raise MaxRetriesExceededError(f"Failed to award course certificate. Reason: {error_msg}") try: user = User.objects.get(username=username) @@ -499,71 +506,98 @@ def award_course_certificate(self, username, course_run_key): LOGGER.info(f"Running task award_course_certificate for user {user}") try: course_key = CourseKey.from_string(course_run_key) - # Get the cert for the course key and username if it's both passing and available in professional/verified - try: - certificate = GeneratedCertificate.eligible_certificates.get( - user=user.id, - course_id=course_key, - ) - except GeneratedCertificate.DoesNotExist: + except InvalidKeyError as exc: + error_msg = "Failed to determine course key" + LOGGER.warning( + f"Failed to award course certificate for user {user.id} for course {course_run_key}. Reason: {error_msg}" + ) + return + + # Get the cert for the course key and username if it's both passing and available in professional/verified + try: + certificate = GeneratedCertificate.eligible_certificates.get( + user=user.id, + course_id=course_key, + ) + except GeneratedCertificate.DoesNotExist: + LOGGER.warning( + f"Task award_course_certificate was called for user {user.id} in course run {course_key} but this learner " + "has not earned a course certificate in this course run" + ) + return + + try: + if ( + certificate.mode not in CourseMode.CERTIFICATE_RELEVANT_MODES + and not CourseMode.is_eligible_for_certificate(certificate.mode) + ): LOGGER.warning( - f"Task award_course_certificate was called for user {user} in course run {course_key} but this learner " - "has not earned a course certificate in this course run" + f"Task award_course_certificate was called for user {user.id} in course run {course_key} but " + f"this course has an ineligible mode of {certificate.mode} for a certificate on this instance." ) return - - if ( - certificate.mode in CourseMode.CERTIFICATE_RELEVANT_MODES - or CourseMode.is_eligible_for_certificate(certificate.mode) - ): - course_overview = get_course_overview_or_none(course_key) - if not course_overview: - LOGGER.warning( - f"Task award_course_certificate was called for user {user} in course {course_key} but no course " - "overview could be retrieved for the course run" - ) - return - - visible_date = available_date_for_certificate(course_overview, certificate) - LOGGER.info( - f"Task award_course_certificate will award a course certificate to user {user} in course run " - f"{course_key} with a visible date of {visible_date}" - ) - - # If the certificate has an associated CertificateDateOverride, send it along - try: - date_override = certificate.date_override.date - LOGGER.info( - f"Task award_course_certificate will award a course certificate to user {user} in course run " - f"{course_key} with an override date of {date_override}" - ) - except ObjectDoesNotExist: - date_override = None - - credentials_client = get_credentials_api_client( - User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), - ) - post_course_certificate( - credentials_client, - username, - certificate, - visible_date, - date_override, - org=course_key.org, - ) - LOGGER.info(f"Awarded a course certificate to user {user} in course run {course_key}") except Exception as exc: - error_msg = f"Failed to determine course certificates to be awarded for user {user}." - LOGGER.exception(error_msg) - raise _retry_with_custom_exception( - username=username, - course_run_key=course_run_key, - reason=error_msg, - countdown=countdown, + error_msg = f"Failed to determine course mode certificate eligibility for {certificate}." + LOGGER.error(error_msg) + raise MaxRetriesExceededError( + f"Failed to award course certificate for user {user.id} for course {course_run_key}. Reason: {error_msg}" ) from exc + course_overview = get_course_overview_or_none(course_key) + if not course_overview: + LOGGER.warning( + f"Task award_course_certificate was called for user {user.id} in course {course_key} but no course " + "overview could be retrieved for the course run" + ) + return -@shared_task(bind=True, ignore_result=True) + visible_date = available_date_for_certificate(course_overview, certificate) + LOGGER.info( + f"Task award_course_certificate will award a course certificate to user {user.id} in course run " + f"{course_key} with a visible date of {visible_date}" + ) + + # If the certificate has an associated CertificateDateOverride, send it along + try: + date_override = certificate.date_override.date + LOGGER.info( + f"Task award_course_certificate will award a course certificate to user {user.id} in course run " + f"{course_key} with an override date of {date_override}" + ) + except ObjectDoesNotExist: + date_override = None + + try: + credentials_client = get_credentials_api_client( + User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME), + ) + post_course_certificate( + credentials_client, + username, + certificate, + visible_date, + date_override, + org=course_key.org, + ) + except Exception as exc: + error_msg = f"Failed to post course certificate to be awarded for user {user}." + raise MaxRetriesExceededError( + f"Failed to award course certificate for user {user.id} for course {course_run_key}. Reason: {error_msg}" + ) from exc + + # Successfully posted the cert to credentials + LOGGER.info(f"Awarded a course certificate to user {user.id} in course run {course_key}") + + +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def revoke_program_certificates(self, username, course_key): # lint-amnesty, pylint: disable=too-many-statements """ @@ -572,7 +606,7 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py It will consult with a variety of APIs to determine whether or not the specified user's certificate should be revoked in one or more programs, and use the credentials service to revoke the said certificates if so. - If this function is moved, make sure to update it's entry in EXPLICIT_QUEUES in the settings files so it runs in the + If this function is moved, make sure to update its entry in EXPLICIT_QUEUES in the settings files so it runs in the correct queue. Args: @@ -582,28 +616,14 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py Returns: None """ - def _retry_with_custom_exception(username, course_key, reason, countdown): - exception = MaxRetriesExceededError( - f"Failed to revoke program certificate for user {username} for course {course_key}. Reason: {reason}" - ) - return self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) - - countdown = 2**self.request.retries - # If the credentials config model is disabled for this feature, it may indicate a condition where processing of such - # tasks has been temporarily disabled. Since this is a recoverable situation, mark this task for retry instead of - # failing it altogether. + # tasks has been temporarily disabled. Since this is a recoverable situation, let celery retry. if not is_credentials_enabled(): error_msg = ( "Task revoke_program_certificates cannot be executed, use of the Credentials service is disabled by config" ) LOGGER.warning(error_msg) - raise _retry_with_custom_exception( - username=username, - course_key=course_key, - reason=error_msg, - countdown=countdown, - ) + raise MaxRetriesExceededError(f"Failed to revoke program certificate. Reason: {error_msg}") try: student = User.objects.get(username=username) @@ -633,11 +653,8 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py f"revoked from user {student}" ) LOGGER.exception(error_msg) - raise _retry_with_custom_exception( - username=username, - course_key=course_key, - reason=error_msg, - countdown=countdown, + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} for course {course_key}. Reason: {error_msg}" ) from exc if program_uuids_to_revoke: @@ -648,8 +665,10 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py except Exception as exc: error_msg = "Failed to create a credentials API client to revoke program certificates" LOGGER.exception(error_msg) - # Retry because a misconfiguration could be fixed - raise _retry_with_custom_exception(username, course_key, reason=exc, countdown=countdown) from exc + # Stil retryable because a misconfiguration could be fixed + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} for course {course_key}. Reason: {exc}" + ) from exc failed_program_certificate_revoke_attempts = [] for program_uuid in program_uuids_to_revoke: @@ -663,25 +682,21 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py "program certificate could not be found" ) elif exc.response.status_code == 429: - rate_limit_countdown = 60 + # Let celery handle retry attempts and backoff error_msg = ( - f"Rate limited. Retrying task to revoke a program certificate from user {student} in program " - f"{program_uuid} in {rate_limit_countdown} seconds" + f"Rate limited. Attempting to revoke a program certificate from user {student} in program " + f"{program_uuid}." ) LOGGER.warning(error_msg) - # Retry after 60 seconds, when we should be in a new throttling window - raise _retry_with_custom_exception( - username, - course_key, - reason=error_msg, - countdown=rate_limit_countdown, + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} Reason: {error_msg}" ) from exc else: LOGGER.warning( f"Unable to revoke program certificate from user {student} in program {program_uuid}" ) except Exception as exc: # pylint: disable=broad-except - # keep trying to revoke other certs, but retry the whole task to fix any missing entries + # keep trying to revoke other certs, but let celery retry the whole task to fix any missing entries LOGGER.exception( f"Failed to revoke program certificate from user {student} in program {program_uuid}: {exc}" ) @@ -689,7 +704,7 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py if failed_program_certificate_revoke_attempts: # N.B. This logic assumes that this task is idempotent - LOGGER.info(f"Retrying failed task to revoke program certificate(s) from user {student}") + LOGGER.info(f"Failed task to revoke program certificate(s) from user {student}") # The error message may change on each reattempt but will never be raised until the max number of retries # have been exceeded. It is unlikely that this list will change by the time it reaches its maximimum number # of attempts. @@ -697,14 +712,25 @@ def revoke_program_certificates(self, username, course_key): # lint-amnesty, py f"Failed to revoke program certificate(s) from user {student} for programs " f"{failed_program_certificate_revoke_attempts}" ) - raise _retry_with_custom_exception(username, course_key, reason=error_msg, countdown=countdown) + raise MaxRetriesExceededError( + f"Failed to revoke program certificate for user {student.id} for course {course_key}. " + f"Reason: {error_msg}" + ) else: LOGGER.info(f"No program certificates to revoke from user {student}") LOGGER.info(f"Successfully completed the task revoke_program_certificates for user {student}") -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def update_certificate_visible_date_on_course_update(self, course_key): """ @@ -714,28 +740,24 @@ def update_certificate_visible_date_on_course_update(self, course_key): Next, we will enqueue an additional `award_course_certificate` task for each learner in this list. These subtasks will be responsible for updating the `visible_date` attribute on each certificate the Credentials IDA knows about. - If this function is moved, make sure to update it's entry in EXPLICIT_QUEUES in the settings files so it runs in the + If this function is moved, make sure to update its entry in EXPLICIT_QUEUES in the settings files so it runs in the correct queue. Arguments: course_key(str): The course identifier """ - countdown = 2**self.request.retries - # If the CredentialsApiConfig configuration model is disabled for this feature, it may indicate a condition where - # processing of such tasks has been temporarily disabled. Since this is a recoverable situation, mark this task for - # retry instead of failing it. + # processing of such tasks has been temporarily disabled. This is a recoverable situation, so let celery retry. if not is_credentials_enabled(): error_msg = ( "Cannot execute the `update_certificate_visible_date_on_course_update` task. Issuing user credentials " "through the Credentials IDA is disabled." ) LOGGER.warning(error_msg) - exception = MaxRetriesExceededError( + raise MaxRetriesExceededError( f"Failed to update the `visible_date` attribute for certificates in course {course_key}. Reason: " f"{error_msg}" ) - raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) # Retrieve a list of all usernames of learners who have a certificate record in this course-run. The # Credentials IDA REST API still requires a username as the main identifier for the learner. @@ -751,7 +773,15 @@ def update_certificate_visible_date_on_course_update(self, course_key): award_course_certificate.delay(user, str(course_key)) -@shared_task(bind=True, ignore_result=True) +@shared_task( + bind=True, + ignore_result=True, + autoretry_for=(Exception,), + max_retries=10, + retry_backoff=30, + retry_backoff_max=600, + retry_jitter=True, +) @set_code_owner_attribute def update_certificate_available_date_on_course_update(self, course_key): """ @@ -764,8 +794,6 @@ def update_certificate_available_date_on_course_update(self, course_key): Args: course_key(str): The course run's identifier """ - countdown = 2**self.request.retries - # If the CredentialsApiConfig configuration model is disabled for this feature, it may indicate a condition where # processing of such tasks has been temporarily disabled. Since this is a recoverable situation, mark this task for # retry instead of failing it. @@ -775,11 +803,10 @@ def update_certificate_available_date_on_course_update(self, course_key): "through the Credentials IDA is disabled." ) LOGGER.warning(error_msg) - exception = MaxRetriesExceededError( + raise MaxRetriesExceededError( "Failed to update the `certificate_available_date` in the Credentials service for course-run " f"{course_key}. Reason: {error_msg}" ) - raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES) course_overview = get_course_overview_or_none(course_key) if not course_overview: @@ -818,6 +845,5 @@ def update_certificate_available_date_on_course_update(self, course_key): new_certificate_available_date = None update_credentials_course_certificate_configuration_available_date.delay( - str(course_key), - new_certificate_available_date + str(course_key), new_certificate_available_date ) diff --git a/openedx/core/djangoapps/programs/tests/test_tasks.py b/openedx/core/djangoapps/programs/tests/test_tasks.py index 4b3a143fc9..0c37323499 100644 --- a/openedx/core/djangoapps/programs/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tests/test_tasks.py @@ -2,7 +2,6 @@ Tests for programs celery tasks. """ - import json import logging from datetime import datetime, timedelta @@ -22,21 +21,13 @@ from testfixtures import LogCapture from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.certificates.tests.factories import ( - CertificateDateOverrideFactory, - GeneratedCertificateFactory, -) +from lms.djangoapps.certificates.tests.factories import CertificateDateOverrideFactory, GeneratedCertificateFactory from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin -from openedx.core.djangoapps.content.course_overviews.tests.factories import ( - CourseOverviewFactory, -) +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfigMixin from openedx.core.djangoapps.oauth_dispatch.tests.factories import ApplicationFactory from openedx.core.djangoapps.programs import tasks -from openedx.core.djangoapps.site_configuration.tests.factories import ( - SiteConfigurationFactory, - SiteFactory, -) +from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory from openedx.core.djangolib.testing.utils import skip_unless_lms from xmodule.data import CertificatesDisplayBehaviors @@ -311,7 +302,7 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo tasks.award_program_certificates.delay(self.student.username).get() assert mock_exception.called - assert mock_get_api_client.call_count == (tasks.MAX_RETRIES + 1) + assert mock_get_api_client.call_count == (tasks.MAX_RETRIES) assert not mock_award_program_certificate.called def _make_side_effect(self, side_effects): @@ -611,16 +602,6 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): assert mock_exception.called assert not mock_post_course_certificate.called - def test_award_course_cert_not_called_if_certificate_not_found(self, mock_post_course_certificate): - """ - Test that the post method is never called if the certificate doesn't exist for the user and course - """ - self.certificate.delete() - with mock.patch(TASKS_MODULE + ".LOGGER.warning") as mock_exception: - tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() - assert mock_exception.called - assert not mock_post_course_certificate.called - def test_award_course_cert_not_called_if_course_overview_not_found(self, mock_post_course_certificate): """ Test that the post method is never called if the CourseOverview isn't found @@ -632,6 +613,32 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): assert mock_exception.called assert not mock_post_course_certificate.called + def test_award_course_cert_not_called_if_certificate_not_found(self, mock_post_course_certificate): + """ + Test that the post method is never called if the certificate doesn't exist for the user and course + """ + self.certificate.delete() + with mock.patch(TASKS_MODULE + ".LOGGER.warning") as mock_exception: + tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() + assert mock_exception.called + assert not mock_post_course_certificate.called + + def test_award_course_cert_not_called_if_course_run_key_is_bad(self, mock_post_course_certificate): + """ + Test that the post method is never called if the course run key is invalid + """ + bad_course_run_key = "I/Am/The/Keymaster" + expected_message = ( + f"Failed to award course certificate for user {self.student.id} for course " + f"{bad_course_run_key}. Reason: Failed to determine course key" + ) + with LogCapture(level=logging.WARNING) as log_capture: + tasks.award_course_certificate.delay(self.student.username, bad_course_run_key).get() + assert not mock_post_course_certificate.called + log_capture.check_present( + ("openedx.core.djangoapps.programs.tasks", "WARNING", expected_message), + ) + def test_award_course_cert_not_called_if_certificated_not_verified_mode(self, mock_post_course_certificate): """ Test that the post method is never called if the GeneratedCertificate is an 'audit' cert @@ -922,7 +929,7 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC with pytest.raises(MaxRetriesExceededError): tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get() assert mock_exception.called - assert mock_get_api_client.call_count == (tasks.MAX_RETRIES + 1) + assert mock_get_api_client.call_count == (tasks.MAX_RETRIES) assert not mock_revoke_program_certificate.called @@ -1050,7 +1057,8 @@ class UpdateCertificateVisibleDatesOnCourseUpdateTestCase(CredentialsApiConfigMi exception when the max number of retries has reached. """ with pytest.raises(MaxRetriesExceededError): - tasks.update_certificate_visible_date_on_course_update(self.course.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_visible_date_on_course_update(self.course.id) def test_update_visible_dates(self): """ @@ -1065,7 +1073,8 @@ class UpdateCertificateVisibleDatesOnCourseUpdateTestCase(CredentialsApiConfigMi self.credentials_api_config.enable_learner_issuance = True with mock.patch(f"{TASKS_MODULE}.award_course_certificate.delay") as award_course_cert: - tasks.update_certificate_visible_date_on_course_update(self.course.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_visible_date_on_course_update(self.course.id) assert award_course_cert.call_count == 3 @@ -1117,7 +1126,8 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM ) with pytest.raises(MaxRetriesExceededError): - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") def test_update_certificate_available_date_instructor_paced_cdb_early_no_info(self, mock_update): @@ -1143,7 +1153,8 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) mock_update.assert_called_once_with(str(course_overview.id), None) @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") @@ -1168,7 +1179,8 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) mock_update.assert_called_once_with(str(course_overview.id), str(self.end_date)) @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") @@ -1196,7 +1208,8 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) mock_update.assert_called_once_with(str(course_overview.id), str(certificate_available_date)) @mock.patch(f"{TASKS_MODULE}.update_credentials_course_certificate_configuration_available_date.delay") @@ -1228,7 +1241,8 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM self.end_date, ) - tasks.update_certificate_available_date_on_course_update(course_overview.id) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(course_overview.id) mock_update.assert_called_once_with(str(course_overview.id), None) def test_update_certificate_available_date_no_course_overview(self): @@ -1246,8 +1260,9 @@ class UpdateCertificateAvailableDateOnCourseUpdateTestCase(CredentialsApiConfigM self._update_credentials_api_config(True) with LogCapture(level=logging.WARNING) as log_capture: - tasks.update_certificate_available_date_on_course_update(bad_course_run_key) # pylint: disable=no-value-for-parameter + # pylint: disable=no-value-for-parameter + tasks.update_certificate_available_date_on_course_update(bad_course_run_key) log_capture.check_present( - ('openedx.core.djangoapps.programs.tasks', 'WARNING', expected_message), + ("openedx.core.djangoapps.programs.tasks", "WARNING", expected_message), )