Duplicate schools fix - #6318
Closed
tomas-stefano wants to merge 2 commits into
Closed
Conversation
Some providers hold the same school twice. The query we started from grouped site rows by provider and urn but filtered `code <> '-'`, which found 9 groups for 2026 and hid the shape behind the other 95: a provider's main site and the same school added again as a placement school, which the schools list and the course pickers now show side by side because both read Provider::School. The shapes have different causes and different safe answers, so this classifies rather than merges. A Kind subclass per shape says how to recognise itself and which flags matter to the decision it needs - including whether the existing deduplicator's pick_primary_site would discard the provider's main site, which for 2026 it would in 35 of the 95 collisions.
The report printed its findings and exited, so the evidence a merge policy has to be chosen from lived in a terminal buffer. Every other DataHub process records what it did to data_hub_process_summary, so this one does too, and moving it out of lib/ into the DataHub namespace is what makes it a sibling of the deduplication and backfill processes rather than a stray script. short_summary holds the counts - groups, surplus site rows, surplus provider_school rows, and the same split by kind and by flag. Tallies rather than a field per kind, so the Kind list stays the single source of truth. full_summary holds every group and every site behind those counts, which is 88KB for 2026 and means the CSV can be rebuilt without running against production again. Kinds move to a file each, so the inherited hook that collected them no longer fires - Zeitwerk would not have loaded a subclass before Kind.for asked for it. An explicit ORDER replaces it, which is better anyway: precedence is now declared rather than an accident of load order. The run still changes nothing. The only row it writes is its own summary, and jsonb_accessor keys are real model attributes, so the new fields are blocklisted from BigQuery alongside every sibling summary's.
Contributor
Author
|
Closing because I was only demonstrating the kinds and groups of the issue to the tech lead |
|
Review app for PR 6318 was deleted |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tell the duplicated schools apart, and keep what was found
Some providers hold the same school twice. The query we started from grouped
siterows by provider and urn but filtered
code <> '-', which found 9 groups for 2026 andhid the shape behind the other 95.
This adds a read-only classification run that records what it found as a DataHub
process summary. It decides nothing and merges nothing — picking a merge policy is the
next piece of work, and this is the evidence it gets picked from.
What it found for 2026
104 groups · 119 surplus
siterows · 100 surplusprovider_schoolrowsThe
provider_schoolnumber is the one users feel: the schools list, both course schoolpickers and the new search panel all read
Provider::Schoolnow, so each surplus row isa school listed twice.
The four kinds
Each shape is a
Kindsubclass that declares how to recognise itself, what it means, andwhat to do about it. Matching order is precedence —
MainSiteCollisionis tested first,DivergentNameTwinis the terminal fallback.main_site_collision— 95 groups, 95 surplus rowsMatches a group containing the
-row.What it is: the provider's main site and the same school added again as a placement
school.
BackfillMainSiteUrnsgave all 494 main sites a urn, andProviderSchools::Creator#normal_provider_schoolexcludessite_code: '-'when checkingfor an existing row, so re-adding a school you already hold as your main site always makes
a second one. The provider sees
Foo School (Main Site)andFoo Schoolside by side.Action: decide the policy — keep
-and move courses onto it, or keep the named row.Flags:
main_site_at_risk35,courses_on_both_sides50.main_site_at_riskis the reason not to point the existingsites:deduplicatetask atthese:
pick_primary_siteranks by[site_statuses.count, -id], so in 35 of the 95 itwould discard the provider's main site — and it never touches
provider_school/course_school, so it would remove nothing a user can see.clone— 4 groups, 11 surplus rowsMatches one code and one name across the group.
What it is: the same school added repeatedly under one code, from a bulk add that ran
about ten times for one provider on 2025-09-24 (492 sites in a minute). Invisible to
providers —
provider_school's unique(provider_id, gias_school_id, site_code)indexalready collapsed them, so this is legacy litter only.
Action: safe to merge unattended — no user-visible duplicate to remove.
Flags:
gias_closed1.divergent_name_twin— 4 groups, 4 surplus rowsMatches more than one code and more than one name, no
-row.What it is: one urn under two provider-written names. Often deliberate labelling rather
than an accident — 13S holds urn
136906as bothGeorge Abbot School(93 courses) andMain site Secondary- one of our partner schools(88).Action: ask the provider which name they meant before merging.
Flags:
courses_on_both_sides4,provider_authored_name1.split_code_twin— 1 group, 9 surplus rowsMatches more than one code, one name, no
-row.What it is: one school held under two codes, so it is listed twice and its courses are
split between them. Same bulk-add cause as
clone, exceptSites::CodeGeneratorpickscodes randomly, so a re-run produced a different code. 1TZ holds urn
144834asDK(7 courses) and
DN(11).Action: merge onto the code holding more courses, moving the rest across.
Flags:
courses_on_both_sides1.What gets recorded
short_summary— years, groups, surplus site rows, surplusprovider_schoolrows, andthe same counts split by kind and by flag. Tallies rather than a field per kind, so the
Kindlist stays the single source of truth and a fifth shape needs no migration.full_summary— every group and every site behind those counts (88KB for 2026), so theCSV can be rebuilt without running against production again.
Notes for review
executor spec (
not_changeon bothSiteandProvider::School), not just by eye.inheritedhook that used to collect them no longerfires — Zeitwerk would not have loaded a subclass before
Kind.forasked for it. Anexplicit
ORDERreplaces it, which also makes precedence declared rather than anaccident of load order.
jsonb_accessorkeys are real model attributes, so the apprefuses to boot until the new fields are declared. Added by hand rather than with
dfe:analytics:regenerate_blocklist— that task runs withSUPPRESS_DFE_ANALYTICS_INIT=1,so
Provider::SchoolandCourse::Schoolare never autoloaded and it removes theirexisting entries. They are in neither config file, so accepting that would break boot.
The diff is 7 added lines;
rails dfe:analytics:checkpasses.means 2027 is created clean — on a rolled-over dev copy, 95 of the 100 duplicate pairs
come straight across.
Next
Read the recorded summary, choose the
main_site_collisionpolicy, then write the mergeprocess — which must move
provider_school/course_schoolrows, not justsite/course_site, and should record its own summary the same way.