Skip to content

Add a pupil not found on my DfE pupil roll (simple path) AB#297310 - #345

Open
ajs-uk-dev wants to merge 27 commits into
mainfrom
feature/297310-add_journey_simple_path
Open

Add a pupil not found on my DfE pupil roll (simple path) AB#297310#345
ajs-uk-dev wants to merge 27 commits into
mainfrom
feature/297310-add_journey_simple_path

Conversation

@ajs-uk-dev

@ajs-uk-dev ajs-uk-dev commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Adds the Add a pupil not found on my DfE pupil roll journey (simple path) for KS4 June, KS4 Autumn and KS2. AB#297310.

Simple path means no dataset matching against the roll — the soft-match story (AB#297780) sits in front of this one and its interception point is named and commented, but no matching logic exists here.

docs/add-pupil-journey.md is the full write-up; this is the short version.

What it does

A fourth radio, Add a pupil to data, appears on What to change for the three window types that have a flow. Post16 has no Add_Post16.json and never offers it. The journey is three pages — learner details, admission details, evidence — then the shared summary and confirmation, both reused unchanged.

The one genuinely new mechanism is the synthetic pupil. Every other amendment journey opens with a pupil search that resolves a real dataset pupil into RequestState.SelectedPupil. This one can't: the pupil isn't on the roll yet, which is the whole point of the ticket. So learner-details carries a pupilFromAnswers flag, and AddPupilJourney.BuildPupil mints a PupilDto from its own typed answers on a successful POST. Every downstream consumer — summary templating, drafts, the amendment grid, withdraw — reads SelectedPupil exactly as it already does.

No rules-engine outcomes (ticket B2). Submission writes the same ChangeRequests row every amendment writes (Amendment + AmendmentType.Add, SubmittedUnCommitted) plus the journey blob, but skips the enqueue, and the window-close Zendesk replay skips Add rows. Both sites carry a PARKED [AB#297310](https://dfe-gov-uk.visualstudio.com/b947688b-53a4-4c90-9c43-5a5db31e313d/_workitems/edit/297310) comment, mirroring the AB#296648 ResultsEnquiry precedent. QuestionFlowOutcomeKeyAlignmentTests lists Add in FlowPrefixesThatDoNotRouteToTheRulesEngine and asserts it has no outcome key, so it can't be quietly bound to routing later. Downstream is the LDS egress, a separate story.

Two things a reviewer should look at first

WhatToChangeController.Confirm is shared by all five journeys. It now clears the per-request identity — reference, selected pupil, matched pupil, selected result, answers, history — for any flow with no PageType.PupilSearch page. Every other journey has always started clean by accident of its shape, because PupilSearchPost regenerates all of that on every pupil selection; a flow without one inherits whatever the last journey left behind. Two live consequences: a submitted request's reference was reused and its row overwritten by the upsert, and an abandoned Merge journey's matched pupil surfaced on the Add summary as "Second record to merge" — the merge rows are gated on MatchedPupil != null && SelectedPupil != null with no WhatToChange check — and that unrelated pupil's full PupilDto was persisted into the Add request's blob.

It's keyed on the flow's shape rather than on WhatToChange.Add, so the next pupil-search-less journey inherits the guarantee instead of the bug. Every existing flow has at least one pupil-search page, and Confirm_ForAFlowWithAPupilSearchPage_LeavesTheSessionIdentityAlone pins that, but the blast radius is the thing to test.

The merge with main is non-trivial in one file. ad80cbe7 resolves WhatToChangeController, where main's #318 checking-exercise gate and this branch's reset landed on the same two methods. The gate runs first — a closed exercise is turned away before anything touches session state. Index also needed a fix git did not flag: the automatic merge kept both sides' GetCheckingWindowAsync calls and left a duplicate local.

Verified

Against the merged tree, with the dev stack rebuilt so main's six new migrations applied:

Suite Result
Unit 4391 passed, 0 failed
Integration 724 passed, 0 failed
E2E (Category!=VisualRegression) 118 passed, 0 failed, 41 skipped

A full Add walk in a browser submits as CYPMD_KS4June_* with zero queue messages, and a Remove journey started afterwards still routes to select-pupil.

AddPupilJourneyTests covers the happy path, required-field errors, the future-date block and Post16 not offering the option. It does not guard the two session-carryover defects above: PageTest gives each test a fresh browser context, so a test that runs one journey in an empty session has nothing to carry over. Those are pinned at the unit level instead. Closing that gap needs a seeded pupil for the Merge search, which the dev org currently lacks — /pupils/suggestions returns [] for every query.

Open, not blocking

  • Copy is placeholder. The radio label, the sex option U labelled "Not known", the SEN order E→K→N, and every page/question/error string were implemented verbatim and pinned by tests — Figma Epic B1 wasn't reachable from this environment. Needs content-design sign-off.
  • Two flagged assumptions the ticket's validation column doesn't specify: future-date rules on DOB and admission date, and the 150/13 character caps. Cheap to keep, one deletion each if the BA strikes them.
  • KS2 and KS4 Autumn have no seeded dev window, so those two flows ship pinned by AddFlowTests but unwalked in a browser.

Andy Smith and others added 18 commits August 19, 2026 17:21
…ion pupil identity for the Add journey AB#297310

Four defects surfaced only by driving the journey in a real browser (Task 10's
mandatory manual walk), none caught by the unit/integration/E2E suites:

- The admission-date hint's {pupilName} token was never resolved (only
  Title/ValidationFailure are templated) - rendered literally in the browser.
  Reworded the hint to avoid the token instead of adding new engine plumbing.
- ResolveRequestType's "fall back to the first answered question" rule picked
  up the learner-details page's own typed first name, showing "Add - Alice" in
  the amendment grid. Added a guard: a page flagged PupilFromAnswers has no
  request-type sub-category to guess at, so the fallback stops there.
- WhatToChangeNoun/WhatToChangeLabel had no case for WhatToChange.Add, falling
  through to the enum's raw, lower-cased name ("add") in summary/view headings
  and duplicating "Add"/"add" instead of "Add a pupil to data"/"addition".
- WhatToChangeController.Confirm never reset ReferenceNumber/SelectedPupil/
  QuestionAnswers/QuestionHistory when starting a fresh journey. Every other
  flow is protected because its pupil-search step unconditionally regenerates
  both; the Add journey has no such step, so AddPupilJourney.BuildPupil's
  by-design reuse (for stability across re-edits within one journey) silently
  reused a previous, already-submitted Add request's identity for a brand-new
  one in the same browser session - overwriting it. Reproduced live, fixed,
  and re-verified live: two sequential Add submissions in one session now get
  distinct references and pupil ids.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…start

Pre-existing bug, unrelated to AB#297310. Program.cs deliberately opts out of
AddPersistenceDependencies (where every other host gets AddMemoryCache() from)
in favour of its own manual DbContext registration, but AddInfrastructureDependencies
now also registers StudentResultsBlobClient (IStudentResultsClient), which needs
IMemoryCache to cache per-school results JSON. Without it, the worker's service
provider fails validation at startup and the container crash-loops (exit 139).

Found while investigating why cypd_rules_engine kept restarting after a full
docker compose down/up --build.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…arch AB#297310

Widens and completes the reset added in 000e431, which was keyed on
WhatToChange.Add and cleared only part of the per-request state.

Every flow that opens with a pupil search is protected by PupilSearchPost,
which unconditionally regenerates the reference and the selected pupil and
nulls the matched pupil and the selected result on every selection. A flow
WITHOUT one - the Add journey is the first - has nothing that refreshes any of
it, so whatever the previous journey left in session survives into the new one.
Two things went wrong:

- A submitted request's reference was reused. SummaryConfirm clears every other
  per-request field after a successful submission but deliberately keeps
  ReferenceNumber, because the confirmation page reads it back out of session
  to render (so clearing it there is not an option). BuildPupil then reuses it
  - correct for re-edits within one journey - and the upsert overwrote the
  already-submitted row. This is the half 000e431 caught.
- An abandoned Merge journey's matched pupil surfaced on the Add summary.
  JourneyViewModelBuilder and SubmittedRequestService.BuildMergeDisplays build
  the "First/Second record to merge" rows from MatchedPupil != null &&
  SelectedPupil != null alone, with no check on WhatToChange. Select a primary
  and a duplicate pupil on Merge, walk away to /WhatToChange, pick Add, and the
  Add summary and submitted-request view replaced their "Pupil name" row with
  an unrelated pupil's name and CYPMD id - and that pupil's full PupilDto was
  persisted into the Add request's journey blob. This half was missed.

Keying the reset on the flow's shape (no PageType.PupilSearch page) rather than
on WhatToChange.Add means the next pupil-search-less journey inherits the
guarantee instead of the bug. Behaviour for Remove/Include/Merge/IncorrectGrade
is unchanged - all four have a pupil-search page - and a new test pins that.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sent AB#297310

The character-limit arm added for FreeText in 578f8fe kept TextArea's
null-forgiving dereference:

    QuestionType.TextArea or QuestionType.FreeText
        when question.CharacterLimit.HasValue && answer.TextValue!.Length > ...

That is safe for TextArea, which is preceded by its own
"IsNullOrWhiteSpace -> is required" arm, but FreeText has no such arm - its
emptiness is handled by the generic arm further DOWN the switch, so the length
is measured first. ReadFormAnswer yields TextValue = null whenever the form key
is missing, so a POST that omits a required FreeText field with a characterLimit
threw a NullReferenceException instead of reporting the missing answer.

Reproduced against the running app: POSTing the Add journey's learner-details
page with a valid antiforgery token and q_first_name omitted returned HTTP 500
(System.NullReferenceException at JourneyValidationService.ValidateAnswer);
on main the same POST returned the page with "Enter the pupil's first name".

Fixed with a null-conditional dereference - a null length compares false against
the limit, so the answer falls through to the "is required" arm as it should.
Also latent for any future FreeText + characterLimit question on an
EvidenceUpload page, since ValidateEvidencePage synthesises a bare
QuestionAnswer with a null TextValue.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oticed AB#297310

Each of these was verified by mutation - apply the defect, watch the new test
go red, revert.

- WindowCloseReplay could not fail. It wired a bare IQuestionFlowService
  substitute, which returns a null config, so the pre-existing
  "config is null -> continue" produced an identical outcome with or without
  the Add guard; and it had no sibling row, so it could not show the guard
  continues rather than short-circuiting the loop. It now runs the real
  QuestionFlowService over the shipped flow files (ShippedFlowFileClient) with
  a Remove row in the same batch, and asserts the Remove commits and produces
  exactly one Zendesk document while the Add stays SubmittedUnCommitted and its
  reference reaches no queue at all. Disabling the guard now fails it.

- No test pinned that hints are free of {pupilName}. Hints are the one copy
  field rendered verbatim - _FreeText/_Date/_Radio emit Question.Hint straight
  into the markup, with none of the JourneyTemplate resolution Title and
  ValidationFailure get - which is exactly how the admission-date hint shipped
  a literal token before 000e431 reworded it. AddFlowTests pinned pageTitle
  but not hint, so a repeat would have merged green.

- The mint-after-validation property was untested. The existing PagePost test
  stubs ValidateAnswer to always pass, so hoisting MintSyntheticPupilIfNeeded
  above the "if (!isValid)" early return kept the suite green - and a rejected
  page would then stamp a pupil and a reference built from answers the user was
  just told were invalid. Adds a failed-POST test, a failed-edit test (an
  already-minted pupil must survive untouched), and coverage of the fromSummary
  save point, which had none.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The LDS section told the future egress story to read data that is never
produced for an Add request: it said the bound values "egress later via each
answer's RawValue" and that dates are "stored YYYY-MM-DD via the engine's
existing RawValue Date handling". AnswerRecord.RawValue is only produced by
RequestService.BuildRequestDocument, which an Add submission never reaches -
as the same document already says two sections earlier.

What is actually persisted is the journey blob's raw RequestState.QuestionAnswers:

- Radio answers store the option's stable value (the LDS code) in TextValue,
  with the label looked up from the flow config at render time. This half was
  right, just attributed to the wrong mechanism.
- Date answers store a DateAnswer { Day, Month, Year } object. Nothing in the
  Add path emits an ISO string, so the egress story has to format one itself.

Also documents the fresh-journey reset in WhatToChangeController.Confirm, which
was undocumented and is the subtlest behaviour in the feature - including why
the reference cannot simply be cleared after submission (the confirmation page
reads it back out of session).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Conflict: src/DfE.CheckPerformanceData.Web/Controllers/WhatToChangeController.cs

Both sides changed the same two methods for unrelated reasons, so both changes
are kept:

- main (#318) gates Index and Confirm on CheckingExerciseType.PupilData, so a
  bookmarked /WhatToChange URL is rejected once pupil data checking closes.
- this branch (AB#297310) fetches the flow config before the session write and
  resets the per-request identity for any flow with no PageType.PupilSearch page.

Confirm now gates first and resets second — a closed exercise must be turned away
before anything touches session state. The config fetch stays above
SaveRequestState because the reset decision reads it; the trailing null check
reuses that variable rather than re-fetching as main did.

Index needed a fix git did not flag: the automatic merge kept both sides'
GetCheckingWindowAsync calls, leaving a duplicate local (CS0128). main hoisted
the call to the top for its gate, this branch added one lower for
CheckingWindowType. Collapsed to the single call at the top, which feeds both.

Verified on the merged tree, with the dev stack rebuilt so main's six new
migrations were applied: build clean, unit 4391 passed, integration 724 passed,
E2E 118 passed / 41 skipped (AddPupilJourneyTests 4/4), all zero failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhvhVnamKPjCWGDHGCU3HM
The four tests in TurnaroundCommitmentTests share one checking-window row, and
EditPage_PrefillsCurrentValue read its value off the Summary page before
asserting the edit input held the same string. "Not set" is not a stored value —
it is Summary.cshtml's placeholder for an empty column — while the edit input
binds the raw value and renders "". So whenever xUnit ordered
EmptySubmission_IsAllowed_AndShowsNotSet first, which blanks the row, the test
compared "Not set" against "" and failed.

It now seeds its own value through the form and reloads the edit page to assert
the prefill, which removes the coupling and makes the assertion mean something:
on an empty row the old one reduced to "" == "", which a page that does no
prefilling at all would also satisfy. Confirmed by mutation — setting
TurnaroundCommitmentController.Edit's TurnaroundCommitment to null now fails the
test, where before it passed on an empty row.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhvhVnamKPjCWGDHGCU3HM
@ajs-uk-dev ajs-uk-dev linked an issue Aug 21, 2026 that may be closed by this pull request
Conflict: tests/DfE.CheckPerformanceData.E2ETests/WindowAdmin/TurnaroundCommitmentTests.cs

Main fixed EditPage_PrefillsCurrentValue independently in 52dc131 while this
branch was fixing it in f706caa. Both cure the same order dependence: the four
tests share one checking-window row, and comparing the edit input against the
Summary's "Not set" placeholder failed whenever the empty-submission test ran
first. Main maps the placeholder to an empty string before comparing; this
branch seeded its own value instead.

Resolved by taking main's version whole, so the branch no longer touches this
file. The test belongs to main, main's fix works, and this branch is for
AB#297310 — a competing rewrite of a main-owned test does not belong in it.
f706caa stays in history with its effect superseded.

Note for later, not fixed here: main's version asserts "" == "" whenever the
shared row is empty, which a page that does no prefilling at all would satisfy,
so how much it proves depends on the order xUnit picks. Worth a separate PR
against main rather than a change smuggled in through this merge.

The automatic merge had also left the method uncompilable — two `expected`
declarations and a reference to a `value` local that no longer existed.

Verified on the merged tree with the dev stack rebuilt: build clean, unit 4392
passed, integration 724 passed, E2E 118 passed / 41 skipped, and both
TurnaroundCommitmentTests and AddPupilJourneyTests 4/4 run in isolation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XhvhVnamKPjCWGDHGCU3HM
@github-actions

Copy link
Copy Markdown

Deployments

Review app is available at these URLs:
https://check-performance-data-pr-345.test.teacherservices.cloud

Andy Smith and others added 6 commits August 22, 2026 09:43
A null flow config collapsed into "this flow has no pupil search" and took the
full-reset branch, so any radio without a flow file — three of the four on a
Post16/KS2/KS4Autumn window — or a forged enum value destroyed an in-progress
journey's reference, pupils, result, answers and history. An unknown flow now
leaves session state alone, exactly as it did before the Add journey existed;
the redirect to Check your pupil data already handles it.

The reset also missed the two origin-country fields. Only the EAL pages write
them and OriginCountryLanguageCapture no-ops on a page without a country
question, so an abandoned EAL journey's country data rode into the added
pupil's request blob.

Post16 exclusion was enforced by the Razor gate alone. SupportedWindowTypes on
AddPupilJourney is now the single source of truth for both the radio and a
guard on the post it produces, so uploading an Add_Post16.json cannot open the
journey on a window type it was never offered for.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment claimed AddInfrastructureDependencies registers
StudentResultsBlobClient, but #328 moved that registration to the web host's
AddCpdBlobStorage. Nothing left in the worker's service graph consumes
IMemoryCache, and the worker validates on build, so a missed consumer would
fail its startup rather than pass silently.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Add journey only checked each date against today, so a date of birth of
01/09/2025 with an admission date of 01/09/2010 passed validation and reached
the LDS egress. The two dates now have to be in order, equal dates allowed.

They sit on different pages, so the rule reads the journey's whole answer set
with the posted page's answers overlaid, and anchors the violation to whichever
of the two questions is on the page being validated — the message has to point
at a field the user can see. Modelled on PageDateRules, which does the same
shape of ordering check for the EAL dates.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A FreeText question's characterLimit was checked on the server only, so the
browser let the user type well past a cap their submit would then be rejected
for. _TextArea already wires the GOV.UK character count module; that component
supports textareas only, so the single-line input gets maxlength instead.
Appropriate for the questions that declare a limit today — a 13-character UPN
whose hint already states the length, and a 150-cap name — and the server rule
remains the enforcement.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "Pupil name" summary row is only rendered when the journey has a pupil
search page for its Change link to reach. The Add journey has none, so the row
rendered without a link, immediately above the first and last name rows that
carry the same name and do have one.

The merge rows are now keyed on the flow declaring a match pupil search rather
than on a matched pupil being in session, so a matched pupil left behind by an
abandoned merge journey cannot surface on an Add summary as "Second record to
merge". Covers the read-only submitted view, built from the same method.

EditAdviceService names the Add request type instead of falling through to the
generic wording; routing to the summary was already right for it.

AddPupilJourney's and AddJourneyDateRules' id constants are pinned to string
literals, so a rename breaks a test instead of silently blanking the pupil name
in the summary row, the request document and the egress, and the supported
window types are pinned to the flow files that actually exist.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The happy-path E2E test read the pupil's name out of the combined "Pupil name"
row, which this journey no longer renders. The first and last name rows carry
the same name and their Change links work, so the assertion moves onto those.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ajs-uk-dev

Copy link
Copy Markdown
Contributor Author

Thanks for the review — all ten points are addressed across five commits.

Blocking — null flow config wipes journey statef89174c. An unknown flow no longer takes the reset branch, so a radio with no flow file, or a forged enum value, leaves an in-progress journey alone as it did before this PR. Two regression tests cover it, including your repro.

Fix before merge

  • Reset missed the origin-country fields — f89174c. Both are now nulled, and SeedStaleIdentity seeds them so the test can fail.
  • Post16 exclusion enforced only in Razor — f89174c. AddPupilJourney.SupportedWindowTypes is now the single source of truth for the radio and for a guard on the post, checked before the flow service or the session. The new test stubs a non-null Add config, so it pins the window-type rule rather than the missing blob.
  • Worker AddMemoryCachec0458f9. Nothing in the worker's graph consumes IMemoryCache since Stop registering IStudentResultsClient in the worker's dependency bundle #328, so the call and its comment are gone. The worker validates on build; it comes up clean with 0 restarts.
  • Admission date could precede date of birth — 5524828. The two dates must now be in order, equal allowed. They are on different pages, so the rule reads the journey's answers with the posted page's overlaid and anchors the error to whichever question the user is looking at, modelled on PageDateRules.

Minor

  • FreeText character limits were server-only — 69ae835. The GOV.UK character count component is textarea-only, so the single-line input carries maxlength; the server rule stays the enforcement.
  • Actionless "Pupil name" row — 3e1893c. Rendered only when the journey has a pupil-search page for its Change link.
  • Merge rows keyed on session state — 3e1893c. Now keyed on the flow declaring a match pupil search, covering the submitted view too.
  • EditAdviceService Add arm — 3e1893c. Adds the Add wording; the summary target was already correct.
  • Constants asserted against themselves — 3e1893c. AddPupilJourney and AddJourneyDateRules ids are pinned to string literals, and the supported window types to the flow files that exist.

c90eeb3 moves one E2E assertion onto the first/last name rows, since the combined row it read is the one removed above.

Verification after each commit: solution builds clean, unit (4415) and integration (726) green. Mutation-checked the state-reset guard and the window-type rule — reverting either fails the new tests and nothing else. Rebuilt worker and web containers; full non-visual E2E is 118 passed / 0 failed / 41 skipped, matching the pre-change baseline.

The four IncorrectGradeEnquiryTests failures you saw are environment-local — they fail on main too and pass in CI.

Moves ExtractConflictingReasonType, ExtractRequestCategory and ConflictQuery
to the bottom of the class and puts the shorter method signatures back on one
line. Token-for-token equivalent to what it replaces — no behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Visual Studio shares the compose project name with the CLI stack and
docker-compose.dcproj builds with the solution, so a solution build — which
Test Explorer triggers before a run — can remove containers brought up from
the terminal. The E2E suite then fails wholesale with ERR_CONNECTION_REFUSED
on :8080, which reads as a broken test rather than a missing app.

Documents the symptom, how to tell it apart from a real failure, and the two
workflows that avoid it. No config change: the shared project name is not what
performs the teardown, and separating it would collide on the pinned
container_name values and host ports instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

KS4June - Add Journey - Simple path AB#297310

1 participant