Entiy Org Csv now looks for overlap and flags in reponse - #124
Merged
pooleycodes merged 3 commits intoJul 13, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesEntity organisation overlap detection
Estimated code review effort: 2 (Simple) | ~10 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
pooleycodes
marked this pull request as ready for review
July 9, 2026 14:28
Merged
6 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
request-processor/tests/unit/src/application/core/test_pipeline.py (1)
464-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test cases for edge scenarios not covered by the current suite.
The four tests cover the primary paths well. Consider adding tests for:
- Partial overlap: some entities within an existing range, some outside — documents the
all()semantics.- Non-numeric entity values: entity values like
""or"abc"— would catch the unhandledValueErrorfromint().- Multiple datasets in CSV: verifies the dataset filter at line 491.
- Malformed CSV rows: missing or non-numeric
entity-minimum/entity-maximum— verifies thecontinueat line 497.🧪 Suggested additional test cases
def test_create_entity_organisation_partial_overlap_does_not_flag(tmp_path): """Entities split across different ranges should not flag overlap""" pipeline_dir = tmp_path / "pipeline" pipeline_dir.mkdir() (pipeline_dir / "entity-organisation.csv").write_text( "dataset,entity-minimum,entity-maximum,organisation\n" "nature-improvement-area,10100000,10100011,government-organisation:PB202\n" "nature-improvement-area,10200000,10200011,government-organisation:PB202\n" ) new_entities = [{"entity": "10100005"}, {"entity": "10200005"}] result = _create_entity_organisation( new_entities, "nature-improvement-area", "government-organisation:PB202", str(pipeline_dir), ) assert result[0]["overlap"] is False assert result[0]["entity-minimum"] == 10100005 assert result[0]["entity-maximum"] == 10200005 def test_create_entity_organisation_filters_other_datasets(tmp_path): """Rows for other datasets should be ignored""" pipeline_dir = tmp_path / "pipeline" pipeline_dir.mkdir() (pipeline_dir / "entity-organisation.csv").write_text( "dataset,entity-minimum,entity-maximum,organisation\n" "other-dataset,10100000,10100011,government-organisation:PB202\n" ) new_entities = [{"entity": "10100005"}] result = _create_entity_organisation( new_entities, "nature-improvement-area", "government-organisation:PB202", str(pipeline_dir), ) assert result[0]["overlap"] is False def test_create_entity_organisation_skips_malformed_csv_rows(tmp_path): """Rows with non-numeric min/max should be skipped, not crash""" pipeline_dir = tmp_path / "pipeline" pipeline_dir.mkdir() (pipeline_dir / "entity-organisation.csv").write_text( "dataset,entity-minimum,entity-maximum,organisation\n" "nature-improvement-area,N/A,N/A,government-organisation:PB202\n" "nature-improvement-area,10100000,10100011,government-organisation:PB202\n" ) new_entities = [{"entity": "10100005"}] result = _create_entity_organisation( new_entities, "nature-improvement-area", "government-organisation:PB202", str(pipeline_dir), ) assert result[0]["overlap"] is True🤖 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 `@request-processor/tests/unit/src/application/core/test_pipeline.py` around lines 464 - 546, Add edge-case tests for `_create_entity_organisation` to cover behavior not exercised by the current suite. Include a partial-overlap case to confirm the `all()` overlap logic, a non-numeric entity value case to document handling around `int()` conversion, a CSV with multiple datasets to verify the dataset filter, and malformed CSV rows to confirm bad `entity-minimum`/`entity-maximum` values are skipped via the existing parsing path. Use the existing `_create_entity_organisation` helper and `entity-organisation.csv` test setup pattern so the new cases are easy to locate.
🤖 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.
Inline comments:
In `@request-processor/src/application/core/pipeline.py`:
- Around line 469-473: The entity value extraction in the pipeline currently
only filters out None, so int() can still raise ValueError for empty or
non-numeric strings. Update the entity_values comprehension in the pipeline
logic to defensively handle invalid entity fields the same way the CSV parsing
path does, by guarding the int() conversion with try/except for TypeError and
ValueError or by filtering out non-numeric inputs before conversion. Use the
existing new_entities processing block and the surrounding CSV row parsing logic
as the reference points.
- Around line 488-500: The overlap check in the pipeline currently only sets
overlap when all new entity values fit inside a single existing row, so a batch
spanning multiple existing ranges can still pass. Update the overlap logic in
the range handling block to compare the proposed entity range against each
existing row in existing_rows for the same dataset, or test each entity value
against all rows, and only allow the range when none of the existing
[entity-minimum, entity-maximum] intervals intersect it.
---
Nitpick comments:
In `@request-processor/tests/unit/src/application/core/test_pipeline.py`:
- Around line 464-546: Add edge-case tests for `_create_entity_organisation` to
cover behavior not exercised by the current suite. Include a partial-overlap
case to confirm the `all()` overlap logic, a non-numeric entity value case to
document handling around `int()` conversion, a CSV with multiple datasets to
verify the dataset filter, and malformed CSV rows to confirm bad
`entity-minimum`/`entity-maximum` values are skipped via the existing parsing
path. Use the existing `_create_entity_organisation` helper and
`entity-organisation.csv` test setup pattern so the new cases are easy to
locate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fef438c5-392c-437b-a8f0-413741f3ab7a
📒 Files selected for processing (2)
request-processor/src/application/core/pipeline.pyrequest-processor/tests/unit/src/application/core/test_pipeline.py
Keep main's refactored _transform_add_data_resource loop and pass pipeline_dir through to the branch's overlap-aware _create_entity_organisation.
gibahjoe
approved these changes
Jul 13, 2026
pooleycodes
deleted the
2765-for-single-source-datasets-allow-entity-organisation-to-be-entire-entity-range
branch
July 13, 2026 14:41
8 tasks
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
2 participants
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.
What type of PR is this? (check all applicable)
Description
When new entities are created during add-data processing,
_create_entity_organisationnow checks the downloadedentity-organisation.csvto see if the new entities already fall within an existing dataset range:overlap: trueis set if all new entities already fall within an existing entity-minimum/entity-maximum range for the dataset (single source dataset case).error: trueis set ifentity-organisation.csvcan't be loaded, without failing the request.true,entity-minimum/entity-maximumare omitted from the response entry so a downstream consumer (e.g. the config repo's add-data GitHub Action) can't commit an untrustworthy range toentity-organisation.csv.Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Run the request-processor unit tests:
Covers: overlap detected, no overlap, missing/unreadable CSV (error flagged), and empty new-entities cases.
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
None
[optional] Are there any dependencies on other PRs or Work?
The config-manager preview UI and config repo's
add_data.pywere updated separately to consume the newoverlap/errorfields.Summary by CodeRabbit
Bug Fixes
Tests