diff --git a/apps/web/lib/catalogue-sync/persist-source-version.ts b/apps/web/lib/catalogue-sync/persist-source-version.ts index 9454c963..c395e896 100644 --- a/apps/web/lib/catalogue-sync/persist-source-version.ts +++ b/apps/web/lib/catalogue-sync/persist-source-version.ts @@ -418,7 +418,10 @@ async function insertRequirements( } } -/** Writes every content, requirement and evidence row for a new snapshot. */ +/** + * Writes every content, requirement, evidence and review-flag row for a new + * snapshot. + */ export async function insertVersionContent( tx: Tx, { @@ -469,6 +472,16 @@ export async function insertVersionContent( ) `; } + for (const [index, flag] of write.flags.entries()) { + await tx` + insert into public.catalogue_version_flags ( + version_id, position, field_path, severity, code, message + ) values ( + ${snapshotId}, ${index + 1}, ${flag.fieldPath}, ${flag.severity}, + ${flag.code}, ${flag.message} + ) + `; + } } /** Persists one semantic ANU observation without changing local content. */ diff --git a/apps/web/lib/catalogue/review-notes.ts b/apps/web/lib/catalogue/review-notes.ts new file mode 100644 index 00000000..f5cc74df --- /dev/null +++ b/apps/web/lib/catalogue/review-notes.ts @@ -0,0 +1,143 @@ +/** One note the model left on a source version. */ +export type VersionFlag = { + fieldPath: string | null; + severity: "warning" | "error"; + code: string; + message: string; +}; + +/** The model's evidence for one field, and how directly the page states it. */ +export type VersionEvidence = { + fieldPath: string; + confidence: number | null; + excerpt: string | null; +}; + +export type ReviewNote = { + fieldPath: string | null; + label: string; + message: string; +}; + +export type UncertainField = { + fieldPath: string; + label: string; + confidence: number; + excerpt: string | null; +}; + +/** Below this, a field is worth reading against the ANU page. */ +export const UNCERTAIN_CONFIDENCE = 0.8; + +const FIELD_NAMES: Record = { + modelExtraction: "The whole response", + prerequisiteRule: "Prerequisite rule", + corequisiteRule: "Corequisite rule", + prerequisiteText: "Prerequisite wording", + corequisiteText: "Corequisite wording", + incompatibilityText: "Incompatibility wording", + incompatibilityCourseCodes: "Incompatible courses", + softIncompatibilityCourseCodes: "Advisory incompatibilities", + unmodelledText: "Unmodelled requirement wording", + unitValue: "Units", + eftsl: "EFTSL", + totalUnits: "Units", + durationYears: "Length", + selectionRank: "Selection rank", + atar: "ATAR", + learningOutcomes: "Learning outcomes", + assessmentItems: "Assessment", + offerings: "Offerings", + summaryFields: "Key facts", + relationships: "Related structures", + requirements: "Requirements", + rule: "Requirement tree", + sourceUpdatedAt: "ANU update date", + areasOfInterest: "Areas of interest", + relatedCourses: "Related courses", + contactText: "Contact", + convenerText: "Convener", +}; + +function humanise(segment: string) { + const words = segment.replace(/([a-z])([A-Z])/g, "$1 $2").toLowerCase(); + return words.charAt(0).toUpperCase() + words.slice(1); +} + +/** + * A field path from the model's extraction, such as `requisites.prerequisiteRule`, + * `fees[2]` or `requirements.rule.children.3`, in words an administrator reads. + * The section a path belongs to leads; an array index becomes an item number + * and a requirement branch keeps its position. + */ +export function modelFieldLabel(fieldPath: string | null) { + if (!fieldPath) return "The record"; + const segments = fieldPath.split(/[.[\]]/).filter(Boolean); + const names: string[] = []; + const branch: number[] = []; + let inRequirementTree = false; + for (const segment of segments) { + if (/^\d+$/.test(segment)) { + if (inRequirementTree) branch.push(Number(segment) + 1); + else names.push(`item ${Number(segment) + 1}`); + continue; + } + if (segment === "rule" && names.at(-1) === FIELD_NAMES.requirements) { + inRequirementTree = true; + continue; + } + if (segment === "children" || segment === "requisites") continue; + names.push(FIELD_NAMES[segment] ?? humanise(segment)); + } + // Sentence case: only the first name keeps its capital, unless a later one + // is an initialism such as ATAR. + const label = names + .map((name, index) => + index > 0 && /^[A-Z][a-z]/.test(name) + ? name.charAt(0).toLowerCase() + name.slice(1) + : name, + ) + .join(", "); + return branch.length ? `${label}, branch ${branch.join(".")}` : label; +} + +/** + * What the Changes tab asks an administrator to check: every error, every + * warning, and each field whose weakest evidence falls below + * `UNCERTAIN_CONFIDENCE`, least certain first. + */ +export function summariseReviewNotes({ + flags, + evidence, +}: { + flags: readonly VersionFlag[]; + evidence: readonly VersionEvidence[]; +}) { + const note = (flag: VersionFlag): ReviewNote => ({ + fieldPath: flag.fieldPath, + label: modelFieldLabel(flag.fieldPath), + message: flag.message, + }); + const weakest = new Map(); + for (const item of evidence) { + if (item.confidence === null || item.confidence >= UNCERTAIN_CONFIDENCE) { + continue; + } + const current = weakest.get(item.fieldPath); + if (!current || item.confidence < current.confidence) { + weakest.set(item.fieldPath, { + fieldPath: item.fieldPath, + label: modelFieldLabel(item.fieldPath), + confidence: item.confidence, + excerpt: item.excerpt, + }); + } + } + return { + errors: flags.filter(({ severity }) => severity === "error").map(note), + warnings: flags.filter(({ severity }) => severity === "warning").map(note), + uncertain: [...weakest.values()].sort( + (left, right) => left.confidence - right.confidence, + ), + }; +} diff --git a/apps/web/lib/coursemap/admin-catalogue-record.ts b/apps/web/lib/coursemap/admin-catalogue-record.ts index e7a2ebe1..c358d55c 100644 --- a/apps/web/lib/coursemap/admin-catalogue-record.ts +++ b/apps/web/lib/coursemap/admin-catalogue-record.ts @@ -1,4 +1,8 @@ import "server-only"; +import type { + VersionEvidence, + VersionFlag, +} from "@/lib/catalogue/review-notes"; import { readVersionContent } from "@/lib/catalogue-import/version-content"; import { withSyncDatabaseClient } from "@/lib/catalogue-sync/sync-store"; import type { CatalogueContent } from "@/lib/catalogue/content"; @@ -211,6 +215,46 @@ export async function loadVersionWrite( return withSyncDatabaseClient((sql) => readVersionContent(sql, versionId)); } +/** + * The flags and model evidence stored with a source version, for the Changes + * tab. Empty for a version an administrator published, which has neither. + */ +export async function loadVersionReviewNotes(versionId: number): Promise<{ + flags: VersionFlag[]; + evidence: VersionEvidence[]; +}> { + return withSyncDatabaseClient(async (sql) => { + const [flags, evidence] = await Promise.all([ + sql` + select field_path, severity, code, message + from public.catalogue_version_flags + where version_id = ${versionId} + order by position + `, + sql` + select field_path, confidence, source_excerpt + from public.catalogue_version_provenance + where version_id = ${versionId} and method = 'model' + order by id + `, + ]); + return { + flags: flags.map((row) => ({ + fieldPath: row.field_path === null ? null : String(row.field_path), + severity: row.severity === "error" ? "error" : "warning", + code: String(row.code), + message: String(row.message), + })), + evidence: evidence.map((row) => ({ + fieldPath: String(row.field_path), + confidence: row.confidence === null ? null : Number(row.confidence), + excerpt: + row.source_excerpt === null ? null : String(row.source_excerpt), + })), + }; + }); +} + /** The student-facing course details for a version, or null for structures. */ export async function loadVersionCoursePreview(versionId: number) { const supabase = await createClient(); diff --git a/apps/web/tests/catalogue-changes-panel.test.tsx b/apps/web/tests/catalogue-changes-panel.test.tsx index 7750e041..2eb49132 100644 --- a/apps/web/tests/catalogue-changes-panel.test.tsx +++ b/apps/web/tests/catalogue-changes-panel.test.tsx @@ -5,6 +5,7 @@ import type { SourceReview, SourceReviewChange, } from "@/lib/catalogue/source-review-store"; +import { summariseReviewNotes } from "@/lib/catalogue/review-notes"; import { CatalogueChangesPanel } from "@/ui/admin/catalogue/changes/changes-panel"; const actions = vi.hoisted(() => ({ resolve: vi.fn() })); @@ -157,3 +158,53 @@ test("kept values stay available without nagging", () => { ).toBeTruthy(); expect(screen.queryByRole("button", { name: "Keep current" })).toBeNull(); }); + +test("what the model flagged leads the tab, least certain field first", () => { + renderPanel({ + review: review({ incoming: [change()] }), + notes: summariseReviewNotes({ + flags: [ + { + fieldPath: "requisites.prerequisiteRule", + severity: "error", + code: "INVALID", + message: "The rule named a course code ANU does not use.", + }, + { + fieldPath: "requirements.rule.children.3", + severity: "warning", + code: "AMBIGUOUS", + message: "Kept as the page's wording.", + }, + ], + evidence: [ + { fieldPath: "fees", confidence: 0.9, excerpt: "$5520" }, + { fieldPath: "offerings", confidence: 0.55, excerpt: "First Semester" }, + { fieldPath: "college", confidence: 0.7, excerpt: "ANU College" }, + ], + }), + }); + expect( + screen.getByRole("heading", { name: "What to check" }), + ).toBeInTheDocument(); + expect( + screen.getByText("1 part could not be read and was left empty"), + ).toBeInTheDocument(); + expect(screen.getByText("Prerequisite rule:")).toBeInTheDocument(); + expect(screen.getByText("Requirements, branch 4:")).toBeInTheDocument(); + const uncertain = screen + .getAllByText(/% sure$/u) + .map((node) => node.textContent); + // Fees are sure enough not to be listed. + expect(uncertain).toEqual(["55% sure", "70% sure"]); +}); + +test("nothing flagged means no notes section", () => { + renderPanel({ + review: review({ incoming: [change()] }), + notes: summariseReviewNotes({ flags: [], evidence: [] }), + }); + expect( + screen.queryByRole("heading", { name: "What to check" }), + ).not.toBeInTheDocument(); +}); diff --git a/apps/web/tests/review-notes.test.ts b/apps/web/tests/review-notes.test.ts new file mode 100644 index 00000000..a9569a9e --- /dev/null +++ b/apps/web/tests/review-notes.test.ts @@ -0,0 +1,42 @@ +import { expect, test } from "vitest"; + +import { + modelFieldLabel, + summariseReviewNotes, +} from "@/lib/catalogue/review-notes"; + +test("model field paths read as the fields an administrator knows", () => { + expect(modelFieldLabel("requisites.prerequisiteRule")).toBe( + "Prerequisite rule", + ); + expect(modelFieldLabel("fees[2]")).toBe("Fees, item 3"); + expect(modelFieldLabel("requirements.rule.children.3.children.0")).toBe( + "Requirements, branch 4.1", + ); + expect(modelFieldLabel("offerings[1].startsOn")).toBe( + "Offerings, item 2, starts on", + ); + expect(modelFieldLabel("atar")).toBe("ATAR"); + expect(modelFieldLabel("modelExtraction")).toBe("The whole response"); + expect(modelFieldLabel(null)).toBe("The record"); +}); + +test("a field is listed once, at its weakest evidence", () => { + const { uncertain } = summariseReviewNotes({ + flags: [], + evidence: [ + { fieldPath: "fees", confidence: 0.6, excerpt: "first" }, + { fieldPath: "fees", confidence: 0.4, excerpt: "weakest" }, + { fieldPath: "title", confidence: null, excerpt: null }, + { fieldPath: "college", confidence: 0.8, excerpt: "at the threshold" }, + ], + }); + expect(uncertain).toEqual([ + { + fieldPath: "fees", + label: "Fees", + confidence: 0.4, + excerpt: "weakest", + }, + ]); +}); diff --git a/apps/web/types/database.ts b/apps/web/types/database.ts index e3909b8e..634bbec4 100644 --- a/apps/web/types/database.ts +++ b/apps/web/types/database.ts @@ -2039,6 +2039,51 @@ export type Database = { }, ] } + catalogue_version_flags: { + Row: { + code: string + field_path: string | null + id: number + message: string + position: number + severity: string + version_id: number + } + Insert: { + code: string + field_path?: string | null + id?: never + message: string + position: number + severity: string + version_id: number + } + Update: { + code?: string + field_path?: string | null + id?: never + message?: string + position?: number + severity?: string + version_id?: number + } + Relationships: [ + { + foreignKeyName: "catalogue_version_flags_version_id_fkey" + columns: ["version_id"] + isOneToOne: false + referencedRelation: "catalogue_versions" + referencedColumns: ["id"] + }, + { + foreignKeyName: "catalogue_version_flags_version_id_fkey" + columns: ["version_id"] + isOneToOne: false + referencedRelation: "published_course_summaries" + referencedColumns: ["version_id"] + }, + ] + } catalogue_version_provenance: { Row: { academic_year_id: number @@ -4272,4 +4317,3 @@ export const Constants = { Enums: {}, }, } as const - diff --git a/apps/web/ui/admin/catalogue/changes/changes-panel.tsx b/apps/web/ui/admin/catalogue/changes/changes-panel.tsx index 590d21b0..2f73ac74 100644 --- a/apps/web/ui/admin/catalogue/changes/changes-panel.tsx +++ b/apps/web/ui/admin/catalogue/changes/changes-panel.tsx @@ -1,8 +1,10 @@ import Link from "next/link"; import type { SnapshotChange } from "@/lib/catalogue-import/changes"; +import type { summariseReviewNotes } from "@/lib/catalogue/review-notes"; import type { SourceReview } from "@/lib/catalogue/source-review-store"; import { CatalogueEmpty } from "@/ui/admin/catalogue-table/catalogue-empty"; +import { ModelNotes } from "./model-notes"; import { SourceChangeCard } from "./source-change-card"; import { UnpublishedChanges } from "./unpublished-changes"; @@ -72,6 +74,7 @@ export function CatalogueChangesPanel({ isPublished, kindLabel, latestSync = null, + notes = null, }: { review: SourceReview | null; unpublished: SnapshotChange[]; @@ -83,6 +86,8 @@ export function CatalogueChangesPanel({ kindLabel: string; /** The check these changes came out of, for readers allowed to open it. */ latestSync?: { id: string; completedAt: string | null } | null; + /** What the model flagged on the latest ANU version. */ + notes?: ReturnType | null; }) { const conflicts = review?.conflicts ?? []; const incoming = review?.incoming ?? []; @@ -117,6 +122,7 @@ export function CatalogueChangesPanel({

) : null} + {notes ? : null} {conflicts.length === 0 && incoming.length === 0 ? ( ) : null} diff --git a/apps/web/ui/admin/catalogue/changes/model-notes.tsx b/apps/web/ui/admin/catalogue/changes/model-notes.tsx new file mode 100644 index 00000000..17cc30ac --- /dev/null +++ b/apps/web/ui/admin/catalogue/changes/model-notes.tsx @@ -0,0 +1,107 @@ +import { + Alert, + AlertDescription, + AlertTitle, +} from "@coursemap/ui/components/alert"; +import { Badge } from "@coursemap/ui/components/badge"; +import { CircleAlert, Gauge, TriangleAlert } from "lucide-react"; +import type { ReviewNote, UncertainField } from "@/lib/catalogue/review-notes"; + +function NoteList({ notes }: { notes: readonly ReviewNote[] }) { + return ( +
    + {notes.map((note, index) => ( +
  • + {note.label}:{" "} + {note.message} +
    • + ))} +
+ ); +} + +function plural(count: number, singular: string, pluralForm: string) { + return `${count} ${count === 1 ? singular : pluralForm}`; +} + +/** + * What the model flagged on the latest ANU check, and the fields it was least + * sure of. Review is the only check on a model-owned sync, so this leads the + * Changes tab: it says where to look before any change is accepted. Nothing + * is shown when the model flagged nothing. + */ +export function ModelNotes({ + errors, + warnings, + uncertain, +}: { + errors: readonly ReviewNote[]; + warnings: readonly ReviewNote[]; + uncertain: readonly UncertainField[]; +}) { + if (!errors.length && !warnings.length && !uncertain.length) return null; + return ( +
+

+ What to check +

+ {errors.length ? ( + + + ) : null} + {warnings.length ? ( + + + ) : null} + {uncertain.length ? ( + + + ) : null} +
+ ); +} diff --git a/apps/web/ui/admin/catalogue/record-page.tsx b/apps/web/ui/admin/catalogue/record-page.tsx index a88762b8..8f58ea33 100644 --- a/apps/web/ui/admin/catalogue/record-page.tsx +++ b/apps/web/ui/admin/catalogue/record-page.tsx @@ -13,8 +13,10 @@ import { loadCatalogueChangelog } from "@/lib/coursemap/admin-catalogue-changelo import { loadCatalogueRecord, loadVersionCoursePreview, + loadVersionReviewNotes, loadVersionWrite, } from "@/lib/coursemap/admin-catalogue-record"; +import { summariseReviewNotes } from "@/lib/catalogue/review-notes"; import { courseDetailsFromWrite } from "@/lib/coursemap/course-version-view"; import { ADMIN_CATALOGUE_OPERATIONS_PATH, @@ -106,7 +108,14 @@ export async function CatalogueRecordPage({ const publishedPreview = studentContent ? { course: studentCourse, content: studentCourse ? null : studentContent } : null; - const review = await loadSourceReview(record.recordId, draft.content); + const [review, notes] = await Promise.all([ + loadSourceReview(record.recordId, draft.content), + record.latestSourceVersionId + ? loadVersionReviewNotes(record.latestSourceVersionId).then( + summariseReviewNotes, + ) + : null, + ]); const unpublished = hasChanges ? diffSnapshotWrites(studentContent, draft.content) : []; @@ -183,6 +192,7 @@ export async function CatalogueRecordPage({ hasEverSynced={record.syncs.length > 0} isPublished={record.publishedVersionId !== null} kindLabel={labels.singular.toLowerCase()} + notes={notes} latestSync={ canManageImports && record.syncs[0] ? { diff --git a/supabase/migrations/011_catalogue_version_flags.sql b/supabase/migrations/011_catalogue_version_flags.sql new file mode 100644 index 00000000..647caa9e --- /dev/null +++ b/supabase/migrations/011_catalogue_version_flags.sql @@ -0,0 +1,42 @@ +-- The review notes the model left on a source version: parts of its response +-- that did not fit the contract, wording the ANU page does not contain, and +-- requirement branches it kept as text. They are stored with the version +-- they describe, sealed with it, and read on the record's Changes tab. They +-- are not content, so they take no part in the version's content hash. + +create table public.catalogue_version_flags ( + id bigint generated always as identity primary key, + version_id bigint not null + references public.catalogue_versions (id) on delete cascade, + position integer not null, + field_path text, + severity text not null, + code text not null, + message text not null, + constraint catalogue_version_flags_position_check check (position > 0), + constraint catalogue_version_flags_severity_check check ( + severity = any (array['warning'::text, 'error'::text]) + ), + constraint catalogue_version_flags_code_check check (btrim(code) <> ''), + constraint catalogue_version_flags_message_check check (btrim(message) <> ''), + constraint catalogue_version_flags_position_unique unique (version_id, position) +); + +create trigger catalogue_version_flags_guard_sealed + before insert or delete or update on public.catalogue_version_flags + for each row execute function private.guard_snapshot_child_mutation(); + +alter table public.catalogue_version_flags enable row level security; + +create policy catalogue_version_flags_read on public.catalogue_version_flags + for select to authenticated + using ((select private.can_read_catalogue_drafts() as can_read_catalogue_drafts)); + +revoke all on table public.catalogue_version_flags + from public, anon, authenticated, service_role; +revoke all on sequence public.catalogue_version_flags_id_seq + from public, anon, authenticated, service_role; + +grant all on table public.catalogue_version_flags to service_role; +grant select on table public.catalogue_version_flags to authenticated; +grant all on sequence public.catalogue_version_flags_id_seq to service_role; diff --git a/supabase/tests/database/catalogue_version_flags.sql b/supabase/tests/database/catalogue_version_flags.sql new file mode 100644 index 00000000..6dd90511 --- /dev/null +++ b/supabase/tests/database/catalogue_version_flags.sql @@ -0,0 +1,70 @@ +-- The model's review notes on a source version are for the people who review +-- catalogue changes. No end-user role writes them, anonymous readers never +-- see them, and they are sealed with the version they describe. + +begin; + +create extension if not exists pgtap with schema extensions; + +select extensions.plan(8); + +select extensions.has_table( + 'public', 'catalogue_version_flags', 'model review notes are stored' +); + +select extensions.ok( + ( + select relrowsecurity from pg_class + where oid = 'public.catalogue_version_flags'::regclass + ), + 'row level security is enabled on model review notes' +); + +select extensions.ok( + not has_table_privilege( + 'authenticated', 'public.catalogue_version_flags', 'insert, update, delete' + ), + 'authenticated clients cannot write model review notes' +); + +select extensions.ok( + has_table_privilege('authenticated', 'public.catalogue_version_flags', 'select'), + 'authenticated clients may read notes their policy allows' +); + +select extensions.ok( + not has_table_privilege('anon', 'public.catalogue_version_flags', 'select'), + 'anonymous readers cannot see model review notes' +); + +select extensions.is( + ( + select array_agg(policyname::text order by policyname) + from pg_policies + where schemaname = 'public' and tablename = 'catalogue_version_flags' + ), + array['catalogue_version_flags_read'], + 'the only policy is the catalogue reviewers read policy' +); + +select extensions.ok( + exists ( + select 1 from pg_trigger + where tgrelid = 'public.catalogue_version_flags'::regclass + and tgname = 'catalogue_version_flags_guard_sealed' + ), + 'notes are sealed with their version' +); + +select extensions.ok( + exists ( + select 1 from pg_constraint + where conrelid = 'public.catalogue_version_flags'::regclass + and conname = 'catalogue_version_flags_severity_check' + ), + 'a note is either a warning or an error' +); + +select * from extensions.finish(); + +rollback;