From c82fc3b6e54af6329f87966fdfb1671e039297ea Mon Sep 17 00:00:00 2001 From: Rebecca Graber Date: Thu, 5 Oct 2023 08:34:07 -0400 Subject: [PATCH] fixup!: changes from review --- CHANGELOG.rst | 2 ++ openedx_events/apps.py | 14 +++++++++----- openedx_events/event_bus/__init__.py | 11 ++++++----- openedx_events/event_bus/tests/test_loader.py | 7 +++++++ openedx_events/tests/test_producer_config.py | 6 +++--- test_utils/test_settings.py | 6 +++--- 6 files changed, 30 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8cd38d57..68cb1238 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,6 +16,8 @@ Unreleased [9.0.0] - 2023-10-04 -------------------- +Changed +~~~~~~~ * Re-licensed this repository from AGPL 3.0 to Apache 2.0 * **Breaking change**: Restructured EVENT_BUS_PRODUCER_CONFIG diff --git a/openedx_events/apps.py b/openedx_events/apps.py index de5e4748..7221603a 100644 --- a/openedx_events/apps.py +++ b/openedx_events/apps.py @@ -14,15 +14,20 @@ def general_signal_handler(sender, signal, **kwargs): # pylint: disable=unused- """ Signal handler for publishing events to configured event bus. """ - configurations = getattr(settings, "EVENT_BUS_PRODUCER_CONFIG", {}).get(signal.event_type, {}) + event_type_publish_configs = getattr(settings, "EVENT_BUS_PRODUCER_CONFIG", {}).get(signal.event_type, {}) + # event_type_publish_configs should look something like + # { + # "topic_a": { "event_key_field": "my.key.field", "enabled": True }, + # "topic_b": { "event_key_field": "my.key.field", "enabled": False } + # }" event_data = {key: kwargs.get(key) for key in signal.init_data} - for topic in configurations.keys(): - if configurations[topic]["enabled"]: + for topic in event_type_publish_configs.keys(): + if event_type_publish_configs[topic]["enabled"] is True: get_producer().send( signal=signal, topic=topic, - event_key_field=configurations[topic]["event_key_field"], + event_key_field=event_type_publish_configs[topic]["event_key_field"], event_data=event_data, event_metadata=kwargs["metadata"], ) @@ -58,7 +63,6 @@ def _get_validated_signal_config(self, event_type, configuration): except KeyError as exc: raise ProducerConfigurationError(message=f"No OpenEdxPublicSignal of type: '{event_type}'.") from exc for _, topic_configuration in configuration.items(): - print(f"{topic_configuration=}") if not isinstance(topic_configuration, dict): raise ProducerConfigurationError( event_type=event_type, diff --git a/openedx_events/event_bus/__init__.py b/openedx_events/event_bus/__init__.py index 80a0f2cb..eb3002ca 100644 --- a/openedx_events/event_bus/__init__.py +++ b/openedx_events/event_bus/__init__.py @@ -14,6 +14,7 @@ ``EVENT_BUS_CONSUMER``. """ +import copy import warnings from abc import ABC, abstractmethod from functools import lru_cache @@ -185,21 +186,21 @@ def _reset_state(sender, **kwargs): # pylint: disable=unused-argument get_producer.cache_clear() -def merge_publisher_configs(publisher_config_a, publisher_config_b): +def merge_publisher_configs(publisher_config_original, publisher_config_overrides): """ Merge two EVENT_BUS_PRODUCER_CONFIG maps. Arguments: - publisher_config_a: An EVENT_BUS_PRODUCER_CONFIG-structured map - publisher_config_b: An EVENT_BUS_PRODUCER_CONFIG-structured map + publisher_config_original: An EVENT_BUS_PRODUCER_CONFIG-structured map + publisher_config_overrides: An EVENT_BUS_PRODUCER_CONFIG-structured map Returns: A new EVENT_BUS_PRODUCER_CONFIG map created by combining the two maps. All event_type/topic pairs in publisher_config_b are added to the publisher_config_a. If there is a conflict on whether a particular event_type/topic pair is enabled, publisher_config_b wins out. """ - combined = {**publisher_config_a} - for event_type, event_type_config_b in publisher_config_b.items(): + combined = copy.deepcopy(publisher_config_original) + for event_type, event_type_config_b in publisher_config_overrides.items(): event_type_config_combined = combined.get(event_type, {}) for topic, topic_config_b in event_type_config_b.items(): topic_config_combined = event_type_config_combined.get(topic, {}) diff --git a/openedx_events/event_bus/tests/test_loader.py b/openedx_events/event_bus/tests/test_loader.py index fee2a6c6..b7d147aa 100644 --- a/openedx_events/event_bus/tests/test_loader.py +++ b/openedx_events/event_bus/tests/test_loader.py @@ -2,6 +2,7 @@ Tests for event bus implementation loader. """ +import copy import warnings from contextlib import contextmanager from unittest import TestCase @@ -139,6 +140,8 @@ def test_merge_configs(self): 'topic_c': {'event_key_field': 'field', 'enabled': True}, } } + # for ensuring we didn't change the original dict + dict_a_copy = copy.deepcopy(dict_a) dict_b = { 'event_type_0': { # disable an existing event/topic pairing @@ -151,6 +154,7 @@ def test_merge_configs(self): 'topic_e': {'event_key_field': 'field', 'enabled': True}, } } + dict_b_copy = copy.deepcopy(dict_b) result = merge_publisher_configs(dict_a, dict_b) self.assertDictEqual(result, { 'event_type_0': { @@ -165,6 +169,8 @@ def test_merge_configs(self): 'topic_e': {'event_key_field': 'field', 'enabled': True}, } }) + self.assertDictEqual(dict_a, dict_a_copy) + self.assertDictEqual(dict_b, dict_b_copy) def test_merge_configs_with_empty(self): dict_a = { @@ -179,3 +185,4 @@ def test_merge_configs_with_empty(self): dict_b = {} result = merge_publisher_configs(dict_a, dict_b) self.assertDictEqual(result, dict_a) + diff --git a/openedx_events/tests/test_producer_config.py b/openedx_events/tests/test_producer_config.py index 16ccecd2..61183827 100644 --- a/openedx_events/tests/test_producer_config.py +++ b/openedx_events/tests/test_producer_config.py @@ -46,12 +46,12 @@ def test_enabled_disabled_events(self, mock_producer): # check that call_args_list only consists of enabled topics. call_args = mock_send.send.call_args_list[0][1] self.assertDictContainsSubset( - {'topic': 'content-authoring-xblock-lifecycle', 'event_key_field': 'xblock_info.usage_key'}, + {'topic': 'enabled_topic_a', 'event_key_field': 'xblock_info.usage_key'}, call_args ) call_args = mock_send.send.call_args_list[1][1] self.assertDictContainsSubset( - {'topic': 'content-authoring-all-status', 'event_key_field': 'xblock_info.usage_key'}, + {'topic': 'enabled_topic_b', 'event_key_field': 'xblock_info.usage_key'}, call_args ) @@ -94,7 +94,7 @@ def test_configuration_is_validated(self): with override_settings( EVENT_BUS_PRODUCER_CONFIG={ - "org.openedx.content_authoring.xblock.deleted.v1": {"some": {"enabled": True}} + "org.openedx.content_authoring.xblock.deleted.v1": {"topic": {"enabled": True}} } ): with pytest.raises(ProducerConfigurationError, match="missing 'event_key_field' key."): diff --git a/test_utils/test_settings.py b/test_utils/test_settings.py index d4edc940..2359bfff 100644 --- a/test_utils/test_settings.py +++ b/test_utils/test_settings.py @@ -32,9 +32,9 @@ EVENT_BUS_PRODUCER_CONFIG = { 'org.openedx.content_authoring.xblock.published.v1': { - 'content-authoring-xblock-lifecycle': {'event_key_field': 'xblock_info.usage_key', 'enabled': True}, - 'content-authoring-all-status': {'event_key_field': 'xblock_info.usage_key', 'enabled': True}, - 'content-authoring-xblock-published': {'event_key_field': 'xblock_info.usage_key', 'enabled': False} + 'enabled_topic_a': {'event_key_field': 'xblock_info.usage_key', 'enabled': True}, + 'enabled_topic_b': {'event_key_field': 'xblock_info.usage_key', 'enabled': True}, + 'disabled_topic': {'event_key_field': 'xblock_info.usage_key', 'enabled': False} }, 'org.openedx.content_authoring.xblock.deleted.v1': {