Checking Window / Checking Exercise refactor AB#297801 - #323
Merged
Conversation
|
Review app for PR 323 was deleted |
paulc1983
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #307.
One PR for the whole epic. The children are not PR'd separately — each lands as commits on this branch and is ticked off below.
Why
The 16-19 window runs two activities on two different date ranges: results enquiry 7 Oct to 31 Mar, pupil data checking 7 Oct to 18 Oct. The user sees one card and one landing page. KS4 Autumn is the same shape, and more window types will follow.
CheckingWindowhas oneStartDate/EndDatepair, so a window is either fully open or fully closed, and whether the enquiry entry point appears is decided by a window-type test rather than by dates. So the enquiry and pupil-data options are visible for the same range, which is wrong for both, and KS4 Autumn offers no enquiry option at all.Approach
Add a
CheckingExerciseschild collection toCheckingWindow. Each row carries its own dates. The window keeps the outer pair as the union of its exercises. A window type with one exercise gets one row and does not change behaviour; a window type with several gets several rows, and a third or fourth needs no schema change and no new branch.Two product decisions this PR must hold to:
Children
Landed so far
#324 — The results-enquiry exercise has its own ingress step, so the enquiry journey works on a deployed environment. Until now nothing wrote
results-enquiry/data/outsideWeb/Seeding/SeedStudentResults.cs, which is development-only: everywhere else the blob simply did not exist and the result picker had nothing to show. This is what makes the journey real, not a tidy-up.Slots belong to an exercise.
WindowDatasets.DefaultsFortakes the exercise as well as the window type, andWindowServicereconciles every exercise rather than only pupil data. A results enquiry gets one slot per source file, each named by theResultsFileTagstag it stamps — five on a 16-19 window, four on KS4, none on KS2, which has no results feed. Naming the slot after the supplier's own file name is what lets an admin match six delivered files to six upload boxes; a plain label would leave them guessing which of three late-results files is which.Provenance is stamped by file of origin, the exact analogue of the existing
Includedflag:Includeddecides inclusion by which file a record came from,SourceFiledecides provenance the same way. No supplier CSV carries aSOURCEcolumn, so the processor stamps it before validation and guards it on the schema declaring the property, exactly asINCLUDEDis guarded —AllowAdditionalPropertiesis false, so an unguarded stamp would fail every run.StudentResultRecord.SourceFile, the result picker's file column andILateResultsAvailabilityall read it.The output name is chosen in the lookup, not by the caller. A run writes
CheckingExerciseBlobPaths.DataBlobName(exercise, laestab), a new switch with no default case for the same reason asExercisePrefix. The two names normalise the laestab differently on purpose —PupilsBlobNamestrips only the slash so every pupil blob already written is still found,ResultsBlobNamerunsLaestabNormaliserbecause that is how the results reader turns a Sign-in claim into a blob name — so a results run that reached for the pupil name would write files the journey cannot find.One slot may be optional, and that is not a nicety. Six required slots and the existing "every dataset must be complete" rule would have made the exercise permanently unvalidatable: the late, revised and retention files land weeks apart and one may never land at all. Only the main file is required.
HasRequiredFilesasks that every required slot is filled and at least one slot is; the run readsDatasetsToIngest, the complete slots only; and because a run rewrites the exercise's whole output, the exercise is simply re-run when the next file arrives. Every pupil-data slot stays required — each 16-19 pupil file carries a whole population, so a run missing one writes a blob missing half the school. The summary page marks optional slots and shows an empty one as "Not supplied".The migration backfills the slots, because slots are only reconciled when a window is saved through
WindowService. Without it every results-enquiry exercise already on a deployed environment — including the ones #317's own backfill created — would read "This exercise has no ingress files to load" until an admin happened to re-save its window, and nobody could upload anything. Only empty slots are created,NOT EXISTS-guarded, andDownremoves only slots nobody has uploaded to.Two things about the input are still assumptions, not confirmed facts. No supplier sample or JSON schema for these files exists in this repo. The CSVs are read as carrying the output contract's own column names (
CYPMD_ID,QAN,QUAL_NAME,SYLLABUS,SESSION,GRADE— AB#296999), because ingress passes CSV columns through verbatim against the admin-supplied JSON schema and has never had a renaming step; if the headers differ, a mapping step is new work rather than a schema edit. And they must carry aLAESTABcolumn, which is what splits one supplier file into one blob per school — a file without it now fails the run naming the file, instead of throwing out of the group-by. Both are recorded indocs/16-19-window-model.mdso the next person does not re-derive them.The results-enquiry ingress slice is tested end to end through
StudentResultsBlobClient, the client the journey itself reads, so the blob name, the JSON shape and the property names are all checked by the code that has to consume them. The migration's backfill is tested against real Postgres for each window type, for idempotency, and for a rollback that keeps an admin's uploaded files.#320 — Three loose ends, and one bug found while tying them.
WhatToChangeCheckingExerciseMapmoved fromApplication/ResultsEnquiry/toApplication/WindowManagement/. It maps everyWhatToChangemember, not just the enquiry one, so filing it under one exercise read as if results enquiry were the special case. #318 had already typed its values, so no exercise-name string constant survives anywhere in the solution — the only"ResultsEnquiry"string left is a comment noting that the value fitsChangeRequest.RequestType's 20-character column.The Amendment Requests grid stays unsplit. Both populations keep one table, one set of checkboxes, one bulk submit. Splitting it would double the bulk-submit control and the empty states for a school that in practice holds a handful of requests, and would make the common case — a window with one exercise — carry a grouping header that says nothing.
But the deadline on it was wrong. The page read
AmendmentRequestsResult.WindowEndDate, which is the outer window's end, and since #319 that is the union of the exercises — so on a 16-19 window it was the results-enquiry close, months after pupil data shuts. The page told a school it still had until 31 March to amend pupil data that closed on 18 October. This is the same defect #317 fixed on Check your pupil data; it survived here because this page reads its own DTO.WindowEndDateis replaced byDeadlines, oneExerciseDeadlineDtoper exercise inSortOrder, each carrying its ownEndDateand its ownIsOpenfromICheckingExerciseService. The page prints one sentence per exercise: "Submit your … requests by …" while open, "The deadline for … requests passed at …" once closed. The Requests tab lists only the open ones, and says so plainly when every exercise has closed rather than offering an edit that #318 blocks.The bulk-submit confirmation page had the same bug and is fixed differently: its banner offers another amendment, and that journey shuts when pupil data shuts, so it reads the pupil-data exercise's end specifically. A window that runs no pupil-data exercise now drops the banner rather than quoting a date from an unrelated exercise.
The config-key trigger is written down in
docs/16-19-window-model.md. The key stays{WhatToChange}_{CheckingWindowType}.json; the one thing that forces a third axis is two exercises wanting the sameWhatToChangefor the same window type. The note records what has to move together when that happens — the rename of both the blobs andWeb/Data/QuestionFlows/, the retirement of the map, and whereIsSessionReadythen gets the exercise from (passed by the entry-point controller, not stored onRequestState, which is the disagreement the map exists to prevent).The design note's open questions 1, 2 and 4 are now answered in place, and 3 is marked as split out to #324.
#319 — An admin can now build a multi-exercise window, which until today had to be done by hand in the database. The wizard gained one step — "Which checking exercises does this window run?" — listing every
CheckingExerciseTypeand pre-ticking the window type's defaults, then asks each ticked exercise for its own dates. That shape satisfies both acceptance criteria at once: a new member of the enum surfaces with no change to the page, and a single-exercise window is one Continue and one date page (fewer steps than the two window-level date pages it replaces). It also means KS4 Autumn can be given a results enquiry by ticking a box, which is the gapdocs/16-19-window-model.mdopens with.The outer pair is derived, not typed.
StartDateControllerandEndDateControllerare deleted, along with their views: there is no window-level date step for the exercises to disagree with.CheckingWindowDto.DeriveDatesFromExercisestakes the earliest start and the latest end, andWindowServicecalls it on every create and update, so moving one exercise's end past the window's own end widens the window rather than being rejected. The summary page shows the derived pair with no Change link and labels it as such.The validation stamp moved down to the exercise and gained a meaning it never had.
CheckingWindow.Validatedis gone;CheckingExercise.Validated(ExerciseValidated, renamed fromWindowValidated) takes its place. The old one was written unconditionally by bothCreateAsyncandUpdateAsync, so every window on every environment read as validated whether or not anything had been — which is why the migration deliberately backfills nothing rather than propagating a value that recorded nothing. The new stamp carries the checksums of the datasets the run actually read, so replacing an ingress file afterwards leaves a stamp the summary shows as "Not since the files changed" rather than as a clean bill of health for data nobody validated.Validation is per exercise, not per window.
ValidateWindowControllertakes the exercise in its route and the summary page renders a Validate button per exercise. A loop over every exercise in one run was considered and rejected: it would emit several terminal events down a progress stream whose client expects one, and would stop an admin revalidating a single exercise after swapping one of its files. One run at a time is exactly the shape the processor and the stream already handle, and is what makes "a window is usable while another exercise is still unvalidated" true rather than merely allowed.The schema and ingress steps take the exercise in their routes too, because a dataset name is only unique within one exercise.
CheckingWindowDto.AllDatasetsis retired with its#319 retires thiscomment — the wizard, the summary and the validate run all name the exercise they mean. Two pre-existing accessibility defects on pages this touched are fixed in passing: the validate page rendered a second<h1>in its no-JS result panel, and the create check-answers page had two<h1>s plus a "Confirm you want to delete this window" heading.Split out: the results-enquiry ingress step is #324, as this ticket's text allows.
docs/results-enquiry.mdalready places the six-file ingestion pipeline with the FACT tickets, and the supplier CSV column headers for those files are not confirmed anywhere in this repo — #324 records the two questions (headers, and whether the files carry LAESTAB) that a sample CSV would settle.#318 — Every entry point into a journey now rejects a closed checking exercise, so a bookmarked URL or a tab left open across the closing date can no longer post into a shut journey. The gate is
ICheckingExerciseService.IsOpenin all five places:JourneyController.IsSessionReady(which every journey action already ran, so all thirteen are covered by one change),WhatToChangeController.Index/.ConfirmandConfirmCorrectController.Index/.ConfirmonPupilData, andResultIssueController.Index/.ConfirmonResultsEnquiry— that last one was previously reachable for as long as the outer window was open, which on a 16-19 window is months too long in one direction and, for pupil data, months too long in the other.CheckYourPupilDataController's next-steps POST was already gated by #317.The journey's exercise is derived from
RequestState.SelectedWhatToChangethroughWhatToChangeCheckingExerciseMap, never stored: a stored copy can disagree with the journey's own change type, and every session in flight on deploy day would have it null. Adding an exercise type therefore means adding a row to that map, not editing the guard. The map returnsCheckingExerciseTypenow rather than the string names it carried before the enum existed.No gated path 404s. Every rejection lands on Check your pupil data with a GOV.UK notification banner saying which deadline has passed and that the data is still viewable and downloadable — closed removes actions, never content.
DownloadEvidencewas the one action answeringNotFoundfor an unready session; it redirects like the rest now. Wording lives inClosedExerciseGuard(Web, besideNextStepLabels) with no default case, and a test pins that everyCheckingExerciseTypehas its own message.Product decision 2026-08-20 on drafts across the boundary: block the resume, not the submit.
AmendmentRequestsController.Editrejects a draft whose exercise has closed, so nobody edits a request that could never be sent. The draft stays listed and readable on Amendment Requests.#317 — The check-your-pupil-data page's "what would you like to do?" options are built from the exercises open right now, for any number of exercises, instead of from a window-type test.
INextStepsServiceholds the mapping —PupilDatagives RequestChange + Confirm, which appear and disappear together because they belong to the same exercise;ResultsEnquirygives its own option. Adding a future exercise type means adding a row to that dictionary, never a new branch, and nothing in the service looks atCheckingWindowType. An unmapped type contributes nothing rather than throwing — one bad row must not take the page down — and a test pins that every type that exists is mapped, so a new one cannot ship unreachable. A KS4 Autumn window configured with an enquiry exercise now offers the option, which the old Post16-only test denied it.The POST re-derives the same list and rejects anything outside it, so not rendering an option stays a UI courtesy rather than the only guard.
OffersResultsEnquiryandShowResultsEnquiryOptionare gone with both theirPARKEDcomments, and the controller no longer takes aTimeProviderat all — its one clock use was the outer-window comparison, which per #315 belongs in one place.Three form states. Many options render radios; one renders a single button with a hidden field, because a one-item radio group is a poor pattern and contradicts its own "select one option" hint; none renders no form at all, plus a line saying the window is closed for changes. The tables, the search and the downloads sit above the form and always render — a closed exercise removes actions, never content. Confirm keeps the last position after the
ordivider, so KS4 and 16-19 both render exactly as they do today.The deadline sentence now reads
EndDateFor(PupilData)rather than the outer window's end date, which on a multi-exercise window is months later and promised slack the school does not have, and it turns past tense fromIsOpen(PupilData). Checking-window dates are UK wall-clock values, so it is formatted as it stands and not routed throughLondonTime.One addition the ticket did not ask for. #313 backfilled
PupilDataand nothing else, so the moment the options follow the exercises, every deployed 16-19 window would have lost "Report an issue with an exam result" — a shipped feature silently withdrawn, since only the local dev seed creates aResultsEnquiryexercise.BackfillResultsEnquiryExercisegives eachPost16window that exercise on the window's own dates, reproducing today's behaviour exactly. It isNOT EXISTS-guarded, so a window already configured with real enquiry dates keeps them, and itsDownremoves only rows still matching the shape it wrote. Deliberately transitional: #319's admin sets the real per-exercise dates.#316 — Each checking exercise owns a prefix inside the existing
{windowId}container, and an ingress run belongs to an exercise rather than to the window. The failure this closes is the clear sweep: it deleted everything underdata/and every{windowId}_summary_blob, so a second exercise's run destroyed the first's output and forced a re-upload of data that was never at fault.Application/WindowManagement/CheckingExerciseBlobPaths.csis now the only description of the layout. Pupil data keeps the bare prefix and results enquiry keepsresults-enquiry/, so this needed no blob migration — blob prefixes match as plain strings, so adata/sweep never reachesresults-enquiry/data/, and every blob already written is still found where it was. The lookup is a kebab-case slug rather than the enum'sToString()(which would emitResultsEnquiry/and orphan every results blob), and it has no default case, so a new exercise type throws instead of silently sharing another's prefix.The ticket named two sweep prefixes. There was a third leak it did not: the error log was named on the window alone, so a second exercise's run would have overwritten it. Summaries and the error log are both exercise-scoped now, with pupil data's staying exactly where they already are.
ICsvSchemaFileProcessor.ProcessAsynctakes the exercise — it selects the write prefix and scopes the sweep. So does every path-naming method onIPupilDataBlobClient,ListSchoolLaestabsAsyncincluded, which is what feeds the dashboard's eligible-schools figure; that now counts the pupil-data prefix explicitly and is unchanged for an already-ingested window.IStudentResultsClientkeeps its parameterless surface, its exercise implied by the class, butResultsEnquiryBlobPathsderives from the shared helper rather than describing the layout twice.ValidateWindowControllerstill makes one processor pass, now scoped: it resolves the exercise that owns the datasets, in sort order, and passes it through.WindowServiceonly ever attaches datasets to the pupil-data exercise today, so this is exactly current behaviour. Looping it raises questions #316 does not answer — multiple terminal progress events, when theValidatedstamp is written, what the no-JS fallback summarises — and #319 owns those, which is why #316's file list omits this controller.#315 —
ICheckingExerciseServiceis the only place in the solution that compares a checking exercise's dates against the clock.IsOpen(exercises, type),OpenCheckingExercises(exercises)inSortOrder, andEndDateFor(exercises, type). Time comes from an injectedTimeProvider(GetLocalNow(), asLandingPageServicealready reads it) and is never accepted from a caller, so no caller can supply its own clock. No branch in the service depends onCheckingWindowType.It takes the exercise rows rather than a window DTO because two unrelated classes are named
CheckingWindowDto, and the WindowManagement one already carries its ownIsOpenproperty that a DTO parameter would read as contradicting.Fails closed: no row for a type, or an empty list, is closed for that type. That is an actions-only rule — an empty
OpenCheckingExercisesmust never remove a card or hide content, and #317/#318 build on that as they land.Both remaining window read paths now carry the list:
LandingPageRepository.GetOpenWindowsAsyncandCheckYourPupilDataRepository.GetCheckingWindowAsyncproject ontoCheckingWindowDto.Exercises;WindowRepositoryalready did from #314. The property isExercises, notCheckingExercisesas the ticket text has it, because #314 had already named that list on the WindowManagement DTO and one list should not carry two names. Persistence imports the sharedCheckingExerciseDtothrough ausingalias, since importing the wholeWindowManagementnamespace makesCheckingWindowDtoambiguous there.One acceptance criterion is met but not guarded by a test: nothing outside the service compares an exercise date. A source scan cannot tell an exercise-date comparison from the window-date comparison
LandingPageRepositorylegitimately does, so the check was done by inspection rather than pinned by a brittle test.#314 —
CheckingWindowDataset.CheckingExerciseIdreplaces the window foreign key, and the unique index moves from(CheckingWindowId, Name)to(CheckingExerciseId, Name), so two exercises of one window may each hold a dataset of the same name.CheckingWindow.Datasetsis gone;CheckingExercise.Datasetsholds them, any number including none.CheckingWindowIdsurvives as a plain column, marked legacy and still written but read by nothing, so the release can be rolled back — a follow-up ticket drops it.Migration
20260819164322_ReparentDatasetsOntoCheckingExerciseadds the column, backfills, then adds the index and foreign key, in that order because the key cannot land before the rows have a parent. It does one thing the ticket did not specify: a window created between the #313 and #314 deploys has dataset rows but no exercise, because the previous release wrote datasets straight to the window, and the foreign key would reject those rows. So the migration re-runs #313's idempotent exercise backfill first, then repoints, then raises a clear error if anything is still unparented rather than letting it surface as an opaque constraint violation.On the read side,
CheckingWindowDto.Exercises(holding a newCheckingExerciseDto) replacesCheckingWindowDto.Datasets, withAllDatasetsandFindDataset(name)as the derived paths.WindowServicesyncs the window type's dataset slots onto the pupil-data exercise, creating that exercise on the window's own dates when a window has none;WindowRepositorysyncs exercises by type and datasets by name within each, so ids and already-uploaded files survive. The four admin readers —SchemaController,IngressFileController,SummaryController,ValidateWindowController— go through the exercise path.AllDatasetsis deliberately transitional.The ingest still runs a window's files in one pass and the summary page still lists them as one table; #316 and #319 make both per-exercise and should retire the flattening.#316 moved the ingest off it —ValidateWindowControllernow reads the ingesting exercise's own datasets. Its last caller is the summary page's one flat table, which #319 retires.#313 —
CheckingExerciseentity andCheckingExerciseTypeenum, wired intoPortalDbContext. Migration20260819144921_AddCheckingExercisesbackfills a singlePupilDataexercise on the window's own dates for every existing window, so single-exercise window types behave exactly as before.SeedCheckingWindowsseeds one row for KS2 / KS4 June and several for 16-19 and KS4 Autumn.Also on this branch, from the results-enquiry side: the
requireResultssearch restriction onPupilSearchpages, applied before the ten-suggestion cap;IStudentResultsClient.GetStudentIdsWithResultsAsyncwith its blob client and seeding; a separateResultSearch.cshtmlview whose no-match text reads "No students found with results", so a school can tell a typo from a student who holds nothing. Docs refreshed:docs/16-19-window-model.md,docs/results-enquiry.md,docs/request-journey.md.Verification
Unit 4207 passed (7 new for #320 covering the per-exercise deadline rows, their SortOrder, the open/closed wording, a window with no exercises, and the confirmation banner's pupil-data-only date; 39 for #319 across the wizard, the draft's step order and derived dates, the stamp's staleness rule and the window-type defaults, replacing the deleted start/end date controller tests; 23 for #318; 21 for #317; 11 for #316; 13 for #315), integration 705 passed (5 new for #319 proving the exercise stamp round-trips, goes stale when a file is replaced, and that an exercise's dates are now editable at all — none of which the schema previously allowed). E2E 149 passed, 4 skipped, 0 failed against a rebuilt container, including two new tests that walk the wizard: one building a 16-19 window whose two exercises run on different ranges, one confirming a KS4 window still takes a single set of dates.
Done when
Design note:
docs/16-19-window-model.md