From f20636194d7363829f4f4d92cfde704bb562879c Mon Sep 17 00:00:00 2001 From: Brian Mesick Date: Fri, 4 Oct 2024 15:45:34 -0400 Subject: [PATCH 1/3] fix: Event sink errors when enabled models are missing When event sinks are enabled in the config or waffle flag, but the model itself doesn't exist we were throwing errors that caused downstream errors. We now warn on this condition and return the model as disabled instead. --- platform_plugin_aspects/sinks/base_sink.py | 13 +++++++++++-- .../sinks/tests/test_base_sink.py | 6 ++++-- .../sinks/tests/test_course_overview_sink.py | 5 ++++- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/platform_plugin_aspects/sinks/base_sink.py b/platform_plugin_aspects/sinks/base_sink.py index 7e80ea2..155498e 100644 --- a/platform_plugin_aspects/sinks/base_sink.py +++ b/platform_plugin_aspects/sinks/base_sink.py @@ -5,6 +5,7 @@ import csv import datetime import io +import logging from collections import namedtuple import requests @@ -342,7 +343,7 @@ def is_enabled(cls): """ Return True if the sink is enabled, False otherwise """ - enabled = getattr( + enabled_settings = getattr( settings, f"{WAFFLE_FLAG_NAMESPACE.upper()}_{cls.model.upper()}_ENABLED", False, @@ -358,7 +359,15 @@ def is_enabled(cls): __name__, ) - return enabled or waffle_flag.is_enabled() + enabled = enabled_settings or waffle_flag.is_enabled() + + if enabled and not get_model(cls.model): + logging.warning( + f"Event Sink Model {cls.model} is not installed, but is enabled in settings or waffle flag." + ) + enabled = False + + return enabled @classmethod def get_sink_by_model_name(cls, model): diff --git a/platform_plugin_aspects/sinks/tests/test_base_sink.py b/platform_plugin_aspects/sinks/tests/test_base_sink.py index 054b1b8..9819e35 100644 --- a/platform_plugin_aspects/sinks/tests/test_base_sink.py +++ b/platform_plugin_aspects/sinks/tests/test_base_sink.py @@ -270,8 +270,9 @@ def test_is_not_enabled_waffle(self, mock_waffle_flag_is_enabled): mock_waffle_flag_is_enabled.return_value = False self.assertEqual(self.child_sink.__class__.is_enabled(), False) + @patch("platform_plugin_aspects.sinks.base_sink.get_model") @patch("platform_plugin_aspects.sinks.base_sink.WaffleFlag.is_enabled") - def test_is_enabled_waffle(self, mock_waffle_flag_is_enabled): + def test_is_enabled_waffle(self, mock_waffle_flag_is_enabled, mock_get_model): """ Test that is_enable() returns the correct data. """ @@ -279,7 +280,8 @@ def test_is_enabled_waffle(self, mock_waffle_flag_is_enabled): self.assertEqual(self.child_sink.__class__.is_enabled(), True) @override_settings(EVENT_SINK_CLICKHOUSE_CHILD_MODEL_ENABLED=True) - def test_is_enabled(self): + @patch("platform_plugin_aspects.sinks.base_sink.get_model") + def test_is_enabled(self, mock_get_model): """ Test that is_enable() returns the correct data. """ diff --git a/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py b/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py index f7ad9c1..bf06d69 100644 --- a/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py +++ b/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py @@ -34,6 +34,7 @@ registry=OrderedRegistry ) @override_settings(EVENT_SINK_CLICKHOUSE_COURSE_OVERVIEW_ENABLED=True) +@patch("platform_plugin_aspects.sinks.base_sink.get_model") @patch("platform_plugin_aspects.sinks.course_overview_sink.get_tags_for_block") @patch("platform_plugin_aspects.sinks.CourseOverviewSink.serialize_item") @patch("platform_plugin_aspects.sinks.CourseOverviewSink.get_model") @@ -47,6 +48,7 @@ def test_course_publish_success( mock_overview, mock_serialize_item, mock_get_tags, + mock_get_model, ): """ Test of a successful end-to-end run. @@ -110,13 +112,14 @@ def test_course_publish_success( @responses.activate( # pylint: disable=unexpected-keyword-arg,no-value-for-parameter registry=OrderedRegistry ) +@patch("platform_plugin_aspects.sinks.base_sink.get_model") @patch("platform_plugin_aspects.sinks.CourseOverviewSink.serialize_item") @patch("platform_plugin_aspects.sinks.CourseOverviewSink.get_model") @patch("platform_plugin_aspects.sinks.course_overview_sink.get_detached_xblock_types") @patch("platform_plugin_aspects.sinks.course_overview_sink.get_modulestore") # pytest:disable=unused-argument def test_course_publish_clickhouse_error( - mock_modulestore, mock_detached, mock_overview, mock_serialize_item, caplog + mock_modulestore, mock_detached, mock_overview, mock_serialize_item, mock_get_model, caplog ): """ Test the case where a ClickHouse POST fails. From 56cb6f20cb1fe8ecea612b215516d32402d62acf Mon Sep 17 00:00:00 2001 From: Brian Mesick Date: Fri, 4 Oct 2024 15:48:45 -0400 Subject: [PATCH 2/3] style: Fix formatting --- .../sinks/tests/test_course_overview_sink.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py b/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py index bf06d69..2bb5801 100644 --- a/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py +++ b/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py @@ -119,7 +119,12 @@ def test_course_publish_success( @patch("platform_plugin_aspects.sinks.course_overview_sink.get_modulestore") # pytest:disable=unused-argument def test_course_publish_clickhouse_error( - mock_modulestore, mock_detached, mock_overview, mock_serialize_item, mock_get_model, caplog + mock_modulestore, + mock_detached, + mock_overview, + mock_serialize_item, + mock_get_model, + caplog, ): """ Test the case where a ClickHouse POST fails. From 055f883c8f99b40a1228e8e03f3250fec3635428 Mon Sep 17 00:00:00 2001 From: Brian Mesick Date: Tue, 8 Oct 2024 12:26:39 -0400 Subject: [PATCH 3/3] chore: Bump version to 0.11.3 --- platform_plugin_aspects/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/platform_plugin_aspects/__init__.py b/platform_plugin_aspects/__init__.py index 61d8f8b..fc3dda4 100644 --- a/platform_plugin_aspects/__init__.py +++ b/platform_plugin_aspects/__init__.py @@ -5,6 +5,6 @@ import os from pathlib import Path -__version__ = "0.11.2" +__version__ = "0.11.3" ROOT_DIRECTORY = Path(os.path.dirname(os.path.abspath(__file__)))