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..5b68806f5330 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,138 @@ 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