fix: use COMMUNICATIONS_MICROFRONTEND_URL for bulk_email tab in instructor dashboard v2 API (#38018)

brianjbuck-wgu · web-flow · commit 847de8ffd990 · 2026-03-26T11:05:51.000-06:00
diff --git a/lms/djangoapps/instructor/tests/test_api_v2.py b/lms/djangoapps/instructor/tests/test_api_v2.py
@@ -8,6 +8,7 @@
 from uuid import uuid4
 
 import ddt
+from django.test import SimpleTestCase, override_settings
 from django.urls import NoReverseMatch
 from django.urls import reverse
 from opaque_keys import InvalidKeyError
@@ -27,6 +28,7 @@
 from common.djangoapps.course_modes.tests.factories import CourseModeFactory
 from common.djangoapps.student.models.course_enrollment import CourseEnrollment
 from lms.djangoapps.courseware.models import StudentModule
+from lms.djangoapps.instructor.views.serializers_v2 import CourseInformationSerializerV2
 from lms.djangoapps.instructor_task.tests.factories import InstructorTaskFactory
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE
 from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory
@@ -456,6 +458,44 @@ def test_bulk_email_tab_not_visible(self, feature_enabled, user_attribute, mock_
 
         self.assertNotIn('bulk_email', tab_ids)
 
+    @patch('lms.djangoapps.instructor.views.serializers_v2.is_bulk_email_feature_enabled')
+    @override_settings(COMMUNICATIONS_MICROFRONTEND_URL='http://localhost:1984')
+    def test_bulk_email_tab_url_uses_communications_mfe(self, mock_bulk_email_enabled):
+        """
+        Test that the bulk_email tab URL uses COMMUNICATIONS_MICROFRONTEND_URL,
+        not INSTRUCTOR_MICROFRONTEND_URL.
+        """
+        mock_bulk_email_enabled.return_value = True
+
+        tabs = self._get_tabs_from_response(self.staff)
+        bulk_email_tab = next((tab for tab in tabs if tab['tab_id'] == 'bulk_email'), None)
+
+        self.assertIsNotNone(bulk_email_tab)
+        expected_url = f'http://localhost:1984/courses/{self.course.id}/bulk_email'
+        self.assertEqual(bulk_email_tab['url'], expected_url)
+
+    @patch('lms.djangoapps.instructor.views.serializers_v2.is_bulk_email_feature_enabled')
+    @override_settings(COMMUNICATIONS_MICROFRONTEND_URL=None)
+    def test_bulk_email_tab_logs_warning_when_communications_mfe_url_not_set(self, mock_bulk_email_enabled):
+        """
+        Test that a warning is logged when COMMUNICATIONS_MICROFRONTEND_URL is not set,
+        and the resulting URL does not contain 'None'.
+        """
+        mock_bulk_email_enabled.return_value = True
+
+        with self.assertLogs('lms.djangoapps.instructor.views.serializers_v2', level='WARNING') as cm:
+            tabs = self._get_tabs_from_response(self.staff)
+
+        self.assertTrue(
+            any('COMMUNICATIONS_MICROFRONTEND_URL is not configured' in msg for msg in cm.output)
+        )
+        bulk_email_tab = next((tab for tab in tabs if tab['tab_id'] == 'bulk_email'), None)
+        self.assertIsNotNone(bulk_email_tab)
+        self.assertFalse(
+            bulk_email_tab['url'].startswith('None'),
+            f"Tab URL should not start with 'None': {bulk_email_tab['url']}"
+        )
+
     def test_tabs_have_sort_order(self):
         """
         Test that all tabs include a sort_order field.
@@ -503,7 +543,7 @@ def test_tabs_log_warning_when_mfe_url_not_set(self):
             tabs = self._get_tabs_from_response(self.staff)
 
         self.assertTrue(
-            any('INSTRUCTOR_MICROFRONTEND_URL is not set' in msg for msg in cm.output)
+            any('INSTRUCTOR_MICROFRONTEND_URL is not configured' in msg for msg in cm.output)
         )
         # Tab URLs should use empty string as base, not "None"
         for tab in tabs:
@@ -534,6 +574,62 @@ def test_pacing_self_for_self_paced_course(self):
         self.assertEqual(response.data['pacing'], 'self')
 
 
+class BuildTabUrlTest(SimpleTestCase):
+    """
+    Unit tests for CourseInformationSerializerV2._build_tab_url.
+
+    Tests the helper directly to verify URL joining behavior without
+    going through the full API stack.
+    """
+
+    def _build(self, setting_name, *parts):
+        return CourseInformationSerializerV2._build_tab_url(setting_name, *parts)  # pylint: disable=protected-access
+
+    @override_settings(INSTRUCTOR_MICROFRONTEND_URL='http://localhost:2003')
+    def test_joins_base_and_path_parts(self):
+        """Parts are joined with '/' separators."""
+        result = self._build('INSTRUCTOR_MICROFRONTEND_URL', 'instructor', 'course-v1:edX+DemoX+Demo', 'grading')
+        self.assertEqual(result, 'http://localhost:2003/instructor/course-v1:edX+DemoX+Demo/grading')
+
+    @override_settings(INSTRUCTOR_MICROFRONTEND_URL='http://localhost:2003/')
+    def test_strips_trailing_slash_from_base(self):
+        """A trailing slash on the base URL does not produce a double slash."""
+        result = self._build('INSTRUCTOR_MICROFRONTEND_URL', 'instructor', 'course-v1:edX+DemoX+Demo', 'grading')
+        self.assertEqual(result, 'http://localhost:2003/instructor/course-v1:edX+DemoX+Demo/grading')
+
+    @override_settings(INSTRUCTOR_MICROFRONTEND_URL='http://localhost:2003')
+    def test_strips_slashes_from_path_parts(self):
+        """Leading and trailing slashes on path parts are stripped before joining."""
+        result = self._build('INSTRUCTOR_MICROFRONTEND_URL', '/instructor/', '/course-v1:edX+DemoX+Demo/', '/grading/')
+        self.assertEqual(result, 'http://localhost:2003/instructor/course-v1:edX+DemoX+Demo/grading')
+
+    @override_settings(COMMUNICATIONS_MICROFRONTEND_URL=None)
+    def test_logs_warning_and_returns_relative_url_when_setting_is_none(self):
+        """When the setting is None, a warning is logged and the URL is relative (no 'None' prefix)."""
+        with self.assertLogs('lms.djangoapps.instructor.views.serializers_v2', level='WARNING') as cm:
+            result = self._build(
+                'COMMUNICATIONS_MICROFRONTEND_URL', 'courses', 'course-v1:edX+DemoX+Demo', 'bulk_email'
+            )
+
+        self.assertTrue(any('COMMUNICATIONS_MICROFRONTEND_URL is not configured' in msg for msg in cm.output))
+        self.assertFalse(result.startswith('None'))
+        self.assertEqual(result, '/courses/course-v1:edX+DemoX+Demo/bulk_email')
+
+    def test_logs_warning_when_setting_does_not_exist(self):
+        """When the setting name is not defined at all, behavior matches the None case."""
+        with self.assertLogs('lms.djangoapps.instructor.views.serializers_v2', level='WARNING') as cm:
+            result = self._build('NONEXISTENT_MFE_URL', 'instructor', 'course-v1:edX+DemoX+Demo', 'grading')
+
+        self.assertTrue(any('NONEXISTENT_MFE_URL is not configured' in msg for msg in cm.output))
+        self.assertEqual(result, '/instructor/course-v1:edX+DemoX+Demo/grading')
+
+    @override_settings(COMMUNICATIONS_MICROFRONTEND_URL='http://localhost:1984/communications/')
+    def test_base_with_subpath_and_trailing_slash(self):
+        """Base URL with a subpath and trailing slash is joined cleanly."""
+        result = self._build('COMMUNICATIONS_MICROFRONTEND_URL', 'courses', 'course-v1:edX+DemoX+Demo', 'bulk_email')
+        self.assertEqual(result, 'http://localhost:1984/communications/courses/course-v1:edX+DemoX+Demo/bulk_email')
+
+
 @ddt.ddt
 class InstructorTaskListViewTest(SharedModuleStoreTestCase):
     """
diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py
@@ -76,16 +76,37 @@ class CourseInformationSerializerV2(serializers.Serializer):
         help_text="Message about analytics dashboard availability"
     )
 
+    @staticmethod
+    def _build_tab_url(setting_name, *path_parts):
+        """
+        Build a tab URL from a Django setting and path parts.
+
+        Retrieves the base URL from `setting_name`, strips any trailing slash,
+        then joins the provided path parts (stripping their leading/trailing
+        slashes) with `/` separators — behaving like ``os.path.join`` for URLs.
+
+        Logs a warning and falls back to a relative URL if the setting is unset.
+
+        Example:
+
+            _build_tab_url('INSTRUCTOR_MICROFRONTEND_URL', 'instructor', course_key, 'grading')
+            # => 'http://localhost:2003/instructor/course-v1:.../grading'
+
+            _build_tab_url('COMMUNICATIONS_MICROFRONTEND_URL', 'courses', course_key, 'bulk_email')
+            # => 'http://localhost:1984/communications/courses/course-v1:.../bulk_email'
+        """
+        base_url = getattr(settings, setting_name, None)
+        if base_url is None:
+            log.warning('%s is not configured.', setting_name)
+            base_url = ''
+        parts = [base_url.rstrip('/')] + [str(part).strip('/') for part in path_parts]
+        return '/'.join(parts)
+
     def get_tabs(self, data):
         """Get serialized course tabs."""
         request = data['request']
         course = data['course']
         course_key = course.id
-        mfe_base_url = settings.INSTRUCTOR_MICROFRONTEND_URL
-
-        if not mfe_base_url:
-            log.warning('INSTRUCTOR_MICROFRONTEND_URL is not set.')
-            mfe_base_url = ''
 
         access = {
             'admin': request.user.is_staff,
@@ -110,31 +131,56 @@ def get_tabs(self, data):
                 {
                     'tab_id': 'course_info',
                     'title': _('Course Info'),
-                    'url': f'{mfe_base_url}/instructor/{str(course_key)}/course_info',
+                    'url': self._build_tab_url(
+                        'INSTRUCTOR_MICROFRONTEND_URL',
+                        'instructor',
+                        course_key,
+                        'course_info'
+                    ),
                     'sort_order': 10,
                 },
                 {
                     'tab_id': 'enrollments',
                     'title': _('Enrollments'),
-                    'url': f'{mfe_base_url}/instructor/{str(course_key)}/enrollments',
+                    'url': self._build_tab_url(
+                        'INSTRUCTOR_MICROFRONTEND_URL',
+                        'instructor',
+                        course_key,
+                        'enrollments'
+                    ),
                     'sort_order': 20,
                 },
                 {
-                    "tab_id": "course_team",
-                    "title": "Course Team",
-                    "url": f'{mfe_base_url}/instructor/{str(course_key)}/course_team',
+                    'tab_id': 'course_team',
+                    'title': _('Course Team'),
+                    'url': self._build_tab_url(
+                        'INSTRUCTOR_MICROFRONTEND_URL',
+                        'instructor',
+                        course_key,
+                        'course_team'
+                    ),
                     'sort_order': 30,
                 },
                 {
                     'tab_id': 'grading',
                     'title': _('Grading'),
-                    'url': f'{mfe_base_url}/instructor/{str(course_key)}/grading',
+                    'url': self._build_tab_url(
+                        'INSTRUCTOR_MICROFRONTEND_URL',
+                        'instructor',
+                        course_key,
+                        'grading'
+                    ),
                     'sort_order': 40,
                 },
                 {
                     'tab_id': 'cohorts',
                     'title': _('Cohorts'),
-                    'url': f'{mfe_base_url}/instructor/{str(course_key)}/cohorts',
+                    'url': self._build_tab_url(
+                        'INSTRUCTOR_MICROFRONTEND_URL',
+                        'instructor',
+                        course_key,
+                        'cohorts'
+                    ),
                     'sort_order': 90,
                 },
             ])
@@ -143,23 +189,38 @@ def get_tabs(self, data):
             tabs.append({
                 'tab_id': 'bulk_email',
                 'title': _('Bulk Email'),
-                'url': f'{mfe_base_url}/instructor/{str(course_key)}/bulk_email',
+                'url': self._build_tab_url(
+                    'COMMUNICATIONS_MICROFRONTEND_URL',
+                    'courses',
+                    course_key,
+                    'bulk_email'
+                ),
                 'sort_order': 100,
             })
 
         if access['instructor'] and is_enabled_for_course(course_key):
             tabs.append({
                 'tab_id': 'date_extensions',
                 'title': _('Date Extensions'),
-                'url': f'{mfe_base_url}/instructor/{str(course_key)}/date_extensions',
+                'url': self._build_tab_url(
+                    'INSTRUCTOR_MICROFRONTEND_URL',
+                    'instructor',
+                    course_key,
+                    'date_extensions'
+                ),
                 'sort_order': 50,
             })
 
         if access['data_researcher']:
             tabs.append({
                 'tab_id': 'data_downloads',
                 'title': _('Data Downloads'),
-                'url': f'{mfe_base_url}/instructor/{str(course_key)}/data_downloads',
+                'url': self._build_tab_url(
+                    'INSTRUCTOR_MICROFRONTEND_URL',
+                    'instructor',
+                    course_key,
+                    'data_downloads'
+                ),
                 'sort_order': 60,
             })
 
@@ -174,7 +235,12 @@ def get_tabs(self, data):
             tabs.append({
                 'tab_id': 'open_responses',
                 'title': _('Open Responses'),
-                'url': f'{mfe_base_url}/instructor/{str(course_key)}/open_responses',
+                'url': self._build_tab_url(
+                    'INSTRUCTOR_MICROFRONTEND_URL',
+                    'instructor',
+                    course_key,
+                    'open_responses'
+                ),
                 'sort_order': 70,
             })
 
@@ -186,7 +252,12 @@ def get_tabs(self, data):
             tabs.append({
                 'tab_id': 'certificates',
                 'title': _('Certificates'),
-                'url': f'{mfe_base_url}/instructor/{str(course_key)}/certificates',
+                'url': self._build_tab_url(
+                    'INSTRUCTOR_MICROFRONTEND_URL',
+                    'instructor',
+                    course_key,
+                    'certificates'
+                ),
                 'sort_order': 80,
             })
 
@@ -203,7 +274,12 @@ def get_tabs(self, data):
             tabs.append({
                 'tab_id': 'special_exams',
                 'title': _('Special Exams'),
-                'url': f'{mfe_base_url}/instructor/{str(course_key)}/special_exams',
+                'url': self._build_tab_url(
+                    'INSTRUCTOR_MICROFRONTEND_URL',
+                    'instructor',
+                    course_key,
+                    'special_exams'
+                ),
                 'sort_order': 110,
             })
 
diff --git a/lms/envs/common.py b/lms/envs/common.py
@@ -2845,6 +2845,10 @@
 # .. setting_default: None
 # .. setting_description: Base URL of the micro-frontend-based instructor app.
 INSTRUCTOR_MICROFRONTEND_URL = None
+# .. setting_name: COMMUNICATIONS_MICROFRONTEND_URL
+# .. setting_default: None
+# .. setting_description: Base URL of the micro-frontend-based communications app.
+COMMUNICATIONS_MICROFRONTEND_URL = None
 # .. setting_name: DISCUSSION_SPAM_URLS
 # .. setting_default: []
 # .. setting_description: Urls to filter from discussion content to avoid spam
