diff --git a/app/controllers/course/gradebook_controller.rb b/app/controllers/course/gradebook_controller.rb index 3a12f399c8..31834a28b5 100644 --- a/app/controllers/course/gradebook_controller.rb +++ b/app/controllers/course/gradebook_controller.rb @@ -64,6 +64,7 @@ def parse_weight_entry(entry) tab_id: entry[:tabId].to_i, weight: entry[:weight].to_f.round(2), weight_mode: entry[:weightMode] || 'equal', + keep_highest: entry[:keepHighest].to_i, excluded_assessment_ids: (entry[:excludedAssessmentIds] || []).map(&:to_i), assessment_weights: (entry[:assessmentWeights] || []).map do |aw| { assessment_id: aw[:assessmentId].to_i, weight: aw[:weight].to_f.round(2) } @@ -73,7 +74,7 @@ def parse_weight_entry(entry) def update_weights_params params.permit( - weights: [:tabId, :weight, :weightMode, + weights: [:tabId, :weight, :weightMode, :keepHighest, excludedAssessmentIds: [], assessmentWeights: [:assessmentId, :weight]] ) end @@ -117,6 +118,7 @@ def serialize_level_contribution(config) def serialize_weight_updates(updates) updates.map do |u| entry = { tabId: u[:tab_id], weight: u[:weight], weightMode: u[:weight_mode].to_s, + keepHighest: u[:keep_highest], excludedAssessmentIds: u[:excluded_assessment_ids] } if u[:weight_mode].to_s == 'custom' entry[:assessmentWeights] = u[:assessment_weights].map do |aw| diff --git a/app/models/course/gradebook/tab_contribution.rb b/app/models/course/gradebook/tab_contribution.rb index 3489794d35..d72d070120 100644 --- a/app/models/course/gradebook/tab_contribution.rb +++ b/app/models/course/gradebook/tab_contribution.rb @@ -41,7 +41,7 @@ def self.apply_entry(course, tabs_by_id, entry) contribution = find_or_initialize_by(tab_id: tab.id) contribution.course = course - contribution.assign_attributes(weight: entry[:weight], weight_mode: mode) + contribution.assign_attributes(weight: entry[:weight], weight_mode: mode, keep_highest: entry[:keep_highest] || 0) contribution.save! excluded_ids = entry[:excluded_assessment_ids] || [] diff --git a/app/views/course/gradebook/index.json.jbuilder b/app/views/course/gradebook/index.json.jbuilder index a493cfe953..623a70d813 100644 --- a/app/views/course/gradebook/index.json.jbuilder +++ b/app/views/course/gradebook/index.json.jbuilder @@ -15,6 +15,7 @@ json.tabs @tabs do |tab| contribution = @tab_contributions[tab.id] json.gradebookWeight (contribution&.weight || 0).to_f json.weightMode(contribution&.weight_mode || 'equal') + json.keepHighest(contribution&.keep_highest || 0) end end diff --git a/client/app/bundles/course/gradebook/__tests__/ConfigureWeightsPrompt.test.tsx b/client/app/bundles/course/gradebook/__tests__/ConfigureWeightsPrompt.test.tsx index 78949dd4d2..9626bd88d5 100644 --- a/client/app/bundles/course/gradebook/__tests__/ConfigureWeightsPrompt.test.tsx +++ b/client/app/bundles/course/gradebook/__tests__/ConfigureWeightsPrompt.test.tsx @@ -228,6 +228,7 @@ describe('', () => { tabId: 10, weight: 50, weightMode: 'custom', + keepHighest: 0, excludedAssessmentIds: [], assessmentWeights: [ { assessmentId: 101, weight: 25 }, @@ -238,6 +239,7 @@ describe('', () => { tabId: 11, weight: 50, weightMode: 'equal', + keepHighest: 0, excludedAssessmentIds: [], }, ], @@ -539,6 +541,7 @@ describe('per-assessment exclusion', () => { expect(arg[0]).toMatchObject({ tabId: 10, weight: 50, + keepHighest: 0, excludedAssessmentIds: [101, 102], }); }); @@ -612,6 +615,192 @@ describe('per-assessment exclusion', () => { expect(screen.queryByText(/no weights set yet/i)).not.toBeInTheDocument(); }); }); + + it('shows assessment weights sum footer in custom mode', () => { + setup(); + fireEvent.click( + within(modeGroup('Assignments')).getByRole('radio', { name: /custom/i }), + ); + // Expand to see the footer + // custom mode auto-expands, so just check the footer + expect(screen.getByText(/Assessment weights:/i)).toBeInTheDocument(); + }); +}); + +describe('per-assessment exclusion (extended)', () => { + beforeEach(() => jest.clearAllMocks()); + + it('shows "Excluded" label in custom mode for excluded assessment', async () => { + setup({ + assessments: [ + { + id: 101, + title: A1, + tabId: 10, + maxGrade: 100, + gradebookExcluded: true, + }, + { id: 102, title: A2, tabId: 10, maxGrade: 50 }, + ], + tabs: [ + { + id: 10, + title: 'Assignments', + categoryId: 1, + gradebookWeight: 50, + weightMode: 'custom', + }, + ], + }); + fireEvent.click(screen.getAllByRole('button', { name: '' })[0]); + expect(await screen.findByText('Excluded')).toBeInTheDocument(); + }); + + it('does not show "Excluded" label in equal mode for excluded assessment', async () => { + setup({ + assessments: [ + { + id: 101, + title: A1, + tabId: 10, + maxGrade: 100, + gradebookExcluded: true, + }, + { id: 102, title: A2, tabId: 10, maxGrade: 50 }, + ], + }); + fireEvent.click(screen.getAllByRole('button', { name: '' })[0]); + // In equal mode excluded shows "Excluded" text too + expect(await screen.findByText('Excluded')).toBeInTheDocument(); + }); +}); + +describe('keep-highest control', () => { + const TOGGLE = 'Enable keep highest for Assignments'; + const INPUT = 'Keep highest for Assignments'; + const three = [ + { id: 101, title: A1, tabId: 10, maxGrade: 100 }, + { id: 102, title: A2, tabId: 10, maxGrade: 50 }, + { id: 103, title: 'Assignment 3', tabId: 10, maxGrade: 80 }, + ]; + + beforeEach(() => jest.clearAllMocks()); + + it('renders a keep-highest checkbox; number field hidden until checked', () => { + setup({ assessments: three }); + expect(screen.getByRole('checkbox', { name: TOGGLE })).toBeInTheDocument(); + expect( + screen.queryByRole('spinbutton', { name: INPUT }), + ).not.toBeInTheDocument(); + fireEvent.click(screen.getByRole('checkbox', { name: TOGGLE })); + expect(screen.getByRole('spinbutton', { name: INPUT })).toBeInTheDocument(); + }); + + it('shows a visible "Keep highest" text label next to the checkbox', () => { + setup({ assessments: three }); + expect(screen.getByText('Keep highest')).toBeInTheDocument(); + }); + + it('defaults the count to included − 1 when checked', () => { + setup({ assessments: three }); // 3 assessments -> default to 2 + fireEvent.click(screen.getByRole('checkbox', { name: TOGGLE })); + expect(screen.getByRole('spinbutton', { name: INPUT })).toHaveValue(2); + }); + + it('hides checkbox + field in custom mode', () => { + setup({ assessments: three }); + fireEvent.click( + within(modeGroup('Assignments')).getByRole('radio', { name: /custom/i }), + ); + expect( + screen.queryByRole('checkbox', { name: TOGGLE }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole('spinbutton', { name: INPUT }), + ).not.toBeInTheDocument(); + }); + + it('seeds the field (checkbox pre-checked) from tab.keepHighest', () => { + setup({ + assessments: three, + tabs: [ + { + id: 10, + title: 'Assignments', + categoryId: 1, + gradebookWeight: 50, + keepHighest: 2, + }, + { id: 11, title: 'Optional', categoryId: 1, gradebookWeight: 50 }, + ], + }); + expect(screen.getByRole('checkbox', { name: TOGGLE })).toBeChecked(); + expect(screen.getByRole('spinbutton', { name: INPUT })).toHaveValue(2); + }); + + it('disables the checkbox when only one assessment is included', () => { + setup({ + assessments: [{ id: 101, title: A1, tabId: 10, maxGrade: 100 }], + }); + expect(screen.getByRole('checkbox', { name: TOGGLE })).toBeDisabled(); + }); + + it('sends keepHighest in the save payload', async () => { + setup({ assessments: three }); + fireEvent.click(screen.getByRole('checkbox', { name: TOGGLE })); + fireEvent.click(screen.getByRole('button', { name: /save/i })); + await waitFor(() => + expect(operations.updateGradebookWeights).toHaveBeenCalled(), + ); + const arg = (operations.updateGradebookWeights as jest.Mock).mock + .calls[0][0]; + const tab10 = arg.find((e: { tabId: number }) => e.tabId === 10); + expect(tab10.keepHighest).toBe(2); + }); + + it('unchecking sends keepHighest 0', async () => { + setup({ + assessments: three, + tabs: [ + { + id: 10, + title: 'Assignments', + categoryId: 1, + gradebookWeight: 50, + keepHighest: 2, + }, + { id: 11, title: 'Optional', categoryId: 1, gradebookWeight: 50 }, + ], + }); + // checkbox is pre-checked; uncheck it + fireEvent.click(screen.getByRole('checkbox', { name: TOGGLE })); + fireEvent.click(screen.getByRole('button', { name: /save/i })); + await waitFor(() => + expect(operations.updateGradebookWeights).toHaveBeenCalled(), + ); + const arg = (operations.updateGradebookWeights as jest.Mock).mock + .calls[0][0]; + const tab10 = arg.find((e: { tabId: number }) => e.tabId === 10); + expect(tab10.keepHighest).toBe(0); + }); + + it('blocks saving on non-integer input but not on keep > included (overflow)', async () => { + setup({ assessments: three }); + fireEvent.click(screen.getByRole('checkbox', { name: TOGGLE })); + const input = screen.getByRole('spinbutton', { name: INPUT }); + + // overflow: keep=5 > included=3 -> warning but save NOT blocked + fireEvent.change(input, { target: { value: '5' } }); + expect(screen.getByRole('button', { name: /save/i })).not.toBeDisabled(); + expect( + screen.getByText(/keeps more assessments than it contains/i), + ).toBeInTheDocument(); + + // non-integer (0): should block saving + fireEvent.change(input, { target: { value: '0' } }); + expect(screen.getByRole('button', { name: /save/i })).toBeDisabled(); + expect(screen.getByText(/keep at least 1/i)).toBeInTheDocument(); + }); }); describe('level contribution section', () => { diff --git a/client/app/bundles/course/gradebook/__tests__/WeightedGradebookTable.test.tsx b/client/app/bundles/course/gradebook/__tests__/WeightedGradebookTable.test.tsx index fcfdb31736..15314399d9 100644 --- a/client/app/bundles/course/gradebook/__tests__/WeightedGradebookTable.test.tsx +++ b/client/app/bundles/course/gradebook/__tests__/WeightedGradebookTable.test.tsx @@ -740,6 +740,17 @@ describe('WeightedGradebookTable', () => { expect(nameCol().style.width).toBe('150px'); }); + // The total column's must carry an explicit width: in the fixed table + // layout a width-less collapses to 0 once the other (fixed) columns + // fill the table — which hid the Weighted Total column entirely whenever the + // External ID column was also shown. + it('gives the total column an explicit width so it never collapses', () => { + renderWeighted({ students: [makeStudent(1, 'Alice')] }); + const cols = document.querySelectorAll('colgroup col'); + const totalCol = cols[cols.length - 1] as HTMLElement; + expect(totalCol.style.width).not.toBe(''); + }); + it('widens the Name column when a row is expanded and restores it on collapse', async () => { const user = userEvent.setup(); renderWeighted({ @@ -1431,6 +1442,205 @@ describe('WeightedGradebookTable', () => { expect(cells[cells.length - 1]).toHaveTextContent('—'); }); }); + + describe('single-open accordion', () => { + it('keeps only one student expanded at a time (opening another collapses the first)', async () => { + const user = userEvent.setup(); + renderWeighted({ + tabs: [makeTab(10, 'Missions', 1, 100)], + assessments: [makeAssessment(1, 'Mission 1', 10, 100)], + students: [makeStudent(1, 'Alice'), makeStudent(2, 'Bob')], + submissions: [makeSub(1, 1, 80), makeSub(2, 1, 60)], + }); + + await user.click(screen.getByRole('button', { name: /expand Alice/i })); + expect( + await screen.findByTestId(breakdownRowId(1, 10, 1)), + ).toBeInTheDocument(); + + // Opening Bob must collapse Alice. + await user.click(screen.getByRole('button', { name: /expand Bob/i })); + expect( + await screen.findByTestId(breakdownRowId(2, 10, 1)), + ).toBeInTheDocument(); + await waitFor(() => + expect( + screen.queryByTestId(breakdownRowId(1, 10, 1)), + ).not.toBeInTheDocument(), + ); + }); + + it('keeps focus on the toggle after expanding (keyboard users stay put)', async () => { + const user = userEvent.setup(); + renderWeighted({ + tabs: [makeTab(10, 'Missions', 1, 100)], + assessments: [makeAssessment(1, 'Mission 1', 10, 100)], + students: [makeStudent(1, 'Alice')], + submissions: [makeSub(1, 1, 80)], + }); + const toggle = screen.getByRole('button', { name: /expand Alice/i }); + await user.click(toggle); + // Same DOM button (label flips expand→collapse), so focus is retained. + expect( + screen.getByRole('button', { name: /collapse Alice/i }), + ).toHaveFocus(); + }); + }); + + describe('pinned summary row', () => { + // Enable both optional identity columns so the assertion covers the cells + // that the per-cell pinning used to miss (and stands in for any future + // optional column, e.g. level / level contribution). + const expandedRow = async (): Promise => { + localStorage.setItem( + WEIGHTED_STORAGE_KEY, + JSON.stringify({ email: true, externalId: true }), + ); + const user = userEvent.setup(); + renderWeighted({ + tabs: [makeTab(10, 'Missions', 1, 60), makeTab(20, 'Quizzes', 1, 40)], + assessments: [ + makeAssessment(1, 'Mission 1', 10, 100), + makeAssessment(3, 'Quiz 1', 20, 100), + ], + students: [{ ...makeStudent(1, 'Alice'), externalId: 'EXT-001' }], + submissions: [makeSub(1, 1, 80), makeSub(1, 3, 90)], + }); + await user.click(screen.getByRole('button', { name: /expand Alice/i })); + return screen + .getByRole('button', { name: /collapse Alice/i }) + .closest('tr') as HTMLTableRowElement; + }; + + // jsdom can't lay out `position: sticky`, but getComputedStyle resolves the + // emotion class, so a pinned cell reports a concrete `top` (0px here, since + // the measured header height is 0 without layout) while an un-pinned sticky + // cell reports an empty `top`. That distinguishes a torn row from a whole one. + it('pins every cell of the expanded row beneath the header, identity columns included', async () => { + const row = await expandedRow(); + const cells = Array.from(row.querySelectorAll('td')); + // checkbox | Name | Email | External ID | Missions | Quizzes | Total + expect(cells).toHaveLength(7); + cells.forEach((cell) => { + const cs = getComputedStyle(cell); + expect(cs.position).toBe('sticky'); + expect(cs.top).not.toBe(''); + }); + }); + + it('does not pin a collapsed student row', () => { + localStorage.setItem( + WEIGHTED_STORAGE_KEY, + JSON.stringify({ email: true, externalId: true }), + ); + renderWeighted({ + students: [{ ...makeStudent(1, 'Alice'), externalId: 'EXT-001' }], + }); + const row = screen + .getByRole('button', { name: /expand Alice/i }) + .closest('tr') as HTMLTableRowElement; + // The data/identity cells are not sticky at all while collapsed. + const emailCell = within(row).getByText('alice@example.com').closest('td')!; + expect(getComputedStyle(emailCell).top).toBe(''); + }); + }); + + describe('auto-scroll on expand', () => { + const one = { + tabs: [makeTab(10, 'Missions', 1, 100)], + assessments: [makeAssessment(1, 'Mission 1', 10, 100)], + students: [makeStudent(1, 'Alice')], + submissions: [makeSub(1, 1, 80)], + }; + + afterEach(() => { + // Restore the scrollTo we stub in (jsdom doesn't implement it). + delete (Element.prototype as unknown as { scrollTo?: unknown }).scrollTo; + }); + + it('smooth-scrolls the expanded row into view by default', async () => { + const scrollSpy = jest.fn(); + (Element.prototype as unknown as { scrollTo: unknown }).scrollTo = + scrollSpy; + const user = userEvent.setup(); + renderWeighted(one); + await user.click(screen.getByRole('button', { name: /expand Alice/i })); + await waitFor(() => expect(scrollSpy).toHaveBeenCalled()); + expect(scrollSpy.mock.calls[0][0]).toMatchObject({ behavior: 'smooth' }); + }); + + it('jumps instantly when the user prefers reduced motion', async () => { + const scrollSpy = jest.fn(); + (Element.prototype as unknown as { scrollTo: unknown }).scrollTo = + scrollSpy; + const originalMatchMedia = window.matchMedia; + window.matchMedia = ((query: string) => ({ + matches: query === '(prefers-reduced-motion: reduce)', + media: query, + onchange: null, + addEventListener: (): void => {}, + removeEventListener: (): void => {}, + addListener: (): void => {}, + removeListener: (): void => {}, + dispatchEvent: (): boolean => false, + })) as unknown as typeof window.matchMedia; + + try { + const user = userEvent.setup(); + renderWeighted(one); + await user.click(screen.getByRole('button', { name: /expand Alice/i })); + await waitFor(() => expect(scrollSpy).toHaveBeenCalled()); + expect(scrollSpy.mock.calls[0][0]).toMatchObject({ behavior: 'auto' }); + } finally { + window.matchMedia = originalMatchMedia; + } + }); + }); + + describe('drop-lowest breakdown marker', () => { + // One equal tab, keepHighest=1, three graded assessments. The lowest (a1) drops. + const dropConfig = { + tabs: [{ ...makeTab(10, 'Missions', 1, 60), keepHighest: 1 }], + assessments: [ + makeAssessment(1, 'Mission 1', 10, 100), + makeAssessment(2, 'Mission 2', 10, 100), + makeAssessment(3, 'Mission 3', 10, 100), + ], + students: [makeStudent(1, 'Alice')], + submissions: [makeSub(1, 1, 30), makeSub(1, 2, 60), makeSub(1, 3, 90)], + }; + + it('labels the dropped assessment "Dropped (lowest)" (not "Excluded")', async () => { + const user = userEvent.setup(); + renderWeighted(dropConfig); + await user.click(screen.getByRole('button', { name: /expand Alice/i })); + const detail = await screen.findByTestId(breakdownRowId(1, 10, 1)); + expect( + within(detail).getByText(/Dropped \(lowest\)/), + ).toBeInTheDocument(); + expect(within(detail).queryByText(/Excluded/i)).not.toBeInTheDocument(); + }); + + it('shows 0 (not —) in the dropped assessment cell in points mode', async () => { + const user = userEvent.setup(); + renderWeighted(dropConfig); + await user.click(screen.getByRole('button', { name: /expand Alice/i })); + const detail = await screen.findByTestId(breakdownRowId(1, 10, 1)); + // Dropped contributes 0 points — distinct from excluded's em dash. + expect(within(detail).getByText('0')).toBeInTheDocument(); + expect(within(detail).queryByText('—')).not.toBeInTheDocument(); + }); + + it("shows the dropped assessment's own grade % in percentage mode", async () => { + const user = userEvent.setup(); + renderWeighted(dropConfig); + await user.click(screen.getByRole('radio', { name: /percentage/i })); + await user.click(screen.getByRole('button', { name: /expand Alice/i })); + const detail = await screen.findByTestId(breakdownRowId(1, 10, 1)); + // a1 = 30/100 = 30% — visible so the instructor sees the dropped grade. + expect(within(detail).getByText('30%')).toBeInTheDocument(); + }); + }); }); describe('level contribution columns', () => { diff --git a/client/app/bundles/course/gradebook/__tests__/computeWeighted.test.ts b/client/app/bundles/course/gradebook/__tests__/computeWeighted.test.ts index 091424f2af..20c58c0e83 100644 --- a/client/app/bundles/course/gradebook/__tests__/computeWeighted.test.ts +++ b/client/app/bundles/course/gradebook/__tests__/computeWeighted.test.ts @@ -1094,3 +1094,203 @@ describe('levelOffenders unscoreable bucket', () => { expect(r.unscoreable.map((o) => o.name)).toEqual(['Amy', 'Zoe']); }); }); + +// --------------------------------------------------------------------------- +// keep-N (keepHighest) — equal mode only +// --------------------------------------------------------------------------- + +// Three equal-mode assessments: a1=60/100=0.60, a2=80/100=0.80, a3=100/100=1.00 +// keepHighest:1 → keep top 1 (a3, ratio 1.0), drop a1 and a2 +const dropConfig = { + tab: { + id: 10, + title: 'Tab', + categoryId: 0, + gradebookWeight: 60, + keepHighest: 1, + }, + assessments: [ + { id: 1, tabId: 10, maxGrade: 100, title: 'A' }, + { id: 2, tabId: 10, maxGrade: 100, title: 'B' }, + { id: 3, tabId: 10, maxGrade: 100, title: 'C' }, + ], + submissions: subs([ + { studentId: 1, assessmentId: 1, grade: 60 }, + { studentId: 1, assessmentId: 2, grade: 80 }, + { studentId: 1, assessmentId: 3, grade: 100 }, + ]), +}; + +describe('equalSubtotal — keepHighest via computeTabSubtotal', () => { + it('keepHighest=0 (unset): keeps all assessments — same as default behavior', () => { + // (0.6 + 0.8 + 1.0) / 3 = 0.8 + expect( + computeTabSubtotal({ + studentId: 1, + tab: { id: 10, title: 'Tab', categoryId: 0, gradebookWeight: 60 }, + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }), + ).toBeCloseTo(0.8); + }); + + it('keepHighest=1: averages only the single highest ratio', () => { + // top 1: a3 ratio=1.0 → subtotal = 1.0/1 = 1.0 + expect( + computeTabSubtotal({ + studentId: 1, + tab: dropConfig.tab, + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }), + ).toBeCloseTo(1.0); + }); + + it('keepHighest > count: clamps to all assessments', () => { + // keepHighest=99 with 3 assessments → keep all 3 → (0.6+0.8+1.0)/3 = 0.8 + expect( + computeTabSubtotal({ + studentId: 1, + tab: { ...dropConfig.tab, keepHighest: 99 }, + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }), + ).toBeCloseTo(0.8); + }); +}); + +describe('computeStudentBreakdown — keepHighest', () => { + it('dropped flag is true on lowest-ratio assessments, false on kept ones', () => { + const [tab] = computeStudentBreakdown({ + studentId: 1, + tabs: [dropConfig.tab], + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }); + const a1 = tab.assessments.find((x) => x.assessmentId === 1)!; // ratio 0.60 — dropped + const a2 = tab.assessments.find((x) => x.assessmentId === 2)!; // ratio 0.80 — dropped + const a3 = tab.assessments.find((x) => x.assessmentId === 3)!; // ratio 1.00 — kept + + expect(a1.dropped).toBe(true); + expect(a2.dropped).toBe(true); + expect(a3.dropped).toBe(false); + }); + + it('dropped assessment has points=0 and effectiveWeight=0', () => { + const [tab] = computeStudentBreakdown({ + studentId: 1, + tabs: [dropConfig.tab], + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }); + const a1 = tab.assessments.find((x) => x.assessmentId === 1)!; + const a2 = tab.assessments.find((x) => x.assessmentId === 2)!; + + expect(a1.points).toBe(0); + expect(a1.effectiveWeight).toBe(0); + expect(a2.points).toBe(0); + expect(a2.effectiveWeight).toBe(0); + }); + + it('kept assessment points/effectiveWeight are computed over keptCount=1', () => { + // tab weight=60, keptCount=1 → effectiveWeight=60/1=60; ratio=1.0 → points=(1.0/1)*60=60 + const [tab] = computeStudentBreakdown({ + studentId: 1, + tabs: [dropConfig.tab], + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }); + const a3 = tab.assessments.find((x) => x.assessmentId === 3)!; + + expect(a3.effectiveWeight).toBeCloseTo(60); + expect(a3.points).toBeCloseTo(60); + }); + + it('excluded=false and dropped=false on kept assessment', () => { + const [tab] = computeStudentBreakdown({ + studentId: 1, + tabs: [dropConfig.tab], + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }); + const a3 = tab.assessments.find((x) => x.assessmentId === 3)!; + expect(a3.excluded).toBe(false); + expect(a3.dropped).toBe(false); + }); + + it('no keepHighest (0): dropped=false for all assessments', () => { + const [tab] = computeStudentBreakdown({ + studentId: 1, + tabs: [{ id: 10, title: 'Tab', categoryId: 0, gradebookWeight: 60 }], + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }); + tab.assessments.forEach((a) => expect(a.dropped).toBe(false)); + }); + + it('tie-break: equal ratios → lower id is dropped first', () => { + // a1 (id=1) and a2 (id=2) both have ratio 0.5; keepHighest=1. + // Tie-break ascending by id → a1 is ranked lower → a1 is dropped; a2 is kept. + const tieAssessments = [ + { id: 1, tabId: 10, maxGrade: 100, title: 'A' }, + { id: 2, tabId: 10, maxGrade: 100, title: 'B' }, + ]; + const [tab] = computeStudentBreakdown({ + studentId: 1, + tabs: [ + { + id: 10, + title: 'Tab', + categoryId: 0, + gradebookWeight: 60, + keepHighest: 1, + }, + ], + assessments: tieAssessments, + submissions: subs([ + { studentId: 1, assessmentId: 1, grade: 50 }, + { studentId: 1, assessmentId: 2, grade: 50 }, + ]), + }); + const a1 = tab.assessments.find((x) => x.assessmentId === 1)!; + const a2 = tab.assessments.find((x) => x.assessmentId === 2)!; + expect(a1.dropped).toBe(true); // lower id dropped + expect(a2.dropped).toBe(false); // higher id kept + }); +}); + +describe('computeWeightedRows — keepHighest', () => { + const keepStudent = [ + { + id: 1, + name: 'Alice', + email: 'alice@e.com', + externalId: null, + level: 1, + totalXp: 0, + levelContribution: null, + }, + ]; + + it('subtotal for tab with keepHighest=1 equals average of the single top ratio', () => { + // top ratio among a1,a2,a3 = 1.0; subtotal = 1.0 + const rows = computeWeightedRows({ + students: keepStudent, + tabs: [dropConfig.tab], + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }); + expect(rows[0].subtotals[0]).toBeCloseTo(1.0); + }); + + it('total reflects keepHighest subtotal multiplied by tab weight', () => { + // subtotal=1.0, tabWeight=60 → total=60 + const rows = computeWeightedRows({ + students: keepStudent, + tabs: [dropConfig.tab], + assessments: dropConfig.assessments, + submissions: dropConfig.submissions, + }); + expect(rows[0].total).toBeCloseTo(60); + }); +}); diff --git a/client/app/bundles/course/gradebook/__tests__/store.test.ts b/client/app/bundles/course/gradebook/__tests__/store.test.ts index 394d406a0b..6fb04c9719 100644 --- a/client/app/bundles/course/gradebook/__tests__/store.test.ts +++ b/client/app/bundles/course/gradebook/__tests__/store.test.ts @@ -451,3 +451,70 @@ describe('level contribution', () => { expect(next.students[0].levelContribution).toBeNull(); }); }); + +describe('UPDATE_TAB_WEIGHTS — keepHighest', () => { + it('writes keepHighest onto the matching tab', () => { + const state = { + ...baseState, + tabs: [{ id: 10, title: 'T', categoryId: 1, gradebookWeight: 50 }], + }; + const next = reducer( + state, + actions.updateTabWeights({ + weights: [{ tabId: 10, weight: 50, keepHighest: 2 }], + }), + ); + expect(next.tabs[0].keepHighest).toBe(2); + }); + + it('defaults keepHighest to 0 when omitted from the payload', () => { + const state = { + ...baseState, + tabs: [ + { + id: 10, + title: 'T', + categoryId: 1, + gradebookWeight: 50, + keepHighest: 3, + }, + ], + }; + const next = reducer( + state, + actions.updateTabWeights({ + weights: [{ tabId: 10, weight: 50 }], + }), + ); + expect(next.tabs[0].keepHighest).toBe(0); + }); + + it('does not modify keepHighest on other tabs', () => { + const state = { + ...baseState, + tabs: [ + { + id: 10, + title: 'T1', + categoryId: 1, + gradebookWeight: 50, + keepHighest: 1, + }, + { + id: 11, + title: 'T2', + categoryId: 1, + gradebookWeight: 50, + keepHighest: 2, + }, + ], + }; + const next = reducer( + state, + actions.updateTabWeights({ + weights: [{ tabId: 10, weight: 50, keepHighest: 5 }], + }), + ); + expect(next.tabs[1].keepHighest).toBe(2); + }); +}); diff --git a/client/app/bundles/course/gradebook/components/ConfigureWeightsPrompt.tsx b/client/app/bundles/course/gradebook/components/ConfigureWeightsPrompt.tsx index 46cfdad6d9..6399790c4a 100644 --- a/client/app/bundles/course/gradebook/components/ConfigureWeightsPrompt.tsx +++ b/client/app/bundles/course/gradebook/components/ConfigureWeightsPrompt.tsx @@ -66,10 +66,36 @@ const translations = defineMessages({ defaultMessage: "Choose Equal (all assessments share the tab's weight) or Custom (set each assessment's share).", }, - descriptionDrop: { - id: 'course.gradebook.ConfigureWeightsPrompt.descriptionDrop', + keepHighestLabel: { + id: 'course.gradebook.ConfigureWeightsPrompt.keepHighestLabel', + defaultMessage: 'Keep highest', + }, + keepHighestAria: { + id: 'course.gradebook.ConfigureWeightsPrompt.keepHighestAria', + defaultMessage: 'Keep highest for {tab}', + }, + keepHighestToggleAria: { + id: 'course.gradebook.ConfigureWeightsPrompt.keepHighestToggleAria', + defaultMessage: 'Enable keep highest for {tab}', + }, + keepInvalid: { + id: 'course.gradebook.ConfigureWeightsPrompt.keepInvalid', + defaultMessage: 'Keep at least 1 (whole number)', + }, + keepSubtitle: { + id: 'course.gradebook.ConfigureWeightsPrompt.keepSubtitle', + defaultMessage: + '— keeps highest {keep} of {included} · each counts as {pct}%', + }, + keepOverflowWarning: { + id: 'course.gradebook.ConfigureWeightsPrompt.keepOverflowWarning', defaultMessage: - "In Equal mode, optionally drop each student's N lowest-scoring assessments before averaging.", + '"{tab}" keeps more assessments than it contains — all of them will count.', + }, + descriptionKeep: { + id: 'course.gradebook.ConfigureWeightsPrompt.descriptionKeep', + defaultMessage: + "In Equal mode, optionally keep only each student's N highest-scoring assessments.", }, total: { id: 'course.gradebook.ConfigureWeightsPrompt.total', @@ -286,6 +312,13 @@ const ConfigureWeightsPrompt: FC = ({ const seedExclusions = (): Record => Object.fromEntries(assessments.map((a) => [a.id, !!a.gradebookExcluded])); + const seedKeepHighest = (): Record => + Object.fromEntries(resolvedTabs.map((tb) => [tb.id, tb.keepHighest ?? 0])); + const seedKeepOn = (): Record => + Object.fromEntries( + resolvedTabs.map((tb) => [tb.id, (tb.keepHighest ?? 0) > 0]), + ); + const [weights, setWeights] = useState>(seedWeights); const [modes, setModes] = useState>(seedModes); const [assessmentWeights, setAssessmentWeights] = useState< @@ -293,6 +326,9 @@ const ConfigureWeightsPrompt: FC = ({ >(seedAssessmentWeights); const [excluded, setExcluded] = useState>(seedExclusions); + const [keepHighest, setKeepHighest] = + useState>(seedKeepHighest); + const [keepOn, setKeepOn] = useState>(seedKeepOn); const [expanded, setExpanded] = useState>({}); const [submitting, setSubmitting] = useState(false); @@ -313,6 +349,8 @@ const ConfigureWeightsPrompt: FC = ({ setModes(seedModes()); setAssessmentWeights(seedAssessmentWeights()); setExcluded(seedExclusions()); + setKeepHighest(seedKeepHighest()); + setKeepOn(seedKeepOn()); setExpanded({}); setLevelEnabled(levelContribution.enabled); setLevelFormula(levelContribution.formula || seedLevelFormula); @@ -369,14 +407,44 @@ const ConfigureWeightsPrompt: FC = ({ if (hasBelow && hasAbove) levelFixMessage = translations.levelFixBetween; else if (hasAbove) levelFixMessage = translations.levelFixAtMost; + const validateKeep = (value: number): string | null => + Number.isInteger(value) && value >= 1 ? null : t(translations.keepInvalid); + + // Returns the effective keep-N value when the feature is active for a tab, + // or null when not applicable (mode != equal, checkbox off, or only 1 included). + const activeKeepValue = (tabId: number): number | null => { + if ((modes[tabId] ?? 'equal') !== 'equal') return null; + if (!keepOn[tabId]) return null; + return keepHighest[tabId] ?? 0; + }; + + const handleKeepChange = (tabId: number, raw: string): void => { + const parsed = raw === '' ? 0 : Number(raw); + setKeepHighest((prev) => ({ ...prev, [tabId]: parsed })); + }; + const sum = tabs.reduce((acc, tb) => acc + effectiveWeight(tb.id), 0) + (levelEnabled ? levelWeight : 0); const hasInvalid = Object.values(weights).some((w) => validate(w) !== null) || - Object.values(assessmentWeights).some((w) => validate(w) !== null); + Object.values(assessmentWeights).some((w) => validate(w) !== null) || + tabs.some((tb) => { + if ((modes[tb.id] ?? 'equal') !== 'equal') return false; + if (!keepOn[tb.id]) return false; + const includedCount = tabIncludedIds(tb.id).length; + if (includedCount <= 1) return false; + return validateKeep(keepHighest[tb.id] ?? 0) !== null; + }); const hasUnbalanced = tabs.some((tb) => isUnbalanced(tb.id)); + const keepOverflowTabs = tabs.filter((tb) => { + const v = activeKeepValue(tb.id); + if (!v) return false; + const included = tabIncludedIds(tb.id).length; + return included > 1 && v > included; + }); + const handleChange = (tabId: number, raw: string): void => { const parsed = raw === '' ? 0 : Number(raw); setWeights((prev) => ({ ...prev, [tabId]: parsed })); @@ -431,6 +499,11 @@ const ConfigureWeightsPrompt: FC = ({ tabId: tb.id, weight: weights[tb.id] ?? 0, weightMode: mode, + keepHighest: + activeKeepValue(tb.id) !== null && + tabIncludedIds(tb.id).length > 1 + ? keepHighest[tb.id] ?? 0 + : 0, excludedAssessmentIds: tabAssessmentIds(tb.id).filter( (id) => excluded[id], ), @@ -481,6 +554,7 @@ const ConfigureWeightsPrompt: FC = ({ translations.descriptionWeights, translations.descriptionExclusion, translations.descriptionModes, + translations.descriptionKeep, ].map((key) => ( = ({ const excludedCount = tabAssessments.length - includedCount; const pct = includedCount > 0 ? r2(value / includedCount) : 0; + // keep-highest vars + const keepValue = keepHighest[tb.id] ?? 0; + const keepDisabled = includedCount <= 1; + const keepEnabled = !keepDisabled && !!keepOn[tb.id]; + const keepErr = keepEnabled ? validateKeep(keepValue) : null; + const kept = Math.min(keepValue, includedCount); + const keepPct = kept > 0 ? r2(value / kept) : 0; + const isKeepOverflow = keepOverflowTabs.some( + (ot) => ot.id === tb.id, + ); + return (
@@ -600,6 +685,104 @@ const ConfigureWeightsPrompt: FC = ({ {err} )} + {mode === 'equal' && !noAssessments && ( + <> +
+ { + if (keepEnabled) { + // Turn off: just toggle the on flag + setKeepOn((prev) => ({ + ...prev, + [tb.id]: false, + })); + } else { + // Turn on: seed default value if not already set + if (keepValue === 0) { + setKeepHighest((prev) => ({ + ...prev, + [tb.id]: Math.max(1, includedCount - 1), + })); + } + setKeepOn((prev) => ({ + ...prev, + [tb.id]: true, + })); + } + }} + size="small" + /> + + {t(translations.keepHighestLabel)} + + {keepEnabled && ( + + handleKeepChange(tb.id, e.target.value) + } + onKeyDown={(e) => { + if ( + ['.', ',', 'e', 'E', '-', '+'].includes( + e.key, + ) + ) + e.preventDefault(); + }} + size="small" + sx={{ width: 80, mx: 0.5 }} + type="number" + value={keepValue} + /> + )} + {keepEnabled && keepErr === null && ( + + {t(translations.keepSubtitle, { + keep: kept, + included: includedCount, + pct: keepPct.toFixed(2), + })} + + )} +
+ {keepEnabled && keepErr && ( + + {keepErr} + + )} + {isKeepOverflow && ( + + {t(translations.keepOverflowWarning, { + tab: tb.title, + })} + + )} + + )} {unbalanced && ( {t(translations.unbalanced, { tab: tb.title })} @@ -682,6 +865,14 @@ const ConfigureWeightsPrompt: FC = ({
); } + let caption = t(translations.ofGrade, { + pct: pct.toFixed(2), + }); + if (isExcluded) { + caption = t(translations.excluded); + } else if (keepEnabled) { + caption = '—'; + } return (
= ({ color="text.disabled" variant="caption" > - {isExcluded - ? t(translations.excluded) - : t(translations.ofGrade, { - pct: pct.toFixed(2), - })} + {caption}
); diff --git a/client/app/bundles/course/gradebook/components/WeightedGradebookTable.tsx b/client/app/bundles/course/gradebook/components/WeightedGradebookTable.tsx index 9edccf4c95..6f0ad2b137 100644 --- a/client/app/bundles/course/gradebook/components/WeightedGradebookTable.tsx +++ b/client/app/bundles/course/gradebook/components/WeightedGradebookTable.tsx @@ -22,6 +22,8 @@ import { Tooltip, Typography, } from '@mui/material'; +import type { Theme } from '@mui/material/styles'; +import { lighten } from '@mui/material/styles'; import type { AssessmentData, CategoryData, @@ -42,7 +44,11 @@ import { DEFAULT_TABLE_ROWS_PER_PAGE } from 'lib/constants/sharedConstants'; import useTranslation from 'lib/hooks/useTranslation'; import tableTranslations from 'lib/translations/table'; -import type { AssessmentContribution, WeightedRow } from '../computeWeighted'; +import type { + AssessmentContribution, + TabBreakdown, + WeightedRow, +} from '../computeWeighted'; import { computeStudentBreakdown, computeWeightedRows, @@ -166,6 +172,10 @@ const translations = defineMessages({ id: 'course.gradebook.WeightedGradebookTable.excluded', defaultMessage: 'Excluded', }, + dropped: { + id: 'course.gradebook.WeightedGradebookTable.dropped', + defaultMessage: 'Dropped (lowest)', + }, total: { id: 'course.gradebook.WeightedGradebookTable.weightedTotal', defaultMessage: 'Weighted Total', @@ -410,20 +420,20 @@ const WeightedGradebookTable = ({ return a.points; }; - const [expandedIds, setExpandedIds] = useState>(new Set()); + // Single-open accordion: auditing one student at a time. Replaces the former + // multi-expand Set so only the focused student's breakdown is on screen (and + // only one summary row ever pins under the header). + const [expandedId, setExpandedId] = useState(null); const toggleExpanded = (studentId: number): void => - setExpandedIds((prev) => { - const next = new Set(prev); - if (next.has(studentId)) next.delete(studentId); - else next.add(studentId); - return next; - }); + setExpandedId((prev) => (prev === studentId ? null : studentId)); - // Widen the sticky Name column while any row's breakdown is open, so long - // assessment titles in the breakdown aren't clipped; shrink back when all - // rows are collapsed. + // The table is fixed-layout, so the Name column can't auto-grow to fit the + // breakdown's assessment titles. Widen its fixed while the audited + // student's breakdown is open (single-open accordion → a row is expanded iff + // expandedId is set); shrink back when collapsed. Titles longer than this are + // ellipsis-clipped with a Tooltip in the breakdown cell below. const nameWidth = - expandedIds.size > 0 ? NAME_WIDTH_EXPANDED : NAME_WIDTH_COLLAPSED; + expandedId !== null ? NAME_WIDTH_EXPANDED : NAME_WIDTH_COLLAPSED; const studentLevelById = useMemo( () => new Map(students.map((s) => [s.id, s.level])), @@ -435,50 +445,82 @@ const WeightedGradebookTable = ({ [students], ); - const breakdownsByStudent = useMemo( - () => - new Map( - [...expandedIds].map((studentId) => { - const breakdown = computeStudentBreakdown({ - studentId, - tabs: resolvedTabs, - assessments, - submissions, - level: studentLevelById.get(studentId) ?? 0, - levelContribution: showLevelContribution - ? levelContribution - : undefined, - levelContributionPoints: showLevelContribution - ? studentLevelContributionById.get(studentId) ?? null - : null, - courseMaxLevel, - }); - // Level row first, mirroring the Level Contribution column being the - // first contribution column (left of the tabs). - const ordered = [ - ...breakdown.filter((tb) => tb.tabId === LEVEL_TAB_ID), - ...breakdown.filter((tb) => tb.tabId !== LEVEL_TAB_ID), - ]; - return [studentId, ordered] as [studentId: number, typeof ordered]; - }), - ), - [ - expandedIds, - resolvedTabs, + const expandedBreakdown = useMemo(() => { + if (expandedId === null) return null; + const breakdown = computeStudentBreakdown({ + studentId: expandedId, + tabs: resolvedTabs, assessments, submissions, - studentLevelById, - studentLevelContributionById, - showLevelContribution, - levelContribution, + level: studentLevelById.get(expandedId) ?? 0, + levelContribution: showLevelContribution ? levelContribution : undefined, + levelContributionPoints: showLevelContribution + ? studentLevelContributionById.get(expandedId) ?? null + : null, courseMaxLevel, - ], - ); + }); + // Level row first, mirroring the Level Contribution column being the + // first contribution column (left of the tabs). + return [ + ...breakdown.filter((tb) => tb.tabId === LEVEL_TAB_ID), + ...breakdown.filter((tb) => tb.tabId !== LEVEL_TAB_ID), + ]; + }, [ + expandedId, + resolvedTabs, + assessments, + submissions, + studentLevelById, + studentLevelContributionById, + showLevelContribution, + levelContribution, + courseMaxLevel, + ]); + const containerRef = useRef(null); + const activeRowRef = useRef(null); const row1Ref = useRef(null); const row2Ref = useRef(null); + const row3Ref = useRef(null); const [row2Top, setRow2Top] = useState(0); const [row3Top, setRow3Top] = useState(0); + // Full header height = where the pinned summary row sticks, and the scroll + // offset that lands a focused row just beneath the header. + const [headerHeight, setHeaderHeight] = useState(0); + + // sx for the focused (expanded) student's summary row. Pinning is applied at + // the ROW level — every cell sticks beneath the header as a unit — so the + // optional identity columns (email, external ID) and any future optional + // column (e.g. level / level contribution) are pinned automatically, without + // having to remember to decorate each new cell. Layering mirrors the header: + // the two left-frozen lead cells (checkbox, name) carry their own left freeze + // and ride above the data cells; the checkbox also carries the left accent + // bar that marks "this is the student you're auditing". + const pinnedRowSx = (theme: Theme): Record => ({ + '& > td': { + position: 'sticky', + top: headerHeight, + zIndex: 3, + // Opaque tint (not alpha) so scrolled body content can't bleed through + // the sticky cell. + backgroundColor: lighten(theme.palette.primary.light, 0.96), + }, + // checkbox + name are also left-frozen; keep them above the data cells (and + // above the header's frozen corner) while both axes stick. + '& > td:nth-of-type(1), & > td:nth-of-type(2)': { + zIndex: 6, + }, + // Accent bar PLUS the cell's usual right/bottom borders — a bare accent + // boxShadow would otherwise replace (and so erase) the grid lines the + // Table's `& th, & td` rule paints via boxShadow. + '& > td:first-of-type': { + boxShadow: [ + `inset 3px 0 0 ${theme.palette.primary.main}`, + `inset -1px 0 0 ${theme.palette.grey[400]}`, + `inset 0 -1px 0 ${theme.palette.grey[400]}`, + ].join(', '), + }, + }); const tabSubheaderLabel = (tab: TabData): string => { if (allExcludedTabIds.has(tab.id)) return t(translations.excluded); @@ -516,22 +558,45 @@ const WeightedGradebookTable = ({ useLayoutEffect(() => { const row1 = row1Ref.current; const row2 = row2Ref.current; - if (!row1 || !row2) return undefined; + const row3 = row3Ref.current; + if (!row1 || !row2 || !row3) return undefined; const measure = (): void => { const h1 = row1.getBoundingClientRect().height; const h2 = row2.getBoundingClientRect().height; + const h3 = row3.getBoundingClientRect().height; setRow2Top(h1); setRow3Top(h1 + h2); + setHeaderHeight(h1 + h2 + h3); }; measure(); const observer = new ResizeObserver(measure); observer.observe(row1); observer.observe(row2); + observer.observe(row3); return () => observer.disconnect(); }, [visibleCategories, resolvedTabs]); + // On expand, glide the focused row to just beneath the header so its + // breakdown is guaranteed in view (even when the clicked row was near the + // bottom). getBoundingClientRect keeps this correct regardless of the row's + // offsetParent; scrollTo is optional-chained because jsdom lacks it. + useLayoutEffect(() => { + if (expandedId === null) return; + const container = containerRef.current; + const rowEl = activeRowRef.current; + if (!container || !rowEl) return; + const prefersReducedMotion = + window.matchMedia?.('(prefers-reduced-motion: reduce)').matches ?? false; + const delta = + rowEl.getBoundingClientRect().top - container.getBoundingClientRect().top; + container.scrollTo?.({ + top: container.scrollTop + delta - headerHeight, + behavior: prefersReducedMotion ? 'auto' : 'smooth', + }); + }, [expandedId, headerHeight]); + const rows = useMemo(() => { const base = computeWeightedRows({ students, @@ -829,6 +894,7 @@ const WeightedGradebookTable = ({ )} ({ maxHeight: 'calc(100vh - 22rem)', overflowX: 'auto', @@ -1034,6 +1100,7 @@ const WeightedGradebookTable = ({ {showLevelContributionCol && ( @@ -1100,10 +1167,20 @@ const WeightedGradebookTable = ({ {body.rows.map((row, idx) => { const rowProps = body.forEachRow(row, idx); const studentId = row.original.studentId; - const isExpanded = expandedIds.has(studentId); + const isExpanded = expandedId === studentId; return ( - + + {/* Body sticky-left cells sit at zIndex 1 — strictly + below the header's sticky cells (MUI gives every + stickyHeader cell zIndex 2). On a z-index tie the cell + later in the DOM (the body) wins, so matching z2 here + lets scrolled rows bleed up over the header in any + column the frozen Name cell (z4) doesn't cover — i.e. + the identity columns once they're toggled on. */} {row.original.email} @@ -1169,7 +1244,6 @@ const WeightedGradebookTable = ({ {showExternalId && ( {row.original.externalId ?? ''} @@ -1248,150 +1322,184 @@ const WeightedGradebookTable = ({ {isExpanded && - (breakdownsByStudent.get(studentId) ?? []).flatMap( - (tb) => - tb.assessments.map((a) => { - const isExcluded = a.excluded; - const weightText = t( - translations.percentOfGrade, - { - weight: - Math.round(a.effectiveWeight * 100) / 100, - }, - ); - const assessmentGradeText = - a.grade === null - ? `-/${a.maxGrade}` - : `${a.grade}/${a.maxGrade}`; - // The level row has no max — courseMaxLevel plays - // no part in the contribution, so showing - // "level/courseMaxLevel" would falsely imply a - // proportional derivation. Show the raw level only. - const gradeText = - tb.tabId === LEVEL_TAB_ID - ? t(translations.levelBreakdownDetail, { - level: a.grade ?? 0, - }) - : assessmentGradeText; - return ( - + tb.assessments.map((a) => { + const isExcluded = a.excluded; + const isDropped = a.dropped; + const isInactive = isExcluded || isDropped; + // Weightage is always "% of grade" — it never + // follows the points/percent lens. + const weightText = t(translations.percentOfGrade, { + weight: Math.round(a.effectiveWeight * 100) / 100, + }); + const assessmentGradeText = + a.grade === null + ? `—/${a.maxGrade}` + : `${a.grade}/${a.maxGrade}`; + // The level row has no max — courseMaxLevel plays + // no part in the contribution, so showing + // "level/courseMaxLevel" would falsely imply a + // proportional derivation. Show the raw level only. + const gradeText = + tb.tabId === LEVEL_TAB_ID + ? t(translations.levelBreakdownDetail, { + level: a.grade ?? 0, + }) + : assessmentGradeText; + let statusText = weightText; + if (isExcluded) { + statusText = t(translations.excluded); + } else if (isDropped) { + statusText = t(translations.dropped); + } + return ( + + {/* Empty checkbox cell so the breakdown row + carries the same checkbox | name divider (the + universal cell borderRight) as the rows above. */} + + {/* Title over a muted "raw mark · weightage" + subtitle, stacked and confined to the (sticky) + Name column. The breakdown row freezes the same + checkbox | Name region as the student rows above — + the identity columns get their own empty cells + after this — so the layout reads identically + whether identity columns are shown or not. A left + indent sits the title under the student name (past + the expand chevron), signalling these are that + student's assessments. */} + - - - - - {a.title} - - + {/* Fixed-layout table: the Name column is a + fixed width (widened to NAME_WIDTH_EXPANDED while + this student is expanded), so a long title can't + grow the column — it's kept on one line (nowrap) + and ellipsis-clipped, with a Tooltip exposing the + full title on hover. The metadata line below is + clipped the same way, so every breakdown row is + exactly 2 lines. */} + - {`${gradeText} · ${isExcluded ? t(translations.excluded) : weightText}`} + {a.title} + + {/* Muted metadata on its own line below the + title: raw mark · effective weightage, kept on + one line and clipped. Weightage is always "% of + grade" — never routed through the points/percent + lens. */} + + {`${gradeText} · ${statusText}`} + + + {/* One empty cell per visible identity column so + the grid lines stay aligned with the rows above. + These scroll with the table (only checkbox + Name + are frozen), matching the student rows. */} + {showEmail && ( + + )} + {showExternalId && ( + + )} + {/* Mirror the parent row's two level columns so + the tab cells below stay column-aligned. Raw + Level has no per-assessment breakdown (empty); + Level Contribution carries its value on the + synthetic level row (tabId === LEVEL_TAB_ID), + matching how each tab cell carries its own. */} + {showRawLevelCol && } + {showLevelContributionCol && ( + + {tb.tabId === LEVEL_TAB_ID + ? fmtDisplay( + a.points, + columnPrecisions.level, + ) + : ''} - {showEmail && ( - - )} - {showExternalId && ( + )} + {resolvedTabs.map((tab, i) => { + const tabCellValue = isExcluded + ? '—' + : fmtDisplay( + breakdownDisplayValue(a), + columnPrecisions.tabs[i], + ); + return ( - )} - {/* Mirror the parent row's two level columns so - the tab cells below stay column-aligned. Raw - Level has no per-assessment breakdown (empty); - Level Contribution carries its value on the - synthetic level row (tabId === LEVEL_TAB_ID), - matching how each tab cell carries its own. */} - {showRawLevelCol && } - {showLevelContributionCol && ( - - {tb.tabId === LEVEL_TAB_ID - ? fmtDisplay( - a.points, - columnPrecisions.level, - ) - : ''} + key={tab.id} + align="right" + {...groupEndIf(tabIsCategoryEnd[i])} + > + {/* Place the value by tab id, not array + position, so the breakdown row order is + free to differ from the column order. */} + {tab.id === tb.tabId ? tabCellValue : ''} - )} - {resolvedTabs.map((tab, i) => { - const tabCellValue = isExcluded - ? '—' - : fmtDisplay( - breakdownDisplayValue(a), - columnPrecisions.tabs[i], - ); - return ( - - {/* Place the value by tab id, not array - position, so the breakdown row order is - free to differ from the column order. */} - {tab.id === tb.tabId - ? tabCellValue - : ''} - - ); - })} - - - ); - }), + ); + })} + + + ); + }), )} ); diff --git a/client/app/bundles/course/gradebook/computeWeighted.ts b/client/app/bundles/course/gradebook/computeWeighted.ts index 40905fe848..e50c98f6f1 100644 --- a/client/app/bundles/course/gradebook/computeWeighted.ts +++ b/client/app/bundles/course/gradebook/computeWeighted.ts @@ -37,6 +37,7 @@ export interface AssessmentContribution { // Custom mode: the assessment's own configured weight. effectiveWeight: number; excluded: boolean; + dropped: boolean; // equal-mode keep-highest: ranked out for this student } export interface TabBreakdown { @@ -80,19 +81,25 @@ const buildAssessmentsByTab = ( // Equal-weight formula: average of (grade/maxGrade) ratios over INCLUDED assessments. // Excluded assessments are dropped from both numerator and count; ungraded included -// contribute 0. Returns null when no assessment is included. +// contribute 0. When keepN > 0, only the top keepN ratios are averaged. +// Returns null when no assessment is included. const equalSubtotal = ( studentId: number, + tab: TabData, tabAssessments: AssessmentData[], gradeLookup: GradeLookup, ): number | null => { const included = tabAssessments.filter((a) => !a.gradebookExcluded); if (included.length === 0) return null; + const keepN = tab.keepHighest ?? 0; const ratios = included.map((a) => { const grade = gradeLookup.get(gradeKey(studentId, a.id)); return grade != null ? gradeRatio(grade, a.maxGrade) : 0; }); - return ratios.reduce((acc, r) => acc + r, 0) / ratios.length; + ratios.sort((x, y) => x - y); // ascending + const keep = keepN > 0 ? Math.min(keepN, included.length) : included.length; + const kept = ratios.slice(included.length - keep); // the `keep` highest + return kept.reduce((acc, r) => acc + r, 0) / kept.length; }; // Custom-weight formula: Σ(grade_i/maxGrade_i × assessmentWeight_i) / tabWeight over @@ -130,7 +137,7 @@ const subtotalFromLookup = ( if (tab.weightMode === 'custom') { return customSubtotal(studentId, tab, tabAssessments, gradeLookup); } - return equalSubtotal(studentId, tabAssessments, gradeLookup); + return equalSubtotal(studentId, tab, tabAssessments, gradeLookup); }; // Weighted, additive total from already-computed subtotals. @@ -282,23 +289,47 @@ export const computeStudentBreakdown = ({ const result: TabBreakdown[] = tabs.map((tab) => { const list = assessmentsByTab.get(tab.id) ?? []; const weight = tab.gradebookWeight ?? 0; - const includedCount = list.filter((a) => !a.gradebookExcluded).length; + const included = list.filter((a) => !a.gradebookExcluded); + const includedCount = included.length; + + let droppedIds = new Set(); + let keptCount = includedCount; + if (tab.weightMode !== 'custom' && includedCount > 0) { + const keepN = tab.keepHighest ?? 0; + keptCount = keepN > 0 ? Math.min(keepN, includedCount) : includedCount; + if (keptCount < includedCount) { + const ranked = included + .map((a) => { + const grade = gradeLookup.get(gradeKey(studentId, a.id)); + return { + id: a.id, + ratio: grade != null && a.maxGrade > 0 ? grade / a.maxGrade : 0, + }; + }) + .sort((x, y) => x.ratio - y.ratio || x.id - y.id); // ascending: lowest first, tie-break by id + // Drop the lowest (includedCount − keptCount). + droppedIds = new Set( + ranked.slice(0, includedCount - keptCount).map((r) => r.id), + ); + } + } const contributions = list.map((a) => { const excluded = !!a.gradebookExcluded; + const dropped = droppedIds.has(a.id); const grade = gradeLookup.get(gradeKey(studentId, a.id)) ?? null; const ratio = grade != null ? gradeRatio(grade, a.maxGrade) : 0; let points: number; let effectiveWeight: number; - if (excluded) { + if (excluded || dropped) { points = 0; effectiveWeight = 0; } else if (tab.weightMode === 'custom') { points = ratio * (a.gradebookWeight ?? 0); effectiveWeight = a.gradebookWeight ?? 0; } else { - points = includedCount > 0 ? (ratio / includedCount) * weight : 0; - effectiveWeight = includedCount > 0 ? weight / includedCount : 0; + points = keptCount > 0 ? (ratio / keptCount) * weight : 0; + effectiveWeight = keptCount > 0 ? weight / keptCount : 0; } return { assessmentId: a.id, @@ -308,6 +339,7 @@ export const computeStudentBreakdown = ({ points, effectiveWeight, excluded, + dropped, }; }); return { tabId: tab.id, assessments: contributions }; @@ -326,6 +358,7 @@ export const computeStudentBreakdown = ({ points: lvl, effectiveWeight: levelContribution.weight, excluded: false, + dropped: false, }, ], }); diff --git a/client/app/bundles/course/gradebook/store.ts b/client/app/bundles/course/gradebook/store.ts index e5a30f044c..72abd1d4b9 100644 --- a/client/app/bundles/course/gradebook/store.ts +++ b/client/app/bundles/course/gradebook/store.ts @@ -85,6 +85,7 @@ const reducer = produce( tabId, weight, weightMode, + keepHighest, assessmentWeights, excludedAssessmentIds, }) => { @@ -92,6 +93,7 @@ const reducer = produce( if (tab) { tab.gradebookWeight = weight; tab.weightMode = weightMode; + tab.keepHighest = keepHighest ?? 0; } const excludedSet = new Set(excludedAssessmentIds ?? []); const tabAssessments = draft.assessments.filter( diff --git a/client/app/types/course/gradebook.ts b/client/app/types/course/gradebook.ts index 49a9c62818..722f56ccbb 100644 --- a/client/app/types/course/gradebook.ts +++ b/client/app/types/course/gradebook.ts @@ -9,6 +9,7 @@ export interface TabData { categoryId: number; gradebookWeight?: number; weightMode?: 'equal' | 'custom'; + keepHighest?: number; } export interface AssessmentData { @@ -63,6 +64,7 @@ export interface UpdateWeightsPayload { tabId: number; weight: number; weightMode?: 'equal' | 'custom'; + keepHighest?: number; excludedAssessmentIds?: number[]; assessmentWeights?: { assessmentId: number; weight: number }[]; }[]; diff --git a/client/locales/en.json b/client/locales/en.json index 642b295640..5323a869cb 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -9452,6 +9452,9 @@ "course.gradebook.WeightedGradebookTable.email": { "defaultMessage": "Email" }, + "course.gradebook.WeightedGradebookTable.dropped": { + "defaultMessage": "Dropped (lowest)" + }, "course.gradebook.WeightedGradebookTable.excluded": { "defaultMessage": "Excluded" }, diff --git a/db/migrate/202606180000_add_keep_highest_to_course_gradebook_tab_contribution_table.rb b/db/migrate/202606180000_add_keep_highest_to_course_gradebook_tab_contribution_table.rb new file mode 100644 index 0000000000..ce9d07bcf8 --- /dev/null +++ b/db/migrate/202606180000_add_keep_highest_to_course_gradebook_tab_contribution_table.rb @@ -0,0 +1,5 @@ +class AddKeepHighestToCourseGradebookTabContributionTable < ActiveRecord::Migration[7.2] + def change + add_column :course_gradebook_tab_contributions, :keep_highest, :integer, null: false, default: 0 + end +end diff --git a/db/schema.rb b/db/schema.rb index 9bb292b31b..2dd53b17a4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_06_16_000000) do +ActiveRecord::Schema[7.2].define(version: 2026_06_18_000000) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "uuid-ossp" @@ -895,10 +895,11 @@ t.bigint "updater_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["course_id"], name: "fk__course_gradebook_tabcontributions_course_id" - t.index ["creator_id"], name: "fk__course_gradebook_tabcontributions_creator_id" - t.index ["tab_id"], name: "index_course_gradebook_tabcontributions_on_tab_id", unique: true - t.index ["updater_id"], name: "fk__course_gradebook_tabcontributions_updater_id" + t.integer "keep_highest", default: 0, null: false + t.index ["course_id"], name: "fk__course_gradebook_tab_contributions_course_id" + t.index ["creator_id"], name: "fk__course_gradebook_tab_contributions_creator_id" + t.index ["tab_id"], name: "index_course_gradebook_tab_contributions_on_tab_id", unique: true + t.index ["updater_id"], name: "fk__course_gradebook_tab_contributions_updater_id" end create_table "course_group_categories", force: :cascade do |t| diff --git a/spec/controllers/course/gradebook_controller_spec.rb b/spec/controllers/course/gradebook_controller_spec.rb index 7c7542b659..9c3b57ff98 100644 --- a/spec/controllers/course/gradebook_controller_spec.rb +++ b/spec/controllers/course/gradebook_controller_spec.rb @@ -501,6 +501,26 @@ def weight_for(tab) end end + describe '#update_weights keepHighest' do + render_views + + let(:category2) { create(:course_assessment_category, course: course) } + let(:tab) { create(:course_assessment_tab, category: category2) } + + before { controller_sign_in(controller, manager.user) } + + it 'persists keepHighest and echoes it back' do + post :update_weights, as: :json, params: { + course_id: course.id, + weights: [{ tabId: tab.id, weight: '50', weightMode: 'equal', keepHighest: 2 }] + } + expect(response).to have_http_status(:ok) + body = JSON.parse(response.body) + expect(body['weights'].first['keepHighest']).to eq(2) + expect(Course::Gradebook::Contribution.find_by(tab_id: tab.id).keep_highest).to eq(2) + end + end + describe '#update_weights with modes' do render_views @@ -618,6 +638,16 @@ def weight_for(tab) expect(body['assessments'].first).to have_key('gradebookExcluded') expect(body['assessments'].first['gradebookExcluded']).to eq(false) end + + it 'includes keepHighest in the weighted tabs response' do + contribution.update!(keep_highest: 3) + controller_sign_in(controller, manager.user) + get :index, params: { course_id: course.id }, format: :json + body = JSON.parse(response.body) + tab_json = body['tabs'].find { |t| t['id'] == tab.id } + expect(tab_json).to have_key('keepHighest') + expect(tab_json['keepHighest']).to eq(3) + end end end end diff --git a/spec/models/course/gradebook/tab_contribution_spec.rb b/spec/models/course/gradebook/tab_contribution_spec.rb index 0f9d1bf11f..5164d6ff09 100644 --- a/spec/models/course/gradebook/tab_contribution_spec.rb +++ b/spec/models/course/gradebook/tab_contribution_spec.rb @@ -234,6 +234,29 @@ def excluded?(assessment) expect(excluded?(a1)).to eq(false) end end + + context 'with keep_highest' do + it 'persists keep_highest in equal mode' do + described_class.bulk_update( + course: course, + updates: [{ tab_id: tab1.id, weight: 50, weight_mode: 'equal', keep_highest: 3 }] + ) + expect(described_class.find_by(tab_id: tab1.id).keep_highest).to eq(3) + end + + it 'accepts 0 as a valid keep_highest value' do + described_class.bulk_update( + course: course, + updates: [{ tab_id: tab1.id, weight: 50, weight_mode: 'equal', keep_highest: 5 }] + ) + expect(described_class.find_by(tab_id: tab1.id).keep_highest).to eq(5) + described_class.bulk_update( + course: course, + updates: [{ tab_id: tab1.id, weight: 50, weight_mode: 'equal', keep_highest: 0 }] + ) + expect(described_class.find_by(tab_id: tab1.id).keep_highest).to eq(0) + end + end end end end