From 98276a543a7bdb8ee1373f219c44f03bc9a1e600 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 14:19:31 +0000 Subject: [PATCH 01/13] #711 Ingress stamps INCLUDABLE from a schema rule A second stamp beside INCLUDED: true when the record is not included, or when its P_INCL is a code the schema's x-ingress.includable rule names. The 16-19 included file names 502. Every pupil schema declares it. Co-Authored-By: Claude Sonnet 5.5 --- docs/blob-storage.md | 5 +- .../Ingress/CsvSchemaFileProcessor.cs | 49 ++++++++ .../Ingress/ks2/pupils-december_schema.json | 13 +++ .../Data/Ingress/ks2/pupils_schema.json | 13 +++ .../Data/Ingress/ks4june/pupils_schema.json | 13 +++ .../Data/Ingress/post16/README.md | 1 + .../post16/students-included_schema.json | 16 ++- .../post16/students-non-included_schema.json | 13 +++ .../Ingress/CsvSchemaFileProcessorTests.cs | 106 ++++++++++++++++++ 9 files changed, 227 insertions(+), 2 deletions(-) diff --git a/docs/blob-storage.md b/docs/blob-storage.md index 2eab066e5..0dfdb1742 100644 --- a/docs/blob-storage.md +++ b/docs/blob-storage.md @@ -44,7 +44,10 @@ reach the journey as `QAN`, `SYLLABUS`, `SESSION` and `QUAL_NAME`; in the 16-18 schools see. Schools see the label, never the tag. A dataset's `SourceFile` is the exact analogue of its `Included`: `Included` stamps inclusion by file of origin, `SourceFile` stamps provenance, and both are stamped by the processor before validation and guarded by the schema declaring the column (`INCLUDED` / `SOURCE`), because -`AllowAdditionalProperties` is false. A run writes +`AllowAdditionalProperties` is false. A third stamp, `INCLUDABLE` (#711), says whether a school may +ask to include the pupil: true when the record is not included, or when its code is one the +schema's `x-ingress.includable` rule names (the 16-19 included file names `P_INCL` 502). It is +derived from `INCLUDED`, so it follows the same guard. The Include journey reads it. A run writes `CheckingExerciseBlobPaths.DataBlobName(exercise, laestab)` — never `PupilsBlobName` directly: the two normalise the laestab differently on purpose. Every feed must carry a `LAESTAB` column, or the column its schema's `x-ingress.institutionKey` names (`LAESTAB_0` in the previously published file): it is what splits one supplier file into one blob per school; a file without it fails the run by name. diff --git a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs index a57cc9a5f..ec4b0db62 100644 --- a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs +++ b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs @@ -193,6 +193,16 @@ public async IAsyncEnumerable ProcessAsync( yield break; } + // #711: who may be asked to be included. Validated like the inclusion rule, so a schema + // that cannot be honoured fails by name before any row is read. + IncludableRule? includable = IncludableRule.From(schema); + if (includable?.ErrorFor(schema) is { } includableError) + { + summaryCapture?.SetFailure("schema", "identifier_mapping_invalid"); + yield return Failed($"Schema '{dataset.SchemaFile}': {includableError}"); + yield break; + } + // Capture each complete source record before any schema transformation. CsvHelper // counts CSV records, so a quoted field containing line breaks remains one row. List> records = []; @@ -308,6 +318,16 @@ public async IAsyncEnumerable ProcessAsync( record["INCLUDED"] = inclusion.IsIncluded(record); } + // #711: who may be asked to be included — not included, or a code the schema's + // x-ingress.includable rule names (the 16-19 included file's P_INCL 502). Derived + // from INCLUDED, so it is stamped only when INCLUDED was, and only when the schema + // declares it (AllowAdditionalProperties is false). Readers read the stamp. + if (schema.Properties.ContainsKey("INCLUDABLE") && + record["INCLUDED"] is { Type: JTokenType.Boolean } stampedIncluded) + { + record["INCLUDABLE"] = !stampedIncluded.Value() || (includable?.Matches(record) ?? false); + } + // Provenance by file of origin, the exact analogue of INCLUDED above: the // results CSVs carry no SOURCE column, so the tag comes from the dataset slot // the file was uploaded to. Stamped BEFORE validation for the same reason @@ -818,6 +838,35 @@ private string Code(JObject record) => record[Field] is { Type: not JTokenType.Null } value ? value.ToString().Trim() : string.Empty; } + /// + /// Which records a school may ask to include although their file counts them as included: + /// "x-ingress": { "includable": { "field": "P_INCL", "codes": ["502"] } } at the + /// schema's root. Codes are compared as trimmed text. A record that is not included is + /// includable without a rule; the rule only adds to that (#711). + /// + private sealed record IncludableRule(string Field, IReadOnlySet Codes) + { + public static IncludableRule? From(JSchema schema) + { + if (!schema.ExtensionData.TryGetValue("x-ingress", out JToken? ingress) || + ingress["includable"] is not JObject rule) + return null; + + return new IncludableRule( + rule["field"]?.Value() ?? string.Empty, + (rule["codes"] as JArray ?? []).Select(code => code.ToString().Trim()).ToHashSet(StringComparer.Ordinal)); + } + + public string? ErrorFor(JSchema schema) => + string.IsNullOrWhiteSpace(Field) ? "x-ingress.includable names no field." + : !schema.Properties.ContainsKey(Field) ? $"x-ingress.includable reads {Field}, which the schema does not declare." + : !schema.Properties.ContainsKey("INCLUDABLE") ? "x-ingress.includable needs the schema to declare INCLUDABLE, the stamp it writes." + : null; + + public bool Matches(JObject record) => + Codes.Contains(record[Field] is { Type: not JTokenType.Null } value ? value.ToString().Trim() : string.Empty); + } + private static IReadOnlyList SourceColumns(JSchema schema) { List sources = []; diff --git a/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils-december_schema.json b/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils-december_schema.json index 5520ad49b..a96425ed5 100644 --- a/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils-december_schema.json +++ b/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils-december_schema.json @@ -45,6 +45,19 @@ "order": 999 } }, + "INCLUDABLE": { + "type": "boolean", + "description": "Stamped by ingress: true when a school may ask to include this pupil - not included, or P_INCL is a code x-ingress.includable names. Not a supplier column, so not downloaded or shown.", + "x-csv": { + "columns": {}, + "heading": "Includable" + }, + "x-display": { + "label": "Includable", + "visible": false, + "order": 999 + } + }, "LAESTAB": { "type": [ "string", diff --git a/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils_schema.json b/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils_schema.json index d7d0d1fae..13e616fda 100644 --- a/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils_schema.json +++ b/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks2/pupils_schema.json @@ -45,6 +45,19 @@ "order": 999 } }, + "INCLUDABLE": { + "type": "boolean", + "description": "Stamped by ingress: true when a school may ask to include this pupil - not included, or P_INCL is a code x-ingress.includable names. Not a supplier column, so not downloaded or shown.", + "x-csv": { + "columns": {}, + "heading": "Includable" + }, + "x-display": { + "label": "Includable", + "visible": false, + "order": 999 + } + }, "LAESTAB": { "type": [ "string", diff --git a/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks4june/pupils_schema.json b/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks4june/pupils_schema.json index 996eb68e3..dcc4fad0b 100644 --- a/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks4june/pupils_schema.json +++ b/src/DfE.CheckPerformanceData.Web/Data/Ingress/ks4june/pupils_schema.json @@ -59,6 +59,19 @@ "order": 999 } }, + "INCLUDABLE": { + "type": "boolean", + "description": "Stamped by ingress: true when a school may ask to include this pupil - not included, or P_INCL is a code x-ingress.includable names. Not a supplier column, so not downloaded or shown.", + "x-csv": { + "columns": {}, + "heading": "Includable" + }, + "x-display": { + "label": "Includable", + "visible": false, + "order": 999 + } + }, "UPN": { "type": [ "string", diff --git a/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/README.md b/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/README.md index 11abddcf5..67c3623e4 100644 --- a/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/README.md +++ b/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/README.md @@ -7,6 +7,7 @@ These row schemas transcribe the field references, SQL types, CSV column letters Each row schema uses the same extensions: - `x-ingress.collection` is the key in the combined JSON; `pageTab` groups collections on the page; `institutionKey` identifies the source field used to split incoming records by institution. +- `x-ingress.includable` (#711), on `students-included_schema.json` only, names the `P_INCL` codes of students a school may ask to include although they are in the included file: `502`, "Student not included in performance measures". Ingress stamps `INCLUDABLE` from it; every row of the non-included file is includable without a rule. - `x-download.fileName` and `properties.*.x-csv.columns` identify the separate CSV and its column order. The variant keys distinguish provisional/revised from retention columns where the workbook does. - `x-display.section` gives the selector label; `properties.*.x-display` gives the page label, visibility, order and, for names, searchability. Only selected pupil identity fields are marked visible by default. Other fields remain available for a details view. diff --git a/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-included_schema.json b/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-included_schema.json index 69bbb39a9..6ecff46ec 100644 --- a/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-included_schema.json +++ b/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-included_schema.json @@ -8,7 +8,8 @@ "x-ingress": { "collection": "students-included", "pageTab": "students", - "institutionKey": "LAESTAB" + "institutionKey": "LAESTAB", + "includable": { "field": "P_INCL", "codes": ["502"] } }, "x-download": { "fileName": "ks5pup25.csv", @@ -26,6 +27,19 @@ "x-csv": { "columns": {}, "heading": "Included" }, "x-display": { "label": "Included", "visible": false, "order": 999 } }, + "INCLUDABLE": { + "type": "boolean", + "description": "Stamped by ingress: true when a school may ask to include this pupil - not included, or P_INCL is a code x-ingress.includable names. Not a supplier column, so not downloaded or shown.", + "x-csv": { + "columns": {}, + "heading": "Includable" + }, + "x-display": { + "label": "Includable", + "visible": false, + "order": 999 + } + }, "CYPMD_ID": { "type": [ "string", diff --git a/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-non-included_schema.json b/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-non-included_schema.json index 80cc35948..00d63a743 100644 --- a/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-non-included_schema.json +++ b/src/DfE.CheckPerformanceData.Web/Data/Ingress/post16/students-non-included_schema.json @@ -26,6 +26,19 @@ "x-csv": { "columns": {}, "heading": "Included" }, "x-display": { "label": "Included", "visible": false, "order": 999 } }, + "INCLUDABLE": { + "type": "boolean", + "description": "Stamped by ingress: true when a school may ask to include this pupil - not included, or P_INCL is a code x-ingress.includable names. Not a supplier column, so not downloaded or shown.", + "x-csv": { + "columns": {}, + "heading": "Includable" + }, + "x-display": { + "label": "Includable", + "visible": false, + "order": 999 + } + }, "SCHCNO": { "type": [ "string", diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs index 3c52e4d3a..aff24c429 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs @@ -367,6 +367,112 @@ public async Task A_dataset_with_no_marker_does_not_claim_inclusion_when_the_sch Assert.Null(await ReadPupilsAsync(container, "8604070")); } + // #711: who may be asked to be included. A schema names the codes in x-ingress.includable + // (16-19 P_INCL 502, a student in the included file who is not in the measures); a record that + // is not included is includable without a rule. Readers read INCLUDABLE, never a code. + private const string SchemaWithIncludableRule = """ + { + "type": "object", + "additionalProperties": false, + "x-ingress": { + "includable": { "field": "P_INCL", "codes": ["502"] } + }, + "properties": { + "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, + "SURNAME": { "type": ["string", "null"] }, + "LAESTAB": { "type": ["string", "null"] }, + "P_INCL": { "type": ["string", "null"] } + } + } + """; + + [Theory] + [InlineData("SURNAME,LAESTAB,P_INCL\nSmith,8604070,502\n", true, true)] + [InlineData("SURNAME,LAESTAB,P_INCL\nSmith,8604070, 502 \n", true, true)] + [InlineData("SURNAME,LAESTAB,P_INCL\nSmith,8604070,501\n", true, false)] + [InlineData("SURNAME,LAESTAB,P_INCL\nSmith,8604070,\n", true, false)] + [InlineData("SURNAME,LAESTAB,P_INCL\nSmith,8604070,501\n", false, true)] + public async Task The_schemas_includable_rule_stamps_INCLUDABLE_from_the_code_or_from_not_included( + string csv, bool slotIncluded, bool includable) + { + var (last, pupil) = await RunInclusionRuleAsync(csv, SchemaWithIncludableRule, slotIncluded); + + Assert.False(last.IsError, last.Message); + Assert.Equal(slotIncluded, pupil!["INCLUDED"]!.Value()); + Assert.Equal(includable, pupil["INCLUDABLE"]!.Value()); + } + + [Fact] + public async Task A_schema_with_no_includable_rule_stamps_INCLUDABLE_as_not_included() + { + var schema = SchemaWithInclusionRule.Replace( + "\"INCLUDED\": { \"type\": \"boolean\" },", + "\"INCLUDED\": { \"type\": \"boolean\" },\n \"INCLUDABLE\": { \"type\": \"boolean\" },"); + + var (included, _) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,401\n", schema); + var (notIncluded, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,402\n", schema); + + Assert.False(included.IsError, included.Message); + Assert.False(notIncluded.IsError, notIncluded.Message); + Assert.True(pupil!["INCLUDABLE"]!.Value()); + } + + [Fact] + public async Task A_schema_that_does_not_declare_INCLUDABLE_stamps_nothing_for_it() + { + var (last, pupil) = await RunInclusionRuleAsync( + "SURNAME,LAESTAB,P_INCL\nSmith,8604070,402\n", SchemaWithInclusionRule); + + Assert.False(last.IsError, last.Message); + Assert.Null(pupil!["INCLUDABLE"]); + } + + [Fact] + public async Task An_includable_rule_on_a_schema_that_does_not_declare_INCLUDABLE_fails_the_run_by_name() + { + var schema = SchemaWithIncludableRule.Replace("\"INCLUDABLE\": { \"type\": \"boolean\" },", string.Empty); + + var (last, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,502\n", schema, slotIncluded: true); + + Assert.True(last.IsError); + Assert.Contains("INCLUDABLE", last.Message); + Assert.Null(pupil); + } + + [Fact] + public async Task An_includable_rule_that_reads_an_undeclared_field_fails_the_run_by_name() + { + var schema = SchemaWithIncludableRule.Replace("\"field\": \"P_INCL\"", "\"field\": \"P_NOPE\""); + + var (last, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,502\n", schema, slotIncluded: true); + + Assert.True(last.IsError); + Assert.Contains("P_NOPE", last.Message); + Assert.Null(pupil); + } + + // The shipped schemas: KS4 and KS2 stamp INCLUDABLE as "not included"; the 16-19 included file + // names 502; the 16-19 non-included file has no rule and every row is includable. + [Theory] + [InlineData("ks4june", "pupils_schema.json", "UPN,CYPMD_ID,SURNAME,FORENAME,P_INCL,LAESTAB\nA1,1,Smith,Al,401,8604070\n", null, false)] + [InlineData("ks4june", "pupils_schema.json", "UPN,CYPMD_ID,SURNAME,FORENAME,P_INCL,LAESTAB\nA1,1,Smith,Al,402,8604070\n", null, true)] + [InlineData("ks2", "pupils_schema.json", "UPN,CYPMD_ID,MATCHREF,SURNAME,FORENAME,PINCL_Flag,LAESTAB\nA1,1,7,Smith,Al,201,8604070\n", null, false)] + [InlineData("post16", "students-included_schema.json", "CYPMD_ID,SURNAME,FORENAMES,P_INCL,LAESTAB\n1,Smith,Al,501,8604070\n", true, false)] + [InlineData("post16", "students-included_schema.json", "CYPMD_ID,SURNAME,FORENAMES,P_INCL,LAESTAB\n1,Smith,Al,502,8604070\n", true, true)] + [InlineData("post16", "students-included_schema.json", "CYPMD_ID,SURNAME,FORENAMES,P_INCL,LAESTAB\n1,Smith,Al,505,8604070\n", true, false)] + [InlineData("post16", "students-non-included_schema.json", "CYPMD_ID,SURNAME,FORENAMES,LAESTAB\n1,Smith,Al,8604070\n", false, true)] + public async Task A_shipped_pupil_schema_stamps_INCLUDABLE(string folder, string file, string csv, bool? slotIncluded, bool includable) + { + var schema = await File.ReadAllTextAsync( + Path.Combine(AppContext.BaseDirectory, "Data", "Ingress", folder, file)); + + var (last, pupil) = await RunInclusionRuleAsync(csv, schema, slotIncluded); + + Assert.False(last.IsError, last.Message); + Assert.Equal(includable, pupil!["INCLUDABLE"]!.Value()); + } + [Fact] public async Task A_schema_with_an_institution_key_splits_the_file_by_that_column() { From 4dc6650c3f0545d086910b2c465a42cdeddb9695 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 14:22:28 +0000 Subject: [PATCH 02/13] #711 Add the Includable and NotIncludable pupil filters IPupilRecord.IsIncludable reads the INCLUDABLE stamp. PupilFilter gains Includable and NotIncludable and loses NonIncluded; the KS4 Include flow uses Includable, which returns the same pupils as before. The unit tests that named NonIncluded are renamed to Includable so the project compiles; Task 3 re-points the guards. Co-Authored-By: Claude Sonnet 5.5 --- AGENTS.md | 2 + docs/domain-model.md | 4 +- docs/question-flows.md | 4 +- docs/request-journey.md | 6 +- .../CheckYourPupilData/IPupilRecord.cs | 8 ++ .../CheckYourPupilData/Post16PupilRecord.cs | 12 +- .../CheckYourPupilData/PupilInclusion.cs | 2 + .../CheckYourPupilData/PupilRecord.cs | 8 ++ .../Journey/PupilFilter.cs | 9 +- .../CheckYourPupilDataRepository.cs | 4 +- .../Controllers/Journey/JourneyController.cs | 2 +- .../Data/QuestionFlows/Include_KS4June.json | 2 +- .../Seeding/SeedExerciseFixtures.cs | 2 +- .../PupilSearchExclusionTests.cs | 27 +++- .../PupilSearchIncludableFilterTests.cs | 129 ++++++++++++++++++ .../PupilSearchTwoPartNameTests.cs | 5 +- .../Post16PupilRecordDeserializationTests.cs | 47 +++++++ .../Journey/IncludeNoResultsDecisionTests.cs | 10 +- .../Journey/PupilSearchJourneyTests.cs | 2 +- 19 files changed, 263 insertions(+), 22 deletions(-) create mode 100644 tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs diff --git a/AGENTS.md b/AGENTS.md index ba73e3005..146da3e29 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -181,6 +181,8 @@ Results enquiries and 16-19 windows: `docs/results-enquiry.md`, `docs/16-19-wind `x-ingress.source`. Never put a supplier column name in a C# property name. - **Inclusion is the `INCLUDED` stamp.** Ingress decides it once. Code that reads pupils must not know a key stage's inclusion codes. +- **Who may be included is the `INCLUDABLE` stamp.** Ingress writes it from the schema's + `x-ingress.includable` rule (#711). The Include journey reads the stamp, never a code. - **The service is not live yet.** Do not write fallbacks for data written before a change. Treat missing new-format data as an error. (This does not change the migration rule above: that rule protects deployed databases.) diff --git a/docs/domain-model.md b/docs/domain-model.md index 12cd6740a..9471487ae 100644 --- a/docs/domain-model.md +++ b/docs/domain-model.md @@ -135,7 +135,7 @@ Session readiness for the journey requires only `SelectedWhatToChange` and `Chec - The controller holds **no `TimeProvider`** — every "is it open" question goes through `ICheckingExerciseService`. - Displays paged, searchable pupil lists for a given checking window and school (by `laestab`). - `CheckYourPupilDataRepository` loads the school's pupil JSON (cached) and filters/searches/sorts/pages **in memory** — sorted by surname/firstname. **The pupil search boxes share one rule, `PupilSearchTerms`** (parse the query once with `Parse`, then `Matches` per row): the query splits on spaces, tabs and commas, and each word must match the start of a word in some field (words start after a space, tab, hyphen or apostrophe), ignoring case, in any order ("John Smith", "Smith, John"; "ohn" finds nothing). Identifiers match from their start too ("A860" finds a UPN, "8604" does not). The journey autocomplete checks names, UPN/ULN, CYPMD ID and displayed DOB; the pupil tabs check names; the results table checks names, CYPMD ID and subject; the schema-driven tabs check the schema's `searchable` columns. Only the Add-pupil duplicate check (fuzzy on purpose, not a search box) does not use it. -- Pupils are split into included (`IsIncluded`, the `INCLUDED` stamp) and non-included (all others) tabs. +- Pupils are split into included (`IsIncluded`, the `INCLUDED` stamp) and non-included (all others) tabs. A second stamp, `INCLUDABLE` (#711), says who a school may ask to include; only the Include journey reads it. - **Since #466 the tabs can be exercise-driven.** `IExerciseTabBuilder` (Application) returns one tab per *visible* exercise — enabled, within `VisibleFrom`/`VisibleUntil` (exclusive at the end), and every exercise has a `TabName`. When no exercise is visible the list is empty and the page @@ -240,7 +240,7 @@ through `IExerciseEarlyClosureService`, then sweeps). Once it has closed, the su Requests page is removed. **`ICheckYourPupilDataService`** — pupil data queries (partial) -`GetPupilSuggestionsAsync(windowId, query, PupilFilter filter, Guid? excludeId, bool requireResults)` returns up to 10 `PupilSuggestionDto(Id, Label)` records. `PupilFilter.Included` restricts to pupils stamped `INCLUDED`; `PupilFilter.All` returns all pupils for the school. `excludeId` removes a specific pupil from results (used by the match-pupil page to prevent selecting the primary pupil again). `requireResults` restricts the search to students in `IResultsClient.GetCypmdIdsWithResultsAsync` (the same cached school results file the enquiry itself reads), and is passed down to `SearchPupilsAsync` as a CYPMD id allow-list so it applies **before** the ten-suggestion cap — filtering afterwards would drop the one student who holds results whenever ten who do not sort ahead of them. Served by `PupilSuggestionsController` at `GET /pupils/suggestions`. The `WhatToChange?`-based overload no longer exists — callers must pass an explicit `PupilFilter`. +`GetPupilSuggestionsAsync(windowId, query, PupilFilter filter, Guid? excludeId, bool requireResults)` returns up to 10 `PupilSuggestionDto(Id, Label)` records. `PupilFilter.Included` restricts to pupils stamped `INCLUDED`; `PupilFilter.All` returns all pupils for the school; `PupilFilter.Includable` restricts to pupils stamped `INCLUDABLE` and `PupilFilter.NotIncludable` to the rest (#711). `excludeId` removes a specific pupil from results (used by the match-pupil page to prevent selecting the primary pupil again). `requireResults` restricts the search to students in `IResultsClient.GetCypmdIdsWithResultsAsync` (the same cached school results file the enquiry itself reads), and is passed down to `SearchPupilsAsync` as a CYPMD id allow-list so it applies **before** the ten-suggestion cap — filtering afterwards would drop the one student who holds results whenever ten who do not sort ahead of them. Served by `PupilSuggestionsController` at `GET /pupils/suggestions`. The `WhatToChange?`-based overload no longer exists — callers must pass an explicit `PupilFilter`. **`ICountryService`** — country/autocomplete lookup `SearchAsync(query)` returns up to 10 `CountrySuggestionDto(Code, Name)` records matching the query against `Country.Name` (case-insensitive `ILike`). Used by `CountryController.Suggestions` to serve the `Autocomplete` question type. The underlying `Countries` table is seeded from the FCDO Geographical Names Index and is not checking-window-specific. diff --git a/docs/question-flows.md b/docs/question-flows.md index d95e64e9d..4eb74ef21 100644 --- a/docs/question-flows.md +++ b/docs/question-flows.md @@ -36,13 +36,13 @@ The journey feature (pupil change requests) is driven by JSON config files that | Field | Required | Values | Notes | |---|---|---|---| -| `pupilFilter` | yes | `"Included"` / `"All"` | `Included` limits results to pupils stamped `INCLUDED`; `All` returns every pupil for the school | +| `pupilFilter` | yes | `"Included"` / `"All"` / `"Includable"` / `"NotIncludable"` | `Included` limits results to pupils stamped `INCLUDED`; `All` returns every pupil for the school; `Includable` returns pupils stamped `INCLUDABLE` — the Include journey's search, which on 16-19 adds the included file's P_INCL 502 students to the non-included file (#711); `NotIncludable` is the rest, which the Include journey's "already included" guards look a name up in | | `pupilKey` | yes | `"primary"` / `"match"` | `primary` saves to `RequestState.SelectedPupil*` and generates the reference number; `match` saves to `RequestState.MatchedPupil*` | | `requireResults` | no | `true` / `false` (default) | Limits the search to students the school holds a 16-19 result for. Independent of `pupilFilter`, which selects by inclusion status. Set on the results-enquiry student search — there is no grade to correct for a student with no result. The page must say so in its `subheading`, because the restriction silently hides students | | `nextPageId` | no | page id string | Absent means redirect to Summary after selection | | `validationFailure` | no | string (supports `{pupilName}`) | Error shown when no pupil is submitted. Falls back to `"Enter the name of the pupil"` | -**Suggestions endpoint:** `GET /pupils/suggestions?windowId={id}&query={q}&filter=Included|All&excludePupilId={guid}&requireResults=true` (served by `PupilSuggestionsController`). The `match` pupil page automatically passes the primary pupil's ID as `excludePupilId` so it cannot be selected twice. `requireResults` is sent only when the page config sets it; it is a search restriction, never a permission — it can reach no pupil outside the signed-in school's own file. When it is on, the autocomplete's no-match text changes to "No students found with results" so a school can tell a typo from a student who holds nothing. +**Suggestions endpoint:** `GET /pupils/suggestions?windowId={id}&query={q}&filter=Included|All|Includable|NotIncludable&excludePupilId={guid}&requireResults=true` (served by `PupilSuggestionsController`). The `match` pupil page automatically passes the primary pupil's ID as `excludePupilId` so it cannot be selected twice. `requireResults` is sent only when the page config sets it; it is a search restriction, never a permission — it can reach no pupil outside the signed-in school's own file. When it is on, the autocomplete's no-match text changes to "No students found with results" so a school can tell a typo from a student who holds nothing. **Navigation:** `JourneyController.Page` (GET) transparently dispatches to `PupilSearchPage` when the requested page has `Type == PupilSearch`, so all navigation (summary redirects, `GetNextPageId`, navigation guard) works uniformly. diff --git a/docs/request-journey.md b/docs/request-journey.md index b97cfc714..c59ab4d9d 100644 --- a/docs/request-journey.md +++ b/docs/request-journey.md @@ -120,7 +120,7 @@ All flow configs begin with one or more `PupilSearch` pages. These are full-page | Field | Required | Values | Notes | |---|---|---|---| -| `pupilFilter` | yes | `"Included"` / `"All"` | `Included` limits results to Pincl codes `[401,403,414,421,431]`; `All` returns every pupil for the school | +| `pupilFilter` | yes | `"Included"` / `"All"` / `"Includable"` / `"NotIncludable"` | `Included` limits results to pupils stamped `INCLUDED`; `All` returns every pupil for the school; `Includable` returns pupils stamped `INCLUDABLE` — the Include journey's search, which on 16-19 adds the included file's P_INCL 502 students to the non-included file (#711); `NotIncludable` is the rest, which the Include journey's "already included" guards look a name up in | | `pupilKey` | yes | `"primary"` / `"match"` | Controls which session field is populated (see below) | | `requireResults` | no | `true` / `false` (default) | Limits the search to students the school holds a 16-19 result for. Independent of `pupilFilter`, which selects by inclusion status. Used by the results enquiry — there is no grade to correct for a student with no result. A page that sets it must say so in its `subheading`, because the restriction hides students silently | | `pupilSearchField` | no | `"All"` / `"CypmdId"` (default `All`) | Which field(s) the query is matched against. `All` (or absent) matches forename, surname, split names, UPN/ULN and CYPMD ID, plus date of birth on 16-19. `CypmdId` matches **only** a trimmed, case-insensitive prefix of `Cypmd_Id` — a name, UPN or date of birth returns nothing. Set on `Merge_KS4June.json`'s `select-match-pupil`, whose `subheading` already reads "Start typing ID to search records"; had it said nothing, narrowing the search would hide records silently and read as a typo | @@ -129,7 +129,7 @@ All flow configs begin with one or more `PupilSearch` pages. These are full-page `pupilSearchField` also changes what the suggestion text says. On a `CypmdId` page a KS4 suggestion is `"Surname, Firstname, DOB (Cypmd_Id)"`, because the KS4 suggestion leads with name and date of birth — which are identical across the duplicate records a merge exists to reconcile, so without the ID two rows in the dropdown cannot be told apart. `All` keeps the original `"Surname, Firstname, DOB"`; 16-19 is unchanged either way, as it already prints `CYPMD ID:`. The narrowed value is a property of one page: the merge journey's `select-pupil` page and every other journey keep `All`. -**Suggestions endpoint:** `GET /pupils/suggestions?windowId={id}&query={q}&filter=Included|All&excludePupilId={guid}&requireResults=true&pupilSearchField=All|CypmdId` +**Suggestions endpoint:** `GET /pupils/suggestions?windowId={id}&query={q}&filter=Included|All|Includable|NotIncludable&excludePupilId={guid}&requireResults=true&pupilSearchField=All|CypmdId` Served by `PupilSuggestionsController`. Reads the school's pupils from the cached per-school-window blob through `ICheckYourPupilDataService.GetPupilSuggestionsAsync` (`CheckYourPupilDataRepository`); it does **not** query PostgreSQL — the `Pupils` table was dropped when pupil data moved to blob storage. The `match` pupil page automatically passes the primary pupil's ID as `excludePupilId` so the same pupil cannot be selected twice. `requireResults` is sent only when the page config sets it. The service resolves the school's set of CYPMD ids with results (`IResultsClient.GetCypmdIdsWithResultsAsync`, served from the same cached results file the enquiry itself reads) and passes it to `SearchPupilsAsync` as an allow-list, which applies it **before** the ten-suggestion cap — filtering afterwards would drop the one student who holds results whenever ten who do not sort ahead of them. It is a search restriction, never a permission: it can reach no pupil outside the signed-in school's own file. With it on, the autocomplete's no-match text becomes "No students found with results", so a school can tell a typo from a student who holds nothing. @@ -164,6 +164,8 @@ POST → saves MatchedPupil → no nextPageId → redirects to Summary ``` +**Include** uses one `PupilSearch` page with `pupilFilter: "Includable"` (the `INCLUDABLE` stamp: not included, or a code the schema names — 16-19 P_INCL 502), then an evidence page; `Include_KS4June.json` and `Include_Post16.json` (#711). + **Session after primary PupilSearch page:** | Field | Value | |---|---| diff --git a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs index 3ee28a9e0..c7490ef70 100644 --- a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs +++ b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs @@ -36,6 +36,14 @@ public interface IPupilRecord /// bool IsIncluded { get; } + /// + /// Whether a school may ask to include this pupil: the INCLUDABLE stamp ingress writes + /// (#711). Not included, or a code the schema's x-ingress.includable rule names — on + /// 16-19, a student in the included file with P_INCL 502. Never a code here: see + /// . + /// + bool IsIncludable { get; } + /// KS4 add-back semantics (see PupilIsAddBackCondition). Null for Post16. int? Pincl { get; } } diff --git a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs index 8e8df2f0d..56087c532 100644 --- a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs +++ b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs @@ -10,7 +10,8 @@ namespace DfE.CheckPerformanceData.Application.CheckYourPupilData; /// /// Ingress stamps an INCLUDED boolean from the file of origin, and that alone decides /// : every student in the included file is included, every one in the -/// non-included file is not, whatever P_INCL says. +/// non-included file is not, whatever P_INCL says. A second stamp, INCLUDABLE, says who may be asked to be +/// included: every non-included student, and included students whose P_INCL is 502 (#711). /// /// Field names differ from KS4: FORENAMES (plural), and identity is ULN — 16-19 /// has no UPN. URN/UKPRN are strings here (the supplier declares them varchar and @@ -33,6 +34,12 @@ public sealed class Post16PupilRecord : IPupilRecord [JsonPropertyName("INCLUDED")] public bool Included { get; init; } + /// Stamped by ingress (#711) from the included file's x-ingress.includable + /// rule (P_INCL 502) and, for the non-included file, from INCLUDED being false. Decides + /// who the Include journey can pick; it never changes . + [JsonPropertyName("INCLUDABLE")] + public bool Includable { get; init; } + [JsonPropertyName("CYPMD_ID")] public string Cypmd_Id { get; init; } = string.Empty; @@ -94,4 +101,7 @@ public sealed class Post16PupilRecord : IPupilRecord [JsonIgnore] public bool IsIncluded => Included; + + [JsonIgnore] + public bool IsIncludable => Includable; } diff --git a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilInclusion.cs b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilInclusion.cs index b97329241..f64acb463 100644 --- a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilInclusion.cs +++ b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilInclusion.cs @@ -5,6 +5,8 @@ namespace DfE.CheckPerformanceData.Application.CheckYourPupilData; /// record: from the slot's file of origin (16-19 included and non-included files), or else from the /// schema's x-ingress.inclusion rule (KS4 and KS2, whose files carry each pupil's own code). /// Readers read only the stamp, so no reader knows any key stage's codes. +/// A second stamp, INCLUDABLE (#711), says who a school may ask to include: ingress writes it +/// from the schema's x-ingress.includable rule, or as not-included where there is no rule. /// public static class PupilInclusion { diff --git a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilRecord.cs b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilRecord.cs index 49280cc31..f2824dcdd 100644 --- a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilRecord.cs +++ b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/PupilRecord.cs @@ -63,6 +63,11 @@ public sealed class PupilRecord : IPupilRecord [JsonPropertyName("INCLUDED")] public bool Included { get; init; } + /// Stamped by ingress (#711): true when a school may ask to include this pupil. On + /// KS4 and KS2 that is "not included". Absent means not includable. + [JsonPropertyName("INCLUDABLE")] + public bool Includable { get; init; } + /// The words schools see for , stamped by ingress from the schema's /// inclusion rule when the supplier file does not carry them. [JsonPropertyName("P_INCL_DESC")] @@ -98,4 +103,7 @@ public sealed class PupilRecord : IPupilRecord [JsonIgnore] public bool IsIncluded => Included; + + [JsonIgnore] + public bool IsIncludable => Includable; } diff --git a/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs b/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs index 6153c63b9..28ae8a2a8 100644 --- a/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs +++ b/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs @@ -1,3 +1,10 @@ namespace DfE.CheckPerformanceData.Application.Journey; -public enum PupilFilter { Included, All, NonIncluded } +/// +/// Which pupils a search returns. and read the +/// INCLUDED stamp. and read the +/// INCLUDABLE stamp (#711): the Include journey searches — on +/// 16-19 that adds the included file's P_INCL 502 students to the non-included file — and its +/// "already included" guards look a name up among . +/// +public enum PupilFilter { Included, All, Includable, NotIncludable } diff --git a/src/DfE.CheckPerformanceData.Persistence/Repositories/CheckYourPupilDataRepository.cs b/src/DfE.CheckPerformanceData.Persistence/Repositories/CheckYourPupilDataRepository.cs index be2177eb5..cb45b9367 100644 --- a/src/DfE.CheckPerformanceData.Persistence/Repositories/CheckYourPupilDataRepository.cs +++ b/src/DfE.CheckPerformanceData.Persistence/Repositories/CheckYourPupilDataRepository.cs @@ -174,7 +174,9 @@ public async Task> SearchPupilsAsync(Guid wind { PupilFilter.All => true, PupilFilter.Included => p.IsIncluded, - _ => !p.IsIncluded + PupilFilter.Includable => p.IsIncludable, + PupilFilter.NotIncludable => !p.IsIncludable, + _ => throw new ArgumentOutOfRangeException(nameof(filter), filter, "No search rule for this pupil filter.") }) .Where(matches); diff --git a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs index 02186d9fd..694c29a95 100644 --- a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs +++ b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs @@ -209,7 +209,7 @@ await analytics.TrackSafeAsync(new ValidationErrorEvent try { nonIncludedSuggestions = await pupilDataService.GetPupilSuggestionsAsync( - windowId, selectedPupilLabel, PupilFilter.NonIncluded); + windowId, selectedPupilLabel, PupilFilter.Includable); } catch (Exception ex) when (ex is not OperationCanceledException) { diff --git a/src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_KS4June.json b/src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_KS4June.json index e93f457d3..3a7375a04 100644 --- a/src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_KS4June.json +++ b/src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_KS4June.json @@ -5,7 +5,7 @@ "id": "select-pupil", "type": "PupilSearch", "title": "What is the name of the pupil to be included?", - "pupilFilter": "NonIncluded", + "pupilFilter": "Includable", "pupilKey": "primary", "nextPageId": "evidence", "validationFailure": "Enter the name of the pupil to be included." diff --git a/src/DfE.CheckPerformanceData.Web/Seeding/SeedExerciseFixtures.cs b/src/DfE.CheckPerformanceData.Web/Seeding/SeedExerciseFixtures.cs index fcf2cfccd..ffb45a1b0 100644 --- a/src/DfE.CheckPerformanceData.Web/Seeding/SeedExerciseFixtures.cs +++ b/src/DfE.CheckPerformanceData.Web/Seeding/SeedExerciseFixtures.cs @@ -91,7 +91,7 @@ internal static async Task IngestAsync(BlobServiceClient blobs, ICheckingExercis // Ours, not the supplier's: ingress generates a pupil's Id, and nothing reads the window id. // INCLUDED and P_INCL_DESC are stamped by ingress from the schema's inclusion rule. - private static readonly HashSet NotSupplierColumns = ["Id", "CheckingWindowId", "INCLUDED", "P_INCL_DESC"]; + private static readonly HashSet NotSupplierColumns = ["Id", "CheckingWindowId", "INCLUDED", "INCLUDABLE", "P_INCL_DESC"]; // One row per record, one column per JSON property, serialised exactly as the read models // bind them. Columns the schema does not declare are dropped by ingress. diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchExclusionTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchExclusionTests.cs index 49e233272..7b4ef9aff 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchExclusionTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchExclusionTests.cs @@ -59,6 +59,7 @@ private async Task ResetAsync() FirstLanguage = "English", Pincl = pincl, Included = pincl == IncludedPincl, + Includable = pincl != IncludedPincl, NewMobile = false, ActualYearGroup = "11", Ethnicity = "A1", @@ -289,7 +290,7 @@ public async Task FilterIncluded_ReturnsOnlyIncludedPupils() } [Fact] - public async Task FilterNonIncluded_ReturnsOnlyNonIncludedPupils() + public async Task FilterIncludable_ReturnsOnlyIncludablePupils() { await ResetAsync(); var windowId = Guid.NewGuid(); @@ -304,11 +305,33 @@ public async Task FilterNonIncluded_ReturnsOnlyNonIncludedPupils() ]); var repo = CreateRepo(blobClient); - var results = await repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Wilson", PupilFilter.NonIncluded); + var results = await repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Wilson", PupilFilter.Includable); Assert.Single(results); } + [Fact] + public async Task FilterNotIncludable_ReturnsOnlyPupilsWhoCannotBeIncluded() + { + await ResetAsync(); + var windowId = Guid.NewGuid(); + + await SeedWindowsAndRequestsAsync([NewWindow(windowId)], []); + + var blobClient = new FakePupilDataBlobClient(); + blobClient.SetPupils(windowId, TestLaestab, + [ + NewPupil(windowId, "Wilson", "A100000000011", pincl: 401), // included, so not includable + NewPupil(windowId, "Wilson", "A100000000012", pincl: 999), // not included, so includable + ]); + + var repo = CreateRepo(blobClient); + var results = await repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Wilson", PupilFilter.NotIncludable); + + var only = Assert.Single(results); + Assert.Contains("A100000000011", only.Label); + } + [Fact] public async Task ExcludeId_ExcludesSpecificPupilFromResults() { diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs new file mode 100644 index 000000000..f70d1b806 --- /dev/null +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs @@ -0,0 +1,129 @@ +using DfE.CheckPerformanceData.Application.CheckYourPupilData; +using DfE.CheckPerformanceData.Application.Journey; +using DfE.CheckPerformanceData.Domain.Enums; +using DfE.CheckPerformanceData.IntegrationTests.Fixtures; +using DfE.CheckPerformanceData.Persistence.Entities; +using DfE.CheckPerformanceData.Persistence.Repositories; +using Microsoft.Extensions.Caching.Memory; +using Npgsql; + +namespace DfE.CheckPerformanceData.IntegrationTests.CheckYourPupilData; + +// #711: the Include journey on 16-19 may pick a student from the non-included file or a student +// the included file carries with P_INCL 502. Ingress stamps INCLUDABLE for both; the repository +// only reads the stamp. Included stays the file of origin, so the 502 student is still included +// for Remove and Merge. +[Collection(nameof(PostgresCollection))] +public sealed class PupilSearchIncludableFilterTests(PostgresFixture fixture) +{ + private const string TestUrn = "136309"; + private const string TestLaestab = "860/4070"; + + private async Task ResetAsync() + { + await using var conn = new NpgsqlConnection(fixture.ConnectionString); + await conn.OpenAsync(); + await using var cmd = conn.CreateCommand(); + cmd.CommandText = @" + TRUNCATE ""ChangeRequests"" CASCADE; + TRUNCATE ""CheckingWindows"" RESTART IDENTITY CASCADE; + "; + await cmd.ExecuteNonQueryAsync(); + } + + private static DateTime Unspecified(DateTime dt) => DateTime.SpecifyKind(dt, DateTimeKind.Unspecified); + + private static CheckingWindow NewWindow(Guid id) => new() + { + Id = id, + Title = "16 to 19 test window", + StartDate = Unspecified(DateTime.UtcNow.AddDays(-7)), + EndDate = Unspecified(DateTime.UtcNow.AddDays(7)), + KeyStage = KeyStages.Post16, + CheckingWindowType = CheckingWindowType.Post16, + }; + + private static Post16PupilRecord NewStudent(string firstname, string surname, string cypmdId, + bool included, bool includable, int? pincl) => new() + { + Id = Guid.NewGuid(), + Included = included, + Includable = includable, + Cypmd_Id = cypmdId, + Surname = surname, + Firstname = firstname, + Sex = "F", + DateOfBirth = "01/09/2007", + Age = 18, + Pincl = pincl, + PinclAims = included ? 503 : null, + Laestab = "8604070", + Urn = TestUrn, + Ukprn = "10001234", + Uln = $"99{cypmdId.PadLeft(8, '0')}", + }; + + private async Task<(Guid WindowId, CheckYourPupilDataRepository Repo)> SeededAsync(params Post16PupilRecord[] students) + { + await ResetAsync(); + var windowId = Guid.NewGuid(); + + await using (var ctx = fixture.CreateContext()) + { + ctx.CheckingWindows.Add(NewWindow(windowId)); + await ctx.SaveChangesAsync(); + } + + var blobClient = new FakePupilDataBlobClient(); + blobClient.SetPupils(windowId, TestLaestab, students); + + return (windowId, new CheckYourPupilDataRepository( + fixture.CreateContext(), blobClient, new MemoryCache(new MemoryCacheOptions()))); + } + + private static (Post16PupilRecord Alice501, Post16PupilRecord Bob502, Post16PupilRecord AliceNonIncluded) Students() => + ( + NewStudent("Alice", "Smith", "500001", included: true, includable: false, pincl: 501), + NewStudent("Bob", "Smith", "500002", included: true, includable: true, pincl: 502), + NewStudent("Alice", "Johnson", "500201", included: false, includable: true, pincl: null) + ); + + [Fact] + public async Task Includable_returns_the_502_student_and_the_non_included_student() + { + var (alice501, bob502, aliceNonIncluded) = Students(); + var (windowId, repo) = await SeededAsync(alice501, bob502, aliceNonIncluded); + + var smiths = await repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Smith", PupilFilter.Includable); + var johnsons = await repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Johnson", PupilFilter.Includable); + var results = smiths.Concat(johnsons).ToList(); + + Assert.Equal(2, results.Count); + Assert.Contains(results, r => r.Id == bob502.Id); + Assert.Contains(results, r => r.Id == aliceNonIncluded.Id); + } + + [Fact] + public async Task NotIncludable_returns_only_the_student_who_is_included_for_good() + { + var (alice501, bob502, aliceNonIncluded) = Students(); + var (windowId, repo) = await SeededAsync(alice501, bob502, aliceNonIncluded); + + var results = await repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Alice", PupilFilter.NotIncludable); + + var only = Assert.Single(results); + Assert.Equal(alice501.Id, only.Id); + } + + [Fact] + public async Task Included_still_counts_the_502_student_as_included() + { + var (alice501, bob502, aliceNonIncluded) = Students(); + var (windowId, repo) = await SeededAsync(alice501, bob502, aliceNonIncluded); + + var results = await repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Smith", PupilFilter.Included); + + Assert.Equal(2, results.Count); + Assert.DoesNotContain(results, r => r.Id == aliceNonIncluded.Id); + } +} diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchTwoPartNameTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchTwoPartNameTests.cs index 0bd6f3d54..eb05f8483 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchTwoPartNameTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchTwoPartNameTests.cs @@ -11,7 +11,7 @@ namespace DfE.CheckPerformanceData.IntegrationTests.CheckYourPupilData; // US3: The Include journey's pupil-search step inherits two-part name matching via // PupilSuggestionFormat.Matches. These integration tests verify SearchPupilsAsync returns -// correct results for two-part queries with both Included and NonIncluded filters. +// correct results for two-part queries with both Included and Includable filters. [Collection(nameof(PostgresCollection))] public sealed class PupilSearchTwoPartNameTests(PostgresFixture fixture) { @@ -56,6 +56,7 @@ private static PupilRecord NewPupil( FirstLanguage = "English", Pincl = included ? 401 : 402, Included = included, + Includable = !included, NewMobile = false, ActualYearGroup = "11", Ethnicity = "A1", @@ -123,7 +124,7 @@ public async Task Two_part_query_matches_both_parts_in_non_included_pupils() NewPupil("Jane", "Smith", "500002", included: false)); var results = await repo.SearchPupilsAsync( - windowId, TestLaestab, TestUrn, "John Smith", PupilFilter.NonIncluded); + windowId, TestLaestab, TestUrn, "John Smith", PupilFilter.Includable); Assert.Single(results); Assert.Contains("John", results[0].Label); diff --git a/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/Post16PupilRecordDeserializationTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/Post16PupilRecordDeserializationTests.cs index fa9e430ea..0ac1a1954 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/Post16PupilRecordDeserializationTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/Post16PupilRecordDeserializationTests.cs @@ -121,4 +121,51 @@ public void Binds_numeric_fields_sent_as_quoted_strings() Assert.Equal(18, pupil.Age); Assert.Equal(502, pupil.Pincl); } + + [Fact] + public void Binds_INCLUDABLE_from_the_stamp() + { + const string json = """ + [ + { + "Id": "11111111-1111-1111-1111-111111111111", + "INCLUDED": true, + "INCLUDABLE": true, + "CYPMD_ID": "500124", + "SURNAME": "Smith", + "FORENAMES": "Bob", + "P_INCL": 502, + "LAESTAB": "860/4070" + } + ] + """; + + var pupil = DeserializeSingle(json); + + Assert.True(pupil.IsIncluded); + Assert.True(pupil.IsIncludable); + } + + [Fact] + public void A_record_with_no_INCLUDABLE_stamp_is_not_includable() + { + const string json = """ + [ + { + "Id": "11111111-1111-1111-1111-111111111111", + "INCLUDED": false, + "CYPMD_ID": "500201", + "SURNAME": "Johnson", + "FORENAMES": "Alice", + "LAESTAB": "860/4070" + } + ] + """; + + var pupil = DeserializeSingle(json); + + // Fail closed: a file ingested before the stamp existed offers nobody for inclusion until + // the exercise is validated again. The service is not live, so there is no fallback. + Assert.False(pupil.IsIncludable); + } } diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs index e6277a36e..5b11b0485 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs @@ -54,7 +54,7 @@ public class IncludeNoResultsDecisionTests Id = "select-pupil", Type = PageType.PupilSearch, Title = "What is the name of the pupil to be included?", - PupilFilter = PupilFilter.NonIncluded, + PupilFilter = PupilFilter.Includable, PupilKey = JourneyPage.PrimaryKey, ValidationFailure = "Enter the name of the pupil to be included." }; @@ -134,7 +134,7 @@ public async Task PupilSearchPost_NeitherListMatch_ReturnsPupilNotFoundView() SetupSession(SessionForInclude()); _pupilDataService.GetPupilSuggestionsAsync(WindowId, "No Such", PupilFilter.Included, Arg.Any(), Arg.Any()) .Returns(new List()); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "No Such", PupilFilter.NonIncluded, Arg.Any(), Arg.Any()) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "No Such", PupilFilter.Includable, Arg.Any(), Arg.Any()) .Returns(new List()); var result = await _sut.PupilSearchPost(WindowId, "select-pupil", null, "No Such"); @@ -159,7 +159,7 @@ public async Task PupilSearchPost_NonIncludedOnlyMatch_ProceedsAsNormal_NoDecisi // included one (included surnames are Smith..Davies). _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.Included, Arg.Any(), Arg.Any()) .Returns(new List()); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.NonIncluded, Arg.Any(), Arg.Any()) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.Includable, Arg.Any(), Arg.Any()) .Returns(new List { NonIncludedSuggestion }); var result = await _sut.PupilSearchPost(WindowId, "select-pupil", null, "Johnson"); @@ -172,7 +172,7 @@ public async Task PupilSearchPost_NonIncludedOnlyMatch_ProceedsAsNormal_NoDecisi Assert.Equal("Johnson", _session.GetRequestState(WindowId).IncludeSearchLabel); // The three-way decision consults the non-included list to discover the valid candidate. await _pupilDataService.Received() - .GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.NonIncluded, Arg.Any(), Arg.Any()); + .GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.Includable, Arg.Any(), Arg.Any()); } // -- T005: non-blank typed text + no selection + included match → "Already included" -- @@ -200,7 +200,7 @@ public async Task PupilSearchPost_IncludedMatch_RedirectsToAlreadyIncluded() Assert.True(_sut.ModelState.IsValid); // An included match is conclusive — there is no need to consult the non-included list. await _pupilDataService.DidNotReceive() - .GetPupilSuggestionsAsync(WindowId, "Smith", PupilFilter.NonIncluded, Arg.Any(), Arg.Any()); + .GetPupilSuggestionsAsync(WindowId, "Smith", PupilFilter.Includable, Arg.Any(), Arg.Any()); } // -- The "Already included" GET surfaces the persisted matches and consumes them on arrival -- diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs index 448cacd94..ecce7f066 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs @@ -725,7 +725,7 @@ private void SetupIncludeJourney() Pages = [new JourneyPage { Id = "select-pupil", Type = PageType.PupilSearch, - PupilFilter = PupilFilter.NonIncluded, PupilKey = JourneyPage.PrimaryKey, + PupilFilter = PupilFilter.Includable, PupilKey = JourneyPage.PrimaryKey, NextPageId = "evidence" }] }; From fb94eec36b5a20d1b53508588a0e7f6c76373f3d Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 14:24:44 +0000 Subject: [PATCH 03/13] #711 Include guards look names up among not-includable pupils A 16-19 student with P_INCL 502 is included by file but includable by rule. The already-included guards now ask NotIncludable, so such a student can never trip the guard against themselves; on KS4 the answer is the same as before. Co-Authored-By: Claude Sonnet 5.5 --- .../Controllers/Journey/JourneyController.cs | 71 ++++++++++--------- .../Journey/IncludeHandoffConflictTests.cs | 10 +-- .../Journey/IncludeNoResultsDecisionTests.cs | 40 ++++++++--- .../Journey/PupilSearchJourneyTests.cs | 10 +-- 4 files changed, 79 insertions(+), 52 deletions(-) diff --git a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs index 694c29a95..da1eeb295 100644 --- a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs +++ b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs @@ -153,11 +153,14 @@ public async Task PupilSearchPost(Guid windowId, string pageId, s // AB#027: Include journey no-results path. When the user typed a name on the Include // primary search but did not select a suggestion, look the typed entry up against BOTH // the included and non-included populations and branch three ways: - // - included match → warn "already included" (abort); - // - non-included-only match → a valid include candidate, so proceed as normal: fall + // - a match among pupils who cannot be included (NotIncludable — included for good; on + // 16-19 the included file's 501 students) → warn "already included" (abort); + // - a match among includable pupils only (#711: not included, or the included file's + // P_INCL 502 students on 16-19) → a valid include candidate, so proceed as normal: fall // through to the ordinary no-selection flow (re-render the search page with the // candidate's suggestions visible) with no warning and no decision page; - // - match on neither list → offer "Pupil not found" (start Add / search again). + // - match on neither → offer "Pupil not found" (start Add where the window type has an + // Add flow / search again). // Blank entry keeps the existing validation, and so does a lookup failure (never a // dead end): the decision text above only ever runs for a deliberately typed name. if (!string.IsNullOrWhiteSpace(selectedPupilLabel) @@ -168,19 +171,19 @@ public async Task PupilSearchPost(Guid windowId, string pageId, s // web server or proxy would log it): it is PII. HttpContext.Session.SaveRequestState(windowId, s => s.IncludeSearchLabel = selectedPupilLabel.Trim()); - IReadOnlyList? includedSuggestions = null; + IReadOnlyList? notIncludableSuggestions = null; try { - includedSuggestions = await pupilDataService.GetPupilSuggestionsAsync( - windowId, selectedPupilLabel, PupilFilter.Included); + notIncludableSuggestions = await pupilDataService.GetPupilSuggestionsAsync( + windowId, selectedPupilLabel, PupilFilter.NotIncludable); } catch (Exception ex) when (ex is not OperationCanceledException) { - logger.LogError(ex, "Included-pupil lookup failed for Include no-results decision; falling back to validation. windowId={WindowId}", windowId); - includedSuggestions = null; + logger.LogError(ex, "Not-includable lookup failed for Include no-results decision; falling back to validation. windowId={WindowId}", windowId); + notIncludableSuggestions = null; } - if (includedSuggestions is null) + if (notIncludableSuggestions is null) { // Lookup failed/timed out → fall through to the existing validation below // (never a redirect to the decision pages, so never a dead end). @@ -195,29 +198,29 @@ await analytics.TrackSafeAsync(new ValidationErrorEvent return View("PupilSearch", viewModelBuilder.BuildPupilSearchVm(windowId, pageId, page, journey, config)); } - if (includedSuggestions.Count > 0) + if (notIncludableSuggestions.Count > 0) { // Carry the matching included pupils across the post/redirect/get round-trip in // session (not the URL, where a web server or proxy would log them): they are PII. HttpContext.Session.SaveRequestState(windowId, s => - s.IncludeMatchedPupils = includedSuggestions.ToList()); + s.IncludeMatchedPupils = notIncludableSuggestions.ToList()); return RedirectToAction(nameof(AlreadyIncluded), new { windowId, pageId }); } - IReadOnlyList? nonIncludedSuggestions = null; + IReadOnlyList? includableSuggestions = null; try { - nonIncludedSuggestions = await pupilDataService.GetPupilSuggestionsAsync( + includableSuggestions = await pupilDataService.GetPupilSuggestionsAsync( windowId, selectedPupilLabel, PupilFilter.Includable); } catch (Exception ex) when (ex is not OperationCanceledException) { - logger.LogError(ex, "Non-included-pupil lookup failed for Include no-results decision; falling back to validation. windowId={WindowId}", windowId); - nonIncludedSuggestions = null; + logger.LogError(ex, "Includable lookup failed for Include no-results decision; falling back to validation. windowId={WindowId}", windowId); + includableSuggestions = null; } - if (nonIncludedSuggestions is null) + if (includableSuggestions is null) { // Lookup failed/timed out → fall through to the existing validation below // (never a redirect to the decision pages, so never a dead end). @@ -232,7 +235,7 @@ await analytics.TrackSafeAsync(new ValidationErrorEvent return View("PupilSearch", viewModelBuilder.BuildPupilSearchVm(windowId, pageId, page, journey, config)); } - if (nonIncludedSuggestions.Count == 0) + if (includableSuggestions.Count == 0) return RedirectToAction(nameof(PupilNotFound), new { windowId, pageId }); // A non-included-only match is a valid include candidate: proceed as normal. Fall @@ -274,29 +277,31 @@ await analytics.TrackSafeAsync(new ValidationErrorEvent // check the selected name against the included list too — a different pupil with the same // name may already be on the included list, and the select-from-dropdown path would // otherwise bypass the included-list guard the typed-entry path (lines 160-239) enforces. - // The search page's autocomplete filters to NonIncluded, so the *selected* record itself is - // non-included; the guard therefore looks up the typed label, exactly as the typed path does. + // The search page's autocomplete filters to Includable, so the *selected* record itself is + // includable and can never match this NotIncludable lookup (#711: a 16-19 P_INCL 502 student + // is INCLUDED by file but includable by rule, so an Included lookup would find them and block + // their own request); the guard looks up the typed label, exactly as the typed path does. if (journey.SelectedWhatToChange == WhatToChange.Include && page.PupilKey == JourneyPage.PrimaryKey) { - IReadOnlyList? includedSuggestions = null; + IReadOnlyList? notIncludableSuggestions = null; try { - includedSuggestions = await pupilDataService.GetPupilSuggestionsAsync( - windowId, $"{pupil.Firstname} {pupil.Surname}".Trim(), PupilFilter.Included); + notIncludableSuggestions = await pupilDataService.GetPupilSuggestionsAsync( + windowId, $"{pupil.Firstname} {pupil.Surname}".Trim(), PupilFilter.NotIncludable); } catch (Exception ex) when (ex is not OperationCanceledException) { - logger.LogError(ex, "Included-pupil lookup failed for Include autocomplete guard; proceeding as normal. windowId={WindowId}", windowId); - includedSuggestions = null; + logger.LogError(ex, "Not-includable lookup failed for Include autocomplete guard; proceeding as normal. windowId={WindowId}", windowId); + notIncludableSuggestions = null; } - if (includedSuggestions is not null && includedSuggestions.Count > 0) + if (notIncludableSuggestions is not null && notIncludableSuggestions.Count > 0) { var displayLabel = selectedPupilLabel ?? $"{pupil.Surname}, {pupil.Firstname}, {pupil.DateOfBirth}"; HttpContext.Session.SaveRequestState(windowId, s => { s.IncludeSearchLabel = displayLabel; - s.IncludeMatchedPupils = includedSuggestions.ToList(); + s.IncludeMatchedPupils = notIncludableSuggestions.ToList(); }); return RedirectToAction(nameof(AlreadyIncluded), new { windowId, pageId }); } @@ -1796,25 +1801,25 @@ await analytics.TrackSafeAsync(new DuplicateCheckDecisionEvent // Include flow's own select-pupil search page. The duplicate check may surface a non-included // pupil whose name matches a pupil already on the included list, and switching to include must // not silently bypass the warning the search page enforces. - IReadOnlyList? includedSuggestions = null; + IReadOnlyList? notIncludableSuggestions = null; try { - includedSuggestions = await pupilDataService.GetPupilSuggestionsAsync( - windowId, $"{pupil.Firstname} {pupil.Surname}".Trim(), PupilFilter.Included); + notIncludableSuggestions = await pupilDataService.GetPupilSuggestionsAsync( + windowId, $"{pupil.Firstname} {pupil.Surname}".Trim(), PupilFilter.NotIncludable); } catch (Exception ex) when (ex is not OperationCanceledException) { - logger.LogError(ex, "Included-pupil lookup failed for Include hand-off guard; proceeding as normal. windowId={WindowId}", windowId); - includedSuggestions = null; + logger.LogError(ex, "Not-includable lookup failed for Include hand-off guard; proceeding as normal. windowId={WindowId}", windowId); + notIncludableSuggestions = null; } - if (includedSuggestions is not null && includedSuggestions.Count > 0) + if (notIncludableSuggestions is not null && notIncludableSuggestions.Count > 0) { // The duplicate-check page already surfaces each match's Included/Not-included status, so a // same-name included pupil that was already on screen is not a bypass — only block when the // included namesake is new information (e.g. a different-DOB pupil the check did not surface). var alreadySurfaced = journey.DuplicateCheck?.Matches.Select(m => m.Id).ToHashSet() ?? []; - var hiddenNamesakes = includedSuggestions.Where(s => !alreadySurfaced.Contains(s.Id)).ToList(); + var hiddenNamesakes = notIncludableSuggestions.Where(s => !alreadySurfaced.Contains(s.Id)).ToList(); if (hiddenNamesakes.Count > 0) { var displayLabel = $"{pupil.Surname}, {pupil.Firstname}, {pupil.DateOfBirth}"; diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeHandoffConflictTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeHandoffConflictTests.cs index 2e17f5fcc..e5afb9529 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeHandoffConflictTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeHandoffConflictTests.cs @@ -58,7 +58,7 @@ public class IncludeHandoffConflictTests Id = "select-pupil", Type = PageType.PupilSearch, Title = "Which pupil do you want to include?", - PupilFilter = PupilFilter.Included, + PupilFilter = PupilFilter.Includable, PupilKey = JourneyPage.PrimaryKey, NextPageId = "evidence" }; @@ -327,7 +327,7 @@ public async Task IncludeThisPupil_WhenSameNameAlreadyIncluded_RedirectsToAlread SetupSession(AddSessionAwaitingInclude(SingleNonIncludedCheck)); _flowService.GetConfigAsync(WhatToChange.Include, CheckingWindowType.KS4June).Returns(IncludeConfig); _pupilDataService.GetPupilAsync(WindowId, MatchPupilId).Returns(MatchPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.NotIncludable) .Returns([new PupilSuggestionDto(IncludedPupilId, "Doe, John", "John", "Doe", "02/02/2010")]); var result = await _sut.IncludeThisPupil(WindowId); @@ -357,7 +357,7 @@ public async Task SwitchToInclude_WhenSameNameAlreadyIncluded_RedirectsToAlready SetupSession(AddSessionAwaitingInclude(MultipleCheck)); _flowService.GetConfigAsync(WhatToChange.Include, CheckingWindowType.KS4June).Returns(IncludeConfig); _pupilDataService.GetPupilAsync(WindowId, MatchPupilId).Returns(MatchPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.NotIncludable) .Returns([new PupilSuggestionDto(IncludedPupilId, "Doe, John", "John", "Doe", "02/02/2010")]); var result = await _sut.SwitchToInclude(WindowId, MatchPupilId); @@ -385,7 +385,7 @@ public async Task SwitchToInclude_WhenIncludedNamesakeAlreadyOnScreen_ProceedsTo SetupSession(AddSessionAwaitingInclude(MultipleSameNameCheck)); _flowService.GetConfigAsync(WhatToChange.Include, CheckingWindowType.KS4June).Returns(IncludeConfig); _pupilDataService.GetPupilAsync(WindowId, MatchPupilId).Returns(MatchPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.NotIncludable) .Returns([new PupilSuggestionDto(IncludedNamesakeId, "Doe, John", "John", "Doe", "02/02/2010")]); var result = await _sut.SwitchToInclude(WindowId, MatchPupilId); @@ -412,7 +412,7 @@ public async Task SwitchToInclude_WhenNoAlreadyIncludedNameMatch_ProceedsToSeed( SetupSession(AddSessionAwaitingInclude(MultipleCheck)); _flowService.GetConfigAsync(WhatToChange.Include, CheckingWindowType.KS4June).Returns(IncludeConfig); _pupilDataService.GetPupilAsync(WindowId, MatchPupilId).Returns(MatchPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "John Doe", PupilFilter.NotIncludable) .Returns([]); var result = await _sut.SwitchToInclude(WindowId, MatchPupilId); diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs index 5b11b0485..9816a0981 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs @@ -22,8 +22,8 @@ namespace DfE.CheckPerformanceData.Application.UnitTests.Journey; /// /// AB#027: Include journey no-results decision flow. When a user on the Include journey's /// select-pupil step types a name, gets no autocomplete result, and clicks Continue without -/// selecting a pupil, the system looks the typed entry up against BOTH the included and -/// non-included populations: +/// selecting a pupil, the system looks the typed entry up among the pupils who cannot be included +/// (`NotIncludable`) and those who can (`Includable`): /// - included match warns "already included" (abort); /// - non-included-only match is a valid include candidate — the journey proceeds as normal /// (re-renders the search page with the candidate visible, no warning, no decision page); @@ -132,7 +132,7 @@ public IncludeNoResultsDecisionTests() public async Task PupilSearchPost_NeitherListMatch_ReturnsPupilNotFoundView() { SetupSession(SessionForInclude()); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "No Such", PupilFilter.Included, Arg.Any(), Arg.Any()) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "No Such", PupilFilter.NotIncludable, Arg.Any(), Arg.Any()) .Returns(new List()); _pupilDataService.GetPupilSuggestionsAsync(WindowId, "No Such", PupilFilter.Includable, Arg.Any(), Arg.Any()) .Returns(new List()); @@ -152,12 +152,12 @@ public async Task PupilSearchPost_NeitherListMatch_ReturnsPupilNotFoundView() // → a valid include candidate: proceed as normal (no decision page, no "not found" warning) -- [Fact] - public async Task PupilSearchPost_NonIncludedOnlyMatch_ProceedsAsNormal_NoDecisionRedirect() + public async Task PupilSearchPost_IncludableOnlyMatch_ProceedsAsNormal_NoDecisionRedirect() { SetupSession(SessionForInclude()); // "Johnson" is a non-included surname bucket — it matches the non-included pupil but no // included one (included surnames are Smith..Davies). - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.Included, Arg.Any(), Arg.Any()) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.NotIncludable, Arg.Any(), Arg.Any()) .Returns(new List()); _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.Includable, Arg.Any(), Arg.Any()) .Returns(new List { NonIncludedSuggestion }); @@ -175,6 +175,28 @@ await _pupilDataService.Received() .GetPupilSuggestionsAsync(WindowId, "Johnson", PupilFilter.Includable, Arg.Any(), Arg.Any()); } + // #711: on 16-19 a P_INCL 502 student is INCLUDED by file but includable by rule. The guards + // ask NotIncludable, so a typed name that matches only includable students is a candidate. + [Fact] + public async Task PupilSearchPost_IncludableMatchOnly_ProceedsAsNormal_EvenWhenThatPupilIsIncluded() + { + SetupSession(SessionForInclude()); + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Bob Smith", PupilFilter.NotIncludable, Arg.Any(), Arg.Any()) + .Returns(new List()); + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Bob Smith", PupilFilter.Includable, Arg.Any(), Arg.Any()) + .Returns(new List + { + new(Guid.NewGuid(), "Bob, Smith, (CYPMD ID:500002, ULN:9900000002, DOB:01/09/2007, INCLUDED)", "Bob", "Smith", "01/09/2007") + }); + + var result = await _sut.PupilSearchPost(WindowId, "select-pupil", null, "Bob Smith"); + + var view = Assert.IsType(result); + Assert.Equal("PupilSearch", view.ViewName); + await _pupilDataService.DidNotReceive() + .GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.Included, Arg.Any(), Arg.Any()); + } + // -- T005: non-blank typed text + no selection + included match → "Already included" -- [Fact] @@ -182,7 +204,7 @@ public async Task PupilSearchPost_IncludedMatch_RedirectsToAlreadyIncluded() { SetupSession(SessionForInclude()); // "Smith" is an included surname bucket — it matches the included pupil Alice Smith. - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Smith", PupilFilter.Included, Arg.Any(), Arg.Any()) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Smith", PupilFilter.NotIncludable, Arg.Any(), Arg.Any()) .Returns(new List { IncludedSuggestion }); var result = await _sut.PupilSearchPost(WindowId, "select-pupil", null, "Smith"); @@ -261,7 +283,7 @@ public async Task PupilSearchPost_BlankLabel_ReturnsExistingValidationError() var error = _sut.ModelState["selectedPupilId"]?.Errors.FirstOrDefault()?.ErrorMessage; Assert.Equal("Enter the name of the pupil to be included.", error); await _pupilDataService.DidNotReceive() - .GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.Included, Arg.Any(), Arg.Any()); + .GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.NotIncludable, Arg.Any(), Arg.Any()); } // -- T007: non-blank typed text + no selection on non-Include journey → unchanged -- @@ -278,7 +300,7 @@ public async Task PupilSearchPost_NonIncludeJourney_ReturnsExistingValidationErr Assert.Equal("PupilSearch", view.ViewName); Assert.True(_sut.ModelState.ContainsKey("selectedPupilId")); await _pupilDataService.DidNotReceive() - .GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.Included, Arg.Any(), Arg.Any()); + .GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.NotIncludable, Arg.Any(), Arg.Any()); } // -- T008: lookup failure → falls back to existing validation (fail safe) -- @@ -287,7 +309,7 @@ await _pupilDataService.DidNotReceive() public async Task PupilSearchPost_LookupFailure_FallsBackToExistingValidation() { SetupSession(SessionForInclude()); - _pupilDataService.GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.Included, Arg.Any(), Arg.Any()) + _pupilDataService.GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.NotIncludable, Arg.Any(), Arg.Any()) .Returns(Task.FromException>(new InvalidOperationException("blob unavailable"))); var result = await _sut.PupilSearchPost(WindowId, "select-pupil", null, "No Such"); diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs index ecce7f066..3fdb81db8 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs @@ -607,7 +607,7 @@ public async Task PupilSearchPost_WhenIncludeJourneyAndSelectedNameMatchesInclud }; SetupIncludeJourney(); _pupilDataService.GetPupilAsync(WindowId, PrimaryPupilId).Returns(nonIncludedPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.NotIncludable) .Returns([new PupilSuggestionDto(Guid.NewGuid(), "Carter, Casey, 01/01/2010", "Casey", "Carter", "01/01/2010")]); var result = await _sut.PupilSearchPost(WindowId, "select-pupil", PrimaryPupilId.ToString(), "Carter, Casey, 01/01/2010"); @@ -627,7 +627,7 @@ public async Task PupilSearchPost_WhenIncludeJourneyAndSelectedNameMatchesInclud }; SetupIncludeJourney(); _pupilDataService.GetPupilAsync(WindowId, PrimaryPupilId).Returns(nonIncludedPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.NotIncludable) .Returns([new PupilSuggestionDto(Guid.NewGuid(), "Carter, Casey, 01/01/2010", "Casey", "Carter", "01/01/2010")]); await _sut.PupilSearchPost(WindowId, "select-pupil", PrimaryPupilId.ToString(), "Carter, Casey, 01/01/2010"); @@ -647,7 +647,7 @@ public async Task PupilSearchPost_WhenIncludeJourneyAndSelectedNameMatchesInclud var includedSuggestion = new PupilSuggestionDto(Guid.NewGuid(), "Carter, Casey, 01/01/2010", "Casey", "Carter", "01/01/2010"); SetupIncludeJourney(); _pupilDataService.GetPupilAsync(WindowId, PrimaryPupilId).Returns(nonIncludedPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.NotIncludable) .Returns([includedSuggestion]); await _sut.PupilSearchPost(WindowId, "select-pupil", PrimaryPupilId.ToString(), "Carter, Casey, 01/01/2010"); @@ -688,7 +688,7 @@ public async Task PupilSearchPost_WhenIncludeJourneyAndNoIncludedNameMatch_Proce }; SetupIncludeJourney(); _pupilDataService.GetPupilAsync(WindowId, PrimaryPupilId).Returns(nonIncludedPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Jane Smith", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Jane Smith", PupilFilter.NotIncludable) .Returns([]); var result = await _sut.PupilSearchPost(WindowId, "select-pupil", PrimaryPupilId.ToString(), "Smith, Jane, 01/01/2010"); @@ -708,7 +708,7 @@ public async Task PupilSearchPost_WhenIncludeJourneyAndIncludedLookupFails_Proce }; SetupIncludeJourney(); _pupilDataService.GetPupilAsync(WindowId, PrimaryPupilId).Returns(nonIncludedPupil); - _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.Included) + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Casey Carter", PupilFilter.NotIncludable) .Returns(Task.FromException>(new TimeoutException())); var result = await _sut.PupilSearchPost(WindowId, "select-pupil", PrimaryPupilId.ToString(), "Carter, Casey, 01/01/2010"); From 62c9ebfda83a3b43414a2574711088c1f77a7af0 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 14:27:16 +0000 Subject: [PATCH 04/13] #711 Include decision pages use the learner noun and offer Add only where it exists 16-19 has no Add flow, so Start adding would only bounce to Check your data. The pages take CanAdd from the same list What to change renders. Co-Authored-By: Claude Sonnet 5.5 --- .../IncludeAlreadyIncludedViewModel.cs | 8 +++ .../Journey/IncludeNoResultsViewModel.cs | 9 +++ .../Controllers/Journey/JourneyController.cs | 16 ++++- .../Views/Journey/AlreadyIncluded.cshtml | 29 ++++---- .../Views/Journey/PupilNotFound.cshtml | 29 +++++--- .../Journey/IncludeNoResultsDecisionTests.cs | 30 +++++++++ .../Views/IncludeDecisionViewSourceTests.cs | 66 +++++++++++++++++++ 7 files changed, 162 insertions(+), 25 deletions(-) create mode 100644 tests/DfE.CheckPerformanceData.UnitTests/Web/Views/IncludeDecisionViewSourceTests.cs diff --git a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeAlreadyIncludedViewModel.cs b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeAlreadyIncludedViewModel.cs index 4303ec50c..e68bc777a 100644 --- a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeAlreadyIncludedViewModel.cs +++ b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeAlreadyIncludedViewModel.cs @@ -1,6 +1,7 @@ namespace DfE.CheckPerformanceData.Web.Controllers.Journey; using DfE.CheckPerformanceData.Application.CheckYourPupilData; +using DfE.CheckPerformanceData.Application.WindowManagement; /// /// View model for the Include journey's "already included" warning page. Shown when the typed @@ -27,4 +28,11 @@ public sealed class IncludeAlreadyIncludedViewModel /// The pupils found on the included list matching the typed entry. Display only — /// never logged or placed in analytics. Empty when no match details were carried. public IReadOnlyList Matches { get; set; } = []; + + /// The window's word for a learner: "student" on 16-19, "pupil" elsewhere. + public LearnerNoun LearnerNoun { get; set; } = LearnerNoun.Pupil; + + /// True when the window type has an Add flow, so "Start adding" leads somewhere. + /// 16-19 has none (#711). + public bool CanAdd { get; set; } } \ No newline at end of file diff --git a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeNoResultsViewModel.cs b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeNoResultsViewModel.cs index e7ba98769..8bc2369d8 100644 --- a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeNoResultsViewModel.cs +++ b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/IncludeNoResultsViewModel.cs @@ -1,3 +1,5 @@ +using DfE.CheckPerformanceData.Application.WindowManagement; + namespace DfE.CheckPerformanceData.Web.Controllers.Journey; /// @@ -15,4 +17,11 @@ public sealed class IncludeNoResultsViewModel /// The name the user typed. Display only — never logged or placed in analytics. public string? TypedPupilLabel { get; set; } + + /// The window's word for a learner: "student" on 16-19, "pupil" elsewhere. + public LearnerNoun LearnerNoun { get; set; } = LearnerNoun.Pupil; + + /// True when the window type has an Add flow, so "Start adding" leads somewhere. + /// 16-19 has none (#711). + public bool CanAdd { get; set; } } \ No newline at end of file diff --git a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs index da1eeb295..7575c290d 100644 --- a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs +++ b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs @@ -1925,6 +1925,14 @@ await analytics.TrackSafeAsync(new ValidationErrorEvent // ── Include no-results decision flow (AB#027) ──────────────────────────── + /// + /// #711: the decision pages offer "Start adding" only when What to change would offer Add — + /// the same list, so the two can never disagree. 16-19 has no Add flow. + /// + private async Task CanAddAsync(RequestState journey) => + journey.CheckingWindow?.CheckingWindowType is { } windowType + && (await flowService.GetPupilDataChangesAsync(windowType)).Contains(WhatToChange.Add); + /// /// AB#027 (US1): shown when the Include journey's select-pupil search found no included match /// for a freely typed (not-selected) name. Offers "Start adding this pupil" (the Add journey) @@ -1947,7 +1955,9 @@ public async Task PupilNotFound(Guid windowId, string pageId) { WindowId = windowId, PageId = pageId, - TypedPupilLabel = label + TypedPupilLabel = label, + LearnerNoun = journey.LearnerNoun, + CanAdd = await CanAddAsync(journey) }); } @@ -1979,7 +1989,9 @@ public async Task AlreadyIncluded(Guid windowId, string? pageId = TypedPupilLabel = label, Matches = matches, BackPageId = pageId, - BackPageAction = backAction ?? nameof(PupilSearchPage) + BackPageAction = backAction ?? nameof(PupilSearchPage), + LearnerNoun = journey.LearnerNoun, + CanAdd = await CanAddAsync(journey) }); } diff --git a/src/DfE.CheckPerformanceData.Web/Views/Journey/AlreadyIncluded.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Journey/AlreadyIncluded.cshtml index a2ebc994f..0b40f9593 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Journey/AlreadyIncluded.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Journey/AlreadyIncluded.cshtml @@ -1,7 +1,7 @@ @model DfE.CheckPerformanceData.Web.Controllers.Journey.IncludeAlreadyIncludedViewModel @{ - ViewBag.Title = "This pupil is already included"; + ViewBag.Title = $"This {Model.LearnerNoun.Singular} is already included"; Layout = "_Layout"; } @@ -15,22 +15,22 @@ QuestionModels = [] }) -

This pupil is already included

+

This @Model.LearnerNoun.Singular is already included

- There may already be a pupil matching "@Model.TypedPupilLabel" on the included list. + There may already be a @Model.LearnerNoun.Singular matching "@Model.TypedPupilLabel" on the included list.

- No non-included pupils were found. The pupil may already be the subject of an active request on the included list. You do not need to include them again. + No @Model.LearnerNoun.Plural who can be included were found. The @Model.LearnerNoun.Singular may already be the subject of an active request on the included list. You do not need to include them again.

@if (Model.Matches.Count > 0) { -

Matching pupils on the included list

+

Matching @Model.LearnerNoun.Plural on the included list

- + @@ -53,14 +53,17 @@
Pupils matching "@Model.TypedPupilLabel"@Model.LearnerNoun.PluralCapitalised matching "@Model.TypedPupilLabel"
Name
} -

- If the pupil should still be included, you can add them as a new pupil instead. -

+@if (Model.CanAdd) +{ +

+ If the @Model.LearnerNoun.Singular should still be included, you can add them as a new @Model.LearnerNoun.Singular instead. +

-
- @Html.AntiForgeryToken() - Start adding this pupil -
+
+ @Html.AntiForgeryToken() + Start adding this @Model.LearnerNoun.Singular +
+}
@Html.AntiForgeryToken() diff --git a/src/DfE.CheckPerformanceData.Web/Views/Journey/PupilNotFound.cshtml b/src/DfE.CheckPerformanceData.Web/Views/Journey/PupilNotFound.cshtml index 67d19aef7..2d7bf8b7a 100644 --- a/src/DfE.CheckPerformanceData.Web/Views/Journey/PupilNotFound.cshtml +++ b/src/DfE.CheckPerformanceData.Web/Views/Journey/PupilNotFound.cshtml @@ -1,24 +1,33 @@ @model DfE.CheckPerformanceData.Web.Controllers.Journey.IncludeNoResultsViewModel @{ - ViewBag.Title = "Pupil not found"; + ViewBag.Title = $"{Model.LearnerNoun.SingularCapitalised} not found"; Layout = "_Layout"; } -

Pupil not found

+

@Model.LearnerNoun.SingularCapitalised not found

- No pupil matching "@Model.TypedPupilLabel" was found. This means the pupil is not currently + No @Model.LearnerNoun.Singular matching "@Model.TypedPupilLabel" was found. This means the @Model.LearnerNoun.Singular is not currently included and is not known to this school's records.

-

- You can add them as a new pupil or search again with a different name. -

+@if (Model.CanAdd) +{ +

+ You can add them as a new @Model.LearnerNoun.Singular or search again with a different name. +

- - @Html.AntiForgeryToken() - Start adding this pupil -
+
+ @Html.AntiForgeryToken() + Start adding this @Model.LearnerNoun.Singular +
+} +else +{ +

+ You can search again with a different name. +

+}
@Html.AntiForgeryToken() diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs index 9816a0981..2541f6184 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs @@ -250,6 +250,36 @@ public async Task AlreadyIncluded_Get_ShowsTypedLabelAndMatches_ThenClearsSessio Assert.Null(_session.GetRequestState(WindowId).IncludeMatchedPupils); } + // #711: 16-19 has no Add flow, so the decision pages must not offer "Start adding". CanAdd + // comes from the same list What to change renders. + [Fact] + public async Task AlreadyIncluded_OnAWindowTypeWithNoAddFlow_DoesNotOfferAdd() + { + SetupSession(SessionForIncludeWith(label: "Bob Smith")); + _flowService.GetPupilDataChangesAsync(Arg.Any()) + .Returns(Task.FromResult>([WhatToChange.Merge, WhatToChange.Remove])); + + var result = await _sut.AlreadyIncluded(WindowId); + + var model = Assert.IsType(Assert.IsType(result).Model); + Assert.False(model.CanAdd); + Assert.Equal("pupil", model.LearnerNoun.Singular); // the session's window is KS4 June + } + + [Fact] + public async Task PupilNotFound_OnAWindowTypeWithAnAddFlow_OffersAdd() + { + SetupSession(SessionForIncludeWith(label: "No Such")); + _flowService.GetPupilDataChangesAsync(Arg.Any()) + .Returns(Task.FromResult>([WhatToChange.Merge, WhatToChange.Include, WhatToChange.Remove, WhatToChange.Add])); + + var result = await _sut.PupilNotFound(WindowId, "select-pupil"); + + var model = Assert.IsType(Assert.IsType(result).Model); + Assert.True(model.CanAdd); + Assert.Equal("pupil", model.LearnerNoun.Singular); + } + // -- AB#297780: when reached from the duplicate-check hand-off, the back link returns to // the match list rather than the Include search page -- diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Web/Views/IncludeDecisionViewSourceTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Web/Views/IncludeDecisionViewSourceTests.cs new file mode 100644 index 000000000..b04c4dc2d --- /dev/null +++ b/tests/DfE.CheckPerformanceData.UnitTests/Web/Views/IncludeDecisionViewSourceTests.cs @@ -0,0 +1,66 @@ +namespace DfE.CheckPerformanceData.Application.UnitTests.Web.Views; + +// #711: the Include journey's two decision pages are shared between key stages. They must take +// the noun from the model ("student" on 16-19) and offer "Start adding" only when the window type +// has an Add flow — 16-19 has none, so the button would only bounce to Check your data. +public sealed class IncludeDecisionViewSourceTests +{ + private static string ReadViewSource(string relativePath) + { + var viewsDir = Path.GetFullPath(Path.Combine( + AppContext.BaseDirectory, + "..", "..", "..", "..", "..", + "src", "DfE.CheckPerformanceData.Web")); + return File.ReadAllText(Path.Combine(viewsDir, relativePath)); + } + + [Theory] + [InlineData("Views/Journey/PupilNotFound.cshtml")] + [InlineData("Views/Journey/AlreadyIncluded.cshtml")] + public void The_page_wraps_the_add_form_in_CanAdd(string view) + { + var source = ReadViewSource(view); + + Assert.Contains("@if (Model.CanAdd)", source); + Assert.Contains("StartAddingPupil", source); + } + + [Theory] + [InlineData("Views/Journey/PupilNotFound.cshtml")] + [InlineData("Views/Journey/AlreadyIncluded.cshtml")] + public void The_page_takes_its_noun_from_the_model_and_never_says_pupil(string view) + { + var source = ReadViewSource(view); + + Assert.Contains("Model.LearnerNoun.", source); + // The rendered sentences. Code may still say "pupil" (the StartAddingPupil action, the + // "select-pupil" page id, the model type), so the check names the old prose, not the word. + foreach (var oldProse in new[] + { + "Pupil not found", "No pupil", "the pupil", "this pupil", "This pupil", "new pupil", + "Matching pupils", "Pupils matching", "non-included pupils", "a pupil matching" + }) + { + Assert.DoesNotContain(oldProse, source); + } + } + + [Theory] + [InlineData("Views/Journey/PupilNotFound.cshtml")] + [InlineData("Views/Journey/AlreadyIncluded.cshtml")] + public void The_title_and_the_heading_are_the_same_words(string view) + { + var source = ReadViewSource(view); + + // docs/accessibility.md: ViewData["Title"] carries the same words as the one

. The + // title is a C# interpolated string and the heading is Razor, so the noun appears as + // {Model.LearnerNoun.X} in one and @Model.LearnerNoun.X in the other. + var title = System.Text.RegularExpressions.Regex.Match(source, @"ViewBag\.Title\s*=\s*(\$?"".*?"");").Groups[1].Value + .Trim('$', '"') + .Replace("{Model.LearnerNoun.SingularCapitalised}", "@Model.LearnerNoun.SingularCapitalised") + .Replace("{Model.LearnerNoun.Singular}", "@Model.LearnerNoun.Singular"); + var heading = System.Text.RegularExpressions.Regex.Match(source, @"]*>(.*?)

").Groups[1].Value.Trim(); + Assert.False(string.IsNullOrEmpty(title)); + Assert.Equal(title, heading); + } +} From 727137d2ae58036083cf2f28580c851f3b775bcf Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 14:27:41 +0000 Subject: [PATCH 05/13] #711 Add the 16-19 Include journey flow file Include_Post16.json: pick an includable student, give or skip evidence, review, submit. What to change now offers Include on a 16-19 window. Rules: Scrutiny only. Co-Authored-By: Claude Sonnet 5.5 --- docs/domain-model.md | 2 +- docs/ks2-journeys.md | 2 +- ...hnical-stakeholders-functional-overview.md | 2 +- .../Data/QuestionFlows/Include_Post16.json | 38 +++++++++++++++++++ 4 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_Post16.json diff --git a/docs/domain-model.md b/docs/domain-model.md index 9471487ae..29b3221b5 100644 --- a/docs/domain-model.md +++ b/docs/domain-model.md @@ -176,7 +176,7 @@ Session readiness for the journey requires only `SelectedWhatToChange` and `Chec - Multi-step question flow for submitting a pupil data change request. - Session state (`RequestState`) tracks answers, page history, selected pupil(s), and reference number. - Pupil selection is an in-journey `PupilSearch` page (the first page of every flow config). `WhatToChangeController` redirects to `Journey.Page(firstPageId)` after saving the window and `WhatToChange` to session. The journey's `IsSessionReady` check requires only those two fields. -- **What to change offers a change only when its flow file exists** (`IQuestionFlowService.GetPupilDataChangesAsync`: Merge, Include, Remove, Add, in that order, each only when `{WhatToChange}_{CheckingWindowType}.json` exists). The view renders `WhatToChangeViewModel.AvailableChanges` and names no change or window type; the POST refuses any change outside the same list. To give a window type a journey, add its flow file. There are no window-type lists. +- **What to change offers a change only when its flow file exists** (`IQuestionFlowService.GetPupilDataChangesAsync`: Merge, Include, Remove, Add, in that order, each only when `{WhatToChange}_{CheckingWindowType}.json` exists). The view renders `WhatToChangeViewModel.AvailableChanges` and names no change or window type; the POST refuses any change outside the same list. To give a window type a journey, add its flow file. There are no window-type lists. 16-19 offers Include since #711: `Include_Post16.json`, whose search uses `pupilFilter: "Includable"` (the `INCLUDABLE` stamp — the non-included file plus the included file's P_INCL 502 students). Every 16-19 include goes to Scrutiny: the `Inclusion` rules name KS4 codes only. - On submission, `IRequestService.ConfirmRequestAsync` builds the `RequestDocument` (including an optional `MatchedPupil` for Merge) and saves it to blob storage (container named by `windowId`, blob `request_{referenceNumber}.json`). The close sweep sends this saved copy. - The Summary page shows "First record to merge" / "Second record to merge" rows (with Change links to the respective `PupilSearchPage`) when `MatchedPupil` is set; otherwise shows the standard "Pupil name" row. diff --git a/docs/ks2-journeys.md b/docs/ks2-journeys.md index 741dd53af..00036e7b5 100644 --- a/docs/ks2-journeys.md +++ b/docs/ks2-journeys.md @@ -28,7 +28,7 @@ KS2 has no Merge, Include or Add journey. The requirements say so. | Window | Flow files | Options shown | |---|---|---| | KS4 June | Add, Include, Merge, Remove | All four (no change) | - | Post16 | Merge, Remove | Merge, Remove (no change) | + | Post16 | Include, Merge, Remove | Include (since #711), Merge, Remove | | KS4 Autumn | Add | Add only. Before, it also showed Merge, Include and Remove, which had no flow file and went nowhere. | | KS2 | Remove | Remove only | diff --git a/docs/technical-stakeholders-functional-overview.md b/docs/technical-stakeholders-functional-overview.md index f64c5985f..6ece09496 100644 --- a/docs/technical-stakeholders-functional-overview.md +++ b/docs/technical-stakeholders-functional-overview.md @@ -78,7 +78,7 @@ The journey is data-driven from JSON flow configs: - **Types** (`Application/Journey/`): `QuestionFlowConfig` → `JourneyPage` (`PageType`: `Question`, `Content`, `EvidenceUpload`, `PupilSearch`) → `Question` (`Radio`, `FreeText`, `Date`, `FileUpload`, `TextArea`, `Autocomplete`; supports `Optional`, `CharacterLimit`, named `Validator`, per-option `NextPageId` branching, `VisibleWhen` conditions). - **Config source**: the files that ship in the release image at `Web/Data/QuestionFlows/{WhatToChange}_{CheckingWindowType}.json`, read via `IQuestionFlowConfigSource` / `FileSystemQuestionFlowClient` and cached in `IMemoryCache` with `NeverRemove` priority (flow edits need a redeploy). There is **no blob container and no seeding step** — a flow reaches an environment by deploying the commit that contains it. See `docs/question-flow-deployment.md`. -- **Flows that exist today**: `Include_KS4June.json`, `Merge_KS4June.json`, `Remove_KS4June.json` (12-reason branch tree). **Only KS4 June has flows** — for KS2/Post16/KS4Autumn the config lookup returns null and `WhatToChangeController.Confirm` bounces back to the pupil list. +- **Flows that exist today**: KS4 June — `Add_KS4June.json`, `Include_KS4June.json`, `Merge_KS4June.json`, `Remove_KS4June.json`; KS4 Autumn — `Add_KS4Autumn.json`; KS2 — `Remove_KS2.json`; 16-19 — `Include_Post16.json` (#711), `Merge_Post16.json`, `Remove_Post16.json`, plus the results-enquiry flows `IncorrectGrade_Post16.json`, `MissingQualification_Post16.json`, `ResultDoesNotBelong_Post16.json`. What to change offers exactly the pupil-data changes whose flow file exists for the window type (`IQuestionFlowService.GetPupilDataChangesAsync`); a posted change with no flow file is refused. - **State**: `RequestState` per window in session — answers keyed by question id, plus `QuestionHistory` (ordered visited pages). Session is a **Postgres distributed cache** (`AddDistributedPostgreSqlCache`, table `session_cache`) so journeys survive load-balancing across replicas. Auth cookie is 30-min sliding; there is **no auto-save** — drafts persist only on explicit "Save and continue later". - **Mechanics** (`JourneyController`, ~750 lines): navigation guard prevents URL-hacking ahead of answered history; changing a branch answer trims downstream history; `fromSummary=true` returns to Summary after single-page edits unless the branch changed. Evidence upload: **PDF only** (verified by PdfPig page counting in `Web/FileStorage/PdfPageCounter.cs`), **10 MB max per file**; the page-count cap exists but is **disabled** (`maxEvidencePages = 0` at `Application/DependencyManager.cs`). Validation failures emit `ValidationErrorEvent` analytics with coded reasons. - **Reference numbers**: `CYPMD_{CheckingWindowType}_{7-char-uppercase-GUID-fragment}` from `JourneyValidationService.GenerateReference`. diff --git a/src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_Post16.json b/src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_Post16.json new file mode 100644 index 000000000..054344393 --- /dev/null +++ b/src/DfE.CheckPerformanceData.Web/Data/QuestionFlows/Include_Post16.json @@ -0,0 +1,38 @@ +{ + "firstPageId": "select-pupil", + "pages": [ + { + "id": "select-pupil", + "type": "PupilSearch", + "title": "What is the name of the student to be included?", + "subheading": "Start typing the first name, last name, CYPMD ID, ULN or date of birth (DOB)", + "pupilFilter": "Includable", + "pupilKey": "primary", + "nextPageId": "evidence", + "validationFailure": "Enter the name of the student to be included." + }, + { + "id": "evidence", + "pageTitle": "Provide evidence for the inclusion", + "type": "EvidenceUpload", + "title": "Provide evidence for the inclusion of {pupilName}", + "questions": [ + { + "id": "evidence", + "type": "FileUpload", + "title": "Upload files", + "hint": "Evidence must be in a PDF format", + "optional": true + }, + { + "id": "how-evidence-supports", + "type": "TextArea", + "title": "Explain how the evidence supports including this student", + "summaryTitle": "How evidence supports student inclusion", + "characterLimit": 500, + "optional": true + } + ] + } + ] +} From bbad074f32eccc6850ce48b582e9c9501742cb37 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 14:31:49 +0000 Subject: [PATCH 06/13] #711 KS2 dev seed skips the INCLUDABLE stamp The KS2 seed builds its CSV columns from the schema and skips the stamps ingress writes. INCLUDABLE is a new stamp, so the seed stopped the web container at start. Co-Authored-By: Claude Sonnet 5.5 --- src/DfE.CheckPerformanceData.Web/Seeding/SeedKs2PupilData.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DfE.CheckPerformanceData.Web/Seeding/SeedKs2PupilData.cs b/src/DfE.CheckPerformanceData.Web/Seeding/SeedKs2PupilData.cs index 07f57213a..6584260da 100644 --- a/src/DfE.CheckPerformanceData.Web/Seeding/SeedKs2PupilData.cs +++ b/src/DfE.CheckPerformanceData.Web/Seeding/SeedKs2PupilData.cs @@ -37,7 +37,7 @@ public static class SeedKs2PupilData private const int NonIncludedPerSchool = 6; // Stamped by ingress from the schema's inclusion rule, never sent by the supplier. - private static readonly HashSet StampedByIngress = ["INCLUDED", "P_INCL_DESC"]; + private static readonly HashSet StampedByIngress = ["INCLUDED", "INCLUDABLE", "P_INCL_DESC"]; public static async Task ExecuteSeedAsync(IPortalDbContext dbContext, BlobServiceClient blobs, ICheckingExerciseIngress ingress, string contentRootPath) From 5b9259c9f8be3572f26c23d40d535007b0b11058 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 14:36:40 +0000 Subject: [PATCH 07/13] #711 E2E: the 16-19 Include journey A non-included student to the confirmation, a 502 student to the evidence page, a 501 student to the already-included warning with no Add button, an unknown name to Student not found. What to change on 16-19 offers Include, Merge and Remove. The reference token for 16-19 is 16to19, so the test matches CYPMD_16to19_ (the plan said Post16). Co-Authored-By: Claude Sonnet 5.5 --- .../Journey/Post16IncludeJourneyTests.cs | 142 ++++++++++++++++++ .../Journey/WhatToChangePost16OptionsTests.cs | 52 ++++--- 2 files changed, 172 insertions(+), 22 deletions(-) create mode 100644 tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs diff --git a/tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs b/tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs new file mode 100644 index 000000000..e3abb9528 --- /dev/null +++ b/tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs @@ -0,0 +1,142 @@ +using DfE.CheckPerformanceData.E2ETests.Fixtures; +using DfE.CheckPerformanceData.E2ETests.Helpers; +using Microsoft.Playwright; +using DfE.CheckPerformanceData.E2ETests.Retrying; + +namespace DfE.CheckPerformanceData.E2ETests.Journey; + +// #711 (AB#301154): the 16-19 Include journey on the seeded "16 to 19 Oct" window, as Kingsmead +// School (860/4070). Who can be picked is the INCLUDABLE stamp: every student in the non-included +// file, and the included file's P_INCL 502 students. The seed (SeedPupilData.GeneratePost16Pupils) +// gives the included file Alice Smith (501), Bob Smith (502), Charlie Smith (505) … and the +// non-included file Alice Johnson, Bob Johnson … +[Trait("Category", "FullRegression")] +[Collection("E2E")] +public sealed class Post16IncludeJourneyTests(PlaywrightFixture fixture) : SeedingPageTest(fixture) +{ + private static readonly Guid WindowId = Guid.Parse("EC6493B8-9B66-4090-8BE6-9DFC3805751F"); + + private const string NonIncludedStudent = "Alice Johnson"; // non-included file + private const string Code502Student = "Bob Smith"; // included file, P_INCL 502 + private const string Code501Student = "Alice Smith"; // included file, P_INCL 501 + private const string NoSuchName = "No Such"; + + [RetryFact] + [Trait("Category", "Smoke")] + public async Task NonIncludedStudent_CanBeIncluded_ThroughToTheConfirmation() + { + // A leftover request for this student would divert the search into the conflict banner. + await SeedHelpers.CleanupDevRequestsAsync(Fixture.SeedClient); + + await StartIncludeJourneyAsync(); + await SelectStudentAsync(NonIncludedStudent); + + // AC2/AC3: the evidence page, both answers optional — continue with nothing. + await Page.WaitForURLAsync($"**/Journey/{WindowId}/page/evidence"); + await Expect(Page.GetByRole(AriaRole.Heading, new() { Level = 1 })) + .ToContainTextAsync($"Provide evidence for the inclusion of {NonIncludedStudent}"); + await Expect(Page.GetByRole(AriaRole.Button, new() { Name = "Save and continue later" })).ToBeVisibleAsync(); + await ContinueAsync(); + + // AC6: the summary names the student, the change and a Change link. + await Expect(Page.GetByRole(AriaRole.Heading, new() { Level = 1 })) + .ToContainTextAsync("Summary of amendment request"); + var summary = Page.Locator(".govuk-summary-list"); + await Expect(summary).ToContainTextAsync("Include a student in data"); + var nameRow = Page.Locator(".govuk-summary-list__row", new() { HasText = "Student name" }); + await Expect(nameRow).ToContainTextAsync(NonIncludedStudent); + await Expect(nameRow.GetByRole(AriaRole.Link, new() { Name = "Change" })).ToBeVisibleAsync(); + + // AC7/AC8: a reference number and the deadline for further amendments. + await Page.GetByRole(AriaRole.Button, new() { Name = "Submit request" }).ClickAsync(); + await Page.WaitForURLAsync($"**/Journey/{WindowId}/confirmation"); + var panel = await Page.Locator(".govuk-panel").InnerTextAsync(); + Assert.Matches(@"CYPMD_16to19_[0-9A-F]{7}", panel); + await Expect(Page.GetByText("to request any further amendments")).ToBeVisibleAsync(); + } + + [RetryFact] + public async Task Code502Student_IsOffered_AndReachesTheEvidencePage() + { + await SeedHelpers.CleanupDevRequestsAsync(Fixture.SeedClient); + + await StartIncludeJourneyAsync(); + await SelectStudentAsync(Code502Student); + + // Not "This student is already included": the 502 student is includable by rule. + await Page.WaitForURLAsync($"**/Journey/{WindowId}/page/evidence"); + await Expect(Page.GetByRole(AriaRole.Heading, new() { Level = 1 })) + .ToContainTextAsync($"Provide evidence for the inclusion of {Code502Student}"); + } + + [RetryFact] + public async Task Code501Student_IsNotOffered_AndTypingTheName_WarnsAlreadyIncluded_WithoutAdd() + { + await StartIncludeJourneyAsync(); + + var input = Page.Locator("#pupil-search").First; + await input.FillAsync(Code501Student); + // AC9: a 501 student is not a suggestion. Every word of the query must match, so "Alice + // Smith" can match nobody else; wait for the component's empty-state row rather than + // counting options straight after typing (Ks4MergeJourneyTests.ExpectNoSuggestionsAsync). + await Expect(Page.Locator(".autocomplete__option--no-results")).ToBeVisibleAsync(new() { Timeout = 10_000 }); + await Expect(Page.Locator("li[role='option']")).ToHaveCountAsync(0); + await input.PressAsync("Escape"); + await ContinueAsync(); + + await Expect(Page.GetByRole(AriaRole.Heading, new() { Level = 1 })) + .ToContainTextAsync("This student is already included"); + Assert.Contains("/already-included", Page.Url); + await Expect(Page.GetByRole(AriaRole.Button, new() { Name = "Abort" })).ToBeVisibleAsync(); + // 16-19 has no Add journey, so no "Start adding" button. + Assert.Equal(0, await Page.GetByRole(AriaRole.Button, new() { NameRegex = new("^Start adding") }).CountAsync()); + } + + [RetryFact] + public async Task UnknownName_ShowsStudentNotFound_WithSearchAgainOnly() + { + await StartIncludeJourneyAsync(); + + var input = Page.Locator("#pupil-search").First; + await input.FillAsync(NoSuchName); + await Page.WaitForTimeoutAsync(250); + await input.PressAsync("Escape"); + await ContinueAsync(); + + await Expect(Page.GetByRole(AriaRole.Heading, new() { Level = 1 })).ToContainTextAsync("Student not found"); + Assert.Contains("/pupil-not-found", Page.Url); + await Expect(Page.GetByRole(AriaRole.Button, new() { Name = "Search again" })).ToBeVisibleAsync(); + Assert.Equal(0, await Page.GetByRole(AriaRole.Button, new() { NameRegex = new("^Start adding") }).CountAsync()); + + await Page.GetByRole(AriaRole.Button, new() { Name = "Search again" }).ClickAsync(); + await Page.WaitForURLAsync($"**/Journey/{WindowId}/pupil-search/select-pupil"); + } + + // ── helpers ────────────────────────────────────────────────────────────── + + private async Task StartIncludeJourneyAsync() + { + await Page.GotoAsync($"{Fixture.BaseUrl}/WhatToChange/{WindowId}"); + await Page.Locator("input[name='SelectedWhatToChange'][value='Include']").CheckAsync(); + await ContinueAsync(); + await Page.WaitForURLAsync($"**/Journey/{WindowId}/pupil-search/select-pupil"); + } + + // Types the name, picks the suggestion that carries it, and continues. The 16-19 suggestion + // reads "Firstname, Surname, (CYPMD ID:…, ULN:…, DOB:…, INCLUDED|NOT INCLUDED)" + // (PupilSuggestionFormat.Label); the rows are li[role='option'], as Ks4MergeJourneyTests picks them. + private async Task SelectStudentAsync(string name) + { + var input = Page.Locator("#pupil-search").First; + await Expect(input).ToBeVisibleAsync(); + await input.FillAsync(name); + var parts = name.Split(' '); + var option = Page.Locator("li[role='option']").GetByText($"{parts[0]}, {parts[1]},"); + await Expect(option.First).ToBeVisibleAsync(); + await option.First.ClickAsync(); + await ContinueAsync(); + } + + private async Task ContinueAsync() => + await Page.GetByRole(AriaRole.Button, new() { Name = "Continue", Exact = true }).ClickAsync(); +} diff --git a/tests/DfE.CheckPerformanceData.E2ETests/Journey/WhatToChangePost16OptionsTests.cs b/tests/DfE.CheckPerformanceData.E2ETests/Journey/WhatToChangePost16OptionsTests.cs index 66ae2e491..754b7fc06 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/Journey/WhatToChangePost16OptionsTests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/Journey/WhatToChangePost16OptionsTests.cs @@ -4,35 +4,46 @@ namespace DfE.CheckPerformanceData.E2ETests.Journey; -// #439: the Include amendment option is not offered on a 16-19 (Post16) window and a crafted -// submission of it is refused server-side, while a KS4 window keeps the option exactly as before. +// What to change on a 16-19 (Post16) window offers exactly the changes with a flow file: Include +// (#711), Merge and Remove. Add has no 16-19 flow, so it is not offered and a crafted post of it is +// refused server-side (docs/ks2-journeys.md, decision 1). A KS4 June window keeps all four. // -// The seeded "16 to 19 Oct" window (DevDataSeeder.Post16OctoberCheckingWindowId) and the seeded KS4June window -// (DevDataSeeder.KeyStage4JuneCheckingWindowId) — the same ids AddPupilJourneyTests uses. +// The seeded "16 to 19 Oct" window (DevDataSeeder.Post16OctoberCheckingWindowId) and the seeded +// KS4June window (DevDataSeeder.KeyStage4JuneCheckingWindowId) — the same ids AddPupilJourneyTests uses. [Trait("Category", "FullRegression")] [Collection("E2E")] public sealed class WhatToChangePost16OptionsTests(PlaywrightFixture fixture) : SeedingPageTest(fixture) { - // The seeded "16 to 19 Oct" window — Include must not be offered (US1) and a crafted Include post - // must be refused (US3). private static readonly Guid Post16WindowId = Guid.Parse("EC6493B8-9B66-4090-8BE6-9DFC3805751F"); - - // The seeded KS4June window — Include must still be offered (US2). private static readonly Guid Ks4JuneWindowId = Guid.Parse("F34D285B-8660-4D12-9C30-787328DEAA0A"); [RetryFact] [Trait("Category", "Smoke")] - public async Task Post16Window_ShowsOnlyMergeAndRemove() + public async Task Post16Window_OffersIncludeMergeAndRemove_AndNoAdd() { await Page.GotoAsync($"{Fixture.BaseUrl}/WhatToChange/{Post16WindowId}"); - // Exactly two radios: Merge and Remove. + await Expect(Page.Locator("input[name='SelectedWhatToChange'][value='Include']")).ToHaveCountAsync(1); await Expect(Page.Locator("input[name='SelectedWhatToChange'][value='Merge']")).ToHaveCountAsync(1); await Expect(Page.Locator("input[name='SelectedWhatToChange'][value='Remove']")).ToHaveCountAsync(1); - - // No Include (this feature) and no Add (pre-existing AB#297310 gate) anywhere. - Assert.Equal(0, await Page.Locator("input[name='SelectedWhatToChange'][value='Include']").CountAsync()); Assert.Equal(0, await Page.Locator("input[name='SelectedWhatToChange'][value='Add']").CountAsync()); + + // The 16-19 noun. + await Expect(Page.GetByText("Include a student in data")).ToBeVisibleAsync(); + } + + [RetryFact] + public async Task Post16Window_SelectingInclude_OpensTheIncludeJourney() + { + await Page.GotoAsync($"{Fixture.BaseUrl}/WhatToChange/{Post16WindowId}"); + + await Page.Locator("input[name='SelectedWhatToChange'][value='Include']").CheckAsync(); + await Page.GetByRole(AriaRole.Button, new() { Name = "Continue", Exact = true }).ClickAsync(); + + await Page.WaitForURLAsync($"**/Journey/{Post16WindowId}/pupil-search/select-pupil"); + await Expect(Page.GetByRole(AriaRole.Heading, new() { Level = 1 })) + .ToContainTextAsync("What is the name of the student to be included?"); + await Expect(Page.Locator("#pupil-search").First).ToBeVisibleAsync(); } [RetryFact] @@ -40,13 +51,11 @@ public async Task Ks4JuneWindow_StillOffersInclude() { await Page.GotoAsync($"{Fixture.BaseUrl}/WhatToChange/{Ks4JuneWindowId}"); - // The option must be present and selectable next to Merge/Remove/Add exactly as before. await Expect(Page.Locator("input[name='SelectedWhatToChange'][value='Include']")).ToHaveCountAsync(1); await Expect(Page.Locator("input[name='SelectedWhatToChange'][value='Merge']")).ToHaveCountAsync(1); await Expect(Page.Locator("input[name='SelectedWhatToChange'][value='Remove']")).ToHaveCountAsync(1); await Expect(Page.Locator("input[name='SelectedWhatToChange'][value='Add']")).ToHaveCountAsync(1); - // Selecting Include and continuing still opens the Include journey. await Page.Locator("input[name='SelectedWhatToChange'][value='Include']").CheckAsync(); await Page.GetByRole(AriaRole.Button, new() { Name = "Continue", Exact = true }).ClickAsync(); await Page.WaitForURLAsync($"**/Journey/{Ks4JuneWindowId}/pupil-search/select-pupil"); @@ -54,24 +63,23 @@ public async Task Ks4JuneWindow_StillOffersInclude() } [RetryFact] - public async Task Post16Window_CraftedIncludeSubmission_IsRefused() + public async Task Post16Window_CraftedAddSubmission_IsRefused() { await Page.GotoAsync($"{Fixture.BaseUrl}/WhatToChange/{Post16WindowId}"); - // The radio is gone from the page, but the POST endpoint still exists. Re-inject the removed - // Include radio into the live form — the bookmark/replay case — select it, and submit. + // The radio is not on the page, but the POST endpoint still exists. Re-inject an Add radio + // into the live form — the bookmark/replay case — select it, and submit. await Page.EvaluateAsync(""" const input = document.createElement('input'); input.type = 'radio'; input.name = 'SelectedWhatToChange'; - input.value = 'Include'; + input.value = 'Add'; document.querySelector('input[name="SelectedWhatToChange"][value="Merge"]') .closest('form').appendChild(input); input.checked = true; """); - // Check it is selected with the form prepared. - var injected = Page.Locator("input[name='SelectedWhatToChange'][value='Include']"); + var injected = Page.Locator("input[name='SelectedWhatToChange'][value='Add']"); await Expect(injected).ToHaveCountAsync(1); await injected.CheckAsync(); @@ -81,4 +89,4 @@ await Page.EvaluateAsync(""" await Page.WaitForURLAsync($"**/CheckYourPupilData/{Post16WindowId}"); Assert.False(Page.Url.Contains("/Journey/", StringComparison.OrdinalIgnoreCase)); } -} \ No newline at end of file +} From 7063bd3a457db093a0f5193b9eac32d349bcbdd9 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Wed, 28 Oct 2026 16:30:52 +0000 Subject: [PATCH 08/13] #711 Review fixes: rule checks, guard test, comments, E2E wait An includable rule with no codes or a blank code fails the run by name. A blank code would match every empty field. A test covers each rule check, including no field. A unit test proves the autocomplete guard asks NotIncludable and never Included. C# comments no longer name inclusion codes. The schema holds them. The unknown-name E2E test waits for the empty-state row, not a fixed time. Co-Authored-By: Claude Opus 5.5 --- docs/blob-storage.md | 4 ++- .../CheckYourPupilData/IPupilRecord.cs | 3 +- .../CheckYourPupilData/Post16PupilRecord.cs | 5 ++-- .../Journey/PupilFilter.cs | 2 +- .../Ingress/CsvSchemaFileProcessor.cs | 5 +++- .../Controllers/Journey/JourneyController.cs | 12 ++++---- .../Journey/Post16IncludeJourneyTests.cs | 3 +- .../Ingress/CsvSchemaFileProcessorTests.cs | 18 ++++++++++++ .../Journey/PupilSearchJourneyTests.cs | 29 +++++++++++++++++++ 9 files changed, 68 insertions(+), 13 deletions(-) diff --git a/docs/blob-storage.md b/docs/blob-storage.md index 0dfdb1742..bdb13e225 100644 --- a/docs/blob-storage.md +++ b/docs/blob-storage.md @@ -47,7 +47,9 @@ validation and guarded by the schema declaring the column (`INCLUDED` / `SOURCE` `AllowAdditionalProperties` is false. A third stamp, `INCLUDABLE` (#711), says whether a school may ask to include the pupil: true when the record is not included, or when its code is one the schema's `x-ingress.includable` rule names (the 16-19 included file names `P_INCL` 502). It is -derived from `INCLUDED`, so it follows the same guard. The Include journey reads it. A run writes +derived from `INCLUDED`, so it follows the same guard. The Include journey reads it. A rule with +no field, an undeclared field, no codes or a blank code fails the run by name before any row is +read. A run writes `CheckingExerciseBlobPaths.DataBlobName(exercise, laestab)` — never `PupilsBlobName` directly: the two normalise the laestab differently on purpose. Every feed must carry a `LAESTAB` column, or the column its schema's `x-ingress.institutionKey` names (`LAESTAB_0` in the previously published file): it is what splits one supplier file into one blob per school; a file without it fails the run by name. diff --git a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs index c7490ef70..3155dfb64 100644 --- a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs +++ b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/IPupilRecord.cs @@ -39,7 +39,8 @@ public interface IPupilRecord /// /// Whether a school may ask to include this pupil: the INCLUDABLE stamp ingress writes /// (#711). Not included, or a code the schema's x-ingress.includable rule names — on - /// 16-19, a student in the included file with P_INCL 502. Never a code here: see + /// 16-19, a student in the included file who is not in the performance measures. Never a code + /// here: see /// . /// bool IsIncludable { get; } diff --git a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs index 56087c532..817be510d 100644 --- a/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs +++ b/src/DfE.CheckPerformanceData.Application/CheckYourPupilData/Post16PupilRecord.cs @@ -11,7 +11,8 @@ namespace DfE.CheckPerformanceData.Application.CheckYourPupilData; /// Ingress stamps an INCLUDED boolean from the file of origin, and that alone decides /// : every student in the included file is included, every one in the /// non-included file is not, whatever P_INCL says. A second stamp, INCLUDABLE, says who may be asked to be -/// included: every non-included student, and included students whose P_INCL is 502 (#711). +/// included: every non-included student, and included students whose P_INCL is a code the +/// schema's x-ingress.includable rule names (#711). /// /// Field names differ from KS4: FORENAMES (plural), and identity is ULN — 16-19 /// has no UPN. URN/UKPRN are strings here (the supplier declares them varchar and @@ -35,7 +36,7 @@ public sealed class Post16PupilRecord : IPupilRecord public bool Included { get; init; } /// Stamped by ingress (#711) from the included file's x-ingress.includable - /// rule (P_INCL 502) and, for the non-included file, from INCLUDED being false. Decides + /// rule and, for the non-included file, from INCLUDED being false. Decides /// who the Include journey can pick; it never changes . [JsonPropertyName("INCLUDABLE")] public bool Includable { get; init; } diff --git a/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs b/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs index 28ae8a2a8..4322fc623 100644 --- a/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs +++ b/src/DfE.CheckPerformanceData.Application/Journey/PupilFilter.cs @@ -4,7 +4,7 @@ namespace DfE.CheckPerformanceData.Application.Journey; /// Which pupils a search returns. and read the /// INCLUDED stamp. and read the /// INCLUDABLE stamp (#711): the Include journey searches — on -/// 16-19 that adds the included file's P_INCL 502 students to the non-included file — and its +/// 16-19 that adds the included-file students the schema's rule names to the non-included file — and its /// "already included" guards look a name up among . ///
public enum PupilFilter { Included, All, Includable, NotIncludable } diff --git a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs index ec4b0db62..873e00f33 100644 --- a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs +++ b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs @@ -319,7 +319,7 @@ public async IAsyncEnumerable ProcessAsync( } // #711: who may be asked to be included — not included, or a code the schema's - // x-ingress.includable rule names (the 16-19 included file's P_INCL 502). Derived + // x-ingress.includable rule names (only the 16-19 included file has one). Derived // from INCLUDED, so it is stamped only when INCLUDED was, and only when the schema // declares it (AllowAdditionalProperties is false). Readers read the stamp. if (schema.Properties.ContainsKey("INCLUDABLE") && @@ -861,6 +861,9 @@ private sealed record IncludableRule(string Field, IReadOnlySet Codes) string.IsNullOrWhiteSpace(Field) ? "x-ingress.includable names no field." : !schema.Properties.ContainsKey(Field) ? $"x-ingress.includable reads {Field}, which the schema does not declare." : !schema.Properties.ContainsKey("INCLUDABLE") ? "x-ingress.includable needs the schema to declare INCLUDABLE, the stamp it writes." + // A rule with no codes adds nobody, and a blank code would match every empty field. + : Codes.Count == 0 ? "x-ingress.includable names no codes." + : Codes.Contains(string.Empty) ? "x-ingress.includable names a blank code." : null; public bool Matches(JObject record) => diff --git a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs index 7575c290d..74b80240d 100644 --- a/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs +++ b/src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs @@ -153,10 +153,10 @@ public async Task PupilSearchPost(Guid windowId, string pageId, s // AB#027: Include journey no-results path. When the user typed a name on the Include // primary search but did not select a suggestion, look the typed entry up against BOTH // the included and non-included populations and branch three ways: - // - a match among pupils who cannot be included (NotIncludable — included for good; on - // 16-19 the included file's 501 students) → warn "already included" (abort); - // - a match among includable pupils only (#711: not included, or the included file's - // P_INCL 502 students on 16-19) → a valid include candidate, so proceed as normal: fall + // - a match among pupils who cannot be included (NotIncludable — included for good) + // → warn "already included" (abort); + // - a match among includable pupils only (#711: not included, or on 16-19 an included-file + // student the schema's rule names) → a valid include candidate, so proceed as normal: fall // through to the ordinary no-selection flow (re-render the search page with the // candidate's suggestions visible) with no warning and no decision page; // - match on neither → offer "Pupil not found" (start Add where the window type has an @@ -278,8 +278,8 @@ await analytics.TrackSafeAsync(new ValidationErrorEvent // name may already be on the included list, and the select-from-dropdown path would // otherwise bypass the included-list guard the typed-entry path (lines 160-239) enforces. // The search page's autocomplete filters to Includable, so the *selected* record itself is - // includable and can never match this NotIncludable lookup (#711: a 16-19 P_INCL 502 student - // is INCLUDED by file but includable by rule, so an Included lookup would find them and block + // includable and can never match this NotIncludable lookup (#711: a 16-19 student can be + // INCLUDED by file but includable by rule, so an Included lookup would find them and block // their own request); the guard looks up the typed label, exactly as the typed path does. if (journey.SelectedWhatToChange == WhatToChange.Include && page.PupilKey == JourneyPage.PrimaryKey) { diff --git a/tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs b/tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs index e3abb9528..dc98545d9 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/Journey/Post16IncludeJourneyTests.cs @@ -99,7 +99,8 @@ public async Task UnknownName_ShowsStudentNotFound_WithSearchAgainOnly() var input = Page.Locator("#pupil-search").First; await input.FillAsync(NoSuchName); - await Page.WaitForTimeoutAsync(250); + // Wait for the component's empty-state row, not a fixed time, before closing the list. + await Expect(Page.Locator(".autocomplete__option--no-results")).ToBeVisibleAsync(new() { Timeout = 10_000 }); await input.PressAsync("Escape"); await ContinueAsync(); diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs index aff24c429..f652f4b92 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs @@ -452,6 +452,24 @@ public async Task An_includable_rule_that_reads_an_undeclared_field_fails_the_ru Assert.Null(pupil); } + // A rule that cannot be honoured fails the run before any row is read, like the inclusion rule. + [Theory] + [InlineData("{ \"codes\": [\"502\"] }", "names no field")] + [InlineData("{ \"field\": \"P_INCL\" }", "names no codes")] + [InlineData("{ \"field\": \"P_INCL\", \"codes\": [] }", "names no codes")] + [InlineData("{ \"field\": \"P_INCL\", \"codes\": [\"502\", \" \"] }", "names a blank code")] + public async Task An_includable_rule_that_cannot_be_honoured_fails_the_run_by_name(string rule, string error) + { + var schema = SchemaWithIncludableRule.Replace( + "\"includable\": { \"field\": \"P_INCL\", \"codes\": [\"502\"] }", $"\"includable\": {rule}"); + + var (last, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,502\n", schema, slotIncluded: true); + + Assert.True(last.IsError); + Assert.Contains(error, last.Message); + Assert.Null(pupil); + } + // The shipped schemas: KS4 and KS2 stamp INCLUDABLE as "not included"; the 16-19 included file // names 502; the 16-19 non-included file has no rule and every row is includable. [Theory] diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs index 3fdb81db8..b1108cc95 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/PupilSearchJourneyTests.cs @@ -697,6 +697,35 @@ public async Task PupilSearchPost_WhenIncludeJourneyAndNoIncludedNameMatch_Proce Assert.Equal("evidence", redirect.RouteValues!["pageId"]); } + // #711: on 16-19 a student can be included by file and still be includable by the schema's + // rule. An Included lookup would find the selected student and block their own request, so the + // autocomplete guard must ask NotIncludable and never Included. + [Fact] + public async Task PupilSearchPost_WhenIncludeJourneyAndSelectedPupilIsIncludedButIncludable_NeverAsksIncluded() + { + var includableStudent = new PupilDto + { + Id = PrimaryPupilId, Firstname = "Bob", Surname = "Smith", + Sex = "M", DateOfBirth = "01/09/2007", Age = 18, + Cypmd_Id = "500002", Identifier = "9900500002" + }; + SetupIncludeJourney(); + _pupilDataService.GetPupilAsync(WindowId, PrimaryPupilId).Returns(includableStudent); + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Bob Smith", PupilFilter.Included) + .Returns([new PupilSuggestionDto(PrimaryPupilId, "Bob, Smith, (INCLUDED)", "Bob", "Smith", "01/09/2007")]); + _pupilDataService.GetPupilSuggestionsAsync(WindowId, "Bob Smith", PupilFilter.NotIncludable) + .Returns([]); + + var result = await _sut.PupilSearchPost(WindowId, "select-pupil", PrimaryPupilId.ToString(), "Bob, Smith, (INCLUDED)"); + + var redirect = Assert.IsType(result); + Assert.Equal("evidence", redirect.RouteValues!["pageId"]); + await _pupilDataService.Received() + .GetPupilSuggestionsAsync(WindowId, "Bob Smith", PupilFilter.NotIncludable, Arg.Any(), Arg.Any(), Arg.Any()); + await _pupilDataService.DidNotReceive() + .GetPupilSuggestionsAsync(Arg.Any(), Arg.Any(), PupilFilter.Included, Arg.Any(), Arg.Any(), Arg.Any()); + } + [Fact] public async Task PupilSearchPost_WhenIncludeJourneyAndIncludedLookupFails_ProceedsNormally() { From adb0efa72f5107ed98c47d3d3688451bc2f7ec0b Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Thu, 29 Oct 2026 08:26:05 +0000 Subject: [PATCH 09/13] #711 Second review: stale comments, three pinning tests, data note - IncludeNoResultsPathTests: two comments still named the removed NonIncluded filter; they now say Includable / NotIncludable. - Unit test: StartAddingPupil on a window type with no Add flow redirects to Check your data and leaves the Include attempt as it was, so a forged POST from a 16-19 decision page is refused by the action, not just hidden. - Integration test: SearchPupilsAsync throws for a filter it has no rule for. - Integration test: a schema that declares INCLUDABLE when nothing stamps INCLUDED fails the run (both stamps stay null and fail row validation), pinning that this case is not silent. No processor change was needed. - docs/blob-storage.md: a pupil blob written before INCLUDABLE existed reads as nobody includable until the exercise is validated again. Co-Authored-By: Claude Opus 5.5 --- docs/blob-storage.md | 3 ++- .../Journey/IncludeNoResultsPathTests.cs | 6 +++--- .../PupilSearchIncludableFilterTests.cs | 12 ++++++++++++ .../Ingress/CsvSchemaFileProcessorTests.cs | 13 +++++++++++++ .../Journey/IncludeNoResultsDecisionTests.cs | 16 ++++++++++++++++ 5 files changed, 46 insertions(+), 4 deletions(-) diff --git a/docs/blob-storage.md b/docs/blob-storage.md index bdb13e225..d1333846f 100644 --- a/docs/blob-storage.md +++ b/docs/blob-storage.md @@ -49,7 +49,8 @@ ask to include the pupil: true when the record is not included, or when its code schema's `x-ingress.includable` rule names (the 16-19 included file names `P_INCL` 502). It is derived from `INCLUDED`, so it follows the same guard. The Include journey reads it. A rule with no field, an undeclared field, no codes or a blank code fails the run by name before any row is -read. A run writes +read. A pupil blob written before `INCLUDABLE` existed lacks it and reads as nobody includable: +validate the exercise again to restamp it. A run writes `CheckingExerciseBlobPaths.DataBlobName(exercise, laestab)` — never `PupilsBlobName` directly: the two normalise the laestab differently on purpose. Every feed must carry a `LAESTAB` column, or the column its schema's `x-ingress.institutionKey` names (`LAESTAB_0` in the previously published file): it is what splits one supplier file into one blob per school; a file without it fails the run by name. diff --git a/tests/DfE.CheckPerformanceData.E2ETests/Journey/IncludeNoResultsPathTests.cs b/tests/DfE.CheckPerformanceData.E2ETests/Journey/IncludeNoResultsPathTests.cs index ff1409c1f..b5169c3e1 100644 --- a/tests/DfE.CheckPerformanceData.E2ETests/Journey/IncludeNoResultsPathTests.cs +++ b/tests/DfE.CheckPerformanceData.E2ETests/Journey/IncludeNoResultsPathTests.cs @@ -108,7 +108,7 @@ public async Task NonIncludedMatch_ProceedsAsNormal_NoDecisionPage() { await StartIncludeJourneyAsync(); // Bob Johnson is a non-included pupil — a valid include candidate. The Include search's - // autocomplete targets NonIncluded pupils, so typing a single surname token ("Johnson" — + // autocomplete targets Includable pupils, so typing a single surname token ("Johnson" — // "Johnson" is a non-included surname bucket and matches no included pupil) and continuing // without selecting a suggestion must NOT divert to "Pupil not found" or "Already included": // the journey proceeds as normal, re-rendering the search page with the candidate's @@ -130,8 +130,8 @@ public async Task NonIncludedMatch_ProceedsAsNormal_NoDecisionPage() public async Task IncludedMatch_ShowsAlreadyIncluded_WithAbortOption() { await StartIncludeJourneyAsync(); - // Alice Smith is included. The Include search autocomplete targets NonIncluded pupils and - // the feature's included-list lookup reuses the single-field name matcher, so we type a + // Alice Smith is included. The Include search autocomplete targets Includable pupils and + // the guard's NotIncludable lookup reuses the single-field name matcher, so we type a // partial surname — "the user typed a partial name" (US2) — that resolves to her, and // submit WITHOUT selecting a suggestion. await TypePupilWithoutSelectingAsync(IncludedSurname); diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs index f70d1b806..b0af29bce 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/CheckYourPupilData/PupilSearchIncludableFilterTests.cs @@ -126,4 +126,16 @@ public async Task Included_still_counts_the_502_student_as_included() Assert.Equal(2, results.Count); Assert.DoesNotContain(results, r => r.Id == aliceNonIncluded.Id); } + + // A filter the repository has no rule for fails loudly, so a new PupilFilter value cannot + // quietly return the wrong pupils. + [Fact] + public async Task An_unknown_filter_throws() + { + var (alice501, bob502, aliceNonIncluded) = Students(); + var (windowId, repo) = await SeededAsync(alice501, bob502, aliceNonIncluded); + + await Assert.ThrowsAsync(() => + repo.SearchPupilsAsync(windowId, TestLaestab, TestUrn, "Smith", (PupilFilter)99)); + } } diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs index f652f4b92..b312c8e39 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs @@ -452,6 +452,19 @@ public async Task An_includable_rule_that_reads_an_undeclared_field_fails_the_ru Assert.Null(pupil); } + // INCLUDABLE is derived from INCLUDED. A slot with no file of origin and a schema with no + // inclusion rule stamp neither, so both are left null and the rows fail validation: the run + // fails rather than write a file that offers nobody for inclusion. + [Fact] + public async Task A_schema_declaring_INCLUDABLE_with_nothing_to_stamp_INCLUDED_fails_the_run() + { + var (last, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,502\n", SchemaWithIncludableRule); + + Assert.True(last.IsError); + Assert.Contains("Validation failed", last.Message); + Assert.Null(pupil); + } + // A rule that cannot be honoured fails the run before any row is read, like the inclusion rule. [Theory] [InlineData("{ \"codes\": [\"502\"] }", "names no field")] diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs index 2541f6184..a68f65eeb 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/IncludeNoResultsDecisionTests.cs @@ -381,6 +381,22 @@ public async Task StartAddingPupil_ResetsToAddJourney_RedirectsToLearnerDetails( Assert.Equal("learner-details", saved.QuestionHistory.Single()); } + // #711: the decision pages hide "Start adding" where there is no Add flow (16-19), and a forged + // POST is refused by the action itself, leaving the Include attempt untouched. + [Fact] + public async Task StartAddingPupil_OnAWindowTypeWithNoAddFlow_RedirectsToCheckYourData() + { + SetupSession(SessionForIncludeWith(label: "No Such")); + _flowService.GetConfigAsync(WhatToChange.Add, CheckingWindowType.KS4June).Returns((QuestionFlowConfig?)null); + + var result = await _sut.StartAddingPupil(WindowId); + + AssertRedirectToCheckYourData(result); + var saved = _session.GetRequestState(WindowId); + Assert.Equal(WhatToChange.Include, saved.SelectedWhatToChange); + Assert.Equal("No Such", saved.IncludeSearchLabel); + } + // -- T019: PupilSearchAgain returns to the Include select-pupil search page -- [Fact] From 136339e4a0a44b98e476be9e7a18b7c82a992a19 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Thu, 29 Oct 2026 12:09:51 +0000 Subject: [PATCH 10/13] Fix FlowIdCoupling baseline after #712 moved the grade ids #736 (#712) moved q-missing-grade and q-revised-grade out of JourneyController and JourneyViewModelBuilder into EnquiryQuestionIds. It merged after #740 tightened the baseline, so main fails both FlowIdCouplingTests ratchet checks. Move the two ids to their new owner. Co-Authored-By: Claude Opus 5.5 --- .../Journey/FlowIdCouplingTests.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Journey/FlowIdCouplingTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Journey/FlowIdCouplingTests.cs index ab9434702..aa261f7e3 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Journey/FlowIdCouplingTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Journey/FlowIdCouplingTests.cs @@ -50,9 +50,12 @@ public sealed class FlowIdCouplingTests // ── 4. Journey code in the engine and the Web layer. Replace with question roles // (pattern 4) and page types. ["src/DfE.CheckPerformanceData.Application/AmendmentRequests/EditAdviceService.cs"] = ["evidence", "reason"], + // #712 moved the grade question ids out of JourneyController and JourneyViewModelBuilder + // into this single owner, which the results-enquiries tab also reads. Not new coupling. + ["src/DfE.CheckPerformanceData.Application/ResultsEnquiry/EnquiryQuestionIds.cs"] = ["q-missing-grade", "q-revised-grade"], ["src/DfE.CheckPerformanceData.Web/Controllers/Journey/AddPupilJourney.cs"] = ["admission-details", "date-of-birth", "first-name", "last-name", "learner-details", "sex", "upn"], - ["src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs"] = ["q-cohort-scope", "q-missing-grade", "q-revised-grade", "q-syllabus-code"], - ["src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyViewModelBuilder.cs"] = ["additional-info", "q-additional-info", "q-award-date", "q-cohort-count", "q-cohort-scope", "q-missing-grade", "q-ncn", "q-syllabus-code", "qualification-details", "select-qualification"], + ["src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyController.cs"] = ["q-cohort-scope", "q-syllabus-code"], + ["src/DfE.CheckPerformanceData.Web/Controllers/Journey/JourneyViewModelBuilder.cs"] = ["additional-info", "q-additional-info", "q-award-date", "q-cohort-count", "q-cohort-scope", "q-ncn", "q-syllabus-code", "qualification-details", "select-qualification"], ["src/DfE.CheckPerformanceData.Web/Controllers/ResultIssue/ResultIssueController.cs"] = ["check-late-results"], ["src/DfE.CheckPerformanceData.Web/Views/Journey/AlreadyIncluded.cshtml"] = ["select-pupil"], ["src/DfE.CheckPerformanceData.Web/Views/Journey/DuplicateCheck.cshtml"] = ["learner-details"], From 748e7d971a2e178a1fd0753259de47551af650f3 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Thu, 29 Oct 2026 13:18:52 +0000 Subject: [PATCH 11/13] Fail ingress when a schema declares INCLUDED but not INCLUDABLE Such a schema ingested cleanly and then offered nobody for inclusion. The check runs on every schema, including ones an admin uploads, so a shipped-schema test alone would not cover it. The shipped-schema test now also covers the KS2 December file. The blob-storage doc says that pre-change pupil files need re-validating on every key stage. Co-Authored-By: Claude Opus 5.5 --- docs/blob-storage.md | 6 ++-- .../Ingress/CsvSchemaFileProcessor.cs | 9 +++++ .../Ingress/CheckingExerciseReleaseTests.cs | 3 +- .../Ingress/CsvSchemaFileProcessorTests.cs | 36 ++++++++++++------- .../Ingress/ExerciseScopedIngressTests.cs | 1 + .../Ingress/Post16EndToEndTests.cs | 2 ++ 6 files changed, 41 insertions(+), 16 deletions(-) diff --git a/docs/blob-storage.md b/docs/blob-storage.md index d1333846f..eca46ae9b 100644 --- a/docs/blob-storage.md +++ b/docs/blob-storage.md @@ -49,8 +49,10 @@ ask to include the pupil: true when the record is not included, or when its code schema's `x-ingress.includable` rule names (the 16-19 included file names `P_INCL` 502). It is derived from `INCLUDED`, so it follows the same guard. The Include journey reads it. A rule with no field, an undeclared field, no codes or a blank code fails the run by name before any row is -read. A pupil blob written before `INCLUDABLE` existed lacks it and reads as nobody includable: -validate the exercise again to restamp it. A run writes +read, and so does a schema that declares `INCLUDED` but not `INCLUDABLE`: it would otherwise +offer nobody for inclusion. A pupil blob written before `INCLUDABLE` existed lacks it, on every +key stage, and reads as nobody includable: the Include search finds nobody, and the "already +included" check matches every typed name. Validate the exercise again to restamp it. A run writes `CheckingExerciseBlobPaths.DataBlobName(exercise, laestab)` — never `PupilsBlobName` directly: the two normalise the laestab differently on purpose. Every feed must carry a `LAESTAB` column, or the column its schema's `x-ingress.institutionKey` names (`LAESTAB_0` in the previously published file): it is what splits one supplier file into one blob per school; a file without it fails the run by name. diff --git a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs index 873e00f33..03d3560bd 100644 --- a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs +++ b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs @@ -203,6 +203,15 @@ public async IAsyncEnumerable ProcessAsync( yield break; } + // INCLUDABLE is derived from INCLUDED. A pupil file that declares only INCLUDED would + // ingest cleanly and then offer nobody for inclusion, so the pair is required together. + if (schema.Properties.ContainsKey("INCLUDED") && !schema.Properties.ContainsKey("INCLUDABLE")) + { + summaryCapture?.SetFailure("schema", "identifier_mapping_invalid"); + yield return Failed($"Schema '{dataset.SchemaFile}': it declares INCLUDED but not INCLUDABLE, so it would offer nobody for inclusion."); + yield break; + } + // Capture each complete source record before any schema transformation. CsvHelper // counts CSV records, so a quoted field containing line breaks remains one row. List> records = []; diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CheckingExerciseReleaseTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CheckingExerciseReleaseTests.cs index f7b25c4e6..c7ee7e986 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CheckingExerciseReleaseTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CheckingExerciseReleaseTests.cs @@ -51,7 +51,8 @@ public sealed class CheckingExerciseReleaseTests(AzuriteFixture azurite) : IAsyn "SESSION": { "type": ["string", "null"] }, "GRADE": { "type": ["string", "null"] }, "SOURCE": { "type": "string" }, - "INCLUDED": { "type": "boolean" } + "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" } } } """; diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs index b312c8e39..fe5f201bd 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/CsvSchemaFileProcessorTests.cs @@ -24,6 +24,7 @@ public sealed class CsvSchemaFileProcessorTests(AzuriteFixture fixture) "properties": { "Id": { "type": ["string", "null"] }, "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "CYPMD_ID": { "type": ["string", "null"] }, "SURNAME": { "type": ["string", "null"] }, "FORENAMES":{ "type": ["string", "null"] }, @@ -40,6 +41,7 @@ public sealed class CsvSchemaFileProcessorTests(AzuriteFixture fixture) "properties": { "Id": { "type": ["string", "null"] }, "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "CYPMD_ID": { "type": ["string", "null"] }, "SURNAME": { "type": ["string", "null"] }, "FORENAMES":{ "type": ["string", "null"] }, @@ -172,6 +174,7 @@ public async Task An_error_in_either_dataset_writes_nothing() "properties": { "Id": { "type": ["string", "null"] }, "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "SURNAME": { "type": ["string", "null"] }, "FORENAMES":{ "type": ["string", "null"] }, "CYPMD_ID": { "type": ["string", "null"] }, @@ -259,6 +262,7 @@ public async Task A_single_ks4_dataset_run_is_unchanged_and_stamps_nothing() }, "properties": { "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "SURNAME": { "type": ["string", "null"] }, "LAESTAB": { "type": ["string", "null"] }, "P_INCL": { "type": ["string", "null"] }, @@ -406,26 +410,29 @@ public async Task The_schemas_includable_rule_stamps_INCLUDABLE_from_the_code_or [Fact] public async Task A_schema_with_no_includable_rule_stamps_INCLUDABLE_as_not_included() { - var schema = SchemaWithInclusionRule.Replace( - "\"INCLUDED\": { \"type\": \"boolean\" },", - "\"INCLUDED\": { \"type\": \"boolean\" },\n \"INCLUDABLE\": { \"type\": \"boolean\" },"); - - var (included, _) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,401\n", schema); - var (notIncluded, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,402\n", schema); + var (included, includedPupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,401\n", SchemaWithInclusionRule); + var (notIncluded, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,402\n", SchemaWithInclusionRule); Assert.False(included.IsError, included.Message); Assert.False(notIncluded.IsError, notIncluded.Message); + Assert.False(includedPupil!["INCLUDABLE"]!.Value()); Assert.True(pupil!["INCLUDABLE"]!.Value()); } - [Fact] - public async Task A_schema_that_does_not_declare_INCLUDABLE_stamps_nothing_for_it() + // Without INCLUDABLE a pupil file would offer nobody for inclusion, and nothing would say so. + // An uploaded schema can do this as easily as a shipped one, so the run fails by name. + [Theory] + [InlineData(null)] + [InlineData(true)] + public async Task A_schema_declaring_INCLUDED_but_not_INCLUDABLE_fails_the_run_by_name(bool? slotIncluded) { - var (last, pupil) = await RunInclusionRuleAsync( - "SURNAME,LAESTAB,P_INCL\nSmith,8604070,402\n", SchemaWithInclusionRule); + var schema = SchemaWithInclusionRule.Replace("\"INCLUDABLE\": { \"type\": \"boolean\" },", string.Empty); - Assert.False(last.IsError, last.Message); - Assert.Null(pupil!["INCLUDABLE"]); + var (last, pupil) = await RunInclusionRuleAsync("SURNAME,LAESTAB,P_INCL\nSmith,8604070,402\n", schema, slotIncluded); + + Assert.True(last.IsError); + Assert.Contains("declares INCLUDED but not INCLUDABLE", last.Message); + Assert.Null(pupil); } [Fact] @@ -489,6 +496,7 @@ public async Task An_includable_rule_that_cannot_be_honoured_fails_the_run_by_na [InlineData("ks4june", "pupils_schema.json", "UPN,CYPMD_ID,SURNAME,FORENAME,P_INCL,LAESTAB\nA1,1,Smith,Al,401,8604070\n", null, false)] [InlineData("ks4june", "pupils_schema.json", "UPN,CYPMD_ID,SURNAME,FORENAME,P_INCL,LAESTAB\nA1,1,Smith,Al,402,8604070\n", null, true)] [InlineData("ks2", "pupils_schema.json", "UPN,CYPMD_ID,MATCHREF,SURNAME,FORENAME,PINCL_Flag,LAESTAB\nA1,1,7,Smith,Al,201,8604070\n", null, false)] + [InlineData("ks2", "pupils-december_schema.json", "UPN,CYPMD_ID,MATCHREF,SURNAME,FORENAME,PINCL_Flag,LAESTAB\nA1,1,7,Smith,Al,202,8604070\n", null, true)] [InlineData("post16", "students-included_schema.json", "CYPMD_ID,SURNAME,FORENAMES,P_INCL,LAESTAB\n1,Smith,Al,501,8604070\n", true, false)] [InlineData("post16", "students-included_schema.json", "CYPMD_ID,SURNAME,FORENAMES,P_INCL,LAESTAB\n1,Smith,Al,502,8604070\n", true, true)] [InlineData("post16", "students-included_schema.json", "CYPMD_ID,SURNAME,FORENAMES,P_INCL,LAESTAB\n1,Smith,Al,505,8604070\n", true, false)] @@ -607,6 +615,7 @@ public async Task AFailedRun_LeavesThePreviousOutputInPlace() "properties": { "Id": { "type": ["string", "null"] }, "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "SURNAME": { "type": ["string", "null"] }, "FORENAMES":{ "type": ["string", "null"] }, "CYPMD_ID": { "type": ["string", "null"] }, @@ -690,8 +699,9 @@ public async Task Pupil_outputs_keep_their_complete_contract_and_existing_csv_su if (post16) { ((JObject)expected[0])["INCLUDED"] = true; + ((JObject)expected[0])["INCLUDABLE"] = false; if (school == "8604070") - expected.Add(JObject.Parse("""{"CYPMD_ID":"500002","SURNAME":"Jones","FORENAMES":"Bob","LAESTAB":"8604070","ULN":"9900000002","CampID_0":"C0","INCLUDED":false}""")); + expected.Add(JObject.Parse("""{"CYPMD_ID":"500002","SURNAME":"Jones","FORENAMES":"Bob","LAESTAB":"8604070","ULN":"9900000002","CampID_0":"C0","INCLUDED":false,"INCLUDABLE":true}""")); } Assert.True(JToken.DeepEquals(expected, actual), $"Unexpected pupil output for {school}: {actual}"); } diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/ExerciseScopedIngressTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/ExerciseScopedIngressTests.cs index a371ff824..74bd5825b 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/ExerciseScopedIngressTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/ExerciseScopedIngressTests.cs @@ -24,6 +24,7 @@ public sealed class ExerciseScopedIngressTests(AzuriteFixture fixture) "properties": { "Id": { "type": ["string", "null"] }, "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "CYPMD_ID": { "type": ["string", "null"] }, "SURNAME": { "type": ["string", "null"] }, "LAESTAB": { "type": ["string", "null"] } diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/Post16EndToEndTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/Post16EndToEndTests.cs index 22aa5bad2..319cb6788 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/Post16EndToEndTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Ingress/Post16EndToEndTests.cs @@ -38,6 +38,7 @@ public sealed class Post16EndToEndTests(AzuriteFixture azurite, PostgresFixture "properties": { "Id": { "type": ["string", "null"] }, "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "CYPMD_ID": { "type": ["string", "null"] }, "SURNAME": { "type": ["string", "null"] }, "FORENAMES": { "type": ["string", "null"] }, @@ -59,6 +60,7 @@ public sealed class Post16EndToEndTests(AzuriteFixture azurite, PostgresFixture "properties": { "Id": { "type": ["string", "null"] }, "INCLUDED": { "type": "boolean" }, + "INCLUDABLE": { "type": "boolean" }, "CYPMD_ID": { "type": ["string", "null"] }, "SURNAME": { "type": ["string", "null"] }, "FORENAMES": { "type": ["string", "null"] }, From 6d82cd7f62364ff55e5a4da9bc9d3a5f9c7c14f8 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Thu, 29 Oct 2026 15:32:46 +0000 Subject: [PATCH 12/13] Give the INCLUDED-without-INCLUDABLE ingress failure its own code identifier_mapping_invalid describes an identifier problem. This check is about a missing INCLUDABLE stamp, so the run summary now says includable_not_declared. Co-Authored-By: Claude Opus 5.5 --- .../Ingress/CsvSchemaFileProcessor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs index 03d3560bd..24dd0ac50 100644 --- a/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs +++ b/src/DfE.CheckPerformanceData.Infrastructure/Ingress/CsvSchemaFileProcessor.cs @@ -207,7 +207,7 @@ public async IAsyncEnumerable ProcessAsync( // ingest cleanly and then offer nobody for inclusion, so the pair is required together. if (schema.Properties.ContainsKey("INCLUDED") && !schema.Properties.ContainsKey("INCLUDABLE")) { - summaryCapture?.SetFailure("schema", "identifier_mapping_invalid"); + summaryCapture?.SetFailure("schema", "includable_not_declared"); yield return Failed($"Schema '{dataset.SchemaFile}': it declares INCLUDED but not INCLUDABLE, so it would offer nobody for inclusion."); yield break; } From bd186729b06b2f96f1179ebf5557130dd0e5fad6 Mon Sep 17 00:00:00 2001 From: Andy Smith Date: Fri, 30 Oct 2026 11:08:03 +0000 Subject: [PATCH 13/13] #711 Pin the KS4 record's INCLUDABLE binding The 16-19 record had a test that a blob without the stamp reads as not includable. The KS4 and KS2 record had none, though pre-change blobs on every key stage lack it. Co-Authored-By: Claude Opus 5.5 --- .../PupilRecordDeserializationTests.cs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/PupilRecordDeserializationTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/PupilRecordDeserializationTests.cs index 0e9c9100a..9c56660a0 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/PupilRecordDeserializationTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/CheckYourPupilData/PupilRecordDeserializationTests.cs @@ -221,6 +221,27 @@ public void IsIncluded_is_false_when_the_stamp_is_absent() Assert.False(pupil.IsIncluded); } + // Ingress stamps INCLUDABLE (#711); on KS4 and KS2 it is "not included". The record reads only the stamp. + [Theory] + [InlineData("true", true, true)] + [InlineData("false", false, false)] + public void IsIncludable_follows_the_stamp_not_INCLUDED(string stamp, bool included, bool expected) + { + var pupil = DeserializeSingle($$"""[ { "INCLUDED": {{(included ? "true" : "false")}}, "INCLUDABLE": {{stamp}} } ]"""); + + Assert.Equal(expected, pupil.IsIncludable); + } + + [Fact] + public void IsIncludable_is_false_when_the_stamp_is_absent() + { + // A blob written before the stamp existed offers nobody for inclusion until the exercise is + // validated again (docs/blob-storage.md). + var pupil = DeserializeSingle("""[ { "INCLUDED": false, "SURNAME": "Smith" } ]"""); + + Assert.False(pupil.IsIncludable); + } + [Fact] public void The_inclusion_description_is_read_from_P_INCL_DESC() {