Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,547 changes: 1,354 additions & 193 deletions package-lock.json

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@
},
"dependencies": {
"@edx/brand": "npm:@openedx/brand-openedx@^1.2.3",
"@edx/openedx-atlas": "^0.7.0",
"@tanstack/react-query-devtools": "^5.90.2"
"@edx/openedx-atlas": "^0.7.0"
},
"devDependencies": {
"@edx/browserslist-config": "^1.5.0",
"@tanstack/react-query-devtools": "^5.90.2",
"@testing-library/jest-dom": "^6.8.0",
"@testing-library/react": "^16.3.0",
"@testing-library/user-event": "^14.6.1",
Expand Down
11 changes: 6 additions & 5 deletions src/Main.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
import { CurrentAppProvider, getAppConfig } from '@openedx/frontend-base';

import { appId } from './constants';

import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { Outlet } from 'react-router-dom';
import { ReactQueryDevtools } from '@tanstack/react-query-devtools';
import { appId } from './constants';
import PageWrapper from './pageWrapper/PageWrapper';
import './main.scss';

const queryClient = new QueryClient();

const Main = () => (
<CurrentAppProvider appId={appId}>
<QueryClientProvider client={queryClient}>
<main>
<Outlet />
<main className="d-flex flex-column flex-grow-1">
<PageWrapper>
<Outlet />
</PageWrapper>
{ getAppConfig(appId).NODE_ENV === 'development' && <ReactQueryDevtools initialIsOpen={false} /> }
</main>
</QueryClientProvider>
Expand Down
6 changes: 4 additions & 2 deletions src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { App } from '@openedx/frontend-base';
import { appId } from './constants';
import routes from './routes';
import messages from './i18n';
import slots from './slots';
import providers from './providers';

const app: App = {
appId,
routes,
messages,
providers: [],
slots: [],
providers,
slots,
config: {
NODE_ENV: 'development',
LMS_BASE_URL: 'http://local.openedx.io:8000'
Expand Down
9 changes: 9 additions & 0 deletions src/certificates/CertificatesPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const CertificatesPage = () => {
return (
<div>
<h3>Certificates</h3>
</div>
);
};

export default CertificatesPage;
9 changes: 9 additions & 0 deletions src/courseTeam/CourseTeamPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const CourseTeamPage = () => {
return (
<div>
<h3>Course Team</h3>
</div>
);
};

export default CourseTeamPage;
13 changes: 6 additions & 7 deletions src/data/api.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getCourseInfo } from './api';
import { camelCaseObject, getAppConfig, getAuthenticatedHttpClient } from '@openedx/frontend-base';
import { getAppConfig, getAuthenticatedHttpClient, camelCaseObject } from '@openedx/frontend-base';

jest.mock('@openedx/frontend-base');

Expand All @@ -9,18 +9,18 @@ const mockHttpClient = {
};

const mockGetAppConfig = getAppConfig as jest.MockedFunction<typeof getAppConfig>;
const mockGetAuthenticatedHttpClient = getAuthenticatedHttpClient as jest.MockedFunction<typeof getAuthenticatedHttpClient>;
const mockCamelCaseObject = camelCaseObject as jest.MockedFunction<typeof camelCaseObject>;
const mockGetAuthenticatedHttpClient = getAuthenticatedHttpClient as jest.MockedFunction<typeof getAuthenticatedHttpClient>;

describe('getCourseInfo', () => {
const mockCourseData = { course_name: 'Test Course' };
const mockCamelCaseData = { courseName: 'Test Course' };
const mockCourseData = { course_name: 'Test Course', tabs: [{ tab_id: 'course_info', title: 'Course Information', url: 'https://test-lms.com/courses/test-course-123/info' }] };
const mockCamelCasedCourseData = { courseName: 'Test Course', tabs: [{ tabId: 'course_info', title: 'Course Information', url: 'https://test-lms.com/courses/test-course-123/info' }] };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what patterns we're using in other tests, but it'd be nice to reduce the duplication here a bit as the data we're mocking grows. Maybe something like

const mockCourseData = { course_name: 'Test Course', [...] }
const mockCamelCasedCourseData = { courseName: mockCourseData.course_name, [...] }


beforeEach(() => {
jest.clearAllMocks();
mockGetAppConfig.mockReturnValue({ LMS_BASE_URL: 'https://test-lms.com' });
mockGetAuthenticatedHttpClient.mockReturnValue(mockHttpClient as any);
mockCamelCaseObject.mockReturnValue(mockCamelCaseData);
mockCamelCaseObject.mockReturnValue(mockCamelCasedCourseData);
mockHttpClient.get.mockResolvedValue({ data: mockCourseData });
});

Expand All @@ -30,8 +30,7 @@ describe('getCourseInfo', () => {
expect(mockGetAppConfig).toHaveBeenCalledWith('org.openedx.frontend.app.instructor');
expect(mockGetAuthenticatedHttpClient).toHaveBeenCalled();
expect(mockHttpClient.get).toHaveBeenCalledWith('https://test-lms.com/api/instructor/v2/courses/test-course-123');
expect(mockCamelCaseObject).toHaveBeenCalledWith(mockCourseData);
expect(result).toBe(mockCamelCaseData);
expect(result).toBe(mockCamelCasedCourseData);
});

it('throws error when API call fails', async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/data/api.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { camelCaseObject, getAppConfig, getAuthenticatedHttpClient } from '@openedx/frontend-base';
import { getAppConfig, getAuthenticatedHttpClient, camelCaseObject } from '@openedx/frontend-base';
import { appId } from '../constants';

export const getApiBaseUrl = () => getAppConfig(appId).LMS_BASE_URL;
Expand Down
2 changes: 2 additions & 0 deletions src/data/apiHook.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ const createWrapper = () => {
mutations: { retry: false },
},
});

const Wrapper = ({ children }: { children: React.ReactNode }) => (
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
);

Wrapper.displayName = 'TestWrapper';
return Wrapper;
};
Expand Down
1 change: 1 addition & 0 deletions src/data/apiHook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ export const useCourseInfo = (courseId: string) => (
useQuery({
queryKey: courseInfoQueryKeys.byCourse(courseId),
queryFn: () => getCourseInfo(courseId),
enabled: !!courseId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is checking courseId for truthiness here enough validation? My first impression is that this would lead to enabled being true when we have an invalid (but truthy) courseId.

})
);
9 changes: 9 additions & 0 deletions src/dataDownloads/DataDownloadsPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const DataDownloadsPage = () => {
return (
<div>
<h3>Data Downloads</h3>
</div>
);
};

export default DataDownloadsPage;
9 changes: 9 additions & 0 deletions src/dateExtensions/DateExtensionsPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const DateExtensionsPage = () => {
return (
<div>
<h3>Date Extensions</h3>
</div>
);
};

export default DateExtensionsPage;
9 changes: 9 additions & 0 deletions src/enrollments/EnrollmentsPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const EnrollmentsPage = () => {
return (
<div>
<h3>Enrollments</h3>
</div>
);
};

export default EnrollmentsPage;
9 changes: 9 additions & 0 deletions src/grading/GradingPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const GradingPage = () => {
return (
<div>
<h3>Grading</h3>
</div>
);
};

export default GradingPage;
75 changes: 75 additions & 0 deletions src/instructorTabs/InstructorTabs.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { useContext } from 'react';
import { useNavigate, useParams } from 'react-router-dom';
import { Tab, Tabs } from '@openedx/paragon';
import { SlotContext, useWidgetsForId } from '@openedx/frontend-base';
import { useCourseInfo } from '../data/apiHook';

export interface TabProps {
tabId: string,
url: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a clarifying comment here would be great, something like:

Suggested change
url: string,
url: string, // This is not used by frontend-app-instruct, it is a holdover from how the legacy instructor dash used the API

title: string,
}

const extractWidgetProps = (widget: React.ReactNode): TabProps | null => {
if (widget && typeof widget === 'object' && 'props' in widget) {
const props = widget.props.children.props as TabProps;
if (props?.tabId && props?.url && props?.title) {
return props;
}
}
return null;
};

const useWidgetProps = (slotId: string): TabProps[] => {
const widgets = useWidgetsForId(slotId);
return widgets.map(extractWidgetProps).filter((props): props is TabProps => props !== null);
};

const InstructorTabs = () => {
const navigate = useNavigate();
const { courseId, tabId } = useParams<{ courseId: string, tabId?: string }>();
const { id: slotId } = useContext(SlotContext);
const { data: courseInfo, isLoading } = useCourseInfo(courseId ?? '');
const widgetPropsArray = useWidgetProps(slotId);

const apiTabs: TabProps[] = courseInfo?.tabs ?? [];
const allTabs = [...apiTabs];

widgetPropsArray.forEach(slotTab => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to add a clarifying comment here, something like

Suggested change
widgetPropsArray.forEach(slotTab => {
// Tabs added via slot take priority over (read: replace) tabs from the API
// All tabs added via slot are placed at the end of the tabs array
widgetPropsArray.forEach(slotTab => {

if (!apiTabs.find(apiTab => apiTab.tabId === slotTab.tabId)) {
allTabs.push(slotTab);
} else {
const indexToRemove = allTabs.findIndex(({ tabId }) => tabId === slotTab.tabId);
if (indexToRemove !== -1) {
allTabs.splice(indexToRemove, 1);
}
allTabs.push(slotTab);
}
});

const activeKey = tabId ?? 'course_info';
const handleSelect = (eventKey: string | null) => {
if (eventKey && courseId) {
const selectedTab = allTabs.find(({ tabId }) => tabId === eventKey);
if (selectedTab) {
navigate(`/${courseId}/${eventKey}`);
}
}
};

if (isLoading) {
return <div>Loading tabs...</div>;
}

if (allTabs.length === 0) return null;

return (
<Tabs id="instructor-tabs" activeKey={activeKey} onSelect={handleSelect}>
{allTabs.map(({ tabId, title }) => (
<Tab key={tabId} eventKey={tabId} title={title} />
))}
</Tabs>
);
};

export default InstructorTabs;
9 changes: 9 additions & 0 deletions src/openResponses/OpenResponsesPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const OpenResponsesPage = () => {
return (
<div>
<h3>Open Responses</h3>
</div>
);
};

export default OpenResponsesPage;
16 changes: 16 additions & 0 deletions src/pageWrapper/PageWrapper.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { useIntl } from '@openedx/frontend-base';
import messages from './messages';
import InstructorTabsSlot from '../slots/instructorTabsSlot/InstructorTabsSlot';

const PageWrapper = ({ children }: { children: React.ReactNode }) => {
const { formatMessage } = useIntl();
return (
<div className="container-xl">
<h2>{formatMessage(messages.pageTitle)}</h2>
<InstructorTabsSlot />
{children}
</div>
);
};

export default PageWrapper;
11 changes: 11 additions & 0 deletions src/pageWrapper/messages.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { defineMessages } from '@openedx/frontend-base';

const messages = defineMessages({
pageTitle: {
id: 'pageWrapper.pageTitle',
defaultMessage: 'Instructor Dashboard',
description: 'Title for the instructor dashboard page',
},
});

export default messages;
76 changes: 43 additions & 33 deletions src/routes.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,44 @@
import { useParams, Navigate } from 'react-router-dom';
import CertificatesPage from './certificates/CertificatesPage';
import CohortsPage from './cohorts/CohortsPage';
import CourseInfoPage from './courseInfo/CourseInfoPage';
import CourseTeamPage from './courseTeam/CourseTeamPage';
import DataDownloadsPage from './dataDownloads/DataDownloadsPage';
import DateExtensionsPage from './dateExtensions/DateExtensionsPage';
import EnrollmentsPage from './enrollments/EnrollmentsPage';
import GradingPage from './grading/GradingPage';
import Main from './Main';
import OpenResponsesPage from './openResponses/OpenResponsesPage';
import SpecialExamsPage from './specialExams/SpecialExamsPage';

const TabContent = () => {
const { tabId } = useParams<{ tabId: string }>();

switch (tabId) {
case 'course_info':
return <CourseInfoPage />;
case 'enrollments':
return <EnrollmentsPage />;
case 'course_team':
return <CourseTeamPage />;
case 'cohorts':
return <CohortsPage />;
case 'date_extensions':
return <DateExtensionsPage />;
case 'grading':
return <GradingPage />;
case 'data_downloads':
return <DataDownloadsPage />;
case 'special_exams':
return <SpecialExamsPage />;
case 'certificates':
return <CertificatesPage />;
case 'open_responses':
return <OpenResponsesPage />;
default:
return <Navigate to="course_info" replace />;
}
};

const routes = [
{
Expand All @@ -12,41 +50,13 @@ const routes = [
Component: Main,
children: [
{
path: 'course_info',
element: <CourseInfoPage />
path: ':tabId',
element: <TabContent />
Comment on lines +53 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the motivation behind this change. I don't immediately see a need for these pages to have loaders or actions (and therefore errorElements), but moving this logic to a component feels like "locking in" on that decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i remember adding this so we can get tabId from useParams for InstructorTabs component, otherwise i didn't get that info, but if you know a better approach to do this glad to hear about it

},
// {
// path: 'membership',
// element: <MembershipPage />
// },
{
path: 'cohorts',
element: <CohortsPage />
},
// {
// path: 'extensions',
// element: <ExtensionsPage />
// },
// {
// path: 'student_admin',
// element: <StudentAdminPage />
// },
// {
// path: 'data_download',
// element: <DataDownloadPage />
// },
// {
// path: 'special_exams',
// element: <SpecialExamsPage />
// },
// {
// path: 'certificates',
// element: <CertificatesPage />
// },
// {
// path: 'open_responses',
// element: <OpenResponsesPage />
// }
path: '',
element: <Navigate to="course_info" replace />
}
]
}
];
Expand Down
5 changes: 5 additions & 0 deletions src/slots.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { SlotOperation } from '@openedx/frontend-base';

const slots: SlotOperation[] = [];

export default slots;
Loading