fix: handle external routes correctly (#182)

Author: diana-villalvazo-wgu

src/instructorNav/InstructorNav.test.tsx

@@ -8,7 +8,13 @@ import userEvent from '@testing-library/user-event';
 
 jest.mock('react-router-dom', () => ({
   useParams: jest.fn(),
+  Link: ({ to, children, ...props }: any) => (
+    <a href={to} data-testid="react-router-link" {...props} role="button">
+      {children}
+    </a>
+  ),
 }));
+
 jest.mock('@src/data/apiHook', () => ({
   useCourseInfo: jest.fn(),
 }));
@@ -126,4 +132,125 @@ describe('InstructorNav', () => {
       expect(tab2Link.closest('a')).toHaveAttribute('aria-current', 'page');
     });
   });
+
+  describe('Simplified navigation type detection', () => {
+    const courseId = 'course-v1:edX+DemoX+Demo_Course';
+
+    beforeEach(() => {
+      (useParams as jest.Mock).mockReturnValue({ courseId, tabId: 'tab1' });
+      (useCourseInfo as jest.Mock).mockReturnValue({
+        data: { tabs: [] },
+        isLoading: false,
+      });
+    });
+
+    it('uses internal navigation for URLs starting with "/"', () => {
+      (useWidgetProps as jest.Mock).mockReturnValue([
+        { tabId: 'tab1', url: '/instructor-dashboard/course123/course_info', title: 'Course Info', sortOrder: 1 },
+        { tabId: 'tab2', url: '/courses/course123/courseware', title: 'Courseware', sortOrder: 2 },
+        { tabId: 'tab3', url: '/authoring/course123', title: 'Authoring', sortOrder: 3 },
+        { tabId: 'tab4', url: '/admin/dashboard', title: 'Admin', sortOrder: 4 },
+      ]);
+
+      render(<InstructorNav />);
+
+      const courseInfoLink = screen.getByText('Course Info');
+      const coursewareLink = screen.getByText('Courseware');
+      const authoringLink = screen.getByText('Authoring');
+      const adminLink = screen.getByText('Admin');
+
+      // All URLs starting with "/" should use React Router Link (internal navigation)
+      // Our mock Link component has role="button", so we can check for that to confirm it's using our internal navigation
+      expect(courseInfoLink).toHaveAttribute('role', 'button');
+      expect(coursewareLink).toHaveAttribute('role', 'button');
+      expect(authoringLink).toHaveAttribute('role', 'button');
+      expect(adminLink).toHaveAttribute('role', 'button');
+
+      // Verify the links are accessible and clickable
+      expect(courseInfoLink).toBeInTheDocument();
+      expect(coursewareLink).toBeInTheDocument();
+      expect(authoringLink).toBeInTheDocument();
+      expect(adminLink).toBeInTheDocument();
+    });
+
+    it('uses external navigation for URLs NOT starting with "/"', () => {
+      (useWidgetProps as jest.Mock).mockReturnValue([
+        { tabId: 'tab1', url: 'https://external.edx.org/courses', title: 'External Courses', sortOrder: 1 },
+        { tabId: 'tab2', url: 'http://learning.edx.org', title: 'Learning', sortOrder: 2 },
+        { tabId: 'tab3', url: 'course_info', title: 'Relative Tab', sortOrder: 3 },
+        { tabId: 'tab4', url: 'mailto:support@edx.org', title: 'Contact', sortOrder: 4 },
+      ]);
+
+      render(<InstructorNav />);
+
+      const externalLink = screen.getByText('External Courses');
+      const learningLink = screen.getByText('Learning');
+      const relativeLink = screen.getByText('Relative Tab');
+      const contactLink = screen.getByText('Contact');
+
+      // All URLs NOT starting with "/" should use anchor tags (external navigation)
+      // Our mock Link component has role="button", so if it doesn't have that, it's an external link
+      expect(externalLink.closest('a')).not.toHaveAttribute('role', 'button');
+      expect(learningLink.closest('a')).not.toHaveAttribute('role', 'button');
+      expect(relativeLink.closest('a')).not.toHaveAttribute('role', 'button');
+      expect(contactLink.closest('a')).not.toHaveAttribute('role', 'button');
+
+      // Verify the links are accessible and clickable
+      expect(externalLink).toBeInTheDocument();
+      expect(learningLink).toBeInTheDocument();
+      expect(relativeLink).toBeInTheDocument();
+      expect(contactLink).toBeInTheDocument();
+    });
+
+    it('handles mixed internal and external URLs in same navigation', () => {
+      (useWidgetProps as jest.Mock).mockReturnValue([
+        { tabId: 'tab1', url: '/internal/path', title: 'Internal', sortOrder: 1 },
+        { tabId: 'tab2', url: 'external-relative', title: 'External', sortOrder: 2 },
+        { tabId: 'tab3', url: '/another/internal', title: 'Another Internal', sortOrder: 3 },
+        { tabId: 'tab4', url: 'https://example.com', title: 'Absolute External', sortOrder: 4 },
+      ]);
+
+      render(<InstructorNav />);
+
+      const internalLink = screen.getByText('Internal');
+      const externalLink = screen.getByText('External');
+      const anotherInternalLink = screen.getByText('Another Internal');
+      const absoluteExternalLink = screen.getByText('Absolute External');
+
+      // All links should be present and clickable regardless of navigation type
+      expect(internalLink.closest('a')).toHaveAttribute('role', 'button');
+      expect(externalLink.closest('a')).not.toHaveAttribute('role', 'button');
+      expect(anotherInternalLink.closest('a')).toHaveAttribute('role', 'button');
+      expect(absoluteExternalLink.closest('a')).not.toHaveAttribute('role', 'button');
+
+      // Verify the links are accessible
+      expect(internalLink).toBeInTheDocument();
+      expect(externalLink).toBeInTheDocument();
+      expect(anotherInternalLink).toBeInTheDocument();
+      expect(absoluteExternalLink).toBeInTheDocument();
+    });
+
+    it('maintains clearAlerts functionality for both navigation types', async () => {
+      (useWidgetProps as jest.Mock).mockReturnValue([
+        { tabId: 'tab1', url: '/internal', title: 'Internal Tab', sortOrder: 1 },
+        { tabId: 'tab2', url: 'external', title: 'External Tab', sortOrder: 2 },
+      ]);
+
+      render(<InstructorNav />);
+      const user = userEvent.setup();
+
+      const internalTab = screen.getByText('Internal Tab');
+      const externalTab = screen.getByText('External Tab');
+
+      // Click internal navigation tab
+      await user.click(internalTab);
+      expect(mockClearAlerts).toHaveBeenCalled();
+
+      mockClearAlerts.mockClear();
+
+      // Click external navigation tab
+      await user.click(externalTab);
+      expect(mockClearAlerts).toHaveBeenCalled();
+    });
+  });
 });

src/instructorNav/InstructorNav.tsx

@@ -3,7 +3,7 @@ import { useParams, Link } from 'react-router-dom';
 import { Nav, Navbar, Skeleton } from '@openedx/paragon';
 import { useCourseInfo } from '@src/data/apiHook';
 import { useAlert } from '@src/providers/AlertProvider';
-import { useWidgetProps } from '../slots/SlotUtils';
+import { useWidgetProps } from '@src/slots/SlotUtils';
 
 export interface TabProps {
   tabId: string,
@@ -58,18 +58,23 @@ const InstructorNav = () => {
         <Navbar.Toggle aria-controls="instructor-nav" />
         <Navbar.Collapse id="instructor-nav">
           {
-            sortedTabs.map((tab) => (
-              <Nav.Item key={tab.tabId}>
-                <Nav.Link
-                  as={Link}
-                  to={tab.url}
-                  active={tab.tabId === tabId}
-                  onClick={() => clearAlerts()}
-                >
-                  {tab.title}
-                </Nav.Link>
-              </Nav.Item>
-            ))
+            sortedTabs.map((tab) => {
+              const isInternal = tab.url.startsWith('/');
+              return (
+                <Nav.Item key={tab.tabId}>
+                  <Nav.Link
+                    {...(isInternal
+                      ? { to: tab.url, as: Link }
+                      : { href: tab.url }
+                    )}
+                    active={tab.tabId === tabId}
+                    onClick={() => clearAlerts()}
+                  >
+                    {tab.title}
+                  </Nav.Link>
+                </Nav.Item>
+              );
+            })
           }
         </Navbar.Collapse>
       </Nav>