From 4c2a753c151237c62888ed7b158227a613373946 Mon Sep 17 00:00:00 2001 From: Rebecca Graber Date: Wed, 27 Jul 2022 11:18:01 -0400 Subject: [PATCH] feat: Emit COURSE_CATALOG_INFO_CHANGED signal on course publish Implements https://github.com/openedx/edx-platform/issues/30682 Produce signal only once transaction for a course publish is committed, and only for actual courses (not libraries). - Use newer openedx-events version that has a fix for None datetime and that has CourseCatalogData without org, number. - Add edx-event-bus-kafka -- specify recent version that drops confluent-kafka from explicit deps - New functionality behind toggle As per https://github.com/openedx/openedx-events/issues/88 we're going to try explicit dependencies on implementations for now, rather than solve all the problems we'd encounter by using private dependencies. --- .../contentstore/signals/handlers.py | 84 ++++++++++++++++++- .../contentstore/signals/tests/__init__.py | 0 .../signals/tests/test_handlers.py | 83 ++++++++++++++++++ cms/envs/devstack.py | 4 + requirements/edx/base.in | 3 +- requirements/edx/base.txt | 11 ++- requirements/edx/development.txt | 11 ++- requirements/edx/testing.txt | 11 ++- xmodule/modulestore/django.py | 13 +++ 9 files changed, 212 insertions(+), 8 deletions(-) create mode 100644 cms/djangoapps/contentstore/signals/tests/__init__.py create mode 100644 cms/djangoapps/contentstore/signals/tests/test_handlers.py diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index 9945277a1379..62efb6019835 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/cms/djangoapps/contentstore/signals/handlers.py @@ -4,10 +4,17 @@ import logging from datetime import datetime from functools import wraps +from typing import Optional from django.conf import settings from django.core.cache import cache +from django.db import transaction from django.dispatch import receiver +from edx_toggles.toggles import SettingToggle +from edx_event_bus_kafka.publishing.event_producer import send_to_event_bus +from opaque_keys.edx.keys import CourseKey +from openedx_events.content_authoring.data import CourseCatalogData, CourseScheduleData +from openedx_events.content_authoring.signals import COURSE_CATALOG_INFO_CHANGED from pytz import UTC from cms.djangoapps.contentstore.courseware_index import ( @@ -20,8 +27,10 @@ from lms.djangoapps.grades.api import task_compute_all_grades_for_course from openedx.core.djangoapps.content.learning_sequences.api import key_supports_outlines from openedx.core.djangoapps.discussions.tasks import update_discussions_settings_from_course_task +from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.lib.gating import api as gating_api -from xmodule.modulestore.django import SignalHandler, modulestore # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import SignalHandler, modulestore from .signals import GRADING_POLICY_CHANGED log = logging.getLogger(__name__) @@ -43,6 +52,65 @@ def wrapper(*args, **kwargs): return task_decorator +# .. toggle_name: SEND_CATALOG_INFO_SIGNAL +# .. toggle_implementation: SettingToggle +# .. toggle_default: False +# .. toggle_description: When True, sends to catalog-info-changed signal when course_published occurs. +# This is a temporary toggle to allow us to test the event bus integration; it should be removed and +# always-on once the integration is well-tested and the error cases are handled. (This is separate +# from whether the event bus itself is configured; if this toggle is on but the event bus is not +# configured, we should expect a warning at most.) +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2022-08-22 +# .. toggle_target_removal_date: 2022-10-30 +# .. toggle_tickets: https://github.com/openedx/edx-platform/issues/30682 +SEND_CATALOG_INFO_SIGNAL = SettingToggle('SEND_CATALOG_INFO_SIGNAL', default=False, module_name=__name__) + + +def create_catalog_data_for_signal(course_key: CourseKey) -> Optional[CourseCatalogData]: + """ + Creates data for catalog-info-changed signal when course is published. + + Arguments: + course_key: Key of the course to announce catalog info changes for + + Returns: + Data for signal, or None if not appropriate to send on this signal. + """ + # Only operate on real courses, not libraries. + if not course_key.is_course: + return None + + store = modulestore() + with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + course = store.get_course(course_key) + + return CourseCatalogData( + course_key=course_key.for_branch(None), # Shouldn't be necessary, but just in case... + name=course.display_name, + schedule_data=CourseScheduleData( + start=course.start, + pacing='self' if course.self_paced else 'instructor', + end=course.end, + enrollment_start=course.enrollment_start, + enrollment_end=course.enrollment_end, + ), + effort=CourseDetails.fetch_about_attribute(course_key, 'effort'), + hidden=course.catalog_visibility in ['about', 'none'] or course_key.deprecated, + invitation_only=course.invitation_only, + ) + + +def emit_catalog_info_changed_signal(course_key: CourseKey): + """ + Given the key of a recently published course, send course data to catalog-info-changed signal. + """ + if SEND_CATALOG_INFO_SIGNAL.is_enabled(): + catalog_info = create_catalog_data_for_signal(course_key) + if catalog_info is not None: + COURSE_CATALOG_INFO_CHANGED.send_event(catalog_info=catalog_info) + + @receiver(SignalHandler.course_published) def listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument """ @@ -79,6 +147,20 @@ def listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable= update_discussions_settings_from_course_task.delay(course_key_str) + # Send to a signal for catalog info changes as well, but only once we know the transaction is committed. + transaction.on_commit(lambda: emit_catalog_info_changed_signal(course_key)) + + +@receiver(COURSE_CATALOG_INFO_CHANGED) +def listen_for_course_catalog_info_changed(sender, signal, **kwargs): + """ + Publish COURSE_CATALOG_INFO_CHANGED signals onto the event bus. + """ + send_to_event_bus( + signal=COURSE_CATALOG_INFO_CHANGED, topic='course-catalog-info-changed', + event_key_field='catalog_info.course_key', event_data={'catalog_info': kwargs['catalog_info']} + ) + @receiver(SignalHandler.course_deleted) def listen_for_course_delete(sender, course_key, **kwargs): # pylint: disable=unused-argument diff --git a/cms/djangoapps/contentstore/signals/tests/__init__.py b/cms/djangoapps/contentstore/signals/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/signals/tests/test_handlers.py b/cms/djangoapps/contentstore/signals/tests/test_handlers.py new file mode 100644 index 000000000000..943fe4aab2cd --- /dev/null +++ b/cms/djangoapps/contentstore/signals/tests/test_handlers.py @@ -0,0 +1,83 @@ +""" +Tests for signal handlers in the contentstore. +""" + +from datetime import datetime +from unittest.mock import patch + +from django.test.utils import override_settings +from opaque_keys.edx.locator import CourseLocator, LibraryLocator +from openedx_events.content_authoring.data import CourseCatalogData, CourseScheduleData + +import cms.djangoapps.contentstore.signals.handlers as sh +from xmodule.modulestore.django import SignalHandler +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import SampleCourseFactory + + +class TestCatalogInfoSignal(ModuleStoreTestCase): + """ + Test functionality of triggering catalog info signals (and events) from course_published signal. + """ + + def setUp(self): + super().setUp() + self.course = SampleCourseFactory.create( + org='TestU', + number='sig101', + display_name='Signals 101', + run='Summer2022', + ) + self.course_key = self.course.id + + self.expected_data = CourseCatalogData( + course_key=CourseLocator(org='TestU', course='sig101', run='Summer2022', branch=None, version_guid=None), + name='Signals 101', + schedule_data=CourseScheduleData( + start=datetime.fromisoformat('2030-01-01T00:00+00:00'), + pacing='instructor', + end=None, + enrollment_start=None, + enrollment_end=None), + short_description=None, + effort=None, + hidden=False, + invitation_only=False + ) + + @patch( + 'cms.djangoapps.contentstore.signals.handlers.transaction.on_commit', + autospec=True, side_effect=lambda func: func(), # run right away + ) + @patch('cms.djangoapps.contentstore.signals.handlers.emit_catalog_info_changed_signal', autospec=True) + def test_signal_chain(self, mock_emit, _mock_on_commit): + """ + Test that the course_published signal handler invokes the catalog info signal emitter. + + I tested this in a bit of a weird way because I couldn't get the transaction on-commit + to run during the test, so instead I capture it and call the callbacks right away. + """ + with SignalHandler.course_published.for_state(is_enabled=True): + SignalHandler.course_published.send(TestCatalogInfoSignal, course_key=self.course_key) + mock_emit.assert_called_once_with(self.course_key) + + @override_settings(SEND_CATALOG_INFO_SIGNAL=True) + @patch('cms.djangoapps.contentstore.signals.handlers.COURSE_CATALOG_INFO_CHANGED', autospec=True) + def test_emit_regular_course(self, mock_signal): + """On a normal course publish, send an event.""" + sh.emit_catalog_info_changed_signal(self.course_key) + mock_signal.send_event.assert_called_once_with(catalog_info=self.expected_data) + + @override_settings(SEND_CATALOG_INFO_SIGNAL=True) + @patch('cms.djangoapps.contentstore.signals.handlers.COURSE_CATALOG_INFO_CHANGED', autospec=True) + def test_ignore_library(self, mock_signal): + """When course key is actually a library, don't send.""" + sh.emit_catalog_info_changed_signal(LibraryLocator(org='SomeOrg', library='stuff')) + mock_signal.send_event.assert_not_called() + + @override_settings(SEND_CATALOG_INFO_SIGNAL=False) + @patch('cms.djangoapps.contentstore.signals.handlers.COURSE_CATALOG_INFO_CHANGED', autospec=True) + def test_disabled(self, mock_signal): + """When toggle is disabled, don't send.""" + sh.emit_catalog_info_changed_signal(self.course_key) + mock_signal.send_event.assert_not_called() diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 799b7b7e8c76..fddaca966198 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -295,6 +295,10 @@ def should_show_debug_toolbar(request): # lint-amnesty, pylint: disable=missing CREDENTIALS_INTERNAL_SERVICE_URL = 'http://localhost:18150' CREDENTIALS_PUBLIC_SERVICE_URL = 'http://localhost:18150' +#################### Event bus backend ######################## +EVENT_BUS_KAFKA_SCHEMA_REGISTRY_URL = 'http://edx.devstack.schema-registry:8081' +EVENT_BUS_KAFKA_BOOTSTRAP_SERVERS = 'edx.devstack.kafka:29092' + ################# New settings must go ABOVE this line ################# ######################################################################## # See if the developer has any local overrides. diff --git a/requirements/edx/base.in b/requirements/edx/base.in index d1a7db1fd481..ab62ee6bf4a4 100644 --- a/requirements/edx/base.in +++ b/requirements/edx/base.in @@ -84,6 +84,7 @@ edx-django-sites-extensions edx-django-utils>=5.0.0 # Utilities for cache, monitoring, and plugins edx-drf-extensions edx-enterprise +edx-event-bus-kafka>=0.4.1 # Kafka implementation of event bus edx-milestones edx-name-affirmation edx-opaque-keys @@ -123,7 +124,7 @@ nltk # Natural language processing; used by the c nodeenv # Utility for managing Node.js environments; we use this for deployments and testing oauthlib # OAuth specification support for authenticating via LTI or other Open edX services openedx-calc # Library supporting mathematical calculations for Open edX -openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) +openedx-events>=0.12.0 # Open edX Events from Hooks Extension Framework (OEP-50) openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) optimizely-sdk # Optimizely full stack SDK for Python ora2>=4.4.0 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 930e2e9420c7..18162598b718 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -223,6 +223,7 @@ django==3.2.15 # edx-django-utils # edx-drf-extensions # edx-enterprise + # edx-event-bus-kafka # edx-i18n-tools # edx-milestones # edx-name-affirmation @@ -459,6 +460,7 @@ edx-django-utils==5.0.0 # django-config-models # edx-drf-extensions # edx-enterprise + # edx-event-bus-kafka # edx-name-affirmation # edx-rest-api-client # edx-toggles @@ -485,6 +487,8 @@ edx-enterprise==3.56.5 # -c requirements/edx/../constraints.txt # -r requirements/edx/base.in # learner-pathway-progress +edx-event-bus-kafka==0.4.1 + # via -r requirements/edx/base.in edx-i18n-tools==0.9.1 # via ora2 edx-milestones==0.4.0 @@ -539,6 +543,7 @@ edx-toggles==5.0.0 # via # -r requirements/edx/base.in # edx-completion + # edx-event-bus-kafka # edx-name-affirmation # edxval # learner-pathway-progress @@ -754,8 +759,10 @@ oauthlib==3.0.1 # social-auth-core openedx-calc==3.0.1 # via -r requirements/edx/base.in -openedx-events==0.11.1 - # via -r requirements/edx/base.in +openedx-events==0.12.0 + # via + # -r requirements/edx/base.in + # edx-event-bus-kafka openedx-filters==0.8.0 # via # -r requirements/edx/base.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 3cfaf26af0ab..66e449b889e1 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -312,6 +312,7 @@ django==3.2.15 # edx-django-utils # edx-drf-extensions # edx-enterprise + # edx-event-bus-kafka # edx-i18n-tools # edx-milestones # edx-name-affirmation @@ -573,6 +574,7 @@ edx-django-utils==5.0.0 # django-config-models # edx-drf-extensions # edx-enterprise + # edx-event-bus-kafka # edx-name-affirmation # edx-rest-api-client # edx-toggles @@ -599,6 +601,8 @@ edx-enterprise==3.56.5 # -c requirements/edx/../constraints.txt # -r requirements/edx/testing.txt # learner-pathway-progress +edx-event-bus-kafka==0.4.1 + # via -r requirements/edx/testing.txt edx-i18n-tools==0.9.1 # via # -r requirements/edx/testing.txt @@ -662,6 +666,7 @@ edx-toggles==5.0.0 # via # -r requirements/edx/testing.txt # edx-completion + # edx-event-bus-kafka # edx-name-affirmation # edxval # learner-pathway-progress @@ -982,8 +987,10 @@ oauthlib==3.0.1 # social-auth-core openedx-calc==3.0.1 # via -r requirements/edx/testing.txt -openedx-events==0.11.1 - # via -r requirements/edx/testing.txt +openedx-events==0.12.0 + # via + # -r requirements/edx/testing.txt + # edx-event-bus-kafka openedx-filters==0.8.0 # via # -r requirements/edx/testing.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 52e9543a434b..39980ee7f383 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -296,6 +296,7 @@ distlib==0.3.5 # edx-django-utils # edx-drf-extensions # edx-enterprise + # edx-event-bus-kafka # edx-i18n-tools # edx-milestones # edx-name-affirmation @@ -553,6 +554,7 @@ edx-django-utils==5.0.0 # django-config-models # edx-drf-extensions # edx-enterprise + # edx-event-bus-kafka # edx-name-affirmation # edx-rest-api-client # edx-toggles @@ -579,6 +581,8 @@ edx-enterprise==3.56.5 # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # learner-pathway-progress +edx-event-bus-kafka==0.4.1 + # via -r requirements/edx/base.txt edx-i18n-tools==0.9.1 # via # -r requirements/edx/base.txt @@ -641,6 +645,7 @@ edx-toggles==5.0.0 # via # -r requirements/edx/base.txt # edx-completion + # edx-event-bus-kafka # edx-name-affirmation # edxval # learner-pathway-progress @@ -933,8 +938,10 @@ oauthlib==3.0.1 # social-auth-core openedx-calc==3.0.1 # via -r requirements/edx/base.txt -openedx-events==0.11.1 - # via -r requirements/edx/base.txt +openedx-events==0.12.0 + # via + # -r requirements/edx/base.txt + # edx-event-bus-kafka openedx-filters==0.8.0 # via # -r requirements/edx/base.txt diff --git a/xmodule/modulestore/django.py b/xmodule/modulestore/django.py index 0be451cc419f..c7e9fc51cd63 100644 --- a/xmodule/modulestore/django.py +++ b/xmodule/modulestore/django.py @@ -4,6 +4,7 @@ Passes settings.MODULESTORE as kwargs to MongoModuleStore """ +from contextlib import contextmanager from importlib import import_module import gettext import logging @@ -87,6 +88,18 @@ def enable(self): """ self._allow_signals = True + @contextmanager + def for_state(self, *, is_enabled: bool): + """ + Set signal handling to be on or off for the duration of the context. + """ + try: + old_state = self._allow_signals + self._allow_signals = is_enabled + yield + finally: + self._allow_signals = old_state + def send(self, *args, **kwargs): """ See `django.dispatch.Signal.send()`