feat(spp_demo): curated PHL geodata + spp_demo_phl_luzon demo module (re-land from #76)#277
feat(spp_demo): curated PHL geodata + spp_demo_phl_luzon demo module (re-land from #76)#277gonzalesedwin1123 wants to merge 7 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a geodata preparation script (prepare_phl_geodata.py) and updates the spp_demo module with refreshed Philippine geodata using PSA/HDX p-codes. It also introduces a new companion module, spp_demo_phl_luzon, which provides Luzon-scale demo areas, population weights, and an area loader. The review feedback highlights three key improvements: optimizing an N+1 query pattern in the Luzon area loader's shape-loading method, wrapping the streaming HTTP request in a context manager to prevent connection leaks, and updating the script's documented dependencies to include pandas and openpyxl so that population data processing does not silently fail.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… module (from #76) Re-lands the PHL demo data portion of reverted PR #76: curated PHL geojson shapes and areas, demo data generator and area loader test updates, the prepare_phl_geodata.py preparation script, and the new spp_demo_phl_luzon module (Luzon demo areas + population weights). Files restored verbatim from the pre-revert merged state (8bf9a3a). spp_demo bumped to 19.0.2.1.0 with a HISTORY entry.
gonzalesedwin1123
force-pushed
the
reland/phl-demo
branch
from
32a47c5 to
537b32e
Compare
- area_loader: batch-fetch areas by code instead of per-feature search (N+1 over ~700 Luzon features). - prepare_phl_geodata.py: close streamed download via context manager; usage docs include pandas/openpyxl needed for population weights.
|
All three gemini-code-assist findings applied (commit 91c9e06): batched area lookup replacing the per-feature N+1 search, streamed download wrapped in a context manager, and usage docs now include pandas/openpyxl. |
…initial HISTORY readme/DESCRIPTION.md is the repo-standard fragment location (the generator builds README.rst from it); static/description/DESCRIPTION.md was never picked up. README will be aligned to CI's renderer output in a follow-up commit.
Fragments are plain bullet lists (no RST tables), so local rendering matches CI's pinned generator; CI's generated-files check will confirm.
Code review: PR #277 — curated PHL geodata +
|
|
The This PR renames the PHL area external IDs to curated PSGC p-codes, but Merge order: #295 depends on this PR (and #280), so it must merge after both and be rebased onto the result. |
…oaders - test_population_weights: patch file_path at its imported location (odoo.addons.spp_demo_phl_luzon.models.population_weights.file_path) so the stub actually takes effect instead of being a no-op. - area_loader._load_shapes: hoist the shapely import above the feature loop and bail early on ImportError, avoiding one swallowed ImportError (and log warning) per feature when shapely is absent. - population_weights.get_weights: extract pcode inside the try/except so a malformed row skips gracefully instead of aborting the whole load.
kneckinator
left a comment
kneckinator
left a comment
There was a problem hiding this comment.
Code review
Reviewed the ~1,000 lines of hand-written Python (the bulk of the diff is generated GeoJSON/XML data). Overall this is a clean re-land — the _link_existing_areas pre-link strategy neatly solves the overlapping-code problem, XXE hardening on the lxml parse is a nice touch, the N+1 fixes are correctly applied, and test coverage is genuinely good (idempotency, overlap, caching, invalid-row skipping).
Four findings below. 1–3 are now addressed in 464d709 (pushed to this branch); #4 is informational.
1. Ineffective mock patch in test_population_weights.py — ✅ fixed in 464d709
population_weights.py does from odoo.tools.misc import file_path and calls the bare name, so patching odoo.tools.misc.file_path did not rebind the imported reference — the stub was a no-op and the tests only passed because open was mocked and the real CSV path happened to exist. Repointed the patch to odoo.addons.spp_demo_phl_luzon.models.population_weights.file_path.
2. shapely import inside the per-feature loop (area_loader.py::_load_shapes) — ✅ fixed in 464d709
The from shapely.geometry import shape was inside the loop. If shapely is genuinely missing, the ImportError fires once per feature (~700×) and is swallowed by the per-feature except, flooding the log. Hoisted the import above the loop with an early return 0 on ImportError. (No manifest change needed: shapes only load when geo_polygon exists, which requires spp_gis, and spp_gis already declares shapely.)
3. Unwrapped row["pcode"] in get_weights (population_weights.py) — ✅ fixed in 464d709
A missing/renamed pcode column raised an uncaught KeyError that aborted the whole load, whereas a bad population value was skipped gracefully — an asymmetry next to the defensive handling. Moved pcode extraction inside the try (added AttributeError for good measure) so a malformed row skips gracefully.
4. Two full parses of areas_luzon.xml per load (informational — not changed)
_link_existing_areas parses the XML with lxml to extract (xml_id, code) pairs, then convert_file parses it again. Acceptable for a one-shot demo loader; noting it only for completeness, no change recommended.
Risk assessment
- Correctness: the programmatic-load design is the sharp edge and it's handled well and tested (overlap + idempotency). No blocking issues.
- Security: XXE mitigated; the transient loader's
create/writegrant tobase.group_useronly touches bundledspp.areacodes — acceptable for demo tooling. - Performance: N+1s already addressed.
Verdict: Approve. Findings were minor; 1–3 are fixed. Heads-up per the PR body — merged #278's misluzon profile soft-depends on this, so ./spp start --demo misluzon stays broken until this lands.
Note: local pre-commit couldn't run the semgrep hook (Python 3.14 / protobuf incompatibility in the cached env); all other hooks pass. CI's semgrep will cover it.
2 participants
Re-lands the PHL demo-data portion of reverted PR #76 (revert: #271). Everything listed is contained in THIS PR's diff.
Summary
spp_demo: curated PHL shapes/areas (phl_curated.geojson,areas.xml) and demo generator updates.spp_demo_phl_luzonmodule: Luzon administrative areas (regions/provinces/municipalities asspp.arearecords with polygon shapes), population weights, area loader (depends on spp_demo's loader changes).scripts/prepare_phl_geodata.py: the script that generates the curated geodata.Added on top of #76 (not in the original)
spp_demo_phl_luzon's description moved from the non-standardstatic/description/DESCRIPTION.mdtoreadme/DESCRIPTION.md, initialreadme/HISTORY.mdadded, and the module's first generatedREADME.rst/index.htmlcommitted (accepted by CI's generator).Good to know
sppCLI'smisluzondemo profile referencesspp_demo_phl_luzon, so./spp start --demo misluzonerrors until this merges.Verification
./spp t spp_demo: 121 passed, 0 failed./spp t spp_demo_phl_luzon: 17 passed, 0 failed