From ee7735711072659b67b0f9f88c4ca2aa69a591e4 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Mon, 19 Feb 2024 11:14:11 -0500 Subject: [PATCH 1/4] chore: remove unnecessary user profile task --- event_sink_clickhouse/signals.py | 10 ++++++++-- event_sink_clickhouse/tasks.py | 18 ------------------ tests/test_signals.py | 11 ----------- tests/test_tasks.py | 24 +----------------------- 4 files changed, 9 insertions(+), 54 deletions(-) diff --git a/event_sink_clickhouse/signals.py b/event_sink_clickhouse/signals.py index b595e7e..eca4e37 100644 --- a/event_sink_clickhouse/signals.py +++ b/event_sink_clickhouse/signals.py @@ -6,6 +6,7 @@ from event_sink_clickhouse.sinks.external_id_sink import ExternalIdSink from event_sink_clickhouse.sinks.user_retire import UserRetirementSink +from event_sink_clickhouse.sinks.user_profile_sink import UserProfileSink from event_sink_clickhouse.utils import get_model try: @@ -35,9 +36,14 @@ def on_user_profile_updated( # pylint: disable=unused-argument # pragma: no co Receives post save signal and queues the dump job. """ # import here, because signal is registered at startup, but items in tasks are not yet able to be loaded - from event_sink_clickhouse.tasks import dump_user_profile_to_clickhouse # pylint: disable=import-outside-toplevel + from event_sink_clickhouse.tasks import dump_data_to_clickhouse # pylint: disable=import-outside-toplevel - dump_user_profile_to_clickhouse.delay(instance.id) + sink = UserProfileSink(None, None) + dump_data_to_clickhouse.delay( + sink_module=sink.__module__, + sink_name=sink.__class__.__name__, + object_id=str(instance.id), + ) @receiver(post_save, sender=get_model("external_id")) diff --git a/event_sink_clickhouse/tasks.py b/event_sink_clickhouse/tasks.py index 3f58500..efcbb52 100644 --- a/event_sink_clickhouse/tasks.py +++ b/event_sink_clickhouse/tasks.py @@ -33,24 +33,6 @@ def dump_course_to_clickhouse(course_key_string, connection_overrides=None): sink.dump(course_key) -@shared_task -@set_code_owner_attribute -def dump_user_profile_to_clickhouse(user_profile_id, connection_overrides=None): - """ - Serialize a user profile and writes it to ClickHouse. - - Arguments: - user_profile_id: user profile id for the user profile to be exported - connection_overrides (dict): overrides to ClickHouse connection - parameters specified in `settings.EVENT_SINK_CLICKHOUSE_BACKEND_CONFIG`. - """ - if UserProfileSink.is_enabled(): # pragma: no cover - sink = UserProfileSink( - connection_overrides=connection_overrides, log=celery_log - ) - sink.dump(user_profile_id) - - @shared_task @set_code_owner_attribute def dump_data_to_clickhouse( diff --git a/tests/test_signals.py b/tests/test_signals.py index b62e990..11e2d92 100644 --- a/tests/test_signals.py +++ b/tests/test_signals.py @@ -31,17 +31,6 @@ def test_receive_course_publish(self, mock_dump_task): mock_dump_task.delay.assert_called_once_with(course_key) - @patch("event_sink_clickhouse.tasks.dump_user_profile_to_clickhouse") - def test_on_user_profile_updated(self, mock_dump_task): - """ - Test that on_user_profile_updated calls dump_user_profile_to_clickhouse. - """ - instance = Mock() - sender = Mock() - on_user_profile_updated(sender, instance) - - mock_dump_task.delay.assert_called_once_with(instance.id) - @patch("event_sink_clickhouse.tasks.dump_data_to_clickhouse") def test_on_externalid_saved(self, mock_dump_task): """ diff --git a/tests/test_tasks.py b/tests/test_tasks.py index 9b31546..d3f9695 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -4,7 +4,7 @@ import unittest from unittest.mock import MagicMock, patch -from event_sink_clickhouse.tasks import dump_data_to_clickhouse, dump_user_profile_to_clickhouse +from event_sink_clickhouse.tasks import dump_data_to_clickhouse class TestTasks(unittest.TestCase): @@ -12,28 +12,6 @@ class TestTasks(unittest.TestCase): Test cases for tasks. """ - @patch("event_sink_clickhouse.tasks.UserProfileSink.is_enabled", return_value=True) - @patch("event_sink_clickhouse.tasks.UserProfileSink") - @patch("event_sink_clickhouse.tasks.celery_log") - def test_dump_user_profile_to_clickhouse( - self, mock_celery_log, mock_UserProfileSink, mock_is_enabled - ): - # Mock the required objects and methods - mock_sink_instance = mock_UserProfileSink.return_value - mock_sink_instance.dump.return_value = None - - # Call the function - dump_user_profile_to_clickhouse( - "user_profile_id", connection_overrides={"param": "value"} - ) - - # Assertions - mock_is_enabled.assert_called_once() - mock_UserProfileSink.assert_called_once_with( - connection_overrides={"param": "value"}, log=mock_celery_log - ) - mock_sink_instance.dump.assert_called_once_with("user_profile_id") - @patch("event_sink_clickhouse.tasks.import_module") @patch("event_sink_clickhouse.tasks.celery_log") def test_dump_data_to_clickhouse(self, mock_celery_log, mock_import_module): From b4d7b0f0bde593027368dd9e9847ebb3cf2a8df4 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Mon, 19 Feb 2024 13:31:05 -0500 Subject: [PATCH 2/4] fix: sink ccx courses on original course published --- event_sink_clickhouse/settings/common.py | 4 ++++ event_sink_clickhouse/tasks.py | 7 ++++++- event_sink_clickhouse/utils.py | 9 +++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/event_sink_clickhouse/settings/common.py b/event_sink_clickhouse/settings/common.py index 2ff6818..597912f 100644 --- a/event_sink_clickhouse/settings/common.py +++ b/event_sink_clickhouse/settings/common.py @@ -40,4 +40,8 @@ def plugin_settings(settings): "module": "openedx.core.djangoapps.external_user_ids.models", "model": "ExternalId", }, + "custom_course_edx": { + "module": "lms.djangoapps.ccx.models", + "model": "CustomCourseForEdX", + } } diff --git a/event_sink_clickhouse/tasks.py b/event_sink_clickhouse/tasks.py index efcbb52..9b90eb5 100644 --- a/event_sink_clickhouse/tasks.py +++ b/event_sink_clickhouse/tasks.py @@ -10,7 +10,7 @@ from opaque_keys.edx.keys import CourseKey from event_sink_clickhouse.sinks.course_published import CourseOverviewSink -from event_sink_clickhouse.sinks.user_profile_sink import UserProfileSink +from event_sink_clickhouse.utils import get_ccx_courses log = logging.getLogger(__name__) celery_log = logging.getLogger("edx.celery.task") @@ -32,6 +32,11 @@ def dump_course_to_clickhouse(course_key_string, connection_overrides=None): sink = CourseOverviewSink(connection_overrides=connection_overrides, log=celery_log) sink.dump(course_key) + ccx_courses = get_ccx_courses(course_key) + for ccx_course in ccx_courses: + ccx_course_key = str(ccx_course.locator) + sink.dump(ccx_course_key) + @shared_task @set_code_owner_attribute diff --git a/event_sink_clickhouse/utils.py b/event_sink_clickhouse/utils.py index 28745c3..fe06e49 100644 --- a/event_sink_clickhouse/utils.py +++ b/event_sink_clickhouse/utils.py @@ -57,3 +57,12 @@ def get_detached_xblock_types(): # pragma: no cover from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES return DETACHED_XBLOCK_TYPES + + +def get_ccx_courses(course_id): + """ + Get the CCX courses for a given course. + """ + if settings.FEATURES.get("CUSTOM_COURSES_EDX"): + return get_model("custom_course_edx").objects.filter(course_id=course_id) + return [] From 506974fa2db039fd9bda1b3c3537530d604ccf6f Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Mon, 19 Feb 2024 13:58:31 -0500 Subject: [PATCH 3/4] test: update tests for ccx changes --- .coveragerc | 1 - docs/conf.py | 2 +- event_sink_clickhouse/signals.py | 2 +- test_settings.py | 61 -------------------------------- test_utils/test_settings.py | 4 +++ test_utils/test_utils.py | 16 ++++++++- tests/test_course_published.py | 11 +++++- tests/test_signals.py | 7 +--- tox.ini | 4 +-- 9 files changed, 34 insertions(+), 74 deletions(-) delete mode 100644 test_settings.py diff --git a/.coveragerc b/.coveragerc index 5dbcfc0..58fea0e 100644 --- a/.coveragerc +++ b/.coveragerc @@ -3,7 +3,6 @@ branch = True data_file = .coverage source=event_sink_clickhouse omit = - test_settings *migrations* *admin.py *static* diff --git a/docs/conf.py b/docs/conf.py index 26762e1..81768ef 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -41,7 +41,7 @@ def get_version(*file_paths): VERSION = get_version('../event_sink_clickhouse', '__init__.py') # Configure Django for autodoc usage -os.environ['DJANGO_SETTINGS_MODULE'] = 'test_settings' +os.environ['DJANGO_SETTINGS_MODULE'] = 'test_utils.test_settings' django_setup() # If extensions (or modules to document with autodoc) are in another directory, diff --git a/event_sink_clickhouse/signals.py b/event_sink_clickhouse/signals.py index eca4e37..3303057 100644 --- a/event_sink_clickhouse/signals.py +++ b/event_sink_clickhouse/signals.py @@ -5,8 +5,8 @@ from django.dispatch import Signal, receiver from event_sink_clickhouse.sinks.external_id_sink import ExternalIdSink -from event_sink_clickhouse.sinks.user_retire import UserRetirementSink from event_sink_clickhouse.sinks.user_profile_sink import UserProfileSink +from event_sink_clickhouse.sinks.user_retire import UserRetirementSink from event_sink_clickhouse.utils import get_model try: diff --git a/test_settings.py b/test_settings.py deleted file mode 100644 index 58f2d14..0000000 --- a/test_settings.py +++ /dev/null @@ -1,61 +0,0 @@ -""" -These settings are here to use during tests, because django requires them. - -In a real-world use case, apps in this project are installed into other -Django applications, so these settings will not be used. -""" - -from os.path import abspath, dirname, join - - -def root(*args): - """ - Get the absolute path of the given path relative to the project root. - """ - return join(abspath(dirname(__file__)), *args) - - -DATABASES = { - 'default': { - 'ENGINE': 'django.db.backends.sqlite3', - 'NAME': 'default.db', - 'USER': '', - 'PASSWORD': '', - 'HOST': '', - 'PORT': '', - } -} - -INSTALLED_APPS = ( - 'django.contrib.admin', - 'django.contrib.auth', - 'django.contrib.contenttypes', - 'django.contrib.messages', - 'django.contrib.sessions', - 'event_sink_clickhouse', -) - -LOCALE_PATHS = [ - root('event_sink_clickhouse', 'conf', 'locale'), -] - -ROOT_URLCONF = 'event_sink_clickhouse.urls' - -SECRET_KEY = 'insecure-secret-key' - -MIDDLEWARE = ( - 'django.contrib.auth.middleware.AuthenticationMiddleware', - 'django.contrib.messages.middleware.MessageMiddleware', - 'django.contrib.sessions.middleware.SessionMiddleware', -) - -TEMPLATES = [{ - 'BACKEND': 'django.template.backends.django.DjangoTemplates', - 'APP_DIRS': False, - 'OPTIONS': { - 'context_processors': [ - 'django.contrib.auth.context_processors.auth', # this is required for admin - 'django.contrib.messages.context_processors.messages', # this is required for admin - ], - }, -}] diff --git a/test_utils/test_settings.py b/test_utils/test_settings.py index 16e3cdb..4a9fe3d 100644 --- a/test_utils/test_settings.py +++ b/test_utils/test_settings.py @@ -42,3 +42,7 @@ } EVENT_SINK_CLICKHOUSE_COURSE_OVERVIEWS_ENABLED = True + +FEATURES = { + 'CUSTOM_COURSES_EDX': True, +} diff --git a/test_utils/test_utils.py b/test_utils/test_utils.py index 8dd2f4d..6783e9d 100644 --- a/test_utils/test_utils.py +++ b/test_utils/test_utils.py @@ -6,7 +6,7 @@ from django.conf import settings -from event_sink_clickhouse.utils import get_model +from event_sink_clickhouse.utils import get_ccx_courses, get_model class TestUtils(unittest.TestCase): @@ -71,3 +71,17 @@ def test_get_model_missing_module_and_model_2(self): def test_get_model_missing_model_config(self): model = get_model("my_model") self.assertIsNone(model) + + @patch("event_sink_clickhouse.utils.get_model") + def test_get_ccx_courses(self, mock_get_model): + mock_get_model.return_value = mock_model = Mock() + + get_ccx_courses('dummy_key') + + mock_model.objects.filter.assert_called_once_with(course_id='dummy_key') + + @patch.object(settings, "FEATURES", {"CUSTOM_COURSES_EDX": False}) + def test_get_ccx_courses_feature_disabled(self): + courses = get_ccx_courses('dummy_key') + + self.assertEqual(list(courses), []) diff --git a/tests/test_course_published.py b/tests/test_course_published.py index 53567b6..99a71dd 100644 --- a/tests/test_course_published.py +++ b/tests/test_course_published.py @@ -33,7 +33,14 @@ @patch("event_sink_clickhouse.sinks.course_published.CourseOverviewSink.get_model") @patch("event_sink_clickhouse.sinks.course_published.get_detached_xblock_types") @patch("event_sink_clickhouse.sinks.course_published.get_modulestore") -def test_course_publish_success(mock_modulestore, mock_detached, mock_overview, mock_serialize_item): +@patch("event_sink_clickhouse.tasks.get_ccx_courses") +def test_course_publish_success( + mock_get_ccx_courses, + mock_modulestore, + mock_detached, + mock_overview, + mock_serialize_item +): """ Test of a successful end-to-end run. """ @@ -48,6 +55,7 @@ def test_course_publish_success(mock_modulestore, mock_detached, mock_overview, mock_detached.return_value = mock_detached_xblock_types() mock_overview.return_value.get_from_id.return_value = course_overview + mock_get_ccx_courses.return_value = [] # Use the responses library to catch the POSTs to ClickHouse # and match them against the expected values, including CSV @@ -75,6 +83,7 @@ def test_course_publish_success(mock_modulestore, mock_detached, mock_overview, # Just to make sure we're not calling things more than we need to assert mock_modulestore.call_count == 1 assert mock_detached.call_count == 1 + mock_get_ccx_courses.assert_called_once_with(course_overview.id) @responses.activate(registry=OrderedRegistry) # pylint: disable=unexpected-keyword-arg,no-value-for-parameter diff --git a/tests/test_signals.py b/tests/test_signals.py index 11e2d92..cdcec14 100644 --- a/tests/test_signals.py +++ b/tests/test_signals.py @@ -5,12 +5,7 @@ from django.test import TestCase -from event_sink_clickhouse.signals import ( - on_externalid_saved, - on_user_profile_updated, - on_user_retirement, - receive_course_publish, -) +from event_sink_clickhouse.signals import on_externalid_saved, on_user_retirement, receive_course_publish from event_sink_clickhouse.sinks.external_id_sink import ExternalIdSink from event_sink_clickhouse.sinks.user_retire import UserRetirementSink diff --git a/tox.ini b/tox.ini index 709e372..254bd8a 100644 --- a/tox.ini +++ b/tox.ini @@ -78,12 +78,12 @@ commands = rm tests/__init__.py pycodestyle event_sink_clickhouse tests manage.py setup.py pydocstyle event_sink_clickhouse tests manage.py setup.py - isort --check-only --diff tests test_utils event_sink_clickhouse manage.py setup.py test_settings.py + isort --check-only --diff tests test_utils event_sink_clickhouse manage.py setup.py make selfcheck [testenv:pii_check] setenv = - DJANGO_SETTINGS_MODULE = test_settings + DJANGO_SETTINGS_MODULE = test_utils.test_settings deps = -r{toxinidir}/requirements/test.txt commands = From e57592e546d5951cb7a1e0bb5656a9740f19b36f Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 20 Feb 2024 10:26:52 -0500 Subject: [PATCH 4/4] chore: bump version to 1.1.1 --- event_sink_clickhouse/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/event_sink_clickhouse/__init__.py b/event_sink_clickhouse/__init__.py index c713846..85e80d1 100644 --- a/event_sink_clickhouse/__init__.py +++ b/event_sink_clickhouse/__init__.py @@ -2,4 +2,4 @@ A sink for Open edX events to send them to ClickHouse. """ -__version__ = "1.1.0" +__version__ = "1.1.1"