From 5b692466d08598ec918023f86405a2fac63554de Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Wed, 4 Mar 2026 15:06:58 +0500 Subject: [PATCH 1/5] fix: discussion tab visibility on import --- .../core/djangoapps/discussions/handlers.py | 31 ++++ .../discussions/tests/test_handlers.py | 149 +++++++++++++++++- 2 files changed, 174 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index f43aaf4b09eb..54255f2466a8 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -13,6 +13,8 @@ DiscussionsConfiguration, get_default_provider_type, ) +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -92,16 +94,45 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration for topic_context in new_topic_map.values() ]) + # Determine the 'enabled' value from the course's discussion tab state. + # When a course is imported/rerun, course.tabs are copied verbatim from the source, + # but DiscussionsConfiguration is not updated automatically. We read tab.is_hidden + # from modulestore on every publish so that DiscussionsConfiguration.enabled stays + # in sync with the tab state. This is safe because the serializer already keeps + # tab.is_hidden and enabled in agreement for API-driven changes. + enabled = True # default + try: + store = modulestore() + with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + course = store.get_course(course_key) + if course: + for tab in course.tabs: + if getattr(tab, 'tab_id', None) == 'discussion': + enabled = not tab.is_hidden + break + except Exception: # pylint: disable=broad-except + log.exception( + "Failed to read discussion tab state from modulestore for %s, defaulting enabled=True", + course_key, + ) + if not DiscussionsConfiguration.objects.filter(context_key=course_key).exists(): log.info(f"Course {course_key} doesn't have discussion configuration model yet. Creating a new one.") DiscussionsConfiguration( context_key=course_key, + enabled=enabled, provider_type=provider_id, plugin_configuration=configuration.plugin_configuration, enable_in_context=configuration.enable_in_context, enable_graded_units=configuration.enable_graded_units, unit_level_visibility=configuration.unit_level_visibility, ).save() + else: + # Sync enabled from the tab state. This covers imports/reruns into a course + # that already has a DiscussionsConfiguration — the tab state should win. + DiscussionsConfiguration.objects.filter( + context_key=course_key, + ).update(enabled=enabled) COURSE_DISCUSSIONS_CHANGED.connect(handle_course_discussion_config_update) diff --git a/openedx/core/djangoapps/discussions/tests/test_handlers.py b/openedx/core/djangoapps/discussions/tests/test_handlers.py index 7e493063da31..58b811e7e66a 100644 --- a/openedx/core/djangoapps/discussions/tests/test_handlers.py +++ b/openedx/core/djangoapps/discussions/tests/test_handlers.py @@ -1,7 +1,7 @@ """ Tests for discussions signal handlers """ -from unittest.mock import patch +from unittest.mock import MagicMock, patch from uuid import uuid4 import ddt @@ -14,6 +14,7 @@ @ddt.ddt +@patch("openedx.core.djangoapps.discussions.handlers.modulestore") class UpdateCourseDiscussionsConfigTestCase(TestCase): """ Tests for the discussion config update handler. @@ -47,7 +48,7 @@ def create_contexts(self, general=0, unit=0): }, ) - def test_configuration_for_new_course(self): + def test_configuration_for_new_course(self, mock_modulestore): """ Test that a new course gets a new discussion configuration object """ @@ -62,7 +63,7 @@ def test_configuration_for_new_course(self): db_config = DiscussionsConfiguration.objects.get(context_key=new_key) assert db_config.provider_type == "openedx" - def test_creating_new_links(self): + def test_creating_new_links(self, mock_modulestore): """ Test that new links are created in the db when they are added in the config. """ @@ -76,7 +77,7 @@ def test_creating_new_links(self): topic_links = DiscussionTopicLink.objects.filter(context_key=self.course_key) assert topic_links.count() == len(contexts) # 2 general + 3 units - def test_updating_existing_links(self): + def test_updating_existing_links(self, mock_modulestore): """ Test that updating existing links works as expected. """ @@ -112,7 +113,7 @@ def test_updating_existing_links(self): "openedx.core.djangoapps.discussions.models.AVAILABLE_PROVIDER_MAP", {"test": {"supports_in_context_discussions": True}}, ) - def test_provider_change(self): + def test_provider_change(self, mock_modulestore): """ Test that changing providers creates new links, and doesn't update existing ones. """ @@ -148,7 +149,7 @@ def test_provider_change(self): # The new link will get a new id assert new_link.external_id != str(existing_external_id) - def test_enabled_units_change(self): + def test_enabled_units_change(self, mock_modulestore): """ Test that when enabled units change, old unit links are disabled in context. """ @@ -191,3 +192,139 @@ def test_enabled_units_change(self): assert existing_topic_link.title == "Section 10|Subsection 10|Unit 10" # If there is no stored context, then continue using the Unit name. assert existing_topic_link_2.title == "Unit 11" + + +def _make_mock_tab(tab_id, is_hidden=False): + """Helper to create a mock course tab.""" + tab = MagicMock() + tab.tab_id = tab_id + tab.is_hidden = is_hidden + return tab + + +def _mock_modulestore_with_tabs(course_key, tabs): + """ + Return a mock modulestore whose get_course returns a course with the given tabs. + The mock supports the branch_setting context-manager protocol. + """ + mock_course = MagicMock() + mock_course.tabs = tabs + + store = MagicMock() + store.get_course.return_value = mock_course + store.branch_setting.return_value.__enter__ = MagicMock(return_value=store) + store.branch_setting.return_value.__exit__ = MagicMock(return_value=False) + return store + + +@patch("openedx.core.djangoapps.discussions.handlers.modulestore") +class TabStateSyncTestCase(TestCase): + """ + Tests that DiscussionsConfiguration.enabled is synced from the discussion + tab's is_hidden state in the modulestore. + """ + + def setUp(self): + super().setUp() + self.course_key = CourseKey.from_string("course-v1:test+test+test") + + def _build_config_data(self, course_key=None): + return CourseDiscussionConfigurationData( + course_key=course_key or self.course_key, + provider_type="openedx", + ) + + # -- New configuration creation (no existing DiscussionsConfiguration) -- + + def test_new_config_enabled_when_tab_visible(self, mock_modulestore): + """ + When creating a DiscussionsConfiguration for a brand-new course whose + discussion tab is visible (is_hidden=False), enabled should be True. + """ + new_key = CourseKey.from_string("course-v1:test+test+new_visible") + tabs = [ + _make_mock_tab("courseware"), + _make_mock_tab("discussion", is_hidden=False), + ] + mock_modulestore.return_value = _mock_modulestore_with_tabs(new_key, tabs) + + update_course_discussion_config(self._build_config_data(new_key)) + + config = DiscussionsConfiguration.objects.get(context_key=new_key) + assert config.enabled is True + + def test_new_config_disabled_when_tab_hidden(self, mock_modulestore): + """ + When creating a DiscussionsConfiguration for a brand-new course whose + discussion tab is hidden (is_hidden=True), enabled should be False. + """ + new_key = CourseKey.from_string("course-v1:test+test+new_hidden") + tabs = [ + _make_mock_tab("courseware"), + _make_mock_tab("discussion", is_hidden=True), + ] + mock_modulestore.return_value = _mock_modulestore_with_tabs(new_key, tabs) + + update_course_discussion_config(self._build_config_data(new_key)) + + config = DiscussionsConfiguration.objects.get(context_key=new_key) + assert config.enabled is False + + # -- Existing configuration update (import / rerun scenario) -- + + def test_existing_config_updated_to_disabled_on_import(self, mock_modulestore): + """ + Simulates importing a course with a hidden discussion tab into a course + that already has DiscussionsConfiguration(enabled=True). After publish, + enabled should become False to match the imported tab state. + """ + DiscussionsConfiguration.objects.create( + context_key=self.course_key, + provider_type="openedx", + enabled=True, + ) + tabs = [ + _make_mock_tab("courseware"), + _make_mock_tab("discussion", is_hidden=True), + ] + mock_modulestore.return_value = _mock_modulestore_with_tabs(self.course_key, tabs) + + update_course_discussion_config(self._build_config_data()) + + config = DiscussionsConfiguration.objects.get(context_key=self.course_key) + assert config.enabled is False + + def test_existing_config_updated_to_enabled_on_import(self, mock_modulestore): + """ + If an existing config has enabled=False and the imported course has a + visible discussion tab (is_hidden=False), enabled should be set to True. + """ + DiscussionsConfiguration.objects.create( + context_key=self.course_key, + provider_type="openedx", + enabled=False, + ) + tabs = [ + _make_mock_tab("courseware"), + _make_mock_tab("discussion", is_hidden=False), + ] + mock_modulestore.return_value = _mock_modulestore_with_tabs(self.course_key, tabs) + + update_course_discussion_config(self._build_config_data()) + + config = DiscussionsConfiguration.objects.get(context_key=self.course_key) + assert config.enabled is True + + def test_modulestore_failure_defaults_to_enabled(self, mock_modulestore): + """ + If the modulestore read throws an exception, enabled should default to True + so discussion isn't accidentally disabled. + """ + new_key = CourseKey.from_string("course-v1:test+test+ms_fail") + mock_modulestore.return_value.branch_setting.side_effect = Exception("boom") + + update_course_discussion_config(self._build_config_data(new_key)) + + config = DiscussionsConfiguration.objects.get(context_key=new_key) + assert config.enabled is True + From 06222a93a4cef8ca370166ccdada55bd2f65c3c7 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Wed, 4 Mar 2026 16:10:49 +0500 Subject: [PATCH 2/5] fix: quality issues --- openedx/core/djangoapps/discussions/tests/test_handlers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/core/djangoapps/discussions/tests/test_handlers.py b/openedx/core/djangoapps/discussions/tests/test_handlers.py index 58b811e7e66a..5b68806f5330 100644 --- a/openedx/core/djangoapps/discussions/tests/test_handlers.py +++ b/openedx/core/djangoapps/discussions/tests/test_handlers.py @@ -327,4 +327,3 @@ def test_modulestore_failure_defaults_to_enabled(self, mock_modulestore): config = DiscussionsConfiguration.objects.get(context_key=new_key) assert config.enabled is True - From 7d6985b08fb19a0bc0c1ba13fb0727c3e16da8fa Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Wed, 18 Mar 2026 17:01:03 +0500 Subject: [PATCH 3/5] refactor: move the logic to update_discussions_settings_from_course in task --- .../core/djangoapps/discussions/handlers.py | 32 +---- openedx/core/djangoapps/discussions/tasks.py | 28 ++++ .../discussions/tests/test_handlers.py | 128 ++++++------------ 3 files changed, 74 insertions(+), 114 deletions(-) diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index 54255f2466a8..d6a5642d16ff 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -13,8 +13,7 @@ DiscussionsConfiguration, get_default_provider_type, ) -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore + log = logging.getLogger(__name__) @@ -94,45 +93,16 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration for topic_context in new_topic_map.values() ]) - # Determine the 'enabled' value from the course's discussion tab state. - # When a course is imported/rerun, course.tabs are copied verbatim from the source, - # but DiscussionsConfiguration is not updated automatically. We read tab.is_hidden - # from modulestore on every publish so that DiscussionsConfiguration.enabled stays - # in sync with the tab state. This is safe because the serializer already keeps - # tab.is_hidden and enabled in agreement for API-driven changes. - enabled = True # default - try: - store = modulestore() - with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): - course = store.get_course(course_key) - if course: - for tab in course.tabs: - if getattr(tab, 'tab_id', None) == 'discussion': - enabled = not tab.is_hidden - break - except Exception: # pylint: disable=broad-except - log.exception( - "Failed to read discussion tab state from modulestore for %s, defaulting enabled=True", - course_key, - ) - if not DiscussionsConfiguration.objects.filter(context_key=course_key).exists(): log.info(f"Course {course_key} doesn't have discussion configuration model yet. Creating a new one.") DiscussionsConfiguration( context_key=course_key, - enabled=enabled, provider_type=provider_id, plugin_configuration=configuration.plugin_configuration, enable_in_context=configuration.enable_in_context, enable_graded_units=configuration.enable_graded_units, unit_level_visibility=configuration.unit_level_visibility, ).save() - else: - # Sync enabled from the tab state. This covers imports/reruns into a course - # that already has a DiscussionsConfiguration — the tab state should win. - DiscussionsConfiguration.objects.filter( - context_key=course_key, - ).update(enabled=enabled) COURSE_DISCUSSIONS_CHANGED.connect(handle_course_discussion_config_update) diff --git a/openedx/core/djangoapps/discussions/tasks.py b/openedx/core/djangoapps/discussions/tasks.py index afb43b32cd33..ec52405f6313 100644 --- a/openedx/core/djangoapps/discussions/tasks.py +++ b/openedx/core/djangoapps/discussions/tasks.py @@ -97,9 +97,37 @@ def update_discussions_settings_from_course( plugin_configuration=provider_config, contexts=contexts, ) + + # Sync DiscussionsConfiguration.enabled from the discussion tab's + # is_hidden state. When a course is imported or rerun, course.tabs are + # copied verbatim from the source but DiscussionsConfiguration is not, + # so we reconcile here where we already have the course loaded. + _sync_enabled_from_discussion_tab(course_key, course) + return config_data +def _sync_enabled_from_discussion_tab(course_key, course): + """ + Set DiscussionsConfiguration.enabled based on the discussion tab's is_hidden + value from the given course object. + + This keeps the DB model in sync with the modulestore tab state, which is + especially important after course import or rerun where the tab state is + copied but the DiscussionsConfiguration record is not. + """ + enabled = True # default when no discussion tab is found + if course: + for tab in course.tabs: + if getattr(tab, 'tab_id', None) == 'discussion': + enabled = not tab.is_hidden + break + + DiscussionsConfiguration.objects.filter( + context_key=course_key, + ).update(enabled=enabled) + + def get_discussable_units(course, enable_graded_units, discussable_units=None): """ Get all the units in the course that are discussable. diff --git a/openedx/core/djangoapps/discussions/tests/test_handlers.py b/openedx/core/djangoapps/discussions/tests/test_handlers.py index 5b68806f5330..70903abf52d8 100644 --- a/openedx/core/djangoapps/discussions/tests/test_handlers.py +++ b/openedx/core/djangoapps/discussions/tests/test_handlers.py @@ -11,10 +11,10 @@ from openedx_events.learning.data import CourseDiscussionConfigurationData, DiscussionTopicContext from openedx.core.djangoapps.discussions.handlers import update_course_discussion_config from openedx.core.djangoapps.discussions.models import DiscussionTopicLink, DiscussionsConfiguration +from openedx.core.djangoapps.discussions.tasks import _sync_enabled_from_discussion_tab @ddt.ddt -@patch("openedx.core.djangoapps.discussions.handlers.modulestore") class UpdateCourseDiscussionsConfigTestCase(TestCase): """ Tests for the discussion config update handler. @@ -48,7 +48,7 @@ def create_contexts(self, general=0, unit=0): }, ) - def test_configuration_for_new_course(self, mock_modulestore): + def test_configuration_for_new_course(self): """ Test that a new course gets a new discussion configuration object """ @@ -63,7 +63,7 @@ def test_configuration_for_new_course(self, mock_modulestore): db_config = DiscussionsConfiguration.objects.get(context_key=new_key) assert db_config.provider_type == "openedx" - def test_creating_new_links(self, mock_modulestore): + def test_creating_new_links(self): """ Test that new links are created in the db when they are added in the config. """ @@ -77,7 +77,7 @@ def test_creating_new_links(self, mock_modulestore): topic_links = DiscussionTopicLink.objects.filter(context_key=self.course_key) assert topic_links.count() == len(contexts) # 2 general + 3 units - def test_updating_existing_links(self, mock_modulestore): + def test_updating_existing_links(self): """ Test that updating existing links works as expected. """ @@ -113,7 +113,7 @@ def test_updating_existing_links(self, mock_modulestore): "openedx.core.djangoapps.discussions.models.AVAILABLE_PROVIDER_MAP", {"test": {"supports_in_context_discussions": True}}, ) - def test_provider_change(self, mock_modulestore): + def test_provider_change(self): """ Test that changing providers creates new links, and doesn't update existing ones. """ @@ -149,7 +149,7 @@ def test_provider_change(self, mock_modulestore): # The new link will get a new id assert new_link.external_id != str(existing_external_id) - def test_enabled_units_change(self, mock_modulestore): + def test_enabled_units_change(self): """ Test that when enabled units change, old unit links are disabled in context. """ @@ -202,128 +202,90 @@ def _make_mock_tab(tab_id, is_hidden=False): return tab -def _mock_modulestore_with_tabs(course_key, tabs): +class SyncEnabledFromDiscussionTabTestCase(TestCase): """ - Return a mock modulestore whose get_course returns a course with the given tabs. - The mock supports the branch_setting context-manager protocol. - """ - mock_course = MagicMock() - mock_course.tabs = tabs - - store = MagicMock() - store.get_course.return_value = mock_course - store.branch_setting.return_value.__enter__ = MagicMock(return_value=store) - store.branch_setting.return_value.__exit__ = MagicMock(return_value=False) - return store - - -@patch("openedx.core.djangoapps.discussions.handlers.modulestore") -class TabStateSyncTestCase(TestCase): - """ - Tests that DiscussionsConfiguration.enabled is synced from the discussion - tab's is_hidden state in the modulestore. + Tests that _sync_enabled_from_discussion_tab correctly updates + DiscussionsConfiguration.enabled based on the discussion tab's is_hidden state. """ def setUp(self): super().setUp() self.course_key = CourseKey.from_string("course-v1:test+test+test") - def _build_config_data(self, course_key=None): - return CourseDiscussionConfigurationData( - course_key=course_key or self.course_key, - provider_type="openedx", - ) - - # -- New configuration creation (no existing DiscussionsConfiguration) -- + def _make_course(self, tabs): + """Create a mock course with the given tabs.""" + course = MagicMock() + course.tabs = tabs + return course - def test_new_config_enabled_when_tab_visible(self, mock_modulestore): + def test_enabled_when_tab_visible(self): """ - When creating a DiscussionsConfiguration for a brand-new course whose - discussion tab is visible (is_hidden=False), enabled should be True. + When discussion tab is visible (is_hidden=False), enabled should be True. """ - new_key = CourseKey.from_string("course-v1:test+test+new_visible") - tabs = [ + DiscussionsConfiguration.objects.create( + context_key=self.course_key, + provider_type="openedx", + enabled=False, + ) + course = self._make_course([ _make_mock_tab("courseware"), _make_mock_tab("discussion", is_hidden=False), - ] - mock_modulestore.return_value = _mock_modulestore_with_tabs(new_key, tabs) + ]) - update_course_discussion_config(self._build_config_data(new_key)) + _sync_enabled_from_discussion_tab(self.course_key, course) - config = DiscussionsConfiguration.objects.get(context_key=new_key) + config = DiscussionsConfiguration.objects.get(context_key=self.course_key) assert config.enabled is True - def test_new_config_disabled_when_tab_hidden(self, mock_modulestore): - """ - When creating a DiscussionsConfiguration for a brand-new course whose - discussion tab is hidden (is_hidden=True), enabled should be False. - """ - new_key = CourseKey.from_string("course-v1:test+test+new_hidden") - tabs = [ - _make_mock_tab("courseware"), - _make_mock_tab("discussion", is_hidden=True), - ] - mock_modulestore.return_value = _mock_modulestore_with_tabs(new_key, tabs) - - update_course_discussion_config(self._build_config_data(new_key)) - - config = DiscussionsConfiguration.objects.get(context_key=new_key) - assert config.enabled is False - - # -- Existing configuration update (import / rerun scenario) -- - - def test_existing_config_updated_to_disabled_on_import(self, mock_modulestore): + def test_disabled_when_tab_hidden(self): """ - Simulates importing a course with a hidden discussion tab into a course - that already has DiscussionsConfiguration(enabled=True). After publish, - enabled should become False to match the imported tab state. + When discussion tab is hidden (is_hidden=True), enabled should be False. """ DiscussionsConfiguration.objects.create( context_key=self.course_key, provider_type="openedx", enabled=True, ) - tabs = [ + course = self._make_course([ _make_mock_tab("courseware"), _make_mock_tab("discussion", is_hidden=True), - ] - mock_modulestore.return_value = _mock_modulestore_with_tabs(self.course_key, tabs) + ]) - update_course_discussion_config(self._build_config_data()) + _sync_enabled_from_discussion_tab(self.course_key, course) config = DiscussionsConfiguration.objects.get(context_key=self.course_key) assert config.enabled is False - def test_existing_config_updated_to_enabled_on_import(self, mock_modulestore): + def test_defaults_to_enabled_when_no_discussion_tab(self): """ - If an existing config has enabled=False and the imported course has a - visible discussion tab (is_hidden=False), enabled should be set to True. + If the course has no discussion tab, enabled should default to True. """ DiscussionsConfiguration.objects.create( context_key=self.course_key, provider_type="openedx", enabled=False, ) - tabs = [ + course = self._make_course([ _make_mock_tab("courseware"), - _make_mock_tab("discussion", is_hidden=False), - ] - mock_modulestore.return_value = _mock_modulestore_with_tabs(self.course_key, tabs) + _make_mock_tab("progress"), + ]) - update_course_discussion_config(self._build_config_data()) + _sync_enabled_from_discussion_tab(self.course_key, course) config = DiscussionsConfiguration.objects.get(context_key=self.course_key) assert config.enabled is True - def test_modulestore_failure_defaults_to_enabled(self, mock_modulestore): + def test_defaults_to_enabled_when_course_is_none(self): """ - If the modulestore read throws an exception, enabled should default to True - so discussion isn't accidentally disabled. + If the course object is None, enabled should default to True. """ - new_key = CourseKey.from_string("course-v1:test+test+ms_fail") - mock_modulestore.return_value.branch_setting.side_effect = Exception("boom") + DiscussionsConfiguration.objects.create( + context_key=self.course_key, + provider_type="openedx", + enabled=False, + ) - update_course_discussion_config(self._build_config_data(new_key)) + _sync_enabled_from_discussion_tab(self.course_key, None) - config = DiscussionsConfiguration.objects.get(context_key=new_key) + config = DiscussionsConfiguration.objects.get(context_key=self.course_key) assert config.enabled is True From 9a3403c9444d7154d4fdcb95f2a6974c2500ca46 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Wed, 18 Mar 2026 18:15:09 +0500 Subject: [PATCH 4/5] test: add test --- .../discussions/tests/test_tasks.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/openedx/core/djangoapps/discussions/tests/test_tasks.py b/openedx/core/djangoapps/discussions/tests/test_tasks.py index 00b715c945e6..eca7f1d8cf45 100644 --- a/openedx/core/djangoapps/discussions/tests/test_tasks.py +++ b/openedx/core/djangoapps/discussions/tests/test_tasks.py @@ -184,6 +184,33 @@ def test_custom_discussion_settings(self, settings, context_count, present_units assert present_units <= units_in_config assert not missing_units & units_in_config + @ddt.data( + # (initial_enabled, tab_is_hidden, expected_enabled) + (True, True, False), # import with hidden tab → disabled + (False, False, True), # import with visible tab → enabled + ) + @ddt.unpack + def test_syncs_enabled_from_discussion_tab(self, initial_enabled, tab_is_hidden, expected_enabled): + """ + After update_discussions_settings_from_course, DiscussionsConfiguration.enabled + should match `not tab.is_hidden` from the course's discussion tab. + """ + discussion_config = DiscussionsConfiguration.get(self.course.id) + discussion_config.enabled = initial_enabled + discussion_config.save() + + course = self.store.get_course(self.course.id) + for tab in course.tabs: + if getattr(tab, 'tab_id', None) == 'discussion': + tab['is_hidden'] = tab_is_hidden + break + self.store.update_item(course, self.user.id) + + update_discussions_settings_from_course(self.course.id) + + discussion_config = DiscussionsConfiguration.get(self.course.id) + assert discussion_config.enabled is expected_enabled + @ddt.ddt class MigrateUnitDiscussionStateFromXBlockTestCase(ModuleStoreTestCase, DiscussionConfigUpdateMixin): From f07dbb444d696ae8ae2bac590dd3634cc814c352 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Wed, 18 Mar 2026 18:23:05 +0500 Subject: [PATCH 5/5] fix: remove old code --- .../core/djangoapps/discussions/handlers.py | 1 - .../discussions/tests/test_handlers.py | 100 +----------------- 2 files changed, 1 insertion(+), 100 deletions(-) diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index d6a5642d16ff..f43aaf4b09eb 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -14,7 +14,6 @@ get_default_provider_type, ) - log = logging.getLogger(__name__) diff --git a/openedx/core/djangoapps/discussions/tests/test_handlers.py b/openedx/core/djangoapps/discussions/tests/test_handlers.py index 70903abf52d8..7e493063da31 100644 --- a/openedx/core/djangoapps/discussions/tests/test_handlers.py +++ b/openedx/core/djangoapps/discussions/tests/test_handlers.py @@ -1,7 +1,7 @@ """ Tests for discussions signal handlers """ -from unittest.mock import MagicMock, patch +from unittest.mock import patch from uuid import uuid4 import ddt @@ -11,7 +11,6 @@ from openedx_events.learning.data import CourseDiscussionConfigurationData, DiscussionTopicContext from openedx.core.djangoapps.discussions.handlers import update_course_discussion_config from openedx.core.djangoapps.discussions.models import DiscussionTopicLink, DiscussionsConfiguration -from openedx.core.djangoapps.discussions.tasks import _sync_enabled_from_discussion_tab @ddt.ddt @@ -192,100 +191,3 @@ def test_enabled_units_change(self): assert existing_topic_link.title == "Section 10|Subsection 10|Unit 10" # If there is no stored context, then continue using the Unit name. assert existing_topic_link_2.title == "Unit 11" - - -def _make_mock_tab(tab_id, is_hidden=False): - """Helper to create a mock course tab.""" - tab = MagicMock() - tab.tab_id = tab_id - tab.is_hidden = is_hidden - return tab - - -class SyncEnabledFromDiscussionTabTestCase(TestCase): - """ - Tests that _sync_enabled_from_discussion_tab correctly updates - DiscussionsConfiguration.enabled based on the discussion tab's is_hidden state. - """ - - def setUp(self): - super().setUp() - self.course_key = CourseKey.from_string("course-v1:test+test+test") - - def _make_course(self, tabs): - """Create a mock course with the given tabs.""" - course = MagicMock() - course.tabs = tabs - return course - - def test_enabled_when_tab_visible(self): - """ - When discussion tab is visible (is_hidden=False), enabled should be True. - """ - DiscussionsConfiguration.objects.create( - context_key=self.course_key, - provider_type="openedx", - enabled=False, - ) - course = self._make_course([ - _make_mock_tab("courseware"), - _make_mock_tab("discussion", is_hidden=False), - ]) - - _sync_enabled_from_discussion_tab(self.course_key, course) - - config = DiscussionsConfiguration.objects.get(context_key=self.course_key) - assert config.enabled is True - - def test_disabled_when_tab_hidden(self): - """ - When discussion tab is hidden (is_hidden=True), enabled should be False. - """ - DiscussionsConfiguration.objects.create( - context_key=self.course_key, - provider_type="openedx", - enabled=True, - ) - course = self._make_course([ - _make_mock_tab("courseware"), - _make_mock_tab("discussion", is_hidden=True), - ]) - - _sync_enabled_from_discussion_tab(self.course_key, course) - - config = DiscussionsConfiguration.objects.get(context_key=self.course_key) - assert config.enabled is False - - def test_defaults_to_enabled_when_no_discussion_tab(self): - """ - If the course has no discussion tab, enabled should default to True. - """ - DiscussionsConfiguration.objects.create( - context_key=self.course_key, - provider_type="openedx", - enabled=False, - ) - course = self._make_course([ - _make_mock_tab("courseware"), - _make_mock_tab("progress"), - ]) - - _sync_enabled_from_discussion_tab(self.course_key, course) - - config = DiscussionsConfiguration.objects.get(context_key=self.course_key) - assert config.enabled is True - - def test_defaults_to_enabled_when_course_is_none(self): - """ - If the course object is None, enabled should default to True. - """ - DiscussionsConfiguration.objects.create( - context_key=self.course_key, - provider_type="openedx", - enabled=False, - ) - - _sync_enabled_from_discussion_tab(self.course_key, None) - - config = DiscussionsConfiguration.objects.get(context_key=self.course_key) - assert config.enabled is True