From dafd78a833a899536d0d3311d5d58c89b64b260b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 26 Sep 2024 09:50:37 +0200 Subject: [PATCH] Add button to sync grades in dashboard assignment view (#6700) --- lms/static/scripts/frontend_apps/api-types.ts | 7 + .../dashboard/AssignmentActivity.tsx | 110 +++++---- .../components/dashboard/SyncGradesButton.tsx | 128 ++++++++++ .../dashboard/test/AssignmentActivity-test.js | 93 +++++++- .../dashboard/test/SyncGradesButton-test.js | 225 ++++++++++++++++++ lms/static/scripts/frontend_apps/config.ts | 3 + 6 files changed, 518 insertions(+), 48 deletions(-) create mode 100644 lms/static/scripts/frontend_apps/components/dashboard/SyncGradesButton.tsx create mode 100644 lms/static/scripts/frontend_apps/components/dashboard/test/SyncGradesButton-test.js diff --git a/lms/static/scripts/frontend_apps/api-types.ts b/lms/static/scripts/frontend_apps/api-types.ts index b830dda967..e6665b203b 100644 --- a/lms/static/scripts/frontend_apps/api-types.ts +++ b/lms/static/scripts/frontend_apps/api-types.ts @@ -292,3 +292,10 @@ export type StudentsResponse = { students: Student[]; pagination: Pagination; }; + +/** + * Response for `/api/dashboard/assignments/{assignment_id}/grading/sync` + */ +export type GradingSync = { + status: 'scheduled' | 'in_progress' | 'finished' | 'failed'; +}; diff --git a/lms/static/scripts/frontend_apps/components/dashboard/AssignmentActivity.tsx b/lms/static/scripts/frontend_apps/components/dashboard/AssignmentActivity.tsx index 2fc623c653..6df8351395 100644 --- a/lms/static/scripts/frontend_apps/components/dashboard/AssignmentActivity.tsx +++ b/lms/static/scripts/frontend_apps/components/dashboard/AssignmentActivity.tsx @@ -22,6 +22,7 @@ import FormattedDate from './FormattedDate'; import GradeIndicator from './GradeIndicator'; import type { OrderableActivityTableColumn } from './OrderableActivityTable'; import OrderableActivityTable from './OrderableActivityTable'; +import SyncGradesButton from './SyncGradesButton'; type StudentsTableRow = { lms_id: string; @@ -37,7 +38,11 @@ type StudentsTableRow = { */ export default function AssignmentActivity() { const { dashboard } = useConfig(['dashboard']); - const { routes, assignment_segments_filter_enabled } = dashboard; + const { + routes, + auto_grading_sync_enabled, + assignment_segments_filter_enabled, + } = dashboard; const { assignmentId, organizationPublicId } = useParams<{ assignmentId: string; organizationPublicId?: string; @@ -51,14 +56,14 @@ export default function AssignmentActivity() { const assignment = useAPIFetch( replaceURLParams(routes.assignment, { assignment_id: assignmentId }), ); - const autoGradingEnabled = !!assignment.data?.auto_grading_config; + const isAutoGradingAssignment = !!assignment.data?.auto_grading_config; const segments = useMemo((): DashboardActivityFiltersProps['segments'] => { const { data } = assignment; if ( !data || // Display the segments filter only for auto-grading assignments, or // assignments where the feature was explicitly enabled - (!assignment_segments_filter_enabled && !autoGradingEnabled) + (!assignment_segments_filter_enabled && !isAutoGradingAssignment) ) { return undefined; } @@ -85,7 +90,7 @@ export default function AssignmentActivity() { }, [ assignment, assignment_segments_filter_enabled, - autoGradingEnabled, + isAutoGradingAssignment, segmentIds, updateFilters, ]); @@ -99,14 +104,24 @@ export default function AssignmentActivity() { org_public_id: organizationPublicId, }, ); + const studentsToSync = useMemo(() => { + if (!isAutoGradingAssignment || !students.data) { + return undefined; + } + // TODO Filter out students whose grades didn't change + return students.data.students.map( + ({ h_userid, auto_grading_grade = 0 }) => ({ + h_userid, + grade: auto_grading_grade, + }), + ); + }, [isAutoGradingAssignment, students.data]); const rows: StudentsTableRow[] = useMemo( () => (students.data?.students ?? []).map( - ({ lms_id, display_name, auto_grading_grade, annotation_metrics }) => ({ - lms_id, - display_name, - auto_grading_grade, + ({ annotation_metrics, ...rest }) => ({ + ...rest, ...annotation_metrics, }), ), @@ -137,7 +152,7 @@ export default function AssignmentActivity() { }, ]; - if (autoGradingEnabled) { + if (isAutoGradingAssignment) { firstColumns.push({ field: 'auto_grading_grade', label: 'Grade', @@ -145,7 +160,7 @@ export default function AssignmentActivity() { } return [...firstColumns, ...lastColumns]; - }, [autoGradingEnabled]); + }, [isAutoGradingAssignment]); const title = assignment.data?.title ?? 'Untitled assignment'; useDocumentTitle(title); @@ -175,42 +190,47 @@ export default function AssignmentActivity() { {assignment.data && title} - {assignment.data && ( - - navigate( - urlWithFilters( - { studentIds, assignmentIds: [assignmentId] }, - { path: '' }, +
+ {assignment.data && ( + + navigate( + urlWithFilters( + { studentIds, assignmentIds: [assignmentId] }, + { path: '' }, + ), ), - ), - }} - assignments={{ - activeItem: assignment.data, - // When active assignment is cleared, navigate to its course page, - // but keep other query params intact - onClear: () => { - const query = search.length === 0 ? '' : `?${search}`; - navigate(`${courseURL(assignment.data!.course.id)}${query}`); - }, - }} - students={{ - selectedIds: studentIds, - onChange: studentIds => updateFilters({ studentIds }), - }} - segments={segments} - onClearSelection={ - studentIds.length > 0 || - (segments && segments.selectedIds.length > 0) - ? () => updateFilters({ studentIds: [], segmentIds: [] }) - : undefined - } - /> - )} + }} + assignments={{ + activeItem: assignment.data, + // When active assignment is cleared, navigate to its course page, + // but keep other query params intact + onClear: () => { + const query = search.length === 0 ? '' : `?${search}`; + navigate(`${courseURL(assignment.data!.course.id)}${query}`); + }, + }} + students={{ + selectedIds: studentIds, + onChange: studentIds => updateFilters({ studentIds }), + }} + segments={segments} + onClearSelection={ + studentIds.length > 0 || + (segments && segments.selectedIds.length > 0) + ? () => updateFilters({ studentIds: [], segmentIds: [] }) + : undefined + } + /> + )} + {isAutoGradingAssignment && auto_grading_sync_enabled && ( + + )} +
; +}; + +export default function SyncGradesButton({ + studentsToSync, +}: SyncGradesButtonProps) { + const { assignmentId } = useParams<{ assignmentId: string }>(); + const { dashboard, api } = useConfig(['dashboard', 'api']); + const { routes } = dashboard; + const [schedulingSyncFailed, setSchedulingSyncFailed] = useState(false); + + const syncURL = useMemo( + () => + replaceURLParams(routes.assignment_grades_sync, { + assignment_id: assignmentId, + }), + [assignmentId, routes.assignment_grades_sync], + ); + const [lastSyncParams, setLastSyncParams] = useState({}); + const lastSync = usePolledAPIFetch({ + path: syncURL, + params: lastSyncParams, + // Keep polling as long as sync is in progress + shouldRefresh: result => + !result.data || ['scheduled', 'in_progress'].includes(result.data.status), + }); + + const buttonContent = useMemo(() => { + if (!studentsToSync || (lastSync.isLoading && !lastSync.data)) { + return 'Loading...'; + } + + if ( + lastSync.data && + ['scheduled', 'in_progress'].includes(lastSync.data.status) + ) { + return ( + <> + Syncing grades + + + ); + } + + // TODO Maybe these should be represented differently + if ( + schedulingSyncFailed || + lastSync.error || + lastSync.data?.status === 'failed' + ) { + return ( + <> + Error syncing. Click to retry + + + ); + } + + if (studentsToSync.length > 0) { + return `Sync ${studentsToSync.length} grades`; + } + + return 'Grades synced'; + }, [ + studentsToSync, + lastSync.isLoading, + lastSync.data, + lastSync.error, + schedulingSyncFailed, + ]); + + const buttonDisabled = + lastSync.isLoading || + lastSync.data?.status === 'scheduled' || + lastSync.data?.status === 'in_progress' || + !studentsToSync || + studentsToSync.length === 0; + + const syncGrades = useCallback(async () => { + lastSync.mutate({ status: 'scheduled' }); + setSchedulingSyncFailed(false); + + await apiCall({ + authToken: api.authToken, + path: syncURL, + method: 'POST', + data: { + grades: (studentsToSync ?? []).map(({ grade, h_userid }) => ({ + h_userid, + // FIXME This will be fixed separately, but the BE is currently + // returning grades from 0 to 100, but expects them to be sent back + // from 0 to 1. + grade: grade / 100, + })), + }, + }).catch(() => setSchedulingSyncFailed(true)); + + // Once the request succeeds, we update the params so that polling the + // status is triggered again + setLastSyncParams({ t: `${Date.now()}` }); + }, [api.authToken, lastSync, studentsToSync, syncURL]); + + return ( + + ); +} diff --git a/lms/static/scripts/frontend_apps/components/dashboard/test/AssignmentActivity-test.js b/lms/static/scripts/frontend_apps/components/dashboard/test/AssignmentActivity-test.js index 428ecceee4..bcaed50e52 100644 --- a/lms/static/scripts/frontend_apps/components/dashboard/test/AssignmentActivity-test.js +++ b/lms/static/scripts/frontend_apps/components/dashboard/test/AssignmentActivity-test.js @@ -11,7 +11,7 @@ import { formatDateTime } from '../../../utils/date'; import AssignmentActivity, { $imports } from '../AssignmentActivity'; describe('AssignmentActivity', () => { - const students = [ + const activeStudents = [ { display_name: 'b', annotation_metrics: { @@ -52,10 +52,13 @@ describe('AssignmentActivity', () => { let fakeConfig; let wrappers; - function setUpFakeUseAPIFetch(assignment = activeAssignment) { + function setUpFakeUseAPIFetch( + assignment = activeAssignment, + students = { students: activeStudents }, + ) { fakeUseAPIFetch.callsFake(url => ({ isLoading: false, - data: url.endsWith('metrics') ? { students } : assignment, + data: url.endsWith('metrics') ? students : assignment, })); } @@ -77,6 +80,7 @@ describe('AssignmentActivity', () => { assignment: '/api/assignments/:assignment_id', students_metrics: '/api/students/metrics', }, + auto_grading_sync_enabled: true, assignment_segments_filter_enabled: false, }, }; @@ -422,6 +426,89 @@ describe('AssignmentActivity', () => { }); }, ); + + [ + { + syncEnabled: true, + isAutoGradingAssignment: true, + shouldShowButton: true, + }, + { + syncEnabled: false, + isAutoGradingAssignment: true, + shouldShowButton: false, + }, + { + syncEnabled: true, + isAutoGradingAssignment: false, + shouldShowButton: false, + }, + { + syncEnabled: false, + isAutoGradingAssignment: false, + shouldShowButton: false, + }, + ].forEach(({ isAutoGradingAssignment, syncEnabled, shouldShowButton }) => { + it('shows sync button when both sync and auto-grading are enabled', () => { + setUpFakeUseAPIFetch({ + ...activeAssignment, + auto_grading_config: isAutoGradingAssignment ? {} : null, + }); + fakeConfig.dashboard.auto_grading_sync_enabled = syncEnabled; + + const wrapper = createComponent(); + + assert.equal(wrapper.exists('SyncGradesButton'), shouldShowButton); + }); + }); + + [ + { studentsData: null, expectedStudentsToSync: undefined }, + { studentsData: { students: [] }, expectedStudentsToSync: [] }, + { + studentsData: { + students: [ + { + display_name: 'a', + h_userid: 'foo', + auto_grading_grade: 0.5, + }, + { + display_name: 'b', + h_userid: 'bar', + auto_grading_grade: 0.87, + }, + { + display_name: 'c', + h_userid: 'baz', + }, + ], + }, + expectedStudentsToSync: [ + { h_userid: 'foo', grade: 0.5 }, + { h_userid: 'bar', grade: 0.87 }, + { h_userid: 'baz', grade: 0 }, + ], + }, + ].forEach(({ studentsData, expectedStudentsToSync }) => { + it('resolves the right list of students to sync', () => { + setUpFakeUseAPIFetch( + { + ...activeAssignment, + auto_grading_config: {}, + }, + studentsData, + ); + fakeConfig.dashboard.auto_grading_sync_enabled = true; + + const wrapper = createComponent(); + + assert.deepEqual( + wrapper.find('SyncGradesButton').prop('studentsToSync'), + expectedStudentsToSync, + ); + }); + }); }); context('when assignment has segments', () => { diff --git a/lms/static/scripts/frontend_apps/components/dashboard/test/SyncGradesButton-test.js b/lms/static/scripts/frontend_apps/components/dashboard/test/SyncGradesButton-test.js new file mode 100644 index 0000000000..887b7ecc00 --- /dev/null +++ b/lms/static/scripts/frontend_apps/components/dashboard/test/SyncGradesButton-test.js @@ -0,0 +1,225 @@ +import { + checkAccessibility, + mockImportedComponents, +} from '@hypothesis/frontend-testing'; +import { mount } from 'enzyme'; +import { act } from 'preact/test-utils'; +import sinon from 'sinon'; + +import { Config } from '../../../config'; +import SyncGradesButton, { $imports } from '../SyncGradesButton'; + +describe('SyncGradesButton', () => { + let fakeConfig; + let fakeApiCall; + let fakeUsePolledAPIFetch; + let shouldRefreshCallback; + + const studentsToSync = [ + { h_userid: '123', grade: 50 }, + { h_userid: '456', grade: 20 }, + ]; + + beforeEach(() => { + fakeApiCall = sinon.stub().resolves(undefined); + fakeUsePolledAPIFetch = sinon.stub().callsFake(({ shouldRefresh }) => { + // "Collect" shouldRefresh callback so that we can test its behavior + // individually + shouldRefreshCallback = shouldRefresh; + + return { + data: null, + isLoading: true, + }; + }); + + fakeConfig = { + dashboard: { + routes: { + assignment_grades_sync: '/api/assignments/:assignment_id/grades/sync', + }, + }, + api: { authToken: 'authToken' }, + }; + + $imports.$mock(mockImportedComponents()); + $imports.$mock({ + '../../utils/api': { + apiCall: fakeApiCall, + usePolledAPIFetch: fakeUsePolledAPIFetch, + }, + 'wouter-preact': { + useParams: sinon.stub().returns({ assignmentId: '123' }), + }, + }); + }); + + afterEach(() => { + $imports.$restore(); + }); + + function createComponent(studentsToSync) { + return mount( + + + , + ); + } + + function buttonText(wrapper) { + return wrapper.find('Button').text(); + } + + function isButtonDisabled(wrapper) { + return wrapper.find('Button').prop('disabled'); + } + + [ + { + fetchResult: { data: null }, + expectedResult: true, + }, + { + fetchResult: { + data: { status: 'scheduled' }, + }, + expectedResult: true, + }, + { + fetchResult: { + data: { status: 'in_progress' }, + }, + expectedResult: true, + }, + { + fetchResult: { + data: { status: 'finished' }, + }, + expectedResult: false, + }, + ].forEach(({ fetchResult, expectedResult }) => { + it('shouldRefresh callback behaves as expected', () => { + createComponent(); + assert.equal(shouldRefreshCallback(fetchResult), expectedResult); + }); + }); + + [undefined, studentsToSync].forEach(studentsToSync => { + it('shows loading text when getting initial data', () => { + const wrapper = createComponent(studentsToSync); + + assert.equal(buttonText(wrapper), 'Loading...'); + assert.isTrue(isButtonDisabled(wrapper)); + }); + }); + + ['scheduled', 'in_progress'].forEach(status => { + it('shows syncing text when grades are being synced', () => { + fakeUsePolledAPIFetch.returns({ + isLoading: false, + data: { status }, + }); + + const wrapper = createComponent(studentsToSync); + + assert.equal(buttonText(wrapper), 'Syncing grades'); + assert.isTrue(isButtonDisabled(wrapper)); + }); + }); + + [ + { error: new Error() }, + { + data: { status: 'failed' }, + }, + ].forEach(fetchResult => { + it('shows syncing errors and allows to retry', () => { + fakeUsePolledAPIFetch.returns({ + isLoading: false, + ...fetchResult, + }); + + const wrapper = createComponent(studentsToSync); + + assert.equal(buttonText(wrapper), 'Error syncing. Click to retry'); + assert.isFalse(isButtonDisabled(wrapper)); + }); + }); + + [ + { students: studentsToSync, expectedAmount: studentsToSync.length }, + { + students: [...studentsToSync, ...studentsToSync], + expectedAmount: studentsToSync.length * 2, + }, + ].forEach(({ students, expectedAmount }) => { + it('shows the amount of students to be synced when current status is "finished"', () => { + fakeUsePolledAPIFetch.returns({ + isLoading: false, + data: { status: 'finished' }, + }); + + const wrapper = createComponent(students); + + assert.equal(buttonText(wrapper), `Sync ${expectedAmount} grades`); + assert.isFalse(isButtonDisabled(wrapper)); + }); + }); + + it('shows grades synced when no students need to be synced', () => { + fakeUsePolledAPIFetch.returns({ + isLoading: false, + data: { status: 'finished' }, + }); + + const wrapper = createComponent([]); + + assert.equal(buttonText(wrapper), 'Grades synced'); + assert.isTrue(isButtonDisabled(wrapper)); + }); + + it('submits grades when the button is clicked, then triggers sync status polling', async () => { + const mutate = sinon.stub(); + fakeUsePolledAPIFetch.returns({ + isLoading: false, + data: { status: 'finished' }, + mutate, + }); + + const wrapper = createComponent(studentsToSync); + await act(() => wrapper.find('Button').props().onClick()); + + assert.calledWith(mutate, { status: 'scheduled' }); + assert.called(fakeApiCall); + assert.deepEqual( + { + grades: [ + { h_userid: '123', grade: 0.5 }, + { h_userid: '456', grade: 0.2 }, + ], + }, + fakeApiCall.lastCall.args[0].data, + ); + }); + + it('sets status to error when scheduling a sync fails', async () => { + fakeUsePolledAPIFetch.returns({ + isLoading: false, + data: { status: 'finished' }, + mutate: sinon.stub(), + }); + fakeApiCall.rejects(new Error('Error scheduling')); + + const wrapper = createComponent(studentsToSync); + await act(() => wrapper.find('Button').props().onClick()); + + assert.equal(buttonText(wrapper), 'Error syncing. Click to retry'); + }); + + it( + 'should pass a11y checks', + checkAccessibility({ + content: () => createComponent(), + }), + ); +}); diff --git a/lms/static/scripts/frontend_apps/config.ts b/lms/static/scripts/frontend_apps/config.ts index e5e88a2151..e56847b9d4 100644 --- a/lms/static/scripts/frontend_apps/config.ts +++ b/lms/static/scripts/frontend_apps/config.ts @@ -269,6 +269,9 @@ export type DashboardRoutes = { assignments: string; /** Fetch list of students */ students: string; + + /** Sync grades (POST) or check sync status (GET) */ + assignment_grades_sync: string; }; export type DashboardUser = {