diff --git a/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx b/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx index ffc59c3e2a..c9d1bcb7ce 100644 --- a/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx +++ b/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx @@ -1,12 +1,22 @@ import userEvent from '@testing-library/user-event'; -import { render, screen, waitFor } from 'test-utils'; +import { render, screen, waitFor, within } from 'test-utils'; +import type { StudentData } from 'types/course/gradebook'; +import TestApp from 'utilities/TestApp'; import CourseAPI from 'api/course'; +import toast from 'lib/hooks/toast'; +import ExternalGradeConflictPrompt from '../components/import/ExternalGradeConflictPrompt'; import ImportExternalAssessmentsWizard from '../components/import/ImportExternalAssessmentsWizard'; jest.mock('api/course'); jest.mock('lib/components/wrappers/I18nProvider'); +jest.mock('lib/hooks/toast'); + +const EXTERNAL_ID = 'External ID'; +const MIDTERM = 'Midterm'; +const MIDTERMS = 'Midterms'; +const A001 = 'A001'; const defaultProps = { existingAssessments: [], @@ -14,52 +24,123 @@ const defaultProps = { weightedViewEnabled: true, }; +const componentNameInput = (): HTMLElement => + screen.getByRole('textbox', { name: 'Component name' }); + +const file = (text: string): File => + new File([text], 'marks.csv', { type: 'text/csv' }); + +const student = (partial: Partial): StudentData => ({ + id: 1, + name: 'Student', + email: 's@x.com', + externalId: 'E1', + level: 0, + totalXp: 0, + ...partial, +}); + +const studentsState = (students: StudentData[]): object => ({ + gradebook: { + categories: [], + tabs: [], + submissions: [], + assessments: [], + gamificationEnabled: false, + weightedViewEnabled: false, + canManageWeights: true, + students, + }, +}); + +// Render against an isolated store (not the shared singleton, which a commit +// test can leave without a students slice and crash getStudents()). const renderWizard = ( props: Partial<{ weightedViewEnabled: boolean }> = {}, ): void => { render( , + { state: studentsState([]) }, ); }; const advanceToVerifyStep = async (): Promise => { - await userEvent.type(componentNameInput(), 'Midterms'); + await userEvent.type(componentNameInput(), MIDTERM); await userEvent.click(screen.getByRole('button', { name: /next/i })); await userEvent.upload( screen.getByLabelText(/upload/i), - file('Identifier,Midterms\nA001,105\n'), + file(`${EXTERNAL_ID},${MIDTERM}\n${A001},41\n`), ); await userEvent.click(screen.getByRole('button', { name: /verify/i })); await screen.findByRole('button', { name: /confirm import/i }); }; -const studentsState = (students: object[]): object => ({ - gradebook: { - categories: [], - tabs: [], - submissions: [], - assessments: [], - gamificationEnabled: false, - weightedViewEnabled: false, - canManageWeights: true, - students, - }, -}); - -const componentNameInput = (): HTMLElement => - screen.getByRole('textbox', { name: 'Component name' }); +const advanceToVerifyFailure = async (csv: string): Promise => { + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload(screen.getByLabelText(/upload/i), file(csv)); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); +}; const fillOneComponent = async (): Promise => { - await userEvent.type(componentNameInput(), 'Midterm'); + await userEvent.type(componentNameInput(), MIDTERM); }; -const file = (text: string): File => - new File([text], 'marks.csv', { type: 'text/csv' }); +describe('dialog dismissal guards', () => { + it('does not close the wizard on backdrop click or escape', async () => { + const onClose = jest.fn(); + render( + , + ); + + const backdrop = document.querySelector('.MuiBackdrop-root'); + expect(backdrop).not.toBeNull(); + await userEvent.click(backdrop as Element); + expect(onClose).not.toHaveBeenCalled(); + + await userEvent.keyboard('{Escape}'); + expect(onClose).not.toHaveBeenCalled(); + + // The explicit Cancel button still closes it + await userEvent.click(screen.getByRole('button', { name: /cancel/i })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('does not close the conflict prompt on backdrop click', async () => { + const onCancel = jest.fn(); + render( + , + ); + + const backdrop = document.querySelector('.MuiBackdrop-root'); + expect(backdrop).not.toBeNull(); + await userEvent.click(backdrop as Element); + expect(onCancel).not.toHaveBeenCalled(); + + await userEvent.click(screen.getByRole('button', { name: /go back/i })); + expect(onCancel).toHaveBeenCalledTimes(1); + }); +}); describe('ImportExternalAssessmentsWizard', () => { beforeEach(() => jest.clearAllMocks()); @@ -71,8 +152,11 @@ describe('ImportExternalAssessmentsWizard', () => { unresolved: [], malformed: [], outOfRange: [], - sample: [{ studentName: 'Alice', grades: { Midterm: 41 } }], - conflicts: [], + sample: [{ identifier: A001, grades: { Midterm: 41 } }], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, }, }); (CourseAPI.gradebook.importCommit as jest.Mock).mockResolvedValue({ @@ -93,7 +177,7 @@ describe('ImportExternalAssessmentsWizard', () => { renderWizard(); // Step 1: type a component - await userEvent.type(componentNameInput(), 'Midterm'); + await userEvent.type(componentNameInput(), MIDTERM); await userEvent.type(screen.getByLabelText(/weightage/i), '30'); await userEvent.type(screen.getByLabelText(/max marks/i), '50'); await userEvent.click(screen.getByRole('button', { name: /next/i })); @@ -101,18 +185,15 @@ describe('ImportExternalAssessmentsWizard', () => { // Step 2: upload await userEvent.upload( screen.getByLabelText(/upload/i), - file('Identifier,Midterm\nA001,41\n'), + file(`${EXTERNAL_ID},${MIDTERM}\nA001,41\n`), ); await userEvent.click(screen.getByRole('button', { name: /verify/i })); // Step 3: preview shows the sample - expect(await screen.findByText('Alice')).toBeVisible(); + expect(await screen.findByText(A001)).toBeVisible(); expect( - screen.getByRole('columnheader', { name: 'External ID' }), + screen.getByRole('columnheader', { name: EXTERNAL_ID }), ).toBeInTheDocument(); - expect( - screen.queryByRole('columnheader', { name: 'Component name' }), - ).not.toBeInTheDocument(); await userEvent.click( screen.getByRole('button', { name: /continue|confirm/i }), ); @@ -125,10 +206,86 @@ describe('ImportExternalAssessmentsWizard', () => { expect(payload.onConflict).toBe('replace'); expect(payload.identifierMode).toBe('external_id'); expect(payload.components[0]).toMatchObject({ - name: 'Midterm', + name: MIDTERM, weightage: 30, maximumGrade: 50, }); + + // success side-effects + await waitFor(() => + expect(toast.success).toHaveBeenCalledWith('Import complete.'), + ); + }); + + it('uses singular copy for a single unresolved external ID', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: false, + unresolved: ['ZZZ'], + malformed: [], + outOfRange: [], + sample: [], + conflictRows: [], + reassignments: [], + }, + }); + renderWizard(); + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\nZZZ,1\n`); + expect( + await screen.findByText( + /This external ID was not found in the course: ZZZ/, + ), + ).toBeInTheDocument(); + }); + + it('uses plural copy for multiple unresolved external IDs', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: false, + unresolved: ['ZZZ', 'YYY'], + malformed: [], + outOfRange: [], + sample: [], + conflictRows: [], + reassignments: [], + }, + }); + renderWizard(); + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\nZZZ,1\nYYY,2\n`); + expect( + await screen.findByText( + /These external IDs were not found in the course: ZZZ, YYY/, + ), + ).toBeInTheDocument(); + }); + + it('lists up to five malformed cells then summarises the rest', async () => { + const malformed = [ + 'row 2, Midterm: a', + 'row 3, Midterm: b', + 'row 4, Midterm: c', + 'row 5, Midterm: d', + 'row 6, Midterm: e', + 'row 7, Midterm: f', + 'row 8, Midterm: g', + ]; + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: false, + unresolved: [], + malformed, + outOfRange: [], + sample: [], + conflictRows: [], + reassignments: [], + }, + }); + renderWizard(); + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\n${A001},a\n`); + expect(await screen.findByText('row 2, Midterm: a')).toBeInTheDocument(); + expect(screen.getByText('row 6, Midterm: e')).toBeInTheDocument(); + expect(screen.queryByText('row 7, Midterm: f')).not.toBeInTheDocument(); + expect(screen.getByText('and 2 more')).toBeInTheDocument(); }); it('shows unresolved identifiers and stays on the upload step', async () => { @@ -139,19 +296,13 @@ describe('ImportExternalAssessmentsWizard', () => { malformed: [], outOfRange: [], sample: [], - conflicts: [], + conflictRows: [], + reassignments: [], }, }); renderWizard(); - await userEvent.type(componentNameInput(), 'Midterm'); - await userEvent.type(screen.getByLabelText(/weightage/i), '30'); - await userEvent.type(screen.getByLabelText(/max marks/i), '50'); - await userEvent.click(screen.getByRole('button', { name: /next/i })); - await userEvent.upload( - screen.getByLabelText(/upload/i), - file('Identifier,Midterm\nZZZ,1\n'), - ); - await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\nZZZ,1\n`); expect(await screen.findByText(/ZZZ/)).toBeVisible(); expect(CourseAPI.gradebook.importCommit).not.toHaveBeenCalled(); }); @@ -165,7 +316,7 @@ describe('ImportExternalAssessmentsWizard', () => { weightedViewEnabled={false} />, ); - await userEvent.type(componentNameInput(), 'Midterm'); + await userEvent.type(componentNameInput(), MIDTERM); expect(screen.queryByLabelText(/weightage/i)).not.toBeInTheDocument(); expect(screen.getByLabelText(/max marks/i)).toBeInTheDocument(); }); @@ -185,7 +336,7 @@ describe('ImportExternalAssessmentsWizard', () => { />, ); expect( - screen.getByRole('radio', { name: 'External ID' }), + screen.getByRole('radio', { name: EXTERNAL_ID }), ).toBeInTheDocument(); expect( screen.queryByRole('radio', { name: 'Student ID' }), @@ -196,7 +347,7 @@ describe('ImportExternalAssessmentsWizard', () => { renderWizard(); expect(componentNameInput()).toBeInTheDocument(); expect( - screen.queryByRole('textbox', { name: 'External ID' }), + screen.queryByRole('textbox', { name: EXTERNAL_ID }), ).not.toBeInTheDocument(); await userEvent.click(screen.getByRole('radio', { name: 'Email' })); @@ -214,16 +365,17 @@ describe('ImportExternalAssessmentsWizard', () => { unresolved: [], malformed: [], outOfRange: [], - sample: [{ studentName: 'Alice', grades: { Midterm: 20 } }], - conflicts: [ + sample: [{ identifier: A001, grades: { Midterm: 20 } }], + conflictRows: [ { - component: 'Midterm', + identifier: A001, studentName: 'Alice', - existingGrade: 10, - inFileGrade: 20, - identifierMismatch: false, + cells: { Midterm: { existing: 10, inFile: 20, changed: true } }, }, ], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, }, }); (CourseAPI.gradebook.importCommit as jest.Mock).mockResolvedValue({ @@ -242,18 +394,25 @@ describe('ImportExternalAssessmentsWizard', () => { }, }); renderWizard(); - await userEvent.type(componentNameInput(), 'Midterm'); + await userEvent.type(componentNameInput(), MIDTERM); await userEvent.type(screen.getByLabelText(/weightage/i), '30'); await userEvent.type(screen.getByLabelText(/max marks/i), '50'); await userEvent.click(screen.getByRole('button', { name: /next/i })); await userEvent.upload( screen.getByLabelText(/upload/i), - file('Identifier,Midterm\nA001,20\n'), + file(`${EXTERNAL_ID},${MIDTERM}\n${A001},20\n`), ); await userEvent.click(screen.getByRole('button', { name: /verify/i })); await userEvent.click( await screen.findByRole('button', { name: /confirm import/i }), ); + expect( + await screen.findByText(/1 of 1 rows have changes/i), + ).toBeInTheDocument(); + // The struck old value (10) is unique to the matrix; the new value (20) + // also appears in the Verify sample table behind the dialog, so assert + // only the header + old value to avoid a multiple-match error. + expect(screen.getByText('10')).toBeInTheDocument(); // struck old value await userEvent.click( await screen.findByRole('button', { name: /keep existing/i }), ); @@ -268,22 +427,8 @@ describe('ImportExternalAssessmentsWizard', () => { it('blocks Next in External ID mode while a student has no External ID', async () => { render(, { state: studentsState([ - { - id: 1, - name: 'Alice Lim', - email: 'a@x.com', - externalId: null, - level: 0, - totalXp: 0, - }, - { - id: 2, - name: 'Bob Tan', - email: 'b@x.com', - externalId: 'E2', - level: 0, - totalXp: 0, - }, + student({ id: 1, name: 'Alice Lim', externalId: null }), + student({ id: 2, name: 'Bob Tan', externalId: 'E2' }), ]), }); await fillOneComponent(); @@ -296,14 +441,7 @@ describe('ImportExternalAssessmentsWizard', () => { it('enables Next once matching by Email instead', async () => { render(, { state: studentsState([ - { - id: 1, - name: 'Alice Lim', - email: 'a@x.com', - externalId: null, - level: 0, - totalXp: 0, - }, + student({ id: 1, name: 'Alice Lim', externalId: null }), ]), }); await fillOneComponent(); @@ -314,17 +452,10 @@ describe('ImportExternalAssessmentsWizard', () => { it('lists the exact required headers on the upload step', async () => { render(, { state: studentsState([ - { - id: 1, - name: 'Alice', - email: 'a@x.com', - externalId: 'E1', - level: 0, - totalXp: 0, - }, + student({ id: 1, name: 'Alice', externalId: 'E1' }), ]), }); - await userEvent.type(componentNameInput(), 'Midterm'); + await userEvent.type(componentNameInput(), MIDTERM); await userEvent.click(screen.getByRole('button', { name: 'Next' })); expect( screen.getByText( @@ -336,12 +467,15 @@ describe('ImportExternalAssessmentsWizard', () => { it('summarises the count when several students lack an External ID', async () => { render(, { state: studentsState([ - { id: 1, name: 'Alice Lim', email: 'a@x.com', externalId: null, level: 0, totalXp: 0 }, - { id: 2, name: 'Bob Tan', email: 'b@x.com', externalId: '', level: 0, totalXp: 0 }, + student({ id: 1, name: 'Alice Lim', externalId: null }), + student({ id: 2, name: 'Bob Tan', externalId: '' }), + student({ id: 3, name: 'Carol Low', externalId: '' }), ]), }); await fillOneComponent(); - expect(screen.getByText(/2 students have no External ID, including Alice Lim/)).toBeInTheDocument(); + expect( + screen.getByText(/Alice Lim and 2 other students have no External ID/), + ).toBeInTheDocument(); }); it('does not show Confirm import button when preview has errors', async () => { @@ -352,19 +486,12 @@ describe('ImportExternalAssessmentsWizard', () => { malformed: [], outOfRange: [], sample: [], - conflicts: [], + conflictRows: [], + reassignments: [], }, }); renderWizard(); - await userEvent.type(componentNameInput(), 'Midterm'); - await userEvent.type(screen.getByLabelText(/weightage/i), '30'); - await userEvent.type(screen.getByLabelText(/max marks/i), '50'); - await userEvent.click(screen.getByRole('button', { name: /next/i })); - await userEvent.upload( - screen.getByLabelText(/upload/i), - file('Identifier,Midterm\nZZZ,1\n'), - ); - await userEvent.click(screen.getByRole('button', { name: /verify/i })); + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\nZZZ,1\n`); await screen.findByText(/ZZZ/); expect( screen.queryByRole('button', { name: /confirm import/i }), @@ -378,16 +505,17 @@ describe('ImportExternalAssessmentsWizard', () => { unresolved: [], malformed: [], outOfRange: [], - sample: [{ studentName: 'Alice', grades: { Midterm: 20 } }], - conflicts: [ + sample: [{ identifier: A001, grades: { Midterm: 20 } }], + conflictRows: [ { - component: 'Midterm', + identifier: A001, studentName: 'Alice', - existingGrade: 10, - inFileGrade: 20, - identifierMismatch: false, + cells: { Midterm: { existing: 10, inFile: 20, changed: true } }, }, ], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, }, }); (CourseAPI.gradebook.importCommit as jest.Mock).mockResolvedValue({ @@ -408,19 +536,19 @@ describe('ImportExternalAssessmentsWizard', () => { render( , ); - // Step 1: 'Midterm' matches existing → max/weightage locked; just Next. - await userEvent.type(componentNameInput(), 'Midterm'); + // Step 1: MIDTERM matches existing → max/weightage locked; just Next. + await userEvent.type(componentNameInput(), MIDTERM); await userEvent.click(screen.getByRole('button', { name: /next/i })); await userEvent.upload( screen.getByLabelText(/upload/i), - file('Identifier,Midterm\nA001,20\n'), + file(`${EXTERNAL_ID},${MIDTERM}\n${A001},20\n`), ); await userEvent.click(screen.getByRole('button', { name: /verify/i })); await userEvent.click( @@ -442,7 +570,7 @@ describe('ImportExternalAssessmentsWizard', () => { render( { weightedViewEnabled />, ); - expect(screen.getByRole('button', { name: 'Midterm' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: MIDTERM })).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Finals' })).toBeInTheDocument(); }); @@ -458,17 +586,17 @@ describe('ImportExternalAssessmentsWizard', () => { render( , ); - await userEvent.click(screen.getByRole('button', { name: 'Midterm' })); + await userEvent.click(screen.getByRole('button', { name: MIDTERM })); // The chip-inserted row's name field is read-only (disabled input) - const nameInput = screen.getByDisplayValue('Midterm'); + const nameInput = screen.getByDisplayValue(MIDTERM); expect(nameInput).toBeDisabled(); // Max and weight are pre-filled with the existing values @@ -483,17 +611,17 @@ describe('ImportExternalAssessmentsWizard', () => { render( , ); - await userEvent.click(screen.getByRole('button', { name: 'Midterm' })); + await userEvent.click(screen.getByRole('button', { name: MIDTERM })); // After clicking, chip disappears (already in the list) expect( - screen.queryByRole('button', { name: 'Midterm' }), + screen.queryByRole('button', { name: MIDTERM }), ).not.toBeInTheDocument(); }); @@ -509,22 +637,35 @@ describe('ImportExternalAssessmentsWizard', () => { expect(screen.queryByText(/from existing/i)).not.toBeInTheDocument(); }); - it('warns about out-of-range grades at Verify without blocking import (Q2)', async () => { + it('warns about out-of-range grades at Verify without blocking import', async () => { (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ data: { ok: true, unresolved: [], malformed: [], - outOfRange: ['S1 — Midterms: 105 (exceeds 100)'], - sample: [{ studentName: 'S1', grades: { Midterms: 105 } }], - conflicts: [], + outOfRange: [ + { + identifier: 'S1', + component: MIDTERM, + grade: 105, + max: 100, + kind: 'above', + }, + ], + sample: [{ identifier: 'S1', grades: { Midterm: 105 } }], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, }, }); renderWizard({ weightedViewEnabled: true }); await advanceToVerifyStep(); - expect(screen.getByText(/105 \(exceeds 100\)/)).toBeInTheDocument(); expect( - screen.getByText(/capped or floored in the weighted total/i), + screen.getByText(/S1 - Midterm: 105 \(max 100\)/), + ).toBeInTheDocument(); + expect( + screen.getByText(/floored or capped in the weighted total/i), ).toBeInTheDocument(); // non-blocking: Confirm import still enabled expect( @@ -532,20 +673,1005 @@ describe('ImportExternalAssessmentsWizard', () => { ).toBeEnabled(); }); - it('omits the weighted-total wording when weighted view is off (Q2/Q5)', async () => { + it('omits the weighted-total wording when weighted view is off', async () => { (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ data: { ok: true, unresolved: [], malformed: [], - outOfRange: ['S1 — Midterms: 105 (exceeds 100)'], - sample: [{ studentName: 'S1', grades: { Midterms: 105 } }], - conflicts: [], + outOfRange: [ + { + identifier: 'S1', + component: MIDTERM, + grade: 105, + max: 100, + kind: 'above', + }, + ], + sample: [{ identifier: 'S1', grades: { Midterm: 105 } }], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, }, }); renderWizard({ weightedViewEnabled: false }); await advanceToVerifyStep(); - expect(screen.getByText(/105 \(exceeds 100\)/)).toBeInTheDocument(); + expect( + screen.getByText(/S1 - Midterm: 105 \(max 100\)/), + ).toBeInTheDocument(); expect(screen.queryByText(/weighted total/i)).not.toBeInTheDocument(); }); + + it('shows a "did you mean" suggestion for a renamed column, not raw header dumps', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { + message: 'bad_header', + missing: [], + unrecognized: [], + suggestions: [{ expected: MIDTERMS, didYouMean: MIDTERM }], + duplicates: [], + }, + }, + }, + }); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERMS); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},${MIDTERM}\nA001,41\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + // Actionable detail is shown (not the generic "Could not verify" toast) + expect( + await screen.findByText(/did you mean ['‘]Midterms['’]\?/i), + ).toBeInTheDocument(); + // The old eye-diff "Found: …" / "do not match" dump is gone + expect(screen.queryByText(/Found:/)).not.toBeInTheDocument(); + expect(screen.queryByText(/do not match/i)).not.toBeInTheDocument(); + // Stays on the upload step — no preview/confirm advance + expect( + screen.queryByRole('button', { name: /confirm import/i }), + ).not.toBeInTheDocument(); + }); + + it('shows only the duplicate-header line when duplicates are the only problem', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { + message: 'bad_header', + missing: [], + unrecognized: [], + suggestions: [], + duplicates: [{ name: MIDTERM, count: 2 }], + }, + }, + }, + }); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},${MIDTERM},${MIDTERM}\nA001,1,2\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + expect( + await screen.findByText(/appears more than once:.*Midterm \(×2\)/i), + ).toBeInTheDocument(); + // No bogus missing/unrecognized lines when every column is present + expect(screen.queryByText(/is missing/i)).not.toBeInTheDocument(); + expect(screen.queryByText(/recognized/i)).not.toBeInTheDocument(); + }); + + it('uses singular copy for a single missing / unrecognized / duplicate column', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { + message: 'bad_header', + missing: [MIDTERM], + unrecognized: ['Wrong'], + suggestions: [], + duplicates: [{ name: 'Quiz', count: 2 }], + }, + }, + }, + }); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},Wrong\nA001,41\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + expect( + await screen.findByText(/is missing this column:.*Midterm\b/i), + ).toBeInTheDocument(); + expect( + screen.getByText(/This column isn['’]t recognized:.*Wrong/i), + ).toBeInTheDocument(); + expect( + screen.getByText(/This column appears more than once:.*Quiz \(×2\)/i), + ).toBeInTheDocument(); + }); + + it('uses plural copy for multiple missing / unrecognized / duplicate columns', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { + message: 'bad_header', + missing: [MIDTERM, 'Final Exam'], + unrecognized: ['Wrong', 'Extra'], + suggestions: [], + duplicates: [ + { name: 'Quiz', count: 2 }, + { name: 'Project', count: 3 }, + ], + }, + }, + }, + }); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},Wrong,Extra\nA001,41,5\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + expect( + await screen.findByText( + /is missing these columns:.*Midterm, Final Exam/i, + ), + ).toBeInTheDocument(); + expect( + screen.getByText(/These columns aren['’]t recognized:.*Wrong, Extra/i), + ).toBeInTheDocument(); + expect( + screen.getByText( + /These columns appear more than once:.*Quiz \(×2\), Project \(×3\)/i, + ), + ).toBeInTheDocument(); + }); + + it('shows preview subtitle with total row count when preview has more than 5 rows', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + totalRows: 6, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [ + { identifier: A001, grades: { Midterm: 41 } }, + { identifier: 'A002', grades: { Midterm: 42 } }, + { identifier: 'A003', grades: { Midterm: 43 } }, + { identifier: 'A004', grades: { Midterm: 44 } }, + { identifier: 'A005', grades: { Midterm: 45 } }, + ], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + }, + }); + + renderWizard(); + + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + + await userEvent.upload( + screen.getByLabelText(/upload/i), + file( + [ + `${EXTERNAL_ID},${MIDTERM}`, + 'A001,41', + 'A002,42', + 'A003,43', + 'A004,44', + 'A005,45', + 'A006,46', + ].join('\n'), + ), + ); + + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + expect(await screen.findByText(A001)).toBeVisible(); + + expect( + screen.getByText( + /Previewing the first 5 of 6 rows. Check that this preview matches your CSV before continuing./i, + ), + ).toBeInTheDocument(); + }); + + it('shows all rows subtitle variant when totalRows is 5 or fewer', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + totalRows: 5, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [ + { identifier: A001, grades: { Midterm: 41 } }, + { identifier: 'A002', grades: { Midterm: 42 } }, + { identifier: 'A003', grades: { Midterm: 43 } }, + { identifier: 'A004', grades: { Midterm: 44 } }, + { identifier: 'A005', grades: { Midterm: 45 } }, + ], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + }, + }); + + renderWizard(); + + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + + await userEvent.upload( + screen.getByLabelText(/upload/i), + file( + [ + `${EXTERNAL_ID},${MIDTERM}`, + 'A001,41', + 'A002,42', + 'A003,43', + 'A004,44', + 'A005,45', + ].join('\n'), + ), + ); + + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + expect(await screen.findByText(A001)).toBeVisible(); + + expect( + screen.getByText( + /Previewing all 5 rows. Check that this preview matches your CSV before continuing./i, + ), + ).toBeInTheDocument(); + }); + + it('renders the Verify preview columns in the CSV column order', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [{ identifier: 'A001', grades: { Final: 80, Midterm: 41 } }], + conflictRows: [], + reassignments: [], + totalRows: 1, + columnOrder: ['Final', 'Midterm'], + }, + }); + + renderWizard(); + // define Midterm first, then Final (opposite of the CSV order) + await userEvent.type(componentNameInput(), 'Midterm'); + await userEvent.click( + screen.getByRole('button', { name: /add component/i }), + ); + const nameInputs = screen.getAllByRole('textbox', { + name: 'Component name', + }); + await userEvent.type(nameInputs[1], 'Final'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file('Final,External ID,Midterm\n80,A001,41\n'), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + await screen.findByRole('button', { name: /confirm import/i }); + + const headerCells = screen + .getAllByRole('columnheader') + .map((c) => c.textContent); + // identifier header first, then CSV order Final, Midterm + expect(headerCells).toEqual(['External ID', 'Final', 'Midterm']); + }); + + it('states grades import as-is, with the clamping hint only when weighted view is on', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + conflictRows: [], + reassignments: [], + totalRows: 1, + columnOrder: [MIDTERM], + sample: [{ identifier: 'A1', grades: { Midterm: 105 } }], + outOfRange: [ + { + identifier: 'A1', + component: MIDTERM, + grade: 105, + max: 100, + kind: 'above', + }, + ], + }, + }); + renderWizard({ weightedViewEnabled: true }); + await advanceToVerifyStep(); + expect( + screen.getByText( + /Grades will be imported exactly as entered\. This is only a warning; you can turn off this warning in Manage External Assessments\. Out-of-range grades are only floored or capped in the weighted total/, + ), + ).toBeInTheDocument(); + }); + + it('omits the clamping hint when weighted view is off', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + conflictRows: [], + reassignments: [], + totalRows: 1, + sample: [{ identifier: 'A1', grades: { Midterm: 105 } }], + outOfRange: [ + { + identifier: 'A1', + component: MIDTERM, + grade: 105, + max: 100, + kind: 'above', + }, + ], + columnOrder: [MIDTERM], + }, + }); + renderWizard({ weightedViewEnabled: false }); + await advanceToVerifyStep(); + expect( + screen.getByText( + 'Grades will be imported exactly as entered. This is only a warning; you can turn off this warning in Manage External Assessments. If these out-of-range grades are intentional, continue.', + ), + ).toBeInTheDocument(); + + expect(screen.queryByText(/floored or capped/)).not.toBeInTheDocument(); + }); + + it('wraps the preview table in a horizontally scrollable container', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + conflictRows: [], + reassignments: [], + outOfRange: [], + totalRows: 1, + sample: [{ identifier: 'A1', grades: { Midterm: 80 } }], + columnOrder: [MIDTERM], + }, + }); + renderWizard(); + await advanceToVerifyStep(); + const table = screen.getByRole('table'); + expect(table.parentElement).toHaveClass('overflow-x-auto'); + }); + + it('cues the pending change set on the Verify step before the confirm modal', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [], + columnOrder: [MIDTERM], + sample: [{ identifier: A001, grades: { Midterm: 20 } }], + conflictRows: [ + { + identifier: A001, + studentName: 'Alice', + cells: { Midterm: { existing: 10, inFile: 20, changed: true } }, + }, + ], + reassignments: [], + totalRows: 1, + }, + }); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},${MIDTERM}\n${A001},20\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + + // Cue appears on the Verify step, before any confirm click + expect( + await screen.findByText( + /1 row contains changes to existing grades\. After checking this preview, click Confirm import to review these conflicts before anything is imported\./i, + ), + ).toBeInTheDocument(); + }); + + it('shows a spinner on Replace while the commit is in flight', async () => { + let resolveCommit: (v: unknown) => void = () => {}; + (CourseAPI.gradebook.importCommit as jest.Mock).mockReturnValue( + new Promise((res) => { + resolveCommit = res; + }), + ); + (CourseAPI.gradebook.index as jest.Mock).mockResolvedValue({ data: {} }); + + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [{ identifier: 'A001', grades: { Midterm: 41 } }], + conflictRows: [ + { + identifier: 'A001', + studentName: 'student', + identifierMismatch: false, + cells: { Midterm: { existing: 10, inFile: 41, changed: true } }, + }, + ], + reassignments: [], + totalRows: 1, + columnOrder: [MIDTERM], + }, + }); + + renderWizard(); + await advanceToVerifyStep(); + await userEvent.click( + screen.getByRole('button', { name: /confirm import/i }), + ); + // conflict prompt is open + const replaceBtn = await screen.findByRole('button', { name: /replace/i }); + await userEvent.click(replaceBtn); + + // MUI LoadingButton sets aria-disabled and renders a progressbar while loading + expect(await screen.findByRole('progressbar')).toBeInTheDocument(); + + resolveCommit({ + data: { createdComponents: 0, updatedComponents: 1, gradesWritten: 1 }, + }); + }); + + it('renders the diagnostic heading and a single closing instruction', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { + message: 'bad_header', + missing: ['Quiz 2'], + unrecognized: [], + suggestions: [], + duplicates: [], + identifierNotFirst: false, + }, + }, + }, + }); + render(, { + state: studentsState([]), + }); + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\nA001,41\n`); + + expect(await screen.findByText('These headers need fixing:')).toBeVisible(); + expect( + screen.getByText('Correct these in your CSV, then re-upload.'), + ).toBeVisible(); + }); + + it('shows the identifier-first bullet when identifierNotFirst is set', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { + message: 'bad_header', + missing: [], + unrecognized: [], + suggestions: [], + duplicates: [], + identifierNotFirst: true, + }, + }, + }, + }); + render(, { + state: studentsState([]), + }); + await advanceToVerifyFailure(`${MIDTERM},${EXTERNAL_ID}\n41,A001\n`); + + expect( + await screen.findByText('‘External ID’ must be the first column.'), + ).toBeVisible(); + }); + + it('shows a reassignment warning when an identifier now matches a different student', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [{ identifier: A001, grades: { Midterm: 77 } }], + conflictRows: [], + reassignments: [ + { + identifier: A001, + currentStudent: 'Carol', + previousStudents: ['Alice'], + }, + ], + columnOrder: [MIDTERM], + totalRows: 1, + }, + }); + + render(, { + state: studentsState([]), + }); + await advanceToVerifyStep(); + + expect( + await screen.findByText(/now match a different student/i), + ).toBeInTheDocument(); + expect( + screen.getByText(/A001: now Carol \(was Alice\)/), + ).toBeInTheDocument(); + }); + + it('shows the External ID matching hint when every student has one', async () => { + render(, { + state: studentsState([ + student({ id: 1, name: 'Alice', externalId: 'E1' }), + ]), + }); + await fillOneComponent(); + expect( + screen.getByText(/Matching uses each student's External ID/), + ).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Next' })).toBeEnabled(); + }); + + it('hides the External ID hint when matching by Email', async () => { + render(, { + state: studentsState([ + student({ id: 1, name: 'Alice', externalId: 'E1' }), + ]), + }); + await fillOneComponent(); + await userEvent.click(screen.getByRole('radio', { name: 'Email' })); + expect( + screen.queryByText(/Matching uses each student's External ID/), + ).not.toBeInTheDocument(); + }); + + it('does not cue pending changes when no rows conflict', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [{ identifier: A001, grades: { Midterm: 41 } }], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, + }, + }); + // Use an isolated store (not the shared singleton, which an earlier commit + // test may have left without a students slice). + render(, { + state: studentsState([]), + }); + await advanceToVerifyStep(); + expect( + screen.queryByText(/contains? changes to existing grades/i), + ).not.toBeInTheDocument(); + }); + + it('renders the change matrix: changed cells as old→new, unchanged as-is, missing as em-dash', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [ + { identifier: A001, grades: { Midterm: 20, Finals: 88 } }, + { identifier: 'A002', grades: { Midterm: 30 } }, + ], + conflictRows: [ + { + identifier: A001, + studentName: 'Alice', + identifierMismatch: false, + cells: { + Midterm: { existing: 10, inFile: 20, changed: true }, + Finals: { existing: 88, inFile: 88, changed: false }, + }, + }, + { + identifier: 'A002', + studentName: 'Bob', + identifierMismatch: false, + // Finals absent for this row → em-dash in matrix + cells: { Midterm: { existing: 5, inFile: 30, changed: true } }, + }, + ], + reassignments: [], + columnOrder: [MIDTERM, 'Finals'], + totalRows: 2, + }, + }); + (CourseAPI.gradebook.importCommit as jest.Mock).mockResolvedValue({ + data: { createdComponents: 0, updatedComponents: 1, gradesWritten: 2 }, + }); + render( + , + { state: studentsState([]) }, + ); + // Fill the initial blank row with Midterm (locks it as existing), then add + // Finals from its chip — two components, so Next is enabled. + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.click(screen.getByRole('button', { name: 'Finals' })); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},${MIDTERM},Finals\n${A001},20,88\nA002,30,\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + await userEvent.click( + await screen.findByRole('button', { name: /confirm import/i }), + ); + + const dialog = await screen.findByRole('dialog', { + name: /resolve grade conflicts/i, + }); + // changed cell: struck old + bold new + const struck = within(dialog).getByText('10'); + expect(struck).toHaveStyle('text-decoration: line-through'); + expect(within(dialog).getByText('20')).toHaveStyle('font-weight: 700'); + // unchanged cell (Finals 88 → 88): shows the stored value, not struck + expect(within(dialog).getByText('88')).not.toHaveStyle( + 'text-decoration: line-through', + ); + // missing Finals cell on A002 → em-dash + expect(within(dialog).getByText('—')).toBeInTheDocument(); + }); + + it('formats below-minimum out-of-range grades with a min-0 bound', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [ + { + identifier: 'S1', + component: MIDTERM, + grade: -5, + max: 100, + kind: 'below', + }, + ], + sample: [{ identifier: 'S1', grades: { Midterm: -5 } }], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, + }, + }); + renderWizard({ weightedViewEnabled: true }); + await advanceToVerifyStep(); + expect(screen.getByText(/S1 - Midterm: -5 \(min 0\)/)).toBeInTheDocument(); + }); + + it('summarises out-of-range cells beyond the first ten', async () => { + const outOfRange = Array.from({ length: 12 }, (_, i) => ({ + identifier: `S${i + 1}`, + component: MIDTERM, + grade: 105, + max: 100, + kind: 'above' as const, + })); + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange, + sample: [{ identifier: 'S1', grades: { Midterm: 105 } }], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 12, + }, + }); + renderWizard({ weightedViewEnabled: true }); + await advanceToVerifyStep(); + expect( + screen.getByText(/S10 - Midterm: 105 \(max 100\)/), + ).toBeInTheDocument(); + expect(screen.queryByText(/S11 - Midterm/)).not.toBeInTheDocument(); + expect(screen.getByText('+2 more')).toBeInTheDocument(); + }); + + it('uses email copy for unresolved identifiers when matching by Email', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: false, + unresolved: ['nope@x.com'], + malformed: [], + outOfRange: [], + sample: [], + conflictRows: [], + reassignments: [], + }, + }); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('radio', { name: 'Email' })); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`Email,${MIDTERM}\nnope@x.com,1\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + expect( + await screen.findByText( + /This email address was not found in the course: nope@x.com/, + ), + ).toBeInTheDocument(); + }); + + it('disables Next when two components share the same name', async () => { + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click( + screen.getByRole('button', { name: /add component/i }), + ); + const names = screen.getAllByRole('textbox', { name: 'Component name' }); + await userEvent.type(names[1], MIDTERM); + expect(screen.getByRole('button', { name: /next/i })).toBeDisabled(); + }); + + it('lists the Email header when matching by Email', async () => { + render(, { + state: studentsState([ + student({ id: 1, name: 'Alice', externalId: 'E1' }), + ]), + }); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.click(screen.getByRole('radio', { name: 'Email' })); + await userEvent.click(screen.getByRole('button', { name: 'Next' })); + expect( + screen.getByText(/Your CSV needs these column headers: Email, Midterm/), + ).toBeInTheDocument(); + }); + + it('shows the header mismatch without a suggestion list when none is returned', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { + message: 'bad_header', + missing: [MIDTERMS], + unrecognized: ['Quiz'], + suggestions: [], + duplicates: [], + }, + }, + }, + }); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERMS); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},Quiz\nA001,41\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + // The header diagnostic lists the mismatched columns, but with no + // suggestions returned it omits the "did you mean" line entirely. + expect( + await screen.findByText('These headers need fixing:'), + ).toBeInTheDocument(); + expect( + screen.getByText(/This column isn['’]t recognized:.*Quiz/i), + ).toBeInTheDocument(); + expect(screen.queryByText(/did you mean/i)).not.toBeInTheDocument(); + }); + + it('shows a specific error when the CSV has no data rows', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { data: { errors: { message: 'empty_csv' } } }, + }); + renderWizard(); + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\n`); + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + expect.stringMatching(/no data rows|empty/i), + ), + ); + }); + + it('shows the duplicated identifiers when a row identifier repeats', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue({ + response: { + data: { + errors: { message: 'duplicate_identifier', identifiers: ['A001'] }, + }, + }, + }); + renderWizard(); + await advanceToVerifyFailure(`${EXTERNAL_ID},${MIDTERM}\n${A001},40\n${A001},50\n`); + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith(expect.stringMatching(/A001/)), + ); + }); + + it('toasts a generic error when the preview fails without a header diagnosis', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockRejectedValue( + new Error('network'), + ); + renderWizard(); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},${MIDTERM}\nA001,41\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + 'Could not verify the file. Please try again.', + ), + ); + // stays on upload step + expect( + screen.queryByRole('button', { name: /confirm import/i }), + ).not.toBeInTheDocument(); + }); + + it('toasts failure and stays open when the commit request rejects', async () => { + (CourseAPI.gradebook.importPreview as jest.Mock).mockResolvedValue({ + data: { + ok: true, + unresolved: [], + malformed: [], + outOfRange: [], + sample: [{ identifier: A001, grades: { Midterm: 41 } }], + conflictRows: [], + reassignments: [], + columnOrder: [MIDTERM], + totalRows: 1, + }, + }); + (CourseAPI.gradebook.importCommit as jest.Mock).mockRejectedValue( + new Error('boom'), + ); + const onClose = jest.fn(); + render( + , + { state: studentsState([]) }, + ); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file(`${EXTERNAL_ID},${MIDTERM}\n${A001},41\n`), + ); + await userEvent.click(screen.getByRole('button', { name: /verify/i })); + await userEvent.click( + await screen.findByRole('button', { name: /confirm import/i }), + ); + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + 'Import failed. Nothing was saved.', + ), + ); + expect(onClose).not.toHaveBeenCalled(); + }); + + it('resets to the define step when reopened after a close', async () => { + const { rerender } = render( + , + ); + await userEvent.type(componentNameInput(), MIDTERM); + await userEvent.type(screen.getByLabelText(/weightage/i), '30'); + await userEvent.type(screen.getByLabelText(/max marks/i), '50'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + expect(screen.getByLabelText(/upload/i)).toBeInTheDocument(); // on step 1 + // rerender re-renders the root element directly, so re-wrap in TestApp to + // keep the Redux provider; reusing the singleton store keeps the same store + // instance, exercising the open-prop reset effect rather than a remount. + rerender( + + + , + ); + rerender( + + + , + ); + // back on define step with a blank component + expect(componentNameInput()).toHaveValue(''); + expect(screen.queryByLabelText(/upload/i)).not.toBeInTheDocument(); + }); }); diff --git a/client/app/bundles/course/gradebook/__tests__/ManageExternalAssessmentsButton.test.tsx b/client/app/bundles/course/gradebook/__tests__/ManageExternalAssessmentsButton.test.tsx index fc1b01e775..2efd440d79 100644 --- a/client/app/bundles/course/gradebook/__tests__/ManageExternalAssessmentsButton.test.tsx +++ b/client/app/bundles/course/gradebook/__tests__/ManageExternalAssessmentsButton.test.tsx @@ -1,16 +1,25 @@ import { fireEvent, render, screen } from 'test-utils'; + import ManageExternalAssessmentsButton from '../components/manage/ManageExternalAssessmentsButton'; const state = { gradebook: { - categories: [], tabs: [], students: [], submissions: [], assessments: [], - gamificationEnabled: false, weightedViewEnabled: false, canManageWeights: true, + categories: [], + tabs: [], + students: [], + submissions: [], + assessments: [], + gamificationEnabled: false, + weightedViewEnabled: false, + canManageWeights: true, }, }; it('opens the panel on click', async () => { render(, { state }); - fireEvent.click(await screen.findByRole('button', { name: 'Manage external assessments' })); + fireEvent.click( + await screen.findByRole('button', { name: 'Manage external assessments' }), + ); expect(await screen.findByText('External assessments')).toBeVisible(); }); diff --git a/client/app/bundles/course/gradebook/__tests__/buildTemplate.test.ts b/client/app/bundles/course/gradebook/__tests__/buildTemplate.test.ts index c05c7d0796..279e3cc68e 100644 --- a/client/app/bundles/course/gradebook/__tests__/buildTemplate.test.ts +++ b/client/app/bundles/course/gradebook/__tests__/buildTemplate.test.ts @@ -1,8 +1,11 @@ -import { buildTemplateCsv, identifierHeader } from '../components/import/buildTemplate'; +import { + buildTemplateCsv, + identifierHeader, +} from '../components/import/buildTemplate'; describe('identifierHeader', () => { it('maps mode to the concrete header', () => { - expect(identifierHeader('student_id')).toBe('External ID'); + expect(identifierHeader('external_id')).toBe('External ID'); expect(identifierHeader('email')).toBe('Email'); }); }); @@ -10,8 +13,10 @@ describe('identifierHeader', () => { describe('buildTemplateCsv', () => { const components = [{ name: 'Midterm', weightage: 30, maximumGrade: 50 }]; - it('uses the External ID header in student_id mode', () => { - expect(buildTemplateCsv(components, 'student_id')).toBe('External ID,Midterm\n'); + it('uses the External ID header in external_id mode', () => { + expect(buildTemplateCsv(components, 'external_id')).toBe( + 'External ID,Midterm\n', + ); }); it('uses the Email header in email mode', () => { @@ -19,35 +24,39 @@ describe('buildTemplateCsv', () => { }); it('quotes a component name containing a comma', () => { - const csv = buildTemplateCsv([ - { name: 'Lab, week 1', weightage: 10, maximumGrade: 20 }, - ], 'student_id'); + const csv = buildTemplateCsv( + [{ name: 'Lab, week 1', weightage: 10, maximumGrade: 20 }], + 'external_id', + ); expect(csv.split('\n')[0]).toBe('External ID,"Lab, week 1"'); }); - it('returns "External ID\\n" for empty components array in student_id mode', () => { - expect(buildTemplateCsv([], 'student_id')).toBe('External ID\n'); + it('returns "External ID\\n" for empty components array in external_id mode', () => { + expect(buildTemplateCsv([], 'external_id')).toBe('External ID\n'); }); it('quotes a component name containing a double-quote', () => { - const csv = buildTemplateCsv([ - { name: 'My "Best" Quiz', weightage: 10, maximumGrade: 20 }, - ], 'student_id'); + const csv = buildTemplateCsv( + [{ name: 'My "Best" Quiz', weightage: 10, maximumGrade: 20 }], + 'external_id', + ); expect(csv.split('\n')[0]).toBe('External ID,"My ""Best"" Quiz"'); }); it('quotes a component name containing a newline', () => { - const csv = buildTemplateCsv([ - { name: 'Lab\nWeek1', weightage: 10, maximumGrade: 20 }, - ], 'student_id'); + const csv = buildTemplateCsv( + [{ name: 'Lab\nWeek1', weightage: 10, maximumGrade: 20 }], + 'external_id', + ); // The quoted cell spans two lines; verify the full header row content. expect(csv.startsWith('External ID,"Lab\nWeek1"')).toBe(true); }); it('always ends with exactly one newline', () => { - const csv = buildTemplateCsv([ - { name: 'A', weightage: 0, maximumGrade: 100 }, - ], 'student_id'); + const csv = buildTemplateCsv( + [{ name: 'A', weightage: 0, maximumGrade: 100 }], + 'external_id', + ); expect(csv.endsWith('\n')).toBe(true); expect(csv.split('\n')).toHaveLength(2); // header line + empty string after trailing \n }); diff --git a/client/app/bundles/course/gradebook/components/GradebookTable.tsx b/client/app/bundles/course/gradebook/components/GradebookTable.tsx index 43373cbd7a..4a1bb710e2 100644 --- a/client/app/bundles/course/gradebook/components/GradebookTable.tsx +++ b/client/app/bundles/course/gradebook/components/GradebookTable.tsx @@ -738,24 +738,24 @@ const GradebookTable = ({ const toolbarWithLabel = toolbar ? { - ...toolbar, - buttons: toolbarAction - ? [ - ...(toolbar.buttons ?? []), - cloneElement(toolbarAction, { key: 'toolbar-action' }), - ] - : toolbar.buttons, - ...(toolbar.columnPicker && { - columnPicker: { - ...toolbar.columnPicker, - directExportLabel, - directExportTooltip: - selectedCount === 0 - ? t(translations.exportAllTooltip) - : undefined, - }, - }), - } + ...toolbar, + buttons: toolbarAction + ? [ + ...(toolbar.buttons ?? []), + cloneElement(toolbarAction, { key: 'toolbar-action' }), + ] + : toolbar.buttons, + ...(toolbar.columnPicker && { + columnPicker: { + ...toolbar.columnPicker, + directExportLabel, + directExportTooltip: + selectedCount === 0 + ? t(translations.exportAllTooltip) + : undefined, + }, + }), + } : toolbar; const totalWidth = useMemo( diff --git a/client/app/bundles/course/gradebook/components/OutOfRangeAlert.tsx b/client/app/bundles/course/gradebook/components/OutOfRangeAlert.tsx index d09c232200..1b598f6727 100644 --- a/client/app/bundles/course/gradebook/components/OutOfRangeAlert.tsx +++ b/client/app/bundles/course/gradebook/components/OutOfRangeAlert.tsx @@ -31,7 +31,11 @@ const formatAssessmentNames = (names: string[]): string => { return `${names.slice(0, -1).join(', ')} and ${names[names.length - 1]}`; }; -const OutOfRangeAlert: FC = ({ gradeCount, assessmentNames, weightedViewEnabled }) => { +const OutOfRangeAlert: FC = ({ + gradeCount, + assessmentNames, + weightedViewEnabled, +}) => { const { t } = useTranslation(); if (gradeCount === 0) return null; if (weightedViewEnabled) { @@ -43,18 +47,17 @@ const OutOfRangeAlert: FC = ({ gradeCount, assessmentNames, weightedViewE assessmentNames: formatAssessmentNames(assessmentNames), })} - ) - } else { - return ( - - {t(translations.warning, { - gradeCount, - assessmentCount: assessmentNames.length, - assessmentNames: formatAssessmentNames(assessmentNames), - })} - ); } + return ( + + {t(translations.warning, { + gradeCount, + assessmentCount: assessmentNames.length, + assessmentNames: formatAssessmentNames(assessmentNames), + })} + + ); }; export default OutOfRangeAlert; diff --git a/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictPrompt.tsx b/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictPrompt.tsx index 209f8be72b..cd8ea7fbc5 100644 --- a/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictPrompt.tsx +++ b/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictPrompt.tsx @@ -1,5 +1,6 @@ import { FC } from 'react'; import { defineMessages } from 'react-intl'; +import { LoadingButton } from '@mui/lab'; import { Box, Button, @@ -8,8 +9,9 @@ import { DialogContent, DialogContentText, DialogTitle, + Typography, } from '@mui/material'; -import type { ImportConflict } from 'types/course/gradebook'; +import type { ConflictRow } from 'types/course/gradebook'; import useTranslation from 'lib/hooks/useTranslation'; @@ -23,7 +25,7 @@ const translations = defineMessages({ body: { id: 'course.gradebook.ExternalGradeConflictPrompt.body', defaultMessage: - 'These students already have a grade for these components. Keep their existing grades, or replace them with the values from your file? New students and blank cells are unaffected.', + 'Some students already have grades for these components that differ from the values in your file. Replace will overwrite the existing grades with the values from your file. Keep Existing will leave the existing grades unchanged.', }, goBack: { id: 'course.gradebook.ExternalGradeConflictPrompt.goBack', @@ -37,12 +39,21 @@ const translations = defineMessages({ id: 'course.gradebook.ExternalGradeConflictPrompt.replace', defaultMessage: 'Replace', }, + changesSummary: { + id: 'course.gradebook.ExternalGradeConflictPrompt.changesSummary', + defaultMessage: '{changed} of {total} rows have changes', + }, }); interface Props { open: boolean; - conflicts: ImportConflict[]; + rows: ConflictRow[]; + componentNames: string[]; + identifierLabel: string; + totalRows: number; disabled?: boolean; + keepLoading?: boolean; + replaceLoading?: boolean; onKeepExisting: () => void; onReplaceAll: () => void; onCancel: () => void; @@ -50,36 +61,65 @@ interface Props { const ExternalGradeConflictPrompt: FC = ({ open, - conflicts, + rows, + componentNames, + identifierLabel, + totalRows, disabled = false, + keepLoading = false, + replaceLoading = false, onKeepExisting, onReplaceAll, onCancel, }) => { const { t } = useTranslation(); return ( - + { + if (reason === 'backdropClick') return; + onCancel(); + }} + open={open} + > {t(translations.title)} {t(translations.body)} + + {t(translations.changesSummary, { + changed: rows.length, + total: totalRows, + })} + - + - - + ); diff --git a/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictTable.tsx b/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictTable.tsx index 19972bef83..97867067a0 100644 --- a/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictTable.tsx +++ b/client/app/bundles/course/gradebook/components/import/ExternalGradeConflictTable.tsx @@ -1,81 +1,98 @@ -import { FC } from 'react'; +import { FC, memo } from 'react'; import { defineMessages } from 'react-intl'; -import { WarningAmber } from '@mui/icons-material'; +import { ArrowForward } from '@mui/icons-material'; import { Table, TableBody, TableCell, TableHead, TableRow, - Tooltip, Typography, } from '@mui/material'; -import type { ImportConflict } from 'types/course/gradebook'; +import type { ConflictRow } from 'types/course/gradebook'; import useTranslation from 'lib/hooks/useTranslation'; const translations = defineMessages({ - component: { - id: 'course.gradebook.ExternalGradeConflictTable.component', - defaultMessage: 'Component', - }, - student: { - id: 'course.gradebook.ExternalGradeConflictTable.student', - defaultMessage: 'Student', - }, - existing: { - id: 'course.gradebook.ExternalGradeConflictTable.existing', - defaultMessage: 'Existing grade', - }, - inFile: { - id: 'course.gradebook.ExternalGradeConflictTable.inFile', - defaultMessage: 'In-file grade', - }, - mismatch: { - id: 'course.gradebook.ExternalGradeConflictTable.mismatch', - defaultMessage: - 'This identifier now resolves to a different student than the existing grade was imported under.', + name: { + id: 'course.gradebook.ExternalGradeConflictTable.name', + defaultMessage: 'Name', }, }); interface Props { - rows: ImportConflict[]; + rows: ConflictRow[]; + componentNames: string[]; + identifierLabel: string; } -const ExternalGradeConflictTable: FC = ({ rows }) => { +const formatGrade = (value: number | null): string => + value == null ? '—' : String(value); + +const ExternalGradeConflictTable: FC = ({ + rows, + componentNames, + identifierLabel, +}) => { const { t } = useTranslation(); return ( - {t(translations.component)} - {t(translations.student)} - {t(translations.existing)} - {t(translations.inFile)} + {identifierLabel} + {t(translations.name)} + {componentNames.map((name) => ( + + {name} + + ))} {rows.map((row) => ( - - {row.component} - - {row.studentName} - {row.identifierMismatch && ( - - - - )} - - {row.existingGrade} - - - {row.inFileGrade} - + + + {row.identifier} + {row.studentName} + {componentNames.map((name) => { + const cell = row.cells[name]; + if (cell?.changed) { + return ( + + + {formatGrade(cell.existing)} + + + + {formatGrade(cell.inFile)} + + + ); + } + // unchanged / new-fill / blank: show the value that will be stored + const value = cell == null ? null : cell.inFile ?? cell.existing; + return ( + + {formatGrade(value)} + + ); + })} ))} @@ -83,4 +100,4 @@ const ExternalGradeConflictTable: FC = ({ rows }) => { ); }; -export default ExternalGradeConflictTable; +export default memo(ExternalGradeConflictTable); diff --git a/client/app/bundles/course/gradebook/components/import/ImportExternalAssessmentsWizard.tsx b/client/app/bundles/course/gradebook/components/import/ImportExternalAssessmentsWizard.tsx index 89091e1f2e..cbbbf23958 100644 --- a/client/app/bundles/course/gradebook/components/import/ImportExternalAssessmentsWizard.tsx +++ b/client/app/bundles/course/gradebook/components/import/ImportExternalAssessmentsWizard.tsx @@ -1,10 +1,12 @@ import { FC, useEffect, useMemo, useState } from 'react'; +import Dropzone from 'react-dropzone'; import { defineMessages } from 'react-intl'; import { useParams } from 'react-router-dom'; -import Dropzone from 'react-dropzone'; import { Add, Delete } from '@mui/icons-material'; +import { LoadingButton } from '@mui/lab'; import { Alert, + AlertTitle, Button, Chip, Dialog, @@ -24,8 +26,6 @@ import { TextField, Typography, } from '@mui/material'; -import { FilePreview } from 'lib/components/form/fields/SingleFileInput'; -import SegmentedSwitch from 'lib/components/core/buttons/SegmentedSwitch'; import type { ExistingExternalAssessment, IdentifierMode, @@ -33,13 +33,14 @@ import type { ImportPreviewResult, } from 'types/course/gradebook'; +import SegmentedSwitch from 'lib/components/core/buttons/SegmentedSwitch'; +import { FilePreview } from 'lib/components/form/fields/SingleFileInput'; import { useAppDispatch, useAppSelector } from 'lib/hooks/store'; import toast from 'lib/hooks/toast'; import useTranslation from 'lib/hooks/useTranslation'; -import { getStudents } from '../../selectors'; - import { commitImport, previewImport } from '../../operations'; +import { getStudents } from '../../selectors'; import { downloadTemplate, @@ -100,7 +101,20 @@ const translations = defineMessages({ email: { id: 'course.gradebook.ImportWizard.email', defaultMessage: 'Email' }, requiredHeaders: { id: 'course.gradebook.ImportWizard.requiredHeaders', - defaultMessage: 'Your CSV needs these column headers: {headers}', + defaultMessage: + 'Your CSV needs these column headers: {headers}. ‘{identifier}’ must be the first column.', + }, + headerErrorsHeading: { + id: 'course.gradebook.ImportWizard.headerErrorsHeading', + defaultMessage: 'These headers need fixing:', + }, + headerErrorsClosing: { + id: 'course.gradebook.ImportWizard.headerErrorsClosing', + defaultMessage: 'Correct these in your CSV, then re-upload.', + }, + identifierNotFirst: { + id: 'course.gradebook.ImportWizard.identifierNotFirst', + defaultMessage: '‘{identifier}’ must be the first column.', }, dropzone: { id: 'course.gradebook.ImportWizard.dropzone', @@ -128,27 +142,71 @@ const translations = defineMessages({ id: 'course.gradebook.ImportWizard.continue', defaultMessage: 'Confirm import', }, - unresolved: { - id: 'course.gradebook.ImportWizard.unresolved', - defaultMessage: 'These identifiers were not found in the course: {ids}', + unresolvedEmail: { + id: 'course.gradebook.ImportWizard.unresolvedEmail', + defaultMessage: + '{count, plural, one {This email address was not found in the course: {ids}} other {These email addresses were not found in the course: {ids}}}', + }, + unresolvedExternalId: { + id: 'course.gradebook.ImportWizard.unresolvedExternalId', + defaultMessage: + '{count, plural, one {This external ID was not found in the course: {ids}} other {These external IDs were not found in the course: {ids}}}', }, malformed: { id: 'course.gradebook.ImportWizard.malformed', - defaultMessage: 'These cells are not valid numbers: {cells}', + defaultMessage: 'These cells do not contain valid numbers:', + }, + malformedMore: { + id: 'course.gradebook.ImportWizard.malformedMore', + defaultMessage: 'and {count} more', + }, + outOfRangeTitle: { + id: 'course.gradebook.ImportWizard.outOfRangeTitle', + defaultMessage: 'Some grades are outside their valid range.', + }, + outOfRangeSubtitle: { + id: 'course.gradebook.ImportWizard.outOfRangeSubtitle', + defaultMessage: + 'Grades will be imported exactly as entered. This is only a warning; you can turn off this warning in Manage External Assessments. If these out-of-range grades are intentional, continue.', + }, + outOfRangeWeightedSubtitle: { + id: 'course.gradebook.ImportWizard.outOfRangeWeightedSubtitle', + defaultMessage: + 'Grades will be imported exactly as entered. This is only a warning; you can turn off this warning in Manage External Assessments. Out-of-range grades are only floored or capped in the weighted total. If these out-of-range grades are intentional, continue.', }, - outOfRange: { - id: 'course.gradebook.ImportWizard.outOfRange', - defaultMessage: 'Some grades are outside their valid range: {cells}', + reassignmentTitle: { + id: 'course.gradebook.ImportWizard.reassignmentTitle', + defaultMessage: 'Some identifiers now match a different student', }, - outOfRangeWeighted: { - id: 'course.gradebook.ImportWizard.outOfRangeWeighted', + reassignmentSubtitle: { + id: 'course.gradebook.ImportWizard.reassignmentSubtitle', defaultMessage: - 'These will be capped or floored in the weighted total when imported.', + 'These identifiers were previously imported for another student. Grades are matched by the current student, not the identifier — confirm these are the people you intend before importing.', }, committed: { id: 'course.gradebook.ImportWizard.committed', defaultMessage: 'Import complete.', }, + headerSuggestion: { + id: 'course.gradebook.ImportWizard.headerSuggestion', + defaultMessage: + 'No column named ‘{suggestion}’ — did you mean ‘{expected}’?', + }, + duplicateHeaders: { + id: 'course.gradebook.ImportWizard.duplicateHeaders', + defaultMessage: + '{count, plural, one {This column appears more than once: {dupes}.} other {These columns appear more than once: {dupes}.}}', + }, + missingHeaders: { + id: 'course.gradebook.ImportWizard.missingHeaders', + defaultMessage: + '{count, plural, one {Your CSV is missing this column: {missing}.} other {Your CSV is missing these columns: {missing}.}}', + }, + unrecognizedHeaders: { + id: 'course.gradebook.ImportWizard.unrecognizedHeaders', + defaultMessage: + '{count, plural, one {This column isn’t recognized: {unrecognized}.} other {These columns aren’t recognized: {unrecognized}.}}', + }, commitError: { id: 'course.gradebook.ImportWizard.commitError', defaultMessage: 'Import failed. Nothing was saved.', @@ -157,6 +215,16 @@ const translations = defineMessages({ id: 'course.gradebook.ImportWizard.previewError', defaultMessage: 'Could not verify the file. Please try again.', }, + emptyCsv: { + id: 'course.gradebook.ImportWizard.emptyCsv', + defaultMessage: + 'The uploaded file has no data rows. Add at least one student row and try again.', + }, + duplicateIdentifier: { + id: 'course.gradebook.ImportWizard.duplicateIdentifier', + defaultMessage: + 'The file lists {count, plural, one {an identifier} other {identifiers}} more than once: {ids}. Each student should appear on a single row.', + }, externalIdHint: { id: 'course.gradebook.ImportWizard.externalIdHint', defaultMessage: @@ -165,7 +233,22 @@ const translations = defineMessages({ externalIdBlocked: { id: 'course.gradebook.ImportWizard.externalIdBlocked', defaultMessage: - '{count, plural, one {{name} has no External ID.} other {# students have no External ID, including {name}.}} Importing by External ID needs every student to have one, so this import is blocked until they are all filled in. In Manage Users, sort by the External ID column to group the blank ones together and fill them in, then come back here. Prefer to match by Email instead? Switch the toggle above.', + '{count, plural, =0 {{name} has no External ID} one {{name} and one other student have no External ID} other {{name} and # other students have no External ID}}. Add the missing IDs in Manage Users to import by External ID.', + }, + previewRows: { + id: 'course.gradebook.ImportWizard.previewRows', + defaultMessage: + 'Previewing the first 5 of {totalRows} rows. Check that this preview matches your CSV before continuing.', + }, + previewFewRows: { + id: 'course.gradebook.ImportWizard.previewFewRows', + defaultMessage: + 'Previewing all {totalRows} rows. Check that this preview matches your CSV before continuing.', + }, + willChangeExisting: { + id: 'course.gradebook.ImportWizard.willChangeExisting', + defaultMessage: + '{count, plural, one {# row contains} other {# rows contain}} changes to existing grades. After checking this preview, click Confirm import to review these conflicts before anything is imported.', }, }); @@ -182,6 +265,47 @@ const blankComponent = (): ImportComponent & { id: number } => { return { id: rowId, name: '', weightage: 0, maximumGrade: 0 }; }; +interface BadHeaderError { + message: string; + missing: string[]; + unrecognized: string[]; + suggestions: { expected: string; didYouMean: string }[]; + duplicates: { name: string; count: number }[]; + identifierNotFirst: boolean; +} + +const badHeaderFromError = (error: unknown): BadHeaderError | null => { + const body = ( + error as { response?: { data?: { errors?: Partial } } } + )?.response?.data?.errors; + return body?.message === 'bad_header' + ? { + message: body.message, + missing: body.missing ?? [], + unrecognized: body.unrecognized ?? [], + suggestions: body.suggestions ?? [], + duplicates: body.duplicates ?? [], + identifierNotFirst: body.identifierNotFirst ?? false, + } + : null; +}; + +const importErrorCode = ( + error: unknown, +): { message: string; identifiers?: string[] } | null => { + const msg = ( + error as { + response?: { data?: { errors?: { message?: string; identifiers?: string[] } } }; + } + )?.response?.data?.errors?.message; + if (!msg) return null; + return ( + error as { + response: { data: { errors: { message: string; identifiers?: string[] } } }; + } + ).response.data.errors; +}; + const ImportExternalAssessmentsWizard: FC = ({ open, onClose, @@ -202,6 +326,10 @@ const ImportExternalAssessmentsWizard: FC = ({ const [preview, setPreview] = useState(null); const [conflictOpen, setConflictOpen] = useState(false); const [busy, setBusy] = useState(false); + const [headerError, setHeaderError] = useState(null); + const [pendingCommit, setPendingCommit] = useState<'keep' | 'replace' | null>( + null, + ); const students = useAppSelector(getStudents); const missingStudents = useMemo( @@ -221,7 +349,9 @@ const ImportExternalAssessmentsWizard: FC = ({ setCsvData(''); setPreview(null); setConflictOpen(false); + setPendingCommit(null); setBusy(false); + setHeaderError(null); } }, [open]); @@ -276,9 +406,24 @@ const ImportExternalAssessmentsWizard: FC = ({ }), ); setPreview(result); - setStep(2); - } catch { - toast.error(t(translations.previewError)); + if (result.ok) setStep(2); + } catch (error) { + const badHeader = badHeaderFromError(error); + if (badHeader) { + setHeaderError(badHeader); + } else { + const known = importErrorCode(error); + if (known?.message === 'empty_csv') { + toast.error(t(translations.emptyCsv)); + } else if (known?.message === 'duplicate_identifier') { + const ids = known.identifiers ?? []; + toast.error( + t(translations.duplicateIdentifier, { count: ids.length, ids: ids.join(', ') }), + ); + } else { + toast.error(t(translations.previewError)); + } + } } finally { setBusy(false); } @@ -286,6 +431,7 @@ const ImportExternalAssessmentsWizard: FC = ({ const doCommit = async (onConflict: 'keep' | 'replace'): Promise => { setBusy(true); + setPendingCommit(onConflict); try { await dispatch( commitImport({ @@ -302,16 +448,129 @@ const ImportExternalAssessmentsWizard: FC = ({ toast.error(t(translations.commitError)); } finally { setBusy(false); + setPendingCommit(null); } }; const onConfirm = (): void => { - if (preview && preview.conflicts.length > 0) setConflictOpen(true); + if (preview && preview.conflictRows.length > 0) setConflictOpen(true); else doCommit('replace'); }; + const renderAlerts = (includeBlocking: boolean): JSX.Element | null => { + if (!preview) return null; + + return ( + <> + {includeBlocking && !preview.ok && preview.unresolved.length > 0 && ( + + + {t( + mode === 'email' + ? translations.unresolvedEmail + : translations.unresolvedExternalId, + { + count: preview.unresolved.length, + ids: preview.unresolved.join(', '), + }, + )} + + + )} + + {includeBlocking && !preview.ok && preview.malformed.length > 0 && ( + + {t(translations.malformed)} +
    + {preview.malformed.slice(0, 5).map((cell) => ( + + {cell} + + ))} + {preview.malformed.length > 5 && ( + + {t(translations.malformedMore, { + count: preview.malformed.length - 5, + })} + + )} +
+ + )} + + {preview.outOfRange.length > 0 && ( + + {t(translations.outOfRangeTitle)} + +
    + {preview.outOfRange.slice(0, 10).map((cell) => ( + + {cell.kind === 'above' + ? `${cell.identifier} - ${cell.component}: ${cell.grade} (max ${cell.max})` + : `${cell.identifier} - ${cell.component}: ${cell.grade} (min 0)`} + + ))} + + {preview.outOfRange.length > 10 && ( + + +{preview.outOfRange.length - 10} more + + )} +
+ + {weightedViewEnabled && ( + + {t(translations.outOfRangeWeightedSubtitle)} + + )} + + {!weightedViewEnabled && ( + {t(translations.outOfRangeSubtitle)} + )} + + )} + + {preview.reassignments.length > 0 && ( + + {t(translations.reassignmentTitle)} + +
    + {preview.reassignments.slice(0, 5).map((entry) => ( + + {`${entry.identifier}: now ${entry.currentStudent} (was ${entry.previousStudents.join( + ', ', + )})`} + + ))} + + {preview.reassignments.length > 5 && ( + + +{preview.reassignments.length - 5} more + + )} +
+ + {t(translations.reassignmentSubtitle)} + + )} + + ); + }; + return ( - + { + if (reason === 'backdropClick') return; + onClose(); + }} + open={open} + > {t(translations.title)} @@ -450,21 +709,21 @@ const ImportExternalAssessmentsWizard: FC = ({ > {identifierReady ? t(translations.externalIdHint, { - link: (chunks) => ( - - {chunks} - - ), - }) + link: (chunks) => ( + + {chunks} + + ), + }) : t(translations.externalIdBlocked, { - name: missingStudents[0]?.name ?? '', - count: missingStudents.length, - link: (chunks) => ( - - {chunks} - - ), - })} + name: missingStudents[0]?.name ?? '', + count: missingStudents.length - 1, + link: (chunks) => ( + + {chunks} + + ), + })} )} @@ -478,8 +737,63 @@ const ImportExternalAssessmentsWizard: FC = ({ identifierHeader(mode), ...components.map((c) => c.name), ].join(', '), + identifier: identifierHeader(mode), })} + {headerError && ( + + + {t(translations.headerErrorsHeading)} + +
    + {headerError.identifierNotFirst && ( + + {t(translations.identifierNotFirst, { + identifier: identifierHeader(mode), + })} + + )} + {headerError.suggestions.map((s) => ( + + {t(translations.headerSuggestion, { + expected: s.expected, + suggestion: s.didYouMean, + })} + + ))} + {headerError.missing.length > 0 && ( + + {t(translations.missingHeaders, { + count: headerError.missing.length, + missing: headerError.missing.join(', '), + })} + + )} + {headerError.unrecognized.length > 0 && ( + + {t(translations.unrecognizedHeaders, { + count: headerError.unrecognized.length, + unrecognized: headerError.unrecognized.join(', '), + })} + + )} + {headerError.duplicates.length > 0 && ( + + {t(translations.duplicateHeaders, { + count: headerError.duplicates.length, + dupes: headerError.duplicates + .map((d) => `${d.name} (×${d.count})`) + .join(', '), + })} + + )} +
+ + {t(translations.headerErrorsClosing)} + + + )} + {preview && !preview.ok && renderAlerts(true)}
{identifierModeLabel} - {components.map((c) => ( - {c.name} + {preview.columnOrder.map((name) => ( + {name} ))} {preview.sample.map((row) => ( - - {row.studentName} - {components.map((c) => ( - - {row.grades[c.name] ?? '—'} + + {row.identifier} + {preview.columnOrder.map((name) => ( + + {row.grades[name] ?? '—'} ))} ))}
+ + {preview.totalRows > 5 ? ( + + {t(translations.previewRows, { + totalRows: preview.totalRows, + })} + + ) : ( + + {t(translations.previewFewRows, { + totalRows: preview.totalRows, + })} + )} )} @@ -589,28 +903,39 @@ const ImportExternalAssessmentsWizard: FC = ({ )} {step === 1 && ( - + )} {step === 2 && preview?.ok && ( - + )} setConflictOpen(false)} onKeepExisting={() => doCommit('keep')} onReplaceAll={() => doCommit('replace')} open={conflictOpen} + replaceLoading={pendingCommit === 'replace'} + rows={preview?.conflictRows ?? []} + totalRows={preview?.totalRows ?? 0} /> ); diff --git a/client/app/bundles/course/gradebook/components/import/__tests__/ExternalGradeConflictTable.test.tsx b/client/app/bundles/course/gradebook/components/import/__tests__/ExternalGradeConflictTable.test.tsx new file mode 100644 index 0000000000..83007cd189 --- /dev/null +++ b/client/app/bundles/course/gradebook/components/import/__tests__/ExternalGradeConflictTable.test.tsx @@ -0,0 +1,155 @@ +import { render, screen } from 'test-utils'; + +import ExternalGradeConflictTable from '../ExternalGradeConflictTable'; + +jest.mock('lib/components/wrappers/I18nProvider'); + +const rows = [ + { + identifier: 'S1000', + studentName: 'student1000', + cells: { + Midterm: { existing: 83.55, inFile: 90.05, changed: true }, + }, + }, +]; + +it('renders a changed grade cell on a single line (no wrap)', () => { + render( + , + ); + + const oldValue = screen.getByText('83.55'); + // the change cell (the ancestor) must not wrap to two lines + const cell = oldValue.closest('td') as HTMLElement; + expect(cell).toHaveStyle({ whiteSpace: 'nowrap' }); +}); + +it('renders a changed cell as struck old value, arrow, then bold new value', () => { + render( + , + ); + + const oldValue = screen.getByText('83.55'); + expect(oldValue).toHaveStyle({ textDecoration: 'line-through' }); + + const newValue = screen.getByText('90.05'); + expect(newValue).toHaveStyle({ fontWeight: 700 }); + + // arrow separator sits between the two values, struck old before bold new + const cellText = oldValue.closest('td')?.textContent ?? ''; + expect(cellText.indexOf('83.55')).toBeLessThan(cellText.indexOf('90.05')); + expect(screen.getByTestId('ArrowForwardIcon')).toBeInTheDocument(); +}); + +it('renders a new-fill cell (no existing) as a single plain value', () => { + render( + , + ); + + const value = screen.getByText('77'); + expect(value).not.toHaveStyle({ textDecoration: 'line-through' }); + expect(screen.queryByTestId('ArrowForwardIcon')).not.toBeInTheDocument(); +}); + +it('falls back to the existing value when an unchanged cell has no inFile', () => { + render( + , + ); + + expect(screen.getByText('60')).toBeInTheDocument(); +}); + +it('renders an em-dash for a component the row has no cell for', () => { + render( + , + ); + + // Final column has no cell for this row → em-dash + expect(screen.getByText('—')).toBeInTheDocument(); +}); + +it('shows an em-dash for the missing side of a changed cell', () => { + render( + , + ); + + expect(screen.getByText('—')).toBeInTheDocument(); + expect(screen.getByText('88')).toHaveStyle({ fontWeight: 700 }); +}); + +it('renders the identifier label, Name header, and a header per component', () => { + render( + , + ); + + expect(screen.getByText('External ID')).toBeInTheDocument(); + expect(screen.getByText('Name')).toBeInTheDocument(); + expect(screen.getByText('Midterm')).toBeInTheDocument(); + expect(screen.getByText('Final')).toBeInTheDocument(); +}); + +it('renders each row identifier and student name', () => { + render( + , + ); + + expect(screen.getByText('S1000')).toBeInTheDocument(); + expect(screen.getByText('student1000')).toBeInTheDocument(); +}); diff --git a/client/app/bundles/course/gradebook/components/manage/ManageExternalAssessmentsButton.tsx b/client/app/bundles/course/gradebook/components/manage/ManageExternalAssessmentsButton.tsx index 7391c3802e..01870a3034 100644 --- a/client/app/bundles/course/gradebook/components/manage/ManageExternalAssessmentsButton.tsx +++ b/client/app/bundles/course/gradebook/components/manage/ManageExternalAssessmentsButton.tsx @@ -13,16 +13,24 @@ const translations = defineMessages({ }, }); -const ManageExternalAssessmentsButton: FC = () => { +interface Props { + /** Match the host toolbar's button size. Defaults to MUI's `medium`. */ + size?: 'small' | 'medium'; +} + +const ManageExternalAssessmentsButton: FC = ({ size }) => { const { t } = useTranslation(); const [open, setOpen] = useState(false); return ( <> - - setOpen(false)} open={open} /> + setOpen(false)} + open={open} + /> ); }; diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/__test__/index.test.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/__test__/index.test.tsx index 0984ac6c52..8684fbe21b 100644 --- a/client/app/bundles/course/users/components/tables/ManageUsersTable/__test__/index.test.tsx +++ b/client/app/bundles/course/users/components/tables/ManageUsersTable/__test__/index.test.tsx @@ -1,5 +1,11 @@ import userEvent from '@testing-library/user-event'; -import { render, screen, waitFor, waitForElementToBeRemoved, within } from 'test-utils'; +import { + render, + screen, + waitFor, + waitForElementToBeRemoved, + within, +} from 'test-utils'; import { CourseUserMiniEntity } from 'types/course/courseUsers'; import ManageUsersTable from '../index'; @@ -98,28 +104,30 @@ describe('', () => { }); describe('sort by External ID', () => { - it( - 'clusters blank External IDs together when the column is sorted', - async () => { - const user = userEvent.setup(); - const users: CourseUserMiniEntity[] = [ - { ...baseUser, id: 1, name: 'Alice Lim', externalId: 'B-1' }, - { ...baseUser, id: 2, name: 'Bob Tan', externalId: null }, - { ...baseUser, id: 3, name: 'Cara Ng', externalId: 'A-1' }, - ]; - render(); - await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); - - const extIdHeader = screen.getByRole('columnheader', { name: /external id/i }); - await user.click(within(extIdHeader).getByRole('button')); - - await waitFor(() => { - const rows = screen.getAllByRole('row').slice(1); // drop the header row - const order = rows.map((r) => within(r).getByText(/Lim|Tan|Ng/).textContent); - expect(order[0]).toContain('Bob Tan'); // blank External ID sorts first - }); - }, - 10000, - ); + it('clusters blank External IDs together when the column is sorted', async () => { + const user = userEvent.setup(); + const users: CourseUserMiniEntity[] = [ + { ...baseUser, id: 1, name: 'Alice Lim', externalId: 'B-1' }, + { ...baseUser, id: 2, name: 'Bob Tan', externalId: null }, + { ...baseUser, id: 3, name: 'Cara Ng', externalId: 'A-1' }, + ]; + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + const extIdHeader = screen.getByRole('columnheader', { + name: /external id/i, + }); + await user.click(within(extIdHeader).getByRole('button')); + + await waitFor(() => { + const rows = screen.getAllByRole('row').slice(1); // drop the header row + const order = rows.map( + (r) => within(r).getByText(/Lim|Tan|Ng/).textContent, + ); + expect(order[0]).toContain('Bob Tan'); // blank External ID sorts first + }); + }, 10000); }); }); diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx index ec1559934e..4ccd444137 100644 --- a/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx +++ b/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx @@ -79,7 +79,7 @@ const ManageUsersTable = (props: ManageUsersTableProps): JSX.Element => { // single fix pass; non-empty values sort lexicographically after. // descFirst: false ensures the first click sorts ascending (empties first). descFirst: false, - sort: (a, b) => { + sort: (a, b): number => { const av = a.externalId ?? ''; const bv = b.externalId ?? ''; if (!av && bv) return -1;