From 8ade4df0cc89b84916108e84dfba015ca0314a46 Mon Sep 17 00:00:00 2001 From: Andy Kitson Date: Wed, 4 Nov 2026 10:30:41 -0600 Subject: [PATCH 1/6] steer reviewers away from dead-end idrs searches (EDFIAL-689) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit in testing, a reviewer clicked search straight away with the form still pre-filled from the file. idrs had already searched those values for the suggestions, so nothing new could come back, and each search is slow. search is now disabled, with a short hint, while the form holds the file's values or the search whose results are shown. spacing, id order and repeated ids don't count as changes. the directions point reviewers to their sis first, and each field they change is tagged as edited. an invalid date of birth blocks the search with a format hint, shown once the reviewer leaves the field or presses enter, instead of the search route's raw validator message. while a search runs the button reads "searching…", and an error hides the earlier results. some files repeat an id across columns, so ids now show once in the form, the header and the suggestions, and are sent once. the copy now says "the file" rather than "your file": reviewers load vendors' files and don't think of them as their own. Co-Authored-By: Claude Opus 5.5 --- .../app/Pages/Jobs/MatchReview/components.tsx | 89 ++++++++++++++----- .../app/Pages/Jobs/MatchReview/search.test.ts | 78 ++++++++++++++++ .../src/app/Pages/Jobs/MatchReview/search.ts | 56 ++++++++++-- .../Pages/Jobs/MatchReview/studentPane.tsx | 64 ++++++++----- .../src/app/Pages/Jobs/MatchReview/submit.tsx | 2 +- .../app/Pages/Jobs/MatchReview/values.test.ts | 16 ++++ .../src/app/Pages/Jobs/MatchReview/values.ts | 5 +- 7 files changed, 256 insertions(+), 54 deletions(-) create mode 100644 app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts create mode 100644 app/fe/src/app/Pages/Jobs/MatchReview/values.test.ts diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx b/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx index 18895fb7..c648628c 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx +++ b/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx @@ -100,36 +100,77 @@ export const SearchField = ({ label, value, placeholder, + edited, + problem, onChange, onEnter, }: { label: string; value: string; placeholder?: string; + /** Whether the value differs from the file's. */ + edited: boolean; + /** Why the value can't be searched, shown when the reviewer isn't typing in the field. */ + problem?: string; onChange: (value: string) => void; onEnter: () => void; -}) => ( - - - {label} - - onChange(event.target.value)} - onKeyDown={(event) => event.key === 'Enter' && onEnter()} - /> - -); +}) => { + const [focused, setFocused] = useState(false); + // Pressing Enter shows the problem mid-typing; typing again hides it. + const [entered, setEntered] = useState(false); + // The theme clears the border on hover and focus; an edit keeps it. + const editedBorder = { borderColor: 'blue.100' }; + return ( + + + + {label} + + {edited && ( + + edited + + )} + + { + setEntered(false); + onChange(event.target.value); + }} + onKeyDown={(event) => { + if (event.key !== 'Enter') return; + setEntered(true); + onEnter(); + }} + onFocus={() => setFocused(true)} + onBlur={() => setFocused(false)} + /> + {problem && (!focused || entered) && ( + + {problem} + + )} + + ); +}; /** * When no suggestion fits, or there were none: two ways forward, set apart @@ -168,7 +209,7 @@ export const NoSuggestionFits = ({ {children} {search} @@ -179,7 +220,7 @@ export const NoSuggestionFits = ({ = {}): SearchTerms => ({ + first_name: 'Ana', + last_name: 'Lopez', + birth_date: '2012-04-01', + student_ids: '123, 456', + ...overrides, +}); + +describe('changedFields', () => { + it('ignores surrounding spaces and the order and repeats of IDs', () => { + expect(changedFields(terms(), terms({ first_name: ' Ana ', student_ids: '456 123' }))).toEqual( + [] + ); + expect(isSameSearch(terms(), terms({ student_ids: '456,123, 456' }))).toBe(true); + }); + + it('names each changed, added or cleared field', () => { + expect( + changedFields( + terms({ birth_date: '' }), + terms({ last_name: 'López', student_ids: '123, 456, 789' }) + ) + ).toEqual(['last_name', 'birth_date', 'student_ids']); + }); +}); + +describe('searchBlock', () => { + const file = terms(); + const edit = terms({ last_name: 'López' }); + + it("blocks the file's values, already searched for the suggestions", () => { + expect(searchBlock(terms({ first_name: ' Ana ' }), file, null)).toBe('file'); + }); + + it('blocks the search whose results are shown, but not an earlier one', () => { + expect(searchBlock(edit, file, edit)).toBe('shown'); + expect(searchBlock(edit, file, terms({ student_ids: '789' }))).toBeNull(); + }); + + it('allows a retry when nothing is shown, as after a failed search', () => { + expect(searchBlock(edit, file, null)).toBeNull(); + }); + + it('blocks a date that is not a real YYYY-MM-DD date, ahead of any other reason', () => { + expect(searchBlock(terms({ birth_date: '2012-02-30' }), file, null)).toBe('bad-date'); + expect(searchBlock(terms({ birth_date: '4/1/2012' }), file, null)).toBe('bad-date'); + const fileWithBadDate = terms({ birth_date: '2012-4-1' }); + expect(searchBlock(fileWithBadDate, fileWithBadDate, null)).toBe('bad-date'); + }); + + it('allows a blank date', () => { + expect(searchBlock(terms({ birth_date: ' ' }), file, null)).toBeNull(); + }); +}); + +// Some files repeat the same ID across columns. +describe('repeated IDs', () => { + it('appear once in the form', () => { + expect(termsFrom({ student_ids: ['123', '123', '456', '123'] }).student_ids).toBe('123, 456'); + }); + + it('are sent once', () => { + expect(searchDetailsFrom(terms({ student_ids: '123, 123 456' })).student_ids).toEqual([ + '123', + '456', + ]); + }); +}); diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/search.ts b/app/fe/src/app/Pages/Jobs/MatchReview/search.ts index 50da23f1..2419db62 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/search.ts +++ b/app/fe/src/app/Pages/Jobs/MatchReview/search.ts @@ -15,15 +15,21 @@ export const termsFrom = (file: StudentInputDetailsJson): SearchTerms => ({ first_name: valueText(file.first_name) ?? '', last_name: valueText(file.last_name) ?? '', birth_date: valueText(file.birth_date) ?? '', - student_ids: Array.isArray(file.student_ids) ? file.student_ids.map(String).join(', ') : '', + student_ids: Array.isArray(file.student_ids) + ? [...new Set(file.student_ids.map(String))].join(', ') + : '', }); /** The search route's body: IDRS's input, with blank fields left out. */ export const searchDetailsFrom = (terms: SearchTerms): StudentSearchDetailsDto => { - const ids = terms.student_ids - .split(/[\s,]+/) - .map((id) => id.trim()) - .filter(Boolean); + const ids = [ + ...new Set( + terms.student_ids + .split(/[\s,]+/) + .map((id) => id.trim()) + .filter(Boolean) + ), + ]; return { first_name: terms.first_name.trim() || null, last_name: terms.last_name.trim() || null, @@ -31,3 +37,43 @@ export const searchDetailsFrom = (terms: SearchTerms): StudentSearchDetailsDto = student_ids: ids.length ? ids : null, }; }; + +/** + * The fields where two sets of terms send IDRS different searches, ignoring + * spacing and the order and repeats of IDs. + */ +export const changedFields = (a: SearchTerms, b: SearchTerms): (keyof SearchTerms)[] => { + const sent = (terms: SearchTerms) => { + const details = searchDetailsFrom(terms); + return { ...details, student_ids: details.student_ids?.slice().sort().join(' ') ?? null }; + }; + const [sentA, sentB] = [sent(a), sent(b)]; + return (Object.keys(a) as (keyof SearchTerms)[]).filter((key) => sentA[key] !== sentB[key]); +}; + +export const isSameSearch = (a: SearchTerms, b: SearchTerms) => changedFields(a, b).length === 0; + +/** Whether a date is a real YYYY-MM-DD date, the only form the search route takes. */ +export const realDate = (value: string) => { + if (!/^\d{4}-\d{2}-\d{2}$/.test(value)) return false; + const date = new Date(`${value}T00:00:00Z`); + return !Number.isNaN(date.getTime()) && date.toISOString().slice(0, 10) === value; +}; + +/** + * Why the terms can't be searched, or null when they can. A search mustn't + * repeat what the reviewer already has: the file's values, which IDRS + * searched for the suggestions (with the file's school IDs, which the form + * leaves out), or the search whose results are shown. + */ +export const searchBlock = ( + terms: SearchTerms, + fileTerms: SearchTerms, + shownTerms: SearchTerms | null +): 'bad-date' | 'file' | 'shown' | null => { + const birthDate = terms.birth_date.trim(); + if (birthDate && !realDate(birthDate)) return 'bad-date'; + if (isSameSearch(terms, fileTerms)) return 'file'; + if (shownTerms && isSameSearch(terms, shownTerms)) return 'shown'; + return null; +}; diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx b/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx index a6bbc9cf..ccca2d49 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx +++ b/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx @@ -38,25 +38,19 @@ import { import { valueText } from './values'; import { useMe } from '../../../api/queries/me.queries'; import { ContactSupport } from '../../../components/SupportButton'; -import { termsFrom, SearchTerms } from './search'; +import { changedFields, realDate, searchBlock, termsFrom, SearchTerms } from './search'; import { Candidate, DecisionChoice, DecisionResult, useReviewSession } from './reviewSession'; import { outcomeLabel, sendRunOutcome, submissionNumber, submissionReference } from './sendRun'; -const realDate = (value: string) => { - if (!/^\d{4}-\d{2}-\d{2}$/.test(value)) return false; - const date = new Date(`${value}T00:00:00Z`); - return !Number.isNaN(date.getTime()) && date.toISOString().slice(0, 10) === value; -}; - /** Missing or invalid values in the file: clues, not a diagnosis of IDRS. */ const fileIssues = (student: GetStudentInputDetailsDto) => { const d = student.inputDetails; const issues: string[] = []; - if (!valueText(d.first_name)) issues.push('No first name in your file.'); - if (!valueText(d.last_name)) issues.push('No last name in your file.'); - if (!valueText(d.birth_date)) issues.push('No date of birth in your file.'); + if (!valueText(d.first_name)) issues.push('No first name in the file.'); + if (!valueText(d.last_name)) issues.push('No last name in the file.'); + if (!valueText(d.birth_date)) issues.push('No date of birth in the file.'); else if (typeof d.birth_date === 'string' && !realDate(d.birth_date)) - issues.push(`Date of birth ${d.birth_date} isn't a valid date.`); + issues.push(`Date of birth ${d.birth_date} isn't a valid YYYY-MM-DD date.`); return issues; }; @@ -264,7 +258,7 @@ export const StudentPane = ({ {/* The source record stays put while everything else changes. */} - From your file + From the file {name} @@ -356,7 +350,7 @@ const DecisionState = ({ student }: { student: GetStudentInputDetailsDto }) => { matched.source === 'search' ? ', found by searching' : '' }. Not submitted yet.`; } else if (status === 'excluded') { - body = `Excluded from this job${by}. Its records won't be reprocessed.`; + body = `Excluded from this job${by}. This record won't be reprocessed.`; } else if (sendRun) { // No decision can follow a send, so the current one is what was sent. const chosen = matched?.studentUniqueId; @@ -577,7 +571,7 @@ const EvidenceTable = ({ - In your file + In the file {shown.map((c) => ( (null); const run = async () => { + if (block || searching) return; setSearching(true); setError(null); const result = await search(student.correlationId, terms); @@ -708,11 +707,13 @@ const Search = ({ else setError(result.message); setSearching(false); }; - const field = (key: keyof SearchTerms, label: string, placeholder?: string) => ( + const field = (key: keyof SearchTerms, label: string, placeholder?: string, problem?: string) => ( setTerms({ ...terms, [key as string]: value })} onEnter={run} /> @@ -723,17 +724,36 @@ const Search = ({ {field('first_name', 'First name')} {field('last_name', 'Last name')} - {field('birth_date', 'Date of birth', 'YYYY-MM-DD')} + {field( + 'birth_date', + 'Date of birth', + 'YYYY-MM-DD', + block === 'bad-date' ? 'Use YYYY-MM-DD' : undefined + )} {field('student_ids', 'Student IDs', 'any IDs, e.g. state or local')} - - Search - + + + Search + + {(block === 'file' || block === 'shown') && !searching && ( + + {block === 'file' + ? "Runway already searched with the file's values. Edit a field to search again." + : 'Edit a field to search again.'} + + )} + {error && !searching && Couldn't search: {error}} - {hits && !searching && hits.length === 0 && ( - No one in the roster matches those details. + {hits && !searching && !error && hits.length === 0 && ( + No students match your last search. )} - {hits && !searching && hits.length > 0 && ( + {hits && !searching && !error && hits.length > 0 && ( - From your file + From the file Matched to diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/values.test.ts b/app/fe/src/app/Pages/Jobs/MatchReview/values.test.ts new file mode 100644 index 00000000..3687ab94 --- /dev/null +++ b/app/fe/src/app/Pages/Jobs/MatchReview/values.test.ts @@ -0,0 +1,16 @@ +import { describe, expect, it } from '@jest/globals'; +import { valueText } from './values'; + +describe('valueText', () => { + // Some files repeat the same ID across columns. + it('shows a repeated ID once, keeping IDs of different types', () => { + expect(valueText(['123', '123'])).toBe('123'); + expect( + valueText([ + { id_type: 'State', id_value: '123' }, + { id_type: 'State', id_value: '123' }, + { id_type: 'Local', id_value: '123' }, + ]) + ).toBe('State 123, Local 123'); + }); +}); diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/values.ts b/app/fe/src/app/Pages/Jobs/MatchReview/values.ts index 6da643ec..2fed727b 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/values.ts +++ b/app/fe/src/app/Pages/Jobs/MatchReview/values.ts @@ -2,7 +2,8 @@ import { JsonValue } from '@edanalytics/models'; /** * A detail from a file record or a roster student as text, or null when - * there's nothing to show. ID lists read as "type value". + * there's nothing to show. ID lists read as "type value", each once: some + * files repeat an ID across columns. */ export const valueText = (value: JsonValue | undefined): string | null => { if (value === null || value === undefined || value === '') return null; @@ -14,7 +15,7 @@ export const valueText = (value: JsonValue | undefined): string | null => { : valueText(v) ) .filter(Boolean); - return parts.length ? parts.join(', ') : null; + return parts.length ? [...new Set(parts)].join(', ') : null; } return typeof value === 'object' ? JSON.stringify(value) : String(value); }; From e08341df08f0d7699f02736320f81bc89a8f37cb Mon Sep 17 00:00:00 2001 From: Andy Kitson Date: Wed, 4 Nov 2026 11:01:34 -0600 Subject: [PATCH 2/6] shorten the student ids placeholder so it fits in the four-column form it was cut off at "any IDs, e.g. state or lo". the label already says student ids, so "any IDs" repeated it. Co-Authored-By: Claude Opus 5.5 --- app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx b/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx index ccca2d49..ef942696 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx +++ b/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx @@ -730,7 +730,7 @@ const Search = ({ 'YYYY-MM-DD', block === 'bad-date' ? 'Use YYYY-MM-DD' : undefined )} - {field('student_ids', 'Student IDs', 'any IDs, e.g. state or local')} + {field('student_ids', 'Student IDs', 'e.g. state or local')} Date: Wed, 4 Nov 2026 11:01:34 -0600 Subject: [PATCH 3/6] drop the border highlight on edited search fields a highlighted border made an edited field look like the active one. the "edited" tag beside the label is enough. Co-Authored-By: Claude Opus 5.5 --- app/fe/src/app/Pages/Jobs/MatchReview/components.tsx | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx b/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx index c648628c..23fff0b3 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx +++ b/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx @@ -118,8 +118,6 @@ export const SearchField = ({ const [focused, setFocused] = useState(false); // Pressing Enter shows the problem mid-typing; typing again hides it. const [entered, setEntered] = useState(false); - // The theme clears the border on hover and focus; an edit keeps it. - const editedBorder = { borderColor: 'blue.100' }; return ( { setEntered(false); onChange(event.target.value); From 6668050fe2d2b976639559a9769530d186872811 Mon Sep 17 00:00:00 2001 From: Andy Kitson Date: Wed, 4 Nov 2026 11:01:34 -0600 Subject: [PATCH 4/6] keep the date hint out of the search form's layout shown under the field, it made every input in the row taller. it now sits below the field in the gap above the search button. Co-Authored-By: Claude Opus 5.5 --- .../src/app/Pages/Jobs/MatchReview/components.tsx | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx b/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx index 23fff0b3..077f7d19 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx +++ b/app/fe/src/app/Pages/Jobs/MatchReview/components.tsx @@ -119,7 +119,7 @@ export const SearchField = ({ // Pressing Enter shows the problem mid-typing; typing again hides it. const [entered, setEntered] = useState(false); return ( - + setFocused(false)} /> {problem && (!focused || entered) && ( - + // Out of the flow, so the row's other fields keep their height; it + // sits in the gap above the Search button. + {problem} )} From 25a948332bfdfcf6966805b5730309b5655eb964 Mon Sep 17 00:00:00 2001 From: Andy Kitson Date: Wed, 4 Nov 2026 11:01:34 -0600 Subject: [PATCH 5/6] clear a search error once the reviewer edits the form an error hides the results, so after a failed search, editing back to the search already shown left search disabled with nothing to show for it. an error is about the search that failed, so editing clears it. also split the bad-date test so its name says what it shows: a bad date in the file's own values is reported ahead of those values. Co-Authored-By: Claude Opus 5.5 --- app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts | 9 +++++++-- app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx | 6 +++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts b/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts index 2cc37525..cf3b0ee8 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts +++ b/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts @@ -47,13 +47,18 @@ describe('searchBlock', () => { expect(searchBlock(edit, file, terms({ student_ids: '789' }))).toBeNull(); }); - it('allows a retry when nothing is shown, as after a failed search', () => { + it('allows any edit before a search has returned results', () => { expect(searchBlock(edit, file, null)).toBeNull(); }); - it('blocks a date that is not a real YYYY-MM-DD date, ahead of any other reason', () => { + it('blocks a date that is not a real YYYY-MM-DD date', () => { expect(searchBlock(terms({ birth_date: '2012-02-30' }), file, null)).toBe('bad-date'); expect(searchBlock(terms({ birth_date: '4/1/2012' }), file, null)).toBe('bad-date'); + }); + + // So the date hint alone explains the disabled button: editing another + // field wouldn't enable it. + it("reports the bad date, not the file's values, when the file's own date is bad", () => { const fileWithBadDate = terms({ birth_date: '2012-4-1' }); expect(searchBlock(fileWithBadDate, fileWithBadDate, null)).toBe('bad-date'); }); diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx b/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx index ef942696..ff58cb54 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx +++ b/app/fe/src/app/Pages/Jobs/MatchReview/studentPane.tsx @@ -714,7 +714,11 @@ const Search = ({ placeholder={placeholder} problem={problem} edited={edited.includes(key)} - onChange={(value) => setTerms({ ...terms, [key as string]: value })} + onChange={(value) => { + // An error is about the search that failed; editing moves on from it. + setError(null); + setTerms({ ...terms, [key as string]: value }); + }} onEnter={run} /> ); From f85b4cc892b40578873fe8ebb420e46d8eb7b3a8 Mon Sep 17 00:00:00 2001 From: Andy Kitson Date: Wed, 4 Nov 2026 11:38:14 -0600 Subject: [PATCH 6/6] make the file's bad-date test show what it guards it passed the same bad-date terms as both the form and the file, so it only showed that a bad date is a bad date, which the test above already covers. it now contrasts an unedited form with a good date, blocked as the file's values, with the same form carrying the file's bad date, blocked as a bad date. that pins the order of the two checks: swapping them fails this test alone. the test above is renamed to say its bad date is typed. Co-Authored-By: Claude Opus 5.5 --- app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts b/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts index cf3b0ee8..05e90280 100644 --- a/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts +++ b/app/fe/src/app/Pages/Jobs/MatchReview/search.test.ts @@ -51,14 +51,16 @@ describe('searchBlock', () => { expect(searchBlock(edit, file, null)).toBeNull(); }); - it('blocks a date that is not a real YYYY-MM-DD date', () => { + it('blocks a typed date that is not a real YYYY-MM-DD date', () => { expect(searchBlock(terms({ birth_date: '2012-02-30' }), file, null)).toBe('bad-date'); expect(searchBlock(terms({ birth_date: '4/1/2012' }), file, null)).toBe('bad-date'); }); - // So the date hint alone explains the disabled button: editing another - // field wouldn't enable it. - it("reports the bad date, not the file's values, when the file's own date is bad", () => { + // An unedited form holds the file's values, so a bad date in the file is + // in the form too. Editing another field wouldn't enable Search, so the + // date is the reason to give. + it("blocks a bad date from the file as a bad date, ahead of the file's values", () => { + expect(searchBlock(file, file, null)).toBe('file'); const fileWithBadDate = terms({ birth_date: '2012-4-1' }); expect(searchBlock(fileWithBadDate, fileWithBadDate, null)).toBe('bad-date'); });