Filter course schools by education phase - #6263
Draft
tomas-stefano wants to merge 15 commits into
Draft
Conversation
tomas-stefano
added
the
deploy
tomas-stefano
changed the title
CourseSchools::Creator and Remover keyed on gias_school_id, but the provider_school unique index is (provider_id, gias_school_id, site_code) — a provider may legitimately hold the same GIAS school under two site codes. Creator#find_by! then picked between them arbitrarily, and Remover#destroy_all detached both. Both services now take the Provider::School directly. The one caller, UpdateCourseSchoolsService, does the site -> urn -> gias_school -> provider_school hop itself; the warn-and-skip for a provider that has not been backfilled moves there with it. No user-visible change.
Moving the course school pickers onto Provider::School keys them on uuid,
which relies on provider_school.uuid being a copy of site.uuid. Three
checks find the rows where that pairing has broken:
orphan_provider_schools a Provider::School with no kept school Site.
Blocking — Provider::School has no discarded_at,
so a site removed by discard_invalid_schools can
leave its provider_school behind, and that school
would reappear in the migrated picker.
unmapped_sites a kept school Site with no Provider::School;
it would vanish from the picker.
unmapped_attachments a live course_site row with no course_school row;
relies on the picker's union rule.
rake schools_remodel_preflight:run[2026] aborts when the blocking check
finds anything, so it can gate a deploy.
CourseSchools::Identity delegated its school list to ProviderSchools::Identity, so it returned legacy Sites until the schools_remodel_cycle_year cutover. The course pickers need GIAS education phase data, which only hangs off Provider::School, so they move to the new model now and this stops branching on the cycle. current_school_uuids unions two sources: course.schools, plus the provider_schools whose uuid matches an attached legacy site. The second covers courses the schools backfill skipped — without it the picker would render those attachments unticked and the next save would detach them. A legacy attachment with no Provider::School is deliberately left out, since the picker cannot render it and the write path could not address it. Both gaps are counted by the pre-flight report. The cycle predicate survives as legacy_site_writes?, which is now purely a write-path question, and the legacy union is skipped once it is false: after rollover ProviderCopier mints fresh uuids, so site.uuid and provider_school.uuid diverge and matching on them would be wrong. school_records_for no longer raises. A school removed by a colleague between render and submit, a tampered form and a stale wizard session all arrive there, and none of them should 500; it returns the records it resolved alongside the uuids it could not. Still has no callers — the pickers switch over in later commits.
Three gaps that would have degraded silently once the pickers list Provider::School records: NewlyAddedTagComponent guarded on respond_to?(:register_import?), so it rendered nothing for Provider::School — the "Newly added" rollover cue would have vanished with no failing test. added_via is only recorded on the legacy site, so Provider::School reaches it through a legacy_site association matched on the shared uuid, and the component now calls register_import? directly. The association is preloaded in CourseSchools::Identity so the tag costs one query, not one per checkbox. Provider::SchoolDecorator had no full_address, so delegate_all forwarded to the model and the checkbox hints lost the smart quotes that SiteDecorator applies. Both models now render addresses identically. legacy_site returns nil after rollover, when ProviderCopier mints fresh uuids and the pairing no longer holds — correct, since the tag only shows during the 2026 rollover anyway.
The pickers are about to post Provider::School uuids, so the write path diffs uuids and resolves them through CourseSchools::Identity. Orphan protection falls out of that: current_school_uuids only contains schools the picker could render, so an attachment it cannot represent is in neither the submitted nor the current set and is never treated as a removal. Previously course.site_ids included such rows and unticking anything detached them. The legacy SiteStatus write survives via provider_school.legacy_site. Attaching without it would publish a course Find cannot return, since Course.findable joins site_statuses, so a missing site raises and rolls the transaction back. Detaching is the user cleaning up and must not be blocked, so that logs instead. normalise_params translates legacy site_ids payloads. Two senders need it: the pickers until they post uuids themselves, and jobs enqueued before this deploy — which are by definition the largest submissions, since only those above ENQUEUE_THRESHOLD are enqueued. Without it the service would diff against nothing and silently discard them. Notification names now come from Provider::School, so main sites read "<GIAS name> (Main site)", and are sorted: neither course.schools nor the legacy union has an inherent order, and an unstable one would notify for an unchanged course. The system fixtures created provider_schools without copying site.uuid, which production always does. SchoolPairingHelper states that invariant in one place so fixtures stay representative.
The edit picker now lists Provider::School records and posts their uuids, which is what lets the next commits filter the list by the education phase held on gias_school. Labels and hints come from the paired GIAS school rather than the site, so a main site reads "<GIAS name> (Main site)" and addresses use the GIAS county rather than the site's address4. Ordering moves from the site's location_name to the GIAS name, which reshuffles the list for providers whose site names diverged from GIAS. A legacy site with no Provider::School can no longer be rendered, so it disappears from the picker. The pre-flight report counts those; the audit in school-mapping-gaps-by-provider.md records exactly one kept, URN-less school site in the 2026 cycle and no findable course using it. The site_ids error keys move with the field, or the error summary would render "translation missing".
attached_schools_count sits directly above the checkbox list and branched on :course_publishing_uses_new_school_model, so neither branch matched what the page rendered: with the flag off it counted course.sites, which includes discarded sites and sites with no Provider::School. The rollover error copy had the same problem — it chose between "review the schools" and "enter schools" from course.sites, so a provider could be told to review a list that rendered empty. Both now read CourseSchools::Identity#current_school_uuids, so the number, the copy and the ticked boxes cannot disagree. The list display spec's legacy/new-model split goes with it: there is one read path now, and its former "legacy" fixtures were sites with no Provider::School, which is a case worth keeping as its own example.
The course creation path resolved schools by walking site -> urn -> gias_school -> provider_school, which meant a provider holding the same GIAS school twice was ambiguous and a site whose GIAS school had since closed was silently dropped from the new model while still being attached to the legacy one. It now resolves the selection to Provider::School records directly. school_uuids is the form the pickers are moving to; sites_ids is still accepted, translated through the shared site/provider_school uuid, until every caller has moved over. Two behaviour changes fall out. A site with no Provider::School is no longer selectable at all, rather than getting a legacy row on its own — consistent with the picker, which cannot render it either. And the GiasSchool.available filter is gone, so a course can now be attached to a school GIAS has closed; the picker offers it, so refusing to record it would leave the two models disagreeing. The legacy site_status write stays unconditional here, unlike the edit path. Courses::PublishRules::SchoolPresence reads course.sites whenever :course_publishing_uses_new_school_model is off, in every cycle, so skipping it after the cutover fails validation on the schools step. System fixtures gain the provider_school rows production always has.
The wizard step now lists Provider::School records and stores their uuids, matching the edit picker, so both can be filtered by education phase in the same place. Draft#school_uuids drops anything that is not a uuid. Sessions started before this deploy hold integer site ids in the state store, and querying uuid = '123' raises PG::InvalidTextRepresentation — a stale session re-asks the question instead of erroring. The check answers row reads through the decorator, so a main site is listed there as "<GIAS name> (Main site)" too. Note the auto-select-when-one-school shortcut now counts Provider::School records, so a provider whose only other sites are unpaired skips the step entirely. That becomes more visible once phase filtering lands, and is worth confirming with the product owner.
The last picker to move. All three now list Provider::School records and post uuids, so CourseSchools::Identity is the single source for what a provider may attach to a course. CourseDecorator#sites_ids becomes #school_uuids and reads through the identity, since it round-trips the in-progress course through the hidden fields that carry it across the legacy add-course steps. The single-school shortcut in #new and #back counts Provider::School records rather than sites, so a provider whose only other sites are unpaired now skips the page. Same caveat as the wizard. The schools list is exposed as a helper method rather than an ivar: #continue re-renders :new on validation failure without running #new. Two spec-side notes. The error summary anchor moved with the field name, from site-ids to school-uuids. And several fixtures created sites with no Provider::School, which production never has — those schools were silently unselectable, which is exactly the failure mode the pairing helper exists to prevent.
The education phase and age range have been imported from GIAS since the gias_school table was created but read nowhere in the app. This is what reads them. Most phase codes map straight onto a level, with all-through counting as both primary and secondary. "Not applicable" is the largest phase in GIAS, so those are segmented by statutory age range instead, and more than one level's rule can match the same school. The rules fail open. A school with no phase code, a code this enum does not know, or "not applicable" with no usable age range shows on every level — a provider must never lose access to a school because of a gap in GIAS data. minimum_age and maximum_age are free text, so the cast strips non-digits and NULLIFs before ::int. A CASE guard would read more naturally but Postgres does not promise the condition is evaluated before the cast in its branch, so it can still raise 22P02; this cannot. The predicate is one disjunction in a raw SQL fragment and Rails ANDs those onto a relation without wrapping them, so the outer parentheses are load-bearing. There is a spec pinning that. Nothing calls the scopes yet — the pickers pass the level in next.
Filtering by education phase can leave a provider with nothing to select. Further education is the exposed case: only the sixteen_plus phase code matches it outright, so a school-based provider whose schools are all secondary would see an empty list and no way forward but the GIAS link in the callout. The check is built from Provider::School.for_course_level, the same scope the picker uses, so the report cannot drift from what providers see. It reports the provider's total school count alongside, so the size of the problem is visible rather than just its existence. Not blocking — an empty list is a content decision for the product owner, not a data integrity fault.
This is the change providers asked for. The picker listed every school in their account, so a Biology course offered primary schools and providers coped by selecting everything, which polluted Find results. All three pickers now pass the course level through CourseSchools::Identity: the edit form and the legacy add page take it from the course, the wizard from its state store, since it has no course record yet. Schools already attached to the course are unioned back in. Rolled over courses keep schools the filter would now hide, and detaching those is a separate ticket — leaving them visible and ticked means a provider removes them deliberately rather than the next save doing it silently. There is a system spec covering exactly that round trip. out_of_phase_schools? reports when the union has surfaced one, because the callout copy says only schools of a single phase are shown and that stops being true when it does. The callout reads it next.
A filtered list is only helpful if providers understand why a school they expected is missing, so a callout at the bottom of all three pickers names the course level and spells out the two things they can act on: the school may not be in their account, or its GIAS record may be wrong. The heading softens when an out-of-phase school is still listed. Saying "only secondary schools are shown" is untrue the moment a rolled over attachment surfaces, which the picker does deliberately. New app-inset-text--attention modifier: orange, sitting between --important (blue, action needed) and --error. This explains rather than warns. The final design and content were still marked as pending on the Lucid board, so the copy is the version from the mockup and is worth confirming with the designer before release.
tomas-stefano
force-pushed
the
td/filter-course-schools-by-education-phase
branch
from
July 30, 2026 10:13
e7751f3 to
8025a6a
Compare
Brakeman flagged the for_course_level scope as possible SQL injection. It was a false positive — every interpolated value was a frozen constant and the level only picked between them via fetch — but the check cannot see that, and a reader has to trace the constants to be sure too. The three finished predicates are now built once at load time into FOR_COURSE_LEVEL_SQL, so the query call site takes a string it looked up rather than one it interpolated. Nothing a caller passes can reach the SQL, which is now evident rather than inferred. No behaviour change, and no brakeman.ignore entry needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
WIP