From d58456da852b0f02ae3b711b382aa1c87ca15ebe Mon Sep 17 00:00:00 2001 From: lws49 Date: Fri, 26 Jun 2026 11:53:56 +0800 Subject: [PATCH] feat(gradebook): decimal-ceiling cap entry, over-max flags, and out-of-range warnings --- .../external_assessment_import_service.rb | 25 +- .../preview.json.jbuilder | 1 + client/app/bundles/course/gradebook/CLAUDE.md | 95 +++ .../__tests__/GradebookIndex.test.tsx | 193 +++++- .../__tests__/GradebookTable.test.tsx | 610 +++++++++++++++++- .../ImportExternalAssessmentsWizard.test.tsx | 181 +++++- .../__tests__/OutOfRangeAlert.test.tsx | 119 ++++ .../gradebook/__tests__/outOfRange.test.ts | 88 +++ .../gradebook/components/GradebookTable.tsx | 429 +++++++----- .../components/GradebookWeightedTable.tsx | 25 +- .../gradebook/components/OutOfRangeAlert.tsx | 60 ++ .../ImportExternalAssessmentsWizard.tsx | 92 ++- .../bundles/course/gradebook/outOfRange.ts | 36 ++ .../gradebook/pages/GradebookIndex/index.tsx | 32 +- client/app/types/course/gradebook.ts | 41 +- ...rnal_assessment_imports_controller_spec.rb | 10 + ...external_assessment_import_service_spec.rb | 19 + 17 files changed, 1807 insertions(+), 249 deletions(-) create mode 100644 client/app/bundles/course/gradebook/CLAUDE.md create mode 100644 client/app/bundles/course/gradebook/__tests__/OutOfRangeAlert.test.tsx create mode 100644 client/app/bundles/course/gradebook/__tests__/outOfRange.test.ts create mode 100644 client/app/bundles/course/gradebook/components/OutOfRangeAlert.tsx create mode 100644 client/app/bundles/course/gradebook/outOfRange.ts diff --git a/app/services/course/gradebook/external_assessment_import_service.rb b/app/services/course/gradebook/external_assessment_import_service.rb index 95089ee8f1..a4118689aa 100644 --- a/app/services/course/gradebook/external_assessment_import_service.rb +++ b/app/services/course/gradebook/external_assessment_import_service.rb @@ -27,7 +27,8 @@ def preview unresolved: resolution[:unresolved], malformed: resolution[:malformed], sample: sample(resolution[:resolved]), - conflicts: conflicts(resolution[:resolved]) + conflicts: conflicts(resolution[:resolved]), + out_of_range: out_of_range(resolution[:resolved]) } end @@ -140,6 +141,28 @@ def numeric?(value) false end + # Advisory: grades outside [0, max]. Reported but never blocks the import. + def out_of_range(resolved) + cells = [] + resolved.each do |row| + @components.each do |component| + grade = row[:grades][component[:name]] + next if grade.nil? + + max = component[:maximum_grade] + next unless grade < 0 || grade > max + cells << { + identifier: row[:identifier], + component: component[:name], + grade: grade, + max: max, + kind: grade < 0 ? 'below' : 'above' + } + end + end + cells + end + def sample(resolved) resolved.first(5).map do |r| { studentName: r[:course_user].name, grades: r[:grades] } diff --git a/app/views/course/external_assessment_imports/preview.json.jbuilder b/app/views/course/external_assessment_imports/preview.json.jbuilder index ccbe5f3c8f..92c7b19c04 100644 --- a/app/views/course/external_assessment_imports/preview.json.jbuilder +++ b/app/views/course/external_assessment_imports/preview.json.jbuilder @@ -13,3 +13,4 @@ json.conflicts @result[:conflicts] do |c| json.inFileGrade c[:inFileGrade] json.identifierMismatch c[:identifierMismatch] end +json.outOfRange @result[:out_of_range] diff --git a/client/app/bundles/course/gradebook/CLAUDE.md b/client/app/bundles/course/gradebook/CLAUDE.md new file mode 100644 index 0000000000..c244e8f9a2 --- /dev/null +++ b/client/app/bundles/course/gradebook/CLAUDE.md @@ -0,0 +1,95 @@ +# Gradebook — Terminology conventions + +### "Grade" vs "Score" vs "Assessment" + +- **grade** — canonical term. Use in code, API keys, i18n, UI. + - DB columns: `grade` (on answers), `maximum_grade` (on questions) + - Feature = **Gradebook** (book of *grades*) +- **score** — avoid. Informal synonym, no DB grounding. No new "score" terms. +- **assessment** — the *entity* (quiz, mission, tutorial). Use when referring to things selected/listed, not grade values inside. + +**Practical rules:** +- Empty state when no assessment columns chosen → "No assessments selected" (user selects *assessments*, not grades/scores) +- Export tree column group with per-assessment grade columns → **"Grades"** (data category, parallel to "Student info" / "Gamification") +- Never label that group "Scores" + +--- + +## External floor/cap: read-time, weighted-view-only (load-bearing invariant) + +`floorAtZero` / `capAtMaximum` on an external assessment are applied **only** in +`effectiveGrade()` (`computeWeighted.ts`), which runs **only** for the +Weighted-total view. Consequences any change here must preserve: + +- **Stored grades are never mutated.** Toggling cap does NOT rewrite a 105 to 100; + it only clamps that assessment's *contribution* to the weighted total. The raw + "All assessments" view and its CSV export always show the literal stored value. +- **No effect when the weighted view is off.** With weighting off the toggles are + inert. Any UI that names a consequence ("capped in the weighted total") must be + conditional on `weightedViewEnabled` (e.g. `GradebookTable`'s over-max tooltip, + the import Verify warning). +- **Out-of-range signals are three independent layers:** per-cell icons in + `GradebookTable` (locate), the import Verify warning (entry-time), and + `OutOfRangeAlert` above the table (aggregate, pre-export). They share the + read-time contract — none of them change data. +- **Negatives are valid input** (penalty/deduction columns). Both the manual cell + (regex allows a leading `-`) and CSV import accept them; `floorAtZero` is what + neutralises them in the total. + +Design rationale and the per-question decisions live in +`tmp/pr-notes/feat-ext-assessments.md` (D9–D12). + +--- + +## Server-controlled (non-pickable) table columns — never use `defaultVisible` + +`GradebookWeightedTable`'s level columns (`Level`, `Level Contribution`) are driven by +course settings (`enabled`/`show`), not the column picker. For columns like these: + +- **Gate column *presence* on the setting** (push the column into the array only when on), + and add the id to the table's `columnPicker.locked` so it is force-visible. +- **Do NOT plumb the setting through `ColumnTemplate.defaultVisible`.** `defaultVisible` is a + one-time seed in `useTanStackTableBuilder` that loses to persisted `localStorage` + (`initialVisibility` prefers stored over default) and to the reconcile effect's + `prev[id]` preservation. Once persisted hidden, the setting can never re-reveal it — and a + non-pickable column has no picker fallback, so it is stranded. +- `GradebookWeightedTable` renders header rows **and** body rows **by hand** (not from the + `columns` array). Adding/reordering a column means editing 4 sites in lockstep: the + `columns` array, header row 1, header row 3 (subheader), the body ``, and the + expanded-breakdown row. Column-array order alone is not enough. + +--- + +## Gradebook overhaul — implementation index (in progress) + +Beyond the weighted-view work, the gradebook is being extended via **3 initiatives**. Full +design + per-PR plans live in `docs/superpowers/specs/` (local only — gitignored, like this +file). **Before writing any overhaul code, read the relevant specs below for the PR you're on.** + +**Start here:** `docs/superpowers/specs/2026-06-10-gradebook-overhaul-SUMMARY.md` (conclusions + +global PR sequence). `…-overhaul-DETAILED.md` has the reasoning + a current-state code map +(file:line) of the gradebook/grading internals. + +Build in this order. For each PR, read that initiative's **design** (relevant section) + the +matching **PR section** of its **implementation-plan**, then run `writing-plans` for a +task-level plan: + +1. **External Assessments** — keystone; makes the weighted total true. + - design: `specs/2026-06-10-gradebook-external-assessments-design.md` + - plan: `specs/2026-06-10-gradebook-external-assessments-implementation-plan.md` — PR1 BE foundation → PR2 FE manual usage → PR3 CSV import + - decisions: `research-notes/2026-06-10-external-assessments-design-decisions.md` + - ⚠ OPEN before PR1: edit-permission (split vs manager-only) — see design "Authorization". + +2. **Student Gradebook + Verification** — needs (1) for externals to appear; else parallel. + - design: `specs/2026-06-10-gradebook-student-view-and-verification-design.md` + - plan: `specs/2026-06-10-gradebook-student-view-and-verification-implementation-plan.md` — PR1 BE read+publish → PR2 FE view+publish → PR3 verification + - decisions: `research-notes/2026-06-10-student-gradebook-verification-design-decisions.md` + +3. **Grade Audit History** — external-capture hook needs (1) merged. + - design: `specs/2026-06-10-gradebook-grade-audit-history-design.md` + - plan: `specs/2026-06-10-gradebook-grade-audit-history-implementation-plan.md` — PR1 BE capture+API → PR2 FE history view → (opt) per-submission panel + - decisions: `research-notes/2026-06-10-grade-audit-history-design-decisions.md` + +Roadmap context also in memory: `project_gradebook_overhaul`. Paths above are relative to the +repo root; **in a worktree** the specs exist only in the main repo — read them from +`../coursemology2/docs/superpowers/...`. diff --git a/client/app/bundles/course/gradebook/__tests__/GradebookIndex.test.tsx b/client/app/bundles/course/gradebook/__tests__/GradebookIndex.test.tsx index 3baf0b609f..b3ab065f8f 100644 --- a/client/app/bundles/course/gradebook/__tests__/GradebookIndex.test.tsx +++ b/client/app/bundles/course/gradebook/__tests__/GradebookIndex.test.tsx @@ -1,3 +1,4 @@ +import { useLocation } from 'react-router-dom'; import { fireEvent, render, screen, waitFor, within } from 'test-utils'; import toast from 'lib/hooks/toast'; @@ -5,6 +6,13 @@ import toast from 'lib/hooks/toast'; import fetchGradebook from '../operations'; import GradebookIndex from '../pages/GradebookIndex'; +// TestApp mounts a MemoryRouter, whose location lives in memory and never +// touches window.location. This spy surfaces the router's current search +// string into the DOM so tests can assert on URL changes. +const LocationSearch = (): JSX.Element => ( +
{useLocation().search}
+); + jest.mock('../../container/CourseLoader', () => ({ useCourseContext: (): { courseTitle: string; id: number } => ({ courseTitle: 'Test Course', @@ -74,6 +82,16 @@ const populatedState = { }, }; +const studentsNoAssessmentsState = { + gradebook: { + ...populatedState.gradebook, + categories: [], + tabs: [], + assessments: [], + submissions: [], + }, +}; + const populatedStateWithGamification = { gradebook: { ...populatedState.gradebook, @@ -114,6 +132,55 @@ const populatedStateManagerWeightedOn = { }, }; +const populatedStateExternalInRange = { + gradebook: { + ...populatedState.gradebook, + assessments: [ + { + id: 200, + title: 'External Midterm', + tabId: 10, + maxGrade: 100, + external: true, + capAtMaximum: true, + floorAtZero: true, + }, + ], + submissions: [ + { studentId: 1, assessmentId: 200, submissionId: 200, grade: 90 }, // within [0,100] + ], + }, +}; + +const populatedStateWithOutOfRangeGrade = { + gradebook: { + ...populatedState.gradebook, + assessments: [ + { id: 100, title: 'Quiz 1', tabId: 10, maxGrade: 10 }, + { + id: 200, + title: 'External Midterm', + tabId: 10, + maxGrade: 100, + external: true, + capAtMaximum: true, + floorAtZero: true, + }, + ], + submissions: [ + { studentId: 1, assessmentId: 100, submissionId: 1000, grade: 8 }, + { studentId: 1, assessmentId: 200, submissionId: 200, grade: 110 }, // above max, capped + ], + }, +}; + +const populatedStateWithOutOfRangeGradeWeighted = { + gradebook: { + ...populatedStateWithOutOfRangeGrade.gradebook, + weightedViewEnabled: true, + }, +}; + beforeEach(() => { jest.clearAllMocks(); mockFetchGradebook.mockReturnValue((): Promise => Promise.resolve()); @@ -137,18 +204,46 @@ describe('GradebookIndex', () => { expect(await screen.findByText('Gradebook')).toBeInTheDocument(); }); - it('shows empty students message when there are no students', async () => { - render(, { state: noStudentsState }); + it('shows the grade-link hint in the all-assessments view', async () => { + render(, { state: populatedState }); expect( - await screen.findByText('No students enrolled yet'), + await screen.findByText( + /Click any grade to open that submission and adjust the marks/i, + ), ).toBeInTheDocument(); }); - it('shows empty students message when both assessments and students are absent', async () => { + it('hides the grade-link hint in the weighted-total view', async () => { + render(, { state: populatedStateWithWeightedView }); + const byWeightButton = await screen.findByText(/weighted total/i); + fireEvent.click(byWeightButton); + await screen.findByTestId('gradebook-weighted-table'); + expect( + screen.queryByText( + /Click any grade to open that submission and adjust the marks/i, + ), + ).not.toBeInTheDocument(); + }); + + it('shows empty students message and renders no gradebook table when there are no students', async () => { render(, { state: emptyState }); expect( await screen.findByText('No students enrolled yet'), ).toBeInTheDocument(); + expect( + screen.queryByTestId('gradebook-weighted-table'), + ).not.toBeInTheDocument(); + }); + + it('renders the gradebook table when there are students but no assessments', async () => { + render(, { state: studentsNoAssessmentsState }); + expect( + await screen.findByRole('button', { name: /export/i }), + ).toBeInTheDocument(); + expect(screen.getByText('Alice')).toBeInTheDocument(); + expect( + screen.queryByText('No students enrolled yet'), + ).not.toBeInTheDocument(); }); it('shows error toast when fetch fails', async () => { @@ -171,7 +266,7 @@ describe('GradebookIndex', () => { ).toBeInTheDocument(); }); - it('shows grade-and-gamification hint in column picker when gamification is enabled and no data cols selected', async () => { + it('shows grade-and-gamification hint in column picker after enabling a gamification column with no grade columns selected', async () => { render(, { state: populatedStateWithGamification }); fireEvent.click( await screen.findByRole('button', { name: /select columns/i }), @@ -199,13 +294,39 @@ describe('GradebookIndex', () => { expect(await screen.findByText(/weighted total/i)).toBeInTheDocument(); }); - it('switches to Weighted total view on toggle click', async () => { - render(, { state: populatedStateWithWeightedView }); + it('switches to Weighted total view on toggle click and reflects it in the URL', async () => { + render( + <> + + + , + { state: populatedStateWithWeightedView }, + ); const byWeightButton = await screen.findByText(/weighted total/i); fireEvent.click(byWeightButton); expect( await screen.findByTestId('gradebook-weighted-table'), ).toBeInTheDocument(); + expect(screen.getByTestId('location-search')).toHaveTextContent( + 'view=weighted', + ); + + fireEvent.click(await screen.findByText(/all assessments/i)); + await waitFor(() => + expect(screen.getByTestId('location-search')).not.toHaveTextContent( + 'view=weighted', + ), + ); + }); + + it('starts in Weighted total view when the URL requests it', async () => { + render(, { + state: populatedStateWithWeightedView, + at: ['/?view=weighted'], + }); + expect( + await screen.findByTestId('gradebook-weighted-table'), + ).toBeInTheDocument(); }); it('weighted view does not expose gamification columns in picker', async () => { @@ -226,7 +347,9 @@ describe('GradebookIndex', () => { it('shows the manage button and not the old import/add buttons', async () => { render(, { state: populatedStateManagerWeightedOff }); expect( - await screen.findByRole('button', { name: 'Manage external assessments' }), + await screen.findByRole('button', { + name: 'Manage external assessments', + }), ).toBeVisible(); expect( screen.queryByRole('button', { name: 'Import external assessments' }), @@ -236,6 +359,60 @@ describe('GradebookIndex', () => { ).not.toBeInTheDocument(); }); + it('shows the manage button in the weighted-total view for managers', async () => { + render(, { state: populatedStateManagerWeightedOn }); + const byWeightButton = await screen.findByText(/weighted total/i); + fireEvent.click(byWeightButton); + await screen.findByTestId('gradebook-weighted-table'); + expect( + screen.getByRole('button', { name: 'Manage external assessments' }), + ).toBeVisible(); + }); + + it('does not show the manage button to staff who cannot manage weights', async () => { + render(, { state: populatedState }); + await screen.findByRole('button', { name: /export/i }); // wait for load + expect( + screen.queryByRole('button', { name: 'Manage external assessments' }), + ).not.toBeInTheDocument(); + }); + + describe('out-of-range banner', () => { + it('shows the banner when there are out-of-range grades', async () => { + render(, { state: populatedStateWithOutOfRangeGrade }); + expect( + await screen.findByText(/outside their range/i), + ).toBeInTheDocument(); + }); + + it('shows the weighted-total wording when the weighted view is enabled', async () => { + render(, { + state: populatedStateWithOutOfRangeGradeWeighted, + }); + expect( + await screen.findByText( + /being capped or floored in the weighted total/i, + ), + ).toBeInTheDocument(); + }); + + it('does not show the banner when all grades are in range', async () => { + render(, { state: populatedStateExternalInRange }); + await screen.findByRole('button', { name: /export/i }); // wait for load + expect( + screen.queryByText(/outside their range/i), + ).not.toBeInTheDocument(); + }); + + it('does not show the banner when there are no students', async () => { + render(, { state: noStudentsState }); + await screen.findByText('No students enrolled yet'); + expect( + screen.queryByText(/outside their range/i), + ).not.toBeInTheDocument(); + }); + }); + describe('weighted-view discoverability hint', () => { it('shows the hint to managers when the weighted view is off', async () => { render(, { state: populatedStateManagerWeightedOff }); diff --git a/client/app/bundles/course/gradebook/__tests__/GradebookTable.test.tsx b/client/app/bundles/course/gradebook/__tests__/GradebookTable.test.tsx index 904d4513cb..fa8341e661 100644 --- a/client/app/bundles/course/gradebook/__tests__/GradebookTable.test.tsx +++ b/client/app/bundles/course/gradebook/__tests__/GradebookTable.test.tsx @@ -2,7 +2,12 @@ import userEvent from '@testing-library/user-event'; import { store as appStore } from 'store'; import { render, screen, waitFor, within } from 'test-utils'; -import GradebookTable from '../components/GradebookTable'; +import CourseAPI from 'api/course'; +import toast from 'lib/hooks/toast'; + +import GradebookTable, { + EXTERNAL_ASSESSMENT_BACKGROUND, +} from '../components/GradebookTable'; import type { AssessmentData, CategoryData, @@ -11,6 +16,13 @@ import type { TabData, } from '../types'; +jest.mock('api/course'); + +jest.mock('lib/hooks/toast', () => ({ + __esModule: true, + default: { error: jest.fn(), success: jest.fn() }, +})); + const categories: CategoryData[] = [{ id: 1, title: 'Cat A' }]; const tabs: TabData[] = [{ id: 10, title: 'Tab 1', categoryId: 1 }]; const assessments: AssessmentData[] = [ @@ -79,10 +91,12 @@ const userState = { interface RenderOptions { gamificationEnabled?: boolean; + weightedViewEnabled?: boolean; } const renderTable = ({ gamificationEnabled = true, + weightedViewEnabled = false, }: RenderOptions = {}): void => { render( , { state: userState }, ); @@ -142,6 +157,7 @@ describe('GradebookTable', () => { students={students} submissions={submissions} tabs={tabs} + weightedViewEnabled={false} />, { state: userState }, ); @@ -226,6 +242,33 @@ describe('GradebookTable', () => { expect(await screen.findByText('Max Marks')).toBeInTheDocument(); }); + it('leaves the Max Marks cell blank under non-assessment columns', async () => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ name: true, email: true, 'asn-100': true }), + ); + renderTable(); + await screen.findByText('Max Marks'); + // The Max Marks (second header) row has a "/10" under Quiz 1 and the + // "Max Marks" label under name, but the email column's cell is empty. + // The Max Marks row lives in the TableHead, so its cells are + // (role columnheader), not /cell. + const maxMarksCell = screen.getByText('Max Marks').closest('th')!; + const maxMarksRow = maxMarksCell.closest('tr')!; + const cells = within(maxMarksRow as HTMLElement).getAllByRole( + 'columnheader', + ); + // checkbox spacer | name("Max Marks") | email("") | asn-100("/10") + expect(cells[2]).toHaveTextContent(''); + }); + + it('hides the Max Marks header row when all assessment columns are deselected', async () => { + localStorage.setItem(STORAGE_KEY, JSON.stringify({ 'asn-100': false })); + renderTable(); + await screen.findByText('Alice'); + expect(screen.queryByText('Max Marks')).not.toBeInTheDocument(); + }); + it('renders row selection checkboxes', async () => { renderTableWithAssessmentVisible(); await screen.findByText('Alice'); @@ -411,6 +454,36 @@ describe('GradebookTable', () => { }); }); + describe('no-data-columns hint', () => { + it('warns that export is student-info-only, mentioning gamification, when gamification is on', async () => { + const user = userEvent.setup(); + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ 'asn-100': false, level: false, totalXp: false }), + ); + renderTable({ gamificationEnabled: true }); + await screen.findByText('Alice'); + await user.click(screen.getByRole('button', { name: /select columns/i })); + expect( + await screen.findByText(/no grade or gamification columns selected/i), + ).toBeInTheDocument(); + }); + + it('warns that export is student-info-only, no gamification mention, when gamification is off', async () => { + const user = userEvent.setup(); + localStorage.setItem(STORAGE_KEY, JSON.stringify({ 'asn-100': false })); + renderTable({ gamificationEnabled: false }); + await screen.findByText('Alice'); + await user.click(screen.getByRole('button', { name: /select columns/i })); + expect( + await screen.findByText(/no grade columns selected/i), + ).toBeInTheDocument(); + expect( + screen.queryByText(/gamification columns selected/i), + ).not.toBeInTheDocument(); + }); + }); + describe('external ID column', () => { const studentsWithExtId: StudentData[] = [ { @@ -442,6 +515,7 @@ describe('GradebookTable', () => { students={studs} submissions={[]} tabs={tabs} + weightedViewEnabled={false} />, { state: userState }, ); @@ -514,6 +588,7 @@ describe('GradebookTable', () => { students={[students[1], students[0]]} submissions={submissions} tabs={tabs} + weightedViewEnabled={false} />, { state: userState }, ); @@ -572,6 +647,7 @@ describe('GradebookTable', () => { students={[students[1], students[0]]} // Bob first in raw order submissions={submissions} tabs={tabs} + weightedViewEnabled={false} />, { state: userState }, ); @@ -630,6 +706,7 @@ describe('GradebookTable', () => { students={[students[1], students[0]]} submissions={submissions} tabs={tabs} + weightedViewEnabled={false} />, { state: userState }, ); @@ -678,6 +755,7 @@ describe('GradebookTable', () => { students={studs} submissions={subs} tabs={tabs} + weightedViewEnabled={false} />, { state: userState }, ); @@ -737,6 +815,503 @@ describe('GradebookTable', () => { }); }); + describe('assessment grade cell rendering', () => { + const renderGradeCells = (subs: SubmissionData[]): void => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ name: true, 'asn-100': true }), + ); + render( + , + { state: userState }, + ); + }; + + it('renders "—" when the student has no submission for an assessment', async () => { + renderGradeCells([]); + await screen.findByText('Alice'); + const aliceRow = screen.getByText('Alice').closest('tr')!; + expect( + within(aliceRow as HTMLElement).getByText('—'), + ).toBeInTheDocument(); + }); + + it('renders an empty cell (not "—") for a submission with a null grade', async () => { + // Alice: submission with null grade → empty; Bob: no submission → '—' + renderGradeCells([ + { submissionId: 1, studentId: 1, assessmentId: 100, grade: null }, + ]); + await screen.findByText('Alice'); + const aliceRow = screen.getByText('Alice').closest('tr')!; + expect( + within(aliceRow as HTMLElement).queryByText('—'), + ).not.toBeInTheDocument(); + const bobRow = screen.getByText('Bob').closest('tr')!; + expect(within(bobRow as HTMLElement).getByText('—')).toBeInTheDocument(); + }); + + it('renders the grade as a link to the submission when submissionId is present', async () => { + renderGradeCells([ + { submissionId: 42, studentId: 1, assessmentId: 100, grade: 7 }, + ]); + await screen.findByText('Alice'); + const aliceRow = screen.getByText('Alice').closest('tr')!; + expect( + within(aliceRow as HTMLElement).getByRole('link', { name: '7' }), + ).toBeInTheDocument(); + }); + + it('renders the grade as plain text (no link) when there is no submissionId', async () => { + renderGradeCells([{ studentId: 1, assessmentId: 100, grade: 7 }]); + await screen.findByText('Alice'); + const aliceRow = screen.getByText('Alice').closest('tr')!; + expect( + within(aliceRow as HTMLElement).getByText('7'), + ).toBeInTheDocument(); + expect( + within(aliceRow as HTMLElement).queryByRole('link', { name: '7' }), + ).not.toBeInTheDocument(); + }); + }); + + describe('external assessment columns', () => { + const externalAssessments: AssessmentData[] = [ + { id: 100, title: 'Quiz 1', tabId: 10, maxGrade: 10 }, + { id: -5, title: 'Midterm', tabId: 200, maxGrade: 50, external: true }, + ]; + const externalTabs: TabData[] = [ + { id: 10, title: 'Tab 1', categoryId: 1 }, + { id: 200, title: 'Midterm', categoryId: 2 }, + ]; + const externalCategories: CategoryData[] = [ + { id: 1, title: 'Cat A' }, + { id: 2, title: 'External Assessments' }, + ]; + + const renderWithExternal = (): void => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ name: true, 'asn--5': true, 'asn-100': true }), + ); + render( + , + { state: userState }, + ); + }; + + it('renders the External badge in the external column header', async () => { + renderWithExternal(); + expect(await screen.findByText('External')).toBeVisible(); + }); + + it('tints the external assessment body cells with the external background', async () => { + renderWithExternal(); + const gradeCell = (await screen.findByText('30')).closest('td'); + expect(gradeCell).toHaveStyle({ + backgroundColor: EXTERNAL_ASSESSMENT_BACKGROUND, + }); + }); + + it('keeps the external column header the neutral grey (not the blue tint)', async () => { + renderWithExternal(); + const headerCell = (await screen.findByText('Midterm')).closest('th'); + // grey[100] — the same opaque neutral every other header cell uses, so the + // header reads as a header rather than a coloured band. + expect(headerCell).toHaveStyle({ backgroundColor: 'rgb(245, 245, 245)' }); + expect(headerCell).not.toHaveStyle({ + backgroundColor: EXTERNAL_ASSESSMENT_BACKGROUND, + }); + }); + + it('edits an external grade inline and persists optimistically', async () => { + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockResolvedValue({ + data: { studentId: 1, assessmentId: -5, grade: 45 }, + }); + renderWithExternal(); + const cell = await screen.findByText('30'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterm grade for alice/i, + }); + await userEvent.clear(input); + await userEvent.type(input, '45'); + await userEvent.tab(); + await waitFor(() => + expect(CourseAPI.gradebook.setExternalGrade).toHaveBeenCalledWith(5, { + studentId: 1, + grade: 45, + }), + ); + expect(await screen.findByText('45')).toBeVisible(); + }); + + it('rolls back the cell and keeps the old value when the API rejects', async () => { + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockRejectedValue( + new Error('boom'), + ); + renderWithExternal(); + const cell = await screen.findByText('30'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterm grade for alice/i, + }); + await userEvent.clear(input); + await userEvent.type(input, '45'); + await userEvent.tab(); + await waitFor(() => expect(screen.getByText('30')).toBeVisible()); + }); + + it('names the student and assessment in the failure toast', async () => { + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockRejectedValue( + new Error('boom'), + ); + renderWithExternal(); + const cell = await screen.findByText('30'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterm grade for alice/i, + }); + await userEvent.clear(input); + await userEvent.type(input, '45'); + await userEvent.tab(); + await waitFor(() => + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('Midterm'), + ), + ); + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('Alice'), + ); + }); + + it('keeps regular assessment cells read-only (no input on click)', async () => { + renderWithExternal(); + // Alice has no Quiz 1 submission → '—' is rendered (not an ExternalGradeCell). + // Clicking the '—' in the Quiz 1 column must NOT produce a textbox. + await screen.findByText('Midterm'); // wait for render + const dashCells = screen.getAllByText('—'); + // The first '—' in Alice's row is the Quiz 1 cell (no submission). + await userEvent.click(dashCells[0]); + expect( + screen.queryByRole('textbox', { name: /quiz 1/i }), + ).not.toBeInTheDocument(); + }); + + it('renders the external chip without a manage menu', async () => { + renderWithExternal(); + expect(await screen.findByText('External')).toBeVisible(); + expect( + screen.queryByRole('button', { name: /manage/i }), + ).not.toBeInTheDocument(); + }); + + it('shows an in-flight spinner while the grade save is pending', async () => { + let resolveSave: () => void = () => {}; + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockReturnValue( + new Promise((resolve) => { + resolveSave = (): void => + resolve({ data: { studentId: 1, assessmentId: -5, grade: 45 } }); + }), + ); + renderWithExternal(); + const cell = await screen.findByText('30'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterm grade for alice/i, + }); + await userEvent.clear(input); + await userEvent.type(input, '45'); + await userEvent.tab(); + // Pending: spinner visible, optimistic value already shown. + expect(await screen.findByRole('progressbar')).toBeInTheDocument(); + resolveSave(); + // Resolved: spinner gone, value remains. + await waitFor(() => + expect(screen.queryByRole('progressbar')).not.toBeInTheDocument(), + ); + expect(screen.getByText('45')).toBeInTheDocument(); + }); + + it('caps an external grade at three integer digits (999.99 ceiling)', async () => { + renderWithExternal(); + const cell = await screen.findByText('30'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterm grade for alice/i, + }); + await userEvent.clear(input); + // The fourth integer digit is dropped at entry — the DB column maxes at 999.99. + await userEvent.type(input, '1000'); + expect(input).toHaveValue('100'); + }); + + it('accepts up to two decimal places and rejects a third', async () => { + renderWithExternal(); + const cell = await screen.findByText('30'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterm grade for alice/i, + }); + await userEvent.clear(input); + // A third decimal digit is rejected at entry — DB column is numeric(_, 2). + await userEvent.type(input, '12.345'); + expect(input).toHaveValue('12.34'); + }); + + it('does not call the API when the cell is committed without a change', async () => { + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockClear(); + renderWithExternal(); + const cell = await screen.findByText('30'); + await userEvent.click(cell); + // Blur without editing → commit() sees an unchanged value and returns early. + await userEvent.tab(); + await waitFor(() => expect(screen.getByText('30')).toBeVisible()); + expect(CourseAPI.gradebook.setExternalGrade).not.toHaveBeenCalled(); + }); + + const renderCapped = (grade: number): void => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ name: true, 'asn--5': true }), + ); + render( + , + { state: userState }, + ); + }; + + it('flags a grade above the maximum on a capped assessment', async () => { + renderCapped(60); + expect( + await screen.findByLabelText(/exceeds the maximum/i), + ).toBeInTheDocument(); + }); + + it('does not flag a grade within the maximum', async () => { + renderCapped(40); + await screen.findByText('40'); + expect( + screen.queryByLabelText(/exceeds the maximum/i), + ).not.toBeInTheDocument(); + }); + }); + + describe('ExternalGradeCell — negatives, buttons, tooltip copy', () => { + // External assessments use negative IDs on the frontend; the operation + // negates them before calling the API (so -(-901) = 901 is sent to the server). + const EXT_ASN_ID = -901; + const extAssessments: AssessmentData[] = [ + { + id: EXT_ASN_ID, + title: 'Midterms', + tabId: 300, + maxGrade: 100, + external: true, + floorAtZero: true, + capAtMaximum: true, + }, + ]; + const extTabs: TabData[] = [{ id: 300, title: 'Midterms', categoryId: 3 }]; + const extCategories: CategoryData[] = [ + { id: 3, title: 'External Assessments' }, + ]; + const aliceStudent: StudentData[] = [ + { + id: 1, + name: 'Alice', + email: 'alice@example.com', + externalId: null, + level: 1, + totalXp: 0, + }, + ]; + + beforeEach(() => { + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockClear(); + }); + + const renderForExternal = ({ + grade = null, + weightedViewEnabled = false, + }: { + grade?: number | null; + weightedViewEnabled?: boolean; + } = {}): ReturnType => { + localStorage.setItem( + STORAGE_KEY, + JSON.stringify({ name: true, [`asn-${EXT_ASN_ID}`]: true }), + ); + const subs = + grade !== null + ? [{ studentId: 1, assessmentId: EXT_ASN_ID, grade }] + : []; + return render( + , + { state: userState }, + ); + }; + + it('accepts a negative grade on entry and commits it', async () => { + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockResolvedValue({ + data: { studentId: 1, assessmentId: EXT_ASN_ID, grade: -5 }, + }); + renderForExternal({ grade: null }); + const cell = await screen.findByText('—'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterms grade for alice/i, + }); + await userEvent.clear(input); + await userEvent.type(input, '-5'); + await userEvent.keyboard('{Enter}'); + await waitFor(() => + expect(CourseAPI.gradebook.setExternalGrade).toHaveBeenCalledWith( + -EXT_ASN_ID, + { studentId: 1, grade: -5 }, + ), + ); + }); + + it('shows the below-zero icon when floored and grade < 0', async () => { + renderForExternal({ grade: -5, weightedViewEnabled: true }); + await screen.findByText('-5'); + expect(screen.getByLabelText(/below 0/i)).toBeInTheDocument(); + }); + + it('does not show the below-zero icon when the grade is non-negative', async () => { + renderForExternal({ grade: 5, weightedViewEnabled: true }); + await screen.findByText('5'); + expect(screen.queryByLabelText(/below 0/i)).not.toBeInTheDocument(); + }); + + it('revert button discards the edit without calling the API', async () => { + renderForExternal({ grade: 10 }); + const cell = await screen.findByText('10'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterms grade for alice/i, + }); + await userEvent.clear(input); + await userEvent.type(input, '20'); + // userEvent.pointer is used instead of userEvent.click so that the + // mousedown fires (and its preventDefault keeps input focus) before the + // click, mirroring real-browser behaviour and preventing the onBlur=commit + // from firing before the cancel handler runs. + const revertBtn = screen.getByRole('button', { name: /revert/i }); + await userEvent.pointer({ target: revertBtn, keys: '[MouseLeft]' }); + expect(CourseAPI.gradebook.setExternalGrade).not.toHaveBeenCalled(); + }); + + it('accept button commits the edit via the API', async () => { + (CourseAPI.gradebook.setExternalGrade as jest.Mock).mockResolvedValue({ + data: { studentId: 1, assessmentId: EXT_ASN_ID, grade: 20 }, + }); + renderForExternal({ grade: 10 }); + const cell = await screen.findByText('10'); + await userEvent.click(cell); + const input = screen.getByRole('textbox', { + name: /midterms grade for alice/i, + }); + await userEvent.clear(input); + await userEvent.type(input, '20'); + const acceptBtn = screen.getByRole('button', { name: /accept/i }); + await userEvent.click(acceptBtn); + await waitFor(() => + expect(CourseAPI.gradebook.setExternalGrade).toHaveBeenCalledWith( + -EXT_ASN_ID, + { studentId: 1, grade: 20 }, + ), + ); + }); + + it('over-max tooltip mentions the weighted total only when weighted view is on', async () => { + // Weighted: tooltip should mention "weighted total" + renderForExternal({ grade: 150, weightedViewEnabled: true }); + expect( + await screen.findByLabelText( + /contribution to the weighted total is capped/i, + ), + ).toBeInTheDocument(); + }); + + it('over-max tooltip does not mention weighted total when weighted view is off', async () => { + // Non-weighted: tooltip should just say "exceeds the maximum" without weighted mention + renderForExternal({ grade: 150, weightedViewEnabled: false }); + expect( + await screen.findByLabelText(/exceeds the maximum of 100/i), + ).toBeInTheDocument(); + expect( + screen.queryByLabelText(/weighted total/i), + ).not.toBeInTheDocument(); + }); + + it('below-zero tooltip mentions flooring in the weighted total when weighted view is on', async () => { + renderForExternal({ grade: -5, weightedViewEnabled: true }); + expect( + await screen.findByLabelText(/floored to 0 in the weighted total/i), + ).toBeInTheDocument(); + }); + + it('below-zero tooltip does not mention the weighted total when weighted view is off', async () => { + renderForExternal({ grade: -5, weightedViewEnabled: false }); + expect( + await screen.findByLabelText(/this grade is below 0\./i), + ).toBeInTheDocument(); + expect( + screen.queryByLabelText(/weighted total/i), + ).not.toBeInTheDocument(); + }); + }); + describe('cross-page selection', () => { it('export label reflects selection count across pages', async () => { const user = userEvent.setup(); @@ -759,6 +1334,7 @@ describe('GradebookTable', () => { students={makeStudents(101)} submissions={[]} tabs={tabs} + weightedViewEnabled={false} />, { state: userState }, ); @@ -786,4 +1362,36 @@ describe('GradebookTable', () => { ).toBeInTheDocument(); }); }); + + describe('external assessments', () => { + it('shows external assessment columns by default but keeps native ones hidden', async () => { + render( + , + { state: userState }, + ); + expect(await screen.findByText('Olympiad')).toBeInTheDocument(); + expect(screen.queryByText('Quiz 1')).not.toBeInTheDocument(); + }); + }); }); diff --git a/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx b/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx index dd96bf41a9..ffc59c3e2a 100644 --- a/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx +++ b/client/app/bundles/course/gradebook/__tests__/ImportExternalAssessmentsWizard.test.tsx @@ -14,27 +14,48 @@ const defaultProps = { weightedViewEnabled: true, }; -const renderWizard = (): void => { +const renderWizard = ( + props: Partial<{ weightedViewEnabled: boolean }> = {}, +): void => { render( , ); }; +const advanceToVerifyStep = async (): Promise => { + await userEvent.type(componentNameInput(), 'Midterms'); + await userEvent.click(screen.getByRole('button', { name: /next/i })); + await userEvent.upload( + screen.getByLabelText(/upload/i), + file('Identifier,Midterms\nA001,105\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, + categories: [], + tabs: [], + submissions: [], + assessments: [], + gamificationEnabled: false, + weightedViewEnabled: false, + canManageWeights: true, students, }, }); +const componentNameInput = (): HTMLElement => + screen.getByRole('textbox', { name: 'Component name' }); + const fillOneComponent = async (): Promise => { - await userEvent.type(screen.getByLabelText('Component name'), 'Midterm'); + await userEvent.type(componentNameInput(), 'Midterm'); }; const file = (text: string): File => @@ -49,6 +70,7 @@ describe('ImportExternalAssessmentsWizard', () => { ok: true, unresolved: [], malformed: [], + outOfRange: [], sample: [{ studentName: 'Alice', grades: { Midterm: 41 } }], conflicts: [], }, @@ -71,7 +93,7 @@ describe('ImportExternalAssessmentsWizard', () => { renderWizard(); // Step 1: type a component - await userEvent.type(screen.getByLabelText(/component name/i), '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 })); @@ -85,6 +107,12 @@ describe('ImportExternalAssessmentsWizard', () => { // Step 3: preview shows the sample expect(await screen.findByText('Alice')).toBeVisible(); + expect( + 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 }), ); @@ -95,7 +123,7 @@ describe('ImportExternalAssessmentsWizard', () => { const payload = (CourseAPI.gradebook.importCommit as jest.Mock).mock .calls[0][0]; expect(payload.onConflict).toBe('replace'); - expect(payload.identifierMode).toBe('student_id'); + expect(payload.identifierMode).toBe('external_id'); expect(payload.components[0]).toMatchObject({ name: 'Midterm', weightage: 30, @@ -109,12 +137,13 @@ describe('ImportExternalAssessmentsWizard', () => { ok: false, unresolved: ['ZZZ'], malformed: [], + outOfRange: [], sample: [], conflicts: [], }, }); renderWizard(); - await userEvent.type(screen.getByLabelText(/component name/i), '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 })); @@ -136,7 +165,7 @@ describe('ImportExternalAssessmentsWizard', () => { weightedViewEnabled={false} />, ); - await userEvent.type(screen.getByLabelText(/component name/i), 'Midterm'); + await userEvent.type(componentNameInput(), 'Midterm'); expect(screen.queryByLabelText(/weightage/i)).not.toBeInTheDocument(); expect(screen.getByLabelText(/max marks/i)).toBeInTheDocument(); }); @@ -155,8 +184,27 @@ describe('ImportExternalAssessmentsWizard', () => { weightedViewEnabled={false} />, ); - expect(screen.getByRole('radio', { name: 'External ID' })).toBeInTheDocument(); - expect(screen.queryByRole('radio', { name: 'Student ID' })).not.toBeInTheDocument(); + expect( + screen.getByRole('radio', { name: 'External ID' }), + ).toBeInTheDocument(); + expect( + screen.queryByRole('radio', { name: 'Student ID' }), + ).not.toBeInTheDocument(); + }); + + it('keeps the component input label independent of the selected identifier mode', async () => { + renderWizard(); + expect(componentNameInput()).toBeInTheDocument(); + expect( + screen.queryByRole('textbox', { name: 'External ID' }), + ).not.toBeInTheDocument(); + + await userEvent.click(screen.getByRole('radio', { name: 'Email' })); + + expect(componentNameInput()).toBeInTheDocument(); + expect( + screen.queryByRole('textbox', { name: 'Email' }), + ).not.toBeInTheDocument(); }); it('commits with keep when Keep Existing is clicked on the conflict prompt', async () => { @@ -165,6 +213,7 @@ describe('ImportExternalAssessmentsWizard', () => { ok: true, unresolved: [], malformed: [], + outOfRange: [], sample: [{ studentName: 'Alice', grades: { Midterm: 20 } }], conflicts: [ { @@ -193,7 +242,7 @@ describe('ImportExternalAssessmentsWizard', () => { }, }); renderWizard(); - await userEvent.type(screen.getByLabelText(/component name/i), '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 })); @@ -219,19 +268,42 @@ 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 }, + { + 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, + }, ]), }); await fillOneComponent(); - expect(screen.getByText(/Alice Lim has no External ID/)).toBeInTheDocument(); + expect( + screen.getByText(/Alice Lim has no External ID/), + ).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Next' })).toBeDisabled(); }); 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 }, + { + id: 1, + name: 'Alice Lim', + email: 'a@x.com', + externalId: null, + level: 0, + totalXp: 0, + }, ]), }); await fillOneComponent(); @@ -242,13 +314,22 @@ 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 }, + { + id: 1, + name: 'Alice', + email: 'a@x.com', + externalId: 'E1', + level: 0, + totalXp: 0, + }, ]), }); - await userEvent.type(screen.getByLabelText('Component name'), 'Midterm'); + await userEvent.type(componentNameInput(), 'Midterm'); await userEvent.click(screen.getByRole('button', { name: 'Next' })); expect( - screen.getByText(/Your CSV needs these column headers: External ID, Midterm/), + screen.getByText( + /Your CSV needs these column headers: External ID, Midterm/, + ), ).toBeInTheDocument(); }); @@ -269,12 +350,13 @@ describe('ImportExternalAssessmentsWizard', () => { ok: false, unresolved: ['ZZZ'], malformed: [], + outOfRange: [], sample: [], conflicts: [], }, }); renderWizard(); - await userEvent.type(screen.getByLabelText(/component name/i), '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 })); @@ -295,6 +377,7 @@ describe('ImportExternalAssessmentsWizard', () => { ok: true, unresolved: [], malformed: [], + outOfRange: [], sample: [{ studentName: 'Alice', grades: { Midterm: 20 } }], conflicts: [ { @@ -324,14 +407,16 @@ describe('ImportExternalAssessmentsWizard', () => { }); render( , ); // Step 1: 'Midterm' matches existing → max/weightage locked; just Next. - await userEvent.type(screen.getByLabelText(/component name/i), 'Midterm'); + await userEvent.type(componentNameInput(), 'Midterm'); await userEvent.click(screen.getByRole('button', { name: /next/i })); await userEvent.upload( screen.getByLabelText(/upload/i), @@ -372,7 +457,9 @@ describe('ImportExternalAssessmentsWizard', () => { it('clicking an existing chip inserts a locked row pre-filled with correct max and weight', async () => { render( { it('hides a chip once the corresponding external has been added to the component list', async () => { render( { ); await userEvent.click(screen.getByRole('button', { name: 'Midterm' })); // After clicking, chip disappears (already in the list) - expect(screen.queryByRole('button', { name: 'Midterm' })).not.toBeInTheDocument(); + expect( + screen.queryByRole('button', { name: 'Midterm' }), + ).not.toBeInTheDocument(); }); it('does not render the From existing section when there are no existing externals', () => { @@ -417,4 +508,44 @@ describe('ImportExternalAssessmentsWizard', () => { ); expect(screen.queryByText(/from existing/i)).not.toBeInTheDocument(); }); + + it('warns about out-of-range grades at Verify without blocking import (Q2)', 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: [], + }, + }); + renderWizard({ weightedViewEnabled: true }); + await advanceToVerifyStep(); + expect(screen.getByText(/105 \(exceeds 100\)/)).toBeInTheDocument(); + expect( + screen.getByText(/capped or floored in the weighted total/i), + ).toBeInTheDocument(); + // non-blocking: Confirm import still enabled + expect( + screen.getByRole('button', { name: /Confirm import/i }), + ).toBeEnabled(); + }); + + it('omits the weighted-total wording when weighted view is off (Q2/Q5)', 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: [], + }, + }); + renderWizard({ weightedViewEnabled: false }); + await advanceToVerifyStep(); + expect(screen.getByText(/105 \(exceeds 100\)/)).toBeInTheDocument(); + expect(screen.queryByText(/weighted total/i)).not.toBeInTheDocument(); + }); }); diff --git a/client/app/bundles/course/gradebook/__tests__/OutOfRangeAlert.test.tsx b/client/app/bundles/course/gradebook/__tests__/OutOfRangeAlert.test.tsx new file mode 100644 index 0000000000..3cfdec7c06 --- /dev/null +++ b/client/app/bundles/course/gradebook/__tests__/OutOfRangeAlert.test.tsx @@ -0,0 +1,119 @@ +import { render, screen } from 'test-utils'; + +import OutOfRangeAlert from '../components/OutOfRangeAlert'; + +describe('OutOfRangeAlert', () => { + it('renders nothing when there are no out-of-range grades', async () => { + render( + , + ); + + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + }); + + it('summarises counts, assessment names, and prompts review before export', async () => { + render( + , + ); + + expect( + await screen.findByText( + /3 grades in the external assessments External Quiz and Final Exam/i, + ), + ).toBeInTheDocument(); + + expect(screen.getByText(/review before exporting/i)).toBeInTheDocument(); + }); + + it('joins three or more assessment names with commas and a final "and"', async () => { + render( + , + ); + + expect( + await screen.findByText(/Quiz A, Quiz B and Quiz C/i), + ).toBeInTheDocument(); + }); + + it('uses singular grade/assessment wording and a bare name for one grade', async () => { + render( + , + ); + + expect( + await screen.findByText( + /1 grade in the external assessment External Quiz is outside their range/i, + ), + ).toBeInTheDocument(); + }); + + it('names the weighted-total capping/flooring when the weighted view is on', async () => { + render( + , + ); + + expect( + await screen.findByText(/being capped or floored in the weighted total/i), + ).toBeInTheDocument(); + }); + + it('omits the weighted-total clause when the weighted view is off', async () => { + render( + , + ); + + expect(await screen.findByRole('alert')).toBeInTheDocument(); + expect( + screen.queryByText(/being capped or floored in the weighted total/i), + ).not.toBeInTheDocument(); + }); + + it('does not mention the weighted total when weighting is off', async () => { + render( + , + ); + const alert = await screen.findByRole('alert'); + expect(alert.textContent).not.toMatch(/weighted total/i); + expect(alert.textContent).not.toMatch(/capped or floored/i); + }); + + it('mentions the weighted total when weighting is on', async () => { + render( + , + ); + const alert = await screen.findByRole('alert'); + expect(alert.textContent).toMatch(/weighted total/i); + }); +}); diff --git a/client/app/bundles/course/gradebook/__tests__/outOfRange.test.ts b/client/app/bundles/course/gradebook/__tests__/outOfRange.test.ts new file mode 100644 index 0000000000..91852f61d1 --- /dev/null +++ b/client/app/bundles/course/gradebook/__tests__/outOfRange.test.ts @@ -0,0 +1,88 @@ +import type { AssessmentData, SubmissionData } from 'types/course/gradebook'; + +import { outOfRangeSummary } from '../outOfRange'; + +const MIDTERMS = 'Midterms'; + +const ext = (over: Partial): AssessmentData => ({ + id: 1, + title: MIDTERMS, + tabId: -1, + maxGrade: 100, + external: true, + floorAtZero: true, + capAtMaximum: true, + ...over, +}); +const sub = (assessmentId: number, grade: number | null): SubmissionData => ({ + studentId: 1, + assessmentId, + grade, +}); + +describe('outOfRangeSummary', () => { + it('counts grades above max only when capped, below 0 only when floored', () => { + const assessments = [ + ext({ id: 1, capAtMaximum: true, floorAtZero: true }), + ext({ id: 2, capAtMaximum: false, floorAtZero: false, maxGrade: 50 }), + ]; + const submissions = [ + sub(1, 105), // counted (capped) + sub(1, -3), // counted (floored) + sub(2, 999), // NOT counted (cap off) + sub(2, -9), // NOT counted (floor off) + ]; + expect(outOfRangeSummary(assessments, submissions)).toEqual({ + gradeCount: 2, + assessmentNames: [MIDTERMS], + }); + }); + + it('counts each offending grade but de-dups assessment names by id', () => { + const assessments = [ + ext({ id: 1, title: 'Quiz A', maxGrade: 100 }), + ext({ id: 2, title: 'Quiz B', maxGrade: 100 }), + ]; + const submissions = [ + sub(1, 105), // over + sub(1, -1), // under (same assessment) + sub(2, 200), // over (different assessment) + ]; + expect(outOfRangeSummary(assessments, submissions)).toEqual({ + gradeCount: 3, + assessmentNames: ['Quiz A', 'Quiz B'], + }); + }); + + it('treats grade exactly at maxGrade as in range when capped', () => { + expect( + outOfRangeSummary([ext({ id: 1, maxGrade: 100 })], [sub(1, 100)]), + ).toEqual({ gradeCount: 0, assessmentNames: [] }); + }); + + it('treats grade exactly at 0 as in range when floored', () => { + expect( + outOfRangeSummary([ext({ id: 1, floorAtZero: true })], [sub(1, 0)]), + ).toEqual({ gradeCount: 0, assessmentNames: [] }); + }); + + it('ignores null grades and in-range grades', () => { + expect( + outOfRangeSummary([ext({ id: 1 })], [sub(1, null), sub(1, 50)]), + ).toEqual({ gradeCount: 0, assessmentNames: [] }); + }); + + it('ignores native (non-external) assessments', () => { + const native = ext({ id: 9, external: false }); + expect(outOfRangeSummary([native], [sub(9, 999)])).toEqual({ + gradeCount: 0, + assessmentNames: [], + }); + }); + + it('ignores submissions with no matching assessment', () => { + expect( + outOfRangeSummary([ext({ id: 1 })], [sub(404, 999)]), + ).toEqual({ gradeCount: 0, assessmentNames: [] }); + }); +}); diff --git a/client/app/bundles/course/gradebook/components/GradebookTable.tsx b/client/app/bundles/course/gradebook/components/GradebookTable.tsx index c30b362ea2..43373cbd7a 100644 --- a/client/app/bundles/course/gradebook/components/GradebookTable.tsx +++ b/client/app/bundles/course/gradebook/components/GradebookTable.tsx @@ -8,10 +8,14 @@ import { useState, } from 'react'; import { defineMessages } from 'react-intl'; +import Check from '@mui/icons-material/Check'; +import Close from '@mui/icons-material/Close'; +import InfoOutlined from '@mui/icons-material/InfoOutlined'; import { Checkbox, Chip, CircularProgress, + IconButton, Paper, type SxProps, Table, @@ -25,7 +29,9 @@ import { type Theme, Tooltip, } from '@mui/material'; +import { lighten } from '@mui/material/styles'; import { flexRender } from '@tanstack/react-table'; +import palette from 'theme/palette'; import Link from 'lib/components/core/Link'; import type { @@ -72,12 +78,30 @@ const COL_WIDTHS = { const CHECKBOX_WIDTH = 56; +// Faint blue wash that marks external-assessment columns. Kept very light (4% of +// info.main) so the band reads as a subtle tint, not a coloured header. +export const EXTERNAL_ASSESSMENT_BACKGROUND = lighten(palette.info.main, 0.96); + const getColWidth = (id: string): number => COL_WIDTHS[id as keyof typeof COL_WIDTHS] ?? COL_WIDTHS.assessment; const isLeftAligned = (id: string): boolean => id === 'name' || id === 'email' || id === 'externalId'; +// Resolves a column's stable key. TanStack columns identify by `id`, falling back +// to `of` for plain accessor columns; this single helper keeps that rule in one +// place instead of repeating the `??` across every column loop. +const colKey = (c: ColumnTemplate): string => + c.id ?? (c.of as string); + +// The two grey separator weights used across the table. `gridLine` is the hairline +// cell grid; `seamLine` is the heavier 1px edge that bounds the frozen header/column +// block (a full pixel survives sticky-scroll compositing where 0.5px can drop out). +const gridLine = (theme: Theme): string => + `0.5px solid ${theme.palette.grey[200]}`; +const seamLine = (theme: Theme): string => + `1px solid ${theme.palette.grey[200]}`; + const translations = defineMessages({ searchStudents: { id: 'course.gradebook.GradebookIndex.searchStudents', @@ -127,15 +151,34 @@ const translations = defineMessages({ }, gradeSaveError: { id: 'course.gradebook.GradebookTable.gradeSaveError', - defaultMessage: 'Could not save the {title} grade for {name}. Please try again.', + defaultMessage: + 'Could not save the {title} grade for {name}. Please try again.', + }, + gradeExceedsMax: { + id: 'course.gradebook.GradebookTable.gradeExceedsMax', + defaultMessage: 'This grade exceeds the maximum of {max}.', + }, + gradeExceedsMaxWeighted: { + id: 'course.gradebook.GradebookTable.gradeExceedsMaxWeighted', + defaultMessage: + 'This grade exceeds the maximum of {max}; its contribution to the weighted total is capped at {max}.', + }, + gradeBelowZero: { + id: 'course.gradebook.GradebookTable.gradeBelowZero', + defaultMessage: 'This grade is below 0.', + }, + gradeBelowZeroWeighted: { + id: 'course.gradebook.GradebookTable.gradeBelowZeroWeighted', + defaultMessage: + 'This grade is below 0; it is floored to 0 in the weighted total.', }, - externalMaxAria: { - id: 'course.gradebook.GradebookTable.externalMaxAria', - defaultMessage: '{title} max marks', + acceptEdit: { + id: 'course.gradebook.GradebookTable.acceptEdit', + defaultMessage: 'Accept', }, - maxSaveError: { - id: 'course.gradebook.GradebookTable.maxSaveError', - defaultMessage: 'Could not save the max marks. Please try again.', + revertEdit: { + id: 'course.gradebook.GradebookTable.revertEdit', + defaultMessage: 'Revert', }, }); @@ -206,16 +249,24 @@ HeaderLabel.displayName = 'HeaderLabel'; const ExternalGradeCell = ({ assessmentId, + capAtMaximum, + floorAtZero, + maxGrade, studentId, studentName, title, value, + weightedViewEnabled, }: { assessmentId: number; + capAtMaximum: boolean; + floorAtZero: boolean; + maxGrade: number; studentId: number; studentName: string; title: string; value: number | null | undefined; + weightedViewEnabled: boolean; }): JSX.Element => { const { t } = useTranslation(); const dispatch = useAppDispatch(); @@ -245,27 +296,83 @@ const ExternalGradeCell = ({ } }; + const cancel = (): void => setEditing(false); + + const exceedsMax = + capAtMaximum && localValue != null && localValue > maxGrade; + const belowZero = floorAtZero && localValue != null && localValue < 0; + + const exceedsMsg = weightedViewEnabled + ? translations.gradeExceedsMaxWeighted + : translations.gradeExceedsMax; + const belowMsg = weightedViewEnabled + ? translations.gradeBelowZeroWeighted + : translations.gradeBelowZero; + if (editing) { return ( - setText(e.target.value)} - onKeyDown={(e) => { - if (e.key === 'Enter') commit(); - if (e.key === 'Escape') setEditing(false); + + > + { + const next = e.target.value; + if (next === '' || /^-?\d{0,3}(\.\d{0,2})?$/.test(next)) + setText(next); + }} + onKeyDown={(e) => { + if (e.key === 'Enter') commit(); + if (e.key === 'Escape') cancel(); + }} + size="small" + sx={{ + width: 72, + '& .MuiInputBase-root': { + width: 72, + }, + }} + value={text} + variant="standard" + /> + {/* onMouseDown preventDefault keeps focus so the input's onBlur=commit + does not fire before onClick — critical for Revert (a blur-commit + would save the very edit we are discarding). */} + e.preventDefault()} + size="small" + sx={{ flexShrink: 0 }} + > + + + e.preventDefault()} + size="small" + sx={{ flexShrink: 0 }} + > + + + ); } @@ -276,8 +383,6 @@ const ExternalGradeCell = ({ setEditing(true); }} role="button" - // Fill the cell so the whole editable column band is the click target, - // not just the digits at the right edge. style={{ cursor: 'pointer', display: 'flex', @@ -285,77 +390,34 @@ const ExternalGradeCell = ({ justifyContent: 'flex-end', gap: 4, width: '100%', + height: '100%', }} tabIndex={0} > + {exceedsMax && ( + + + + )} + {belowZero && ( + + + + )} {saving && } {localValue == null ? '—' : localValue} ); }; -const ExternalMaxCell = ({ - assessmentId, - title, - value, -}: { - assessmentId: number; - title: string; - value: number; -}): JSX.Element => { - const { t } = useTranslation(); - const dispatch = useAppDispatch(); - const [editing, setEditing] = useState(false); - const [text, setText] = useState(''); - - const commit = async (): Promise => { - setEditing(false); - const next = Number(text.trim()); - if (text.trim() === '' || Number.isNaN(next) || next < 0) return; - if (next === value) return; - try { - await dispatch(updateExternalMaxGrade(assessmentId, next)); - } catch { - toast.error(t(translations.maxSaveError)); - } - }; - - if (editing) { - return ( - setText(e.target.value)} - onKeyDown={(e) => { - if (e.key === 'Enter') commit(); - if (e.key === 'Escape') setEditing(false); - }} - size="small" - value={text} - variant="standard" - /> - ); - } - - return ( - { - setText(String(value)); - setEditing(true); - }} - role="button" - style={{ cursor: 'pointer' }} - tabIndex={0} - > - {`/${value}`} - - ); -}; - interface GradebookRow { studentId: number; name: string; @@ -376,6 +438,7 @@ interface GradebookTableProps { courseTitle: string; courseId: number; gamificationEnabled: boolean; + weightedViewEnabled: boolean; /** Optional action rendered in the toolbar, left of the column picker. */ toolbarAction?: JSX.Element; } @@ -389,6 +452,7 @@ const GradebookTable = ({ courseTitle, courseId, gamificationEnabled, + weightedViewEnabled, toolbarAction, }: GradebookTableProps): JSX.Element => { const { t } = useTranslation(); @@ -516,10 +580,14 @@ const GradebookTable = ({ return ( ); } @@ -540,7 +608,13 @@ const GradebookTable = ({ }); }); return cols; - }, [assessments, gamificationEnabled, hasExternalIds, t]); + }, [ + assessments, + gamificationEnabled, + hasExternalIds, + t, + weightedViewEnabled, + ]); const assessmentMaxGrades = useMemo( () => new Map(assessments.map((a) => [a.id, a.maxGrade])), @@ -555,6 +629,16 @@ const GradebookTable = ({ [assessments], ); + const externalAssessmentColumnIds = useMemo( + () => + new Set( + assessments + .filter((assessment) => assessment.external) + .map((assessment) => buildAssessmentColumnId(assessment.id)), + ), + [assessments], + ); + const columnPicker = useMemo( () => ({ render: (context: ColumnPickerRenderContext) => ( @@ -635,9 +719,7 @@ const GradebookTable = ({ const visibility = toolbar?.getColumnVisibility?.() ?? {}; const isColVisible = (id: string): boolean => visibility[id] ?? true; - const visibleCols = columns.filter((c) => - isColVisible(c.id ?? (c.of as string)), - ); + const visibleCols = columns.filter((c) => isColVisible(colKey(c))); const sortByColId = new Map( (header?.headers ?? []).map( @@ -668,7 +750,9 @@ const GradebookTable = ({ ...toolbar.columnPicker, directExportLabel, directExportTooltip: - selectedCount === 0 ? t(translations.exportAllTooltip) : undefined, + selectedCount === 0 + ? t(translations.exportAllTooltip) + : undefined, }, }), } @@ -677,10 +761,7 @@ const GradebookTable = ({ const totalWidth = useMemo( () => CHECKBOX_WIDTH + - visibleCols.reduce((sum, c) => { - const id = c.id ?? (c.of as string); - return sum + getColWidth(id); - }, 0), + visibleCols.reduce((sum, c) => sum + getColWidth(colKey(c)), 0), [visibleCols], ); @@ -689,10 +770,7 @@ const GradebookTable = ({ const toggleAllRows = (): void => body.toggleAllFiltered?.(); const hasVisibleAssessments = useMemo( - () => - visibleCols.some( - (c) => parseAssessmentColumnId(c.id ?? (c.of as string)) !== null, - ), + () => visibleCols.some((c) => parseAssessmentColumnId(colKey(c)) !== null), [visibleCols], ); @@ -714,7 +792,7 @@ const GradebookTable = ({ () => new Map( visibleCols.map((c) => { - const id = c.id ?? (c.of as string); + const id = colKey(c); return [id, (f: boolean): void => onSingleLine(id, f)]; }), ), @@ -749,14 +827,14 @@ const GradebookTable = ({ border: 0, // Draws the cell grid without relying on collapsed borders. - borderBottom: `0.5px solid ${theme.palette.grey[200]}`, + borderBottom: gridLine(theme), }, })} > {visibleCols.map((c) => { - const id = c.id ?? (c.of as string); + const id = colKey(c); return ; })} @@ -782,7 +860,7 @@ const GradebookTable = ({ // compositing where 0.5px can drop and let the body show // through the row1/row2 seam. '&&': { - borderBottom: `1px solid ${theme.palette.grey[200]}`, + borderBottom: seamLine(theme), }, })} > @@ -794,17 +872,36 @@ const GradebookTable = ({ /> {visibleCols.map((c) => { - const id = c.id ?? (c.of as string); + const id = colKey(c); const label = typeof c.title === 'string' ? c.title : id; const isLeft = isLeftAligned(id); const fits = headerFits[id] ?? false; const sort = sortByColId.get(id); - const asnId = parseAssessmentColumnId(id); - const isExternalCol = - asnId !== null && - assessments.find((a) => a.id === asnId)?.external === - true; - const labelNode = ( + const isExternalCol = externalAssessmentColumnIds.has(id); + // Wrap a header label in the clickable sort control (or leave + // it bare when the column isn't sortable). `extraSx` lets the + // external variant flip the sort arrow to the label's left. + const withSort = ( + inner: JSX.Element, + extraSx?: Record, + ): JSX.Element => + sort ? ( + + {inner} + + ) : ( + inner + ); + const sortedLabel = withSort( - + , ); - const sortedLabel = sort ? ( - - {labelNode} - - ) : ( - labelNode + const externalSortedLabel = withSort( + + + {label} + + , + { + display: 'inline-flex', + flexDirection: 'row-reverse', + justifyContent: 'flex-start', + }, ); return ( {isExternalCol ? ( - {sortedLabel} + {externalSortedLabel} ) : ( @@ -878,8 +995,8 @@ const GradebookTable = ({ // body never shows through on scroll. `& .MuiTableCell-root` // (0,2,0) outranks the table's `& th` rule (0,1,1). '& .MuiTableCell-root': { - borderTop: `1px solid ${theme.palette.grey[200]}`, - borderBottom: `1px solid ${theme.palette.grey[200]}`, + borderTop: seamLine(theme), + borderBottom: seamLine(theme), }, })} > @@ -897,23 +1014,11 @@ const GradebookTable = ({ }} /> {visibleCols.map((c) => { - const id = c.id ?? (c.of as string); + const id = colKey(c); const asnId = parseAssessmentColumnId(id); - const asn = - asnId !== null - ? assessments.find((a) => a.id === asnId) - : undefined; let cellNode: React.ReactNode = ''; if (id === 'name') cellNode = t(translations.maxMarks); - else if (asn?.external) { - cellNode = ( - - ); - } else if (asnId !== null) { + else if (asnId !== null) { const maxGrade = assessmentMaxGrades.get(asnId); cellNode = maxGrade != null ? `/${maxGrade}` : ''; } @@ -929,8 +1034,8 @@ const GradebookTable = ({ zIndex: 3, // Continue the frozen region's right edge. '&&': { - borderTop: `1px solid ${theme.palette.grey[200]}`, - borderRight: `1px solid ${theme.palette.grey[200]}`, + borderTop: seamLine(theme), + borderRight: seamLine(theme), }, }), })} @@ -969,10 +1074,7 @@ const GradebookTable = ({ // layer and always paints. Row 0's top edge is already // the header cell's (higher z-index) bottom border. borderBottom: 'none', - borderTop: - idx === 0 - ? undefined - : `0.5px solid ${theme.palette.grey[200]}`, + borderTop: idx === 0 ? undefined : gridLine(theme), })} > cell.column.id !== 'rowSelector') .map((cell) => { + const isExternalCol = externalAssessmentColumnIds.has( + cell.column.id, + ); // Sticky cover for the frozen `name` column, mirroring // the checkbox cell above. Declared as a directly-typed // const so the callback is contextually typed (a ternary @@ -998,28 +1103,28 @@ const GradebookTable = ({ // the separator as the lower row's `borderTop`, not a // covered `borderBottom`. borderBottom: 'none', - borderTop: - idx === 0 - ? undefined - : `0.5px solid ${theme.palette.grey[200]}`, + borderTop: idx === 0 ? undefined : gridLine(theme), // Continue the frozen region's right edge down the data // rows. `&&` (0,2,0) beats the table's `& td` border // rule (0,1,1). '&&': { - borderRight: `1px solid ${theme.palette.grey[200]}`, + borderRight: seamLine(theme), }, }); + let cellSx: SxProps | undefined; + if (cell.column.id === 'name') cellSx = nameCellSx; + else if (isExternalCol) + // bgcolor is the faint external-column tint. + cellSx = { + bgcolor: EXTERNAL_ASSESSMENT_BACKGROUND, + }; return ( {flexRender( cell.column.columnDef.cell, diff --git a/client/app/bundles/course/gradebook/components/GradebookWeightedTable.tsx b/client/app/bundles/course/gradebook/components/GradebookWeightedTable.tsx index 7307470247..94a7605051 100644 --- a/client/app/bundles/course/gradebook/components/GradebookWeightedTable.tsx +++ b/client/app/bundles/course/gradebook/components/GradebookWeightedTable.tsx @@ -190,6 +190,8 @@ interface Props { canManageWeights: boolean; courseTitle: string; courseId: number; + /** Optional action rendered in the toolbar, left of the column picker. */ + toolbarAction?: JSX.Element; } // How many decimal places a single value needs (0, 1, or 2). @@ -228,6 +230,7 @@ const GradebookWeightedTable = ({ canManageWeights, courseTitle, courseId, + toolbarAction, }: Props): JSX.Element => { const { t } = useTranslation(); const [configureOpen, setConfigureOpen] = useState(false); @@ -570,6 +573,7 @@ const GradebookWeightedTable = ({ ]} value={displayMode} /> + {toolbarAction} {canManageWeights && (