fix: discussion tab visibility on import

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)

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