feat: handle multiple orgaisations per resource#574
Conversation
WalkthroughPipeline.transform now runs once per organisation pass, writes per-pass outputs, then concatenates them into the final CSV and deduplicates related logs. Phase construction is split into a pure builder. Integration tests cover helper behaviour and single-, zero-, and multi-organisation runs. ChangesMulti-org transform orchestration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/pipeline/test_transform_multi_org.py (2)
197-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the "(Option C)" reference.
The comment refers to a design option not documented anywhere in this file, making the rationale opaque to future readers.
✏️ Suggested clarification
- # the entry-number -> entity map is reset between/after passes (Option C) + # the entry-number -> entity map is reset after each pass so entity + # attribution stays unambiguous for the next organisation's pass assert pipeline.issue_log.entry_to_entity == {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/pipeline/test_transform_multi_org.py` around lines 197 - 198, Clarify the ambiguous “(Option C)” note in the test by removing the undocumented option label or replacing it with a concrete description of the behavior being asserted. Update the nearby comment around the pipeline issue log reset so it explains, in plain terms, that issue_log.entry_to_entity is expected to be empty after each pass and why that matters, without referencing any external design option.
86-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding coverage for the
combine_fieldsgating branch.
_run_transformnever exercises the case wherecombine_fields != {}, which skipsduplicate_reference_checkperPipeline.transform(main.py:621-749). Current tests only implicitly cover the default (empty) path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/pipeline/test_transform_multi_org.py` around lines 86 - 133, The current multi-org transform helper only covers the empty combine_fields path, so add a test case through _run_transform/Pipeline.transform that passes a non-empty combine_fields value and verifies the combine_fields gating branch is taken. In that scenario, assert duplicate_reference_check is not invoked while the rest of the transform orchestration still runs, using the existing fake _build_transform_phases and transform() setup to keep the coverage focused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/pipeline/test_transform_multi_org.py`:
- Around line 197-198: Clarify the ambiguous “(Option C)” note in the test by
removing the undocumented option label or replacing it with a concrete
description of the behavior being asserted. Update the nearby comment around the
pipeline issue log reset so it explains, in plain terms, that
issue_log.entry_to_entity is expected to be empty after each pass and why that
matters, without referencing any external design option.
- Around line 86-133: The current multi-org transform helper only covers the
empty combine_fields path, so add a test case through
_run_transform/Pipeline.transform that passes a non-empty combine_fields value
and verifies the combine_fields gating branch is taken. In that scenario, assert
duplicate_reference_check is not invoked while the rest of the transform
orchestration still runs, using the existing fake _build_transform_phases and
transform() setup to keep the coverage focused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce99e12e-024c-4f7c-a996-1a0bb22b93fe
📒 Files selected for processing (2)
digital_land/pipeline/main.pytests/integration/pipeline/test_transform_multi_org.py
ea47e10 to
24960f0
Compare
24960f0 to
ddbb428
Compare
What type of PR is this? (check all applicable)
Description
Changing the way the pipeline generates entities from a resource in a situation where multiple endpoints from different organisations provide identical data and therefore produce the same resource.
Before - No entities are produced from resources with multiple orgs.
After - now generate entities per organisation per resource.
Why
Because we are currently missing entities in our data and this is causing a bunch of problems particularly for local-plan data.
How the bug works
A resource is identified only by the hash of its contents. When two organisations publish the same URL, they produce identical bytes, so the collection merges them into a single resource that carries both organisation codes.
To generate entities, the pipeline stamps a default organisation onto every row of a resource (the data files themselves don't say which authority they belong to). But it only does this when there is exactly one organisation:
Worked example — South & Vale Joint Local Plan 2045
South Oxfordshire and Vale of White Horse jointly publish one local plan timetable at a single URL:
https://www.southandvale.gov.uk/app/uploads/2026/06/Local-Plan-Timetable-CSV.csvIn
source.csvthat one endpoint is attributed to both authorities:local-authority:SOXlocal-authority:VALlookup.csvalready defines separate entities for each authority against the same references, for example:Joint-Local-Plan-2045-scoping-consultation-startsouth-and-vale-joint-local-plan-2045Before: the resource carries two organisations, the guard is skipped, and neither authority's entities are created — both show zero records supplied.
After: the resource is processed once per organisation (each set as the default in turn), both lookups resolve, and both authorities get their entities.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Downstream impacts of this change.
Processing a resource once per organisation means a single resource now produces rows for more than one organisation. I looked at every step that runs after transform to check nothing relies on the old "one organisation per resource" assumption.
Handled by this change
Duplicate-reference check. This flags a reference appearing more than once in a resource. With two organisations, the same reference legitimately appears for each (e.g. the same joint-plan milestone), which would be falsely flagged as "not unique". We now run this check per organisation pass, where references are genuinely expected to be unique, so no false positives are introduced.
Doubled logs. The org-independent side-logs (column-field, converted-resource) would otherwise be written once per pass. We write the converted/harmonised artefacts on the first pass only and de-duplicate those log rows, so a multi-organisation resource produces the same single set of log rows as any other.
Issue → entity attribution. Issues are logged before the entity is known and back-filled afterwards. We apply and reset that mapping per pass so each organisation's issues attach to that organisation's entities rather than the other's.
Not affected (checked, no change needed)
Dataset / entity assembly. Entities are assembled by grouping on the entity number, not on the resource. Each organisation resolves to its own entity, so the two organisations' rows become two distinct entities — exactly as if they'd come from separate resources. The organisation is carried through as a normal field.
Priority. Priority is decided per entity from that entity's authoritative organisation. Each organisation owns its own entity, so both resolve correctly.
Task generation. Tasks are keyed by dataset/resource/field/issue-type/organisation and already expand a resource's organisations into one task per organisation. They take the organisation from the resource, not from anything this change touches, so they are unaffected.
Single / no-organisation resources. The vast majority of resources have one organisation (or none). Those follow the same path as before — this change only alters behaviour when a resource has two or more organisations.
Testing in Staging
I deployed this to staging and ran the
local-plancollection through to the plan-timetable Postgres load. Before this change the South & Vale Joint Local Plan produced no entities; after it, querying the stagingentitytable returns all 12 plan-timetable milestones twice — once for South Oxfordshire and once for Vale of White Horse — with the exact entity numbers defined inlookup.csv(e.g.Joint-Local-Plan-2045-scoping-consultation-start→ 5110200 for South Oxfordshire and 5110188 for Vale of White Horse). That confirms both authorities now get their own entities from the single shared resource, end to end through transform, dataset build and the Postgres loader.Why this is safer than it first looks
Changing how resources map to organisations sounds like it touches a core assumption, but the pipeline is already entity-centric below the resource level — facts, dataset assembly and tasks are all keyed on the entity, not the resource. So "process a resource once per organisation" fits the existing model rather than fighting it: each organisation resolves to its own entity, and from that point on everything treats the two organisations exactly as if they'd come from separate resources.
The load-bearing detail is that a fact's identity is a hash of
entity : field : value, so two organisations producing identical rows still create distinct facts (different entities → different hashes). That's why nothing collapses or overwrites. I also confirmed the resource-level logs are set, not accumulated, so nothing double-counts.The change is also naturally bounded: resources with one organisation (or none) follow exactly the same path as before — only resources with two or more organisations hit the new behaviour. Which is a relatively small subset of the total data.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above. Please refer to the Digital Land Testing Guidance for more information.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
New Features
Bug Fixes