feat: geofence-based geographic targeting for programs (re-land from #76)#273
Open
gonzalesedwin1123 wants to merge 10 commits into
Open
feat: geofence-based geographic targeting for programs (re-land from #76)#273gonzalesedwin1123 wants to merge 10 commits into
gonzalesedwin1123 wants to merge 10 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 introduces geofence-based geographic targeting for OpenSPP programs by adding the new spp_program_geofence module and extending spp_gis to support complex geometry types like MultiPolygon and GeometryCollection. It also replaces vocabulary-based geofence tags with a dedicated spp.gis.geofence.tag model, complete with migration scripts, and implements an OpenStreetMap fallback when no MapTiler API key is configured. The review feedback focuses on improving performance and code quality in the new Odoo models and JS widgets. Key recommendations include avoiding map re-creation on every patch in the GIS edit widget, optimizing recordset operations (using subtraction instead of lambda filtering and avoiding heavy NOT IN clauses in search domains), checking for active records in asynchronous tasks, and wrapping user-facing strings in _() for internationalization.
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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…tion (from #76) Re-lands the PR #76 description scope: spp_gis complex-geometry operators (MultiPolygon/GeometryCollection + distance buffering), OSM fallback when no MapTiler key is configured, renderer/edit-widget lifecycle fixes, geofence uuid in GeoJSON output, the spp.gis.geofence.tag model, and the new spp_program_geofence module (geofence eligibility manager, program UI, creation wizard). Adds what the original PR lacked: a migration pair remapping existing vocabulary-based geofence tag links (release v19.0.2.0.0 schema) onto spp.gis.geofence.tag records — the rel table is reused, so legacy rows are parked pre-upgrade and restored post-upgrade. Includes regression tests.
…generate spp_gis README Semgrep flagged f-string SQL in the tag migration scripts; identifiers were constants but composed SQL via psycopg2.sql removes the pattern entirely. README.rst regenerated from the updated HISTORY fragment (in-scope module only).
gonzalesedwin1123
force-pushed
the
reland/gis-geofence
branch
from
835401d to
9963347
Compare
Replaces psycopg2.sql identifier composition with prebuilt literal queries (the aux table name and the valid-link filter are compile-time constants), clearing the odoo-sql-injection-string-format code-scanning alerts. Values remain parameterized; behavior unchanged (spp_gis suite 92/0).
Every module carries a readme/HISTORY.md; the module was created in #76 without one. README.rst History section will be aligned to CI's renderer output in a follow-up commit.
…r-facing strings in _() - README.rst History section applied verbatim from CI's pre-commit diff. - Address gemini-code-assist review: translatable message_post/lock-reason and preview notification strings (i18n).
…back DESCRIPTION.md predates the geofence move (#74) and did not mention the capabilities this PR adds. README will be aligned to CI's renderer output in a follow-up commit.
Member
Author
|
Disposition of gemini-code-assist findings: Applied (commit 9d9afdb): i18n Deferred as follow-ups (pre-existing #76 code; this PR re-lands reviewed behavior and we're keeping the diff scoped):
|
… ladder The module's fragments use #/## headings; ### produced an RST title-level skip that crashed oca-gen-addon-readme.
…fence READMEs regenerated by CI's pinned generator from the updated DESCRIPTION and new HISTORY fragments; applied verbatim from its pre-commit diff.
Member
Author
Review & merge order for the PR #76 re-land stackThis PR is part of a 10-PR stack re-landing reverted #76 (revert: #271) in description-consistent pieces. Reviews within a batch can run in parallel; order only matters at merge time. All PR descriptions are self-contained. Batch 1 — review now, merge in any order (independent, CI green)
Batch 2 — merge strictly after prerequisites
Cautions
Verification summary per PR is in each description; the deferred gemini-code-assist suggestions are documented as comments on the affected PRs. |
…and MapTiler-key paths (codecov patch target)
- eligibility_manager: guard the queued import against archived/deleted programs (with regression test); drop the NOT-IN anti-pattern from the tier-2 domain (tier1 | tier2 already dedups); recordset subtraction for already-enrolled exclusion; iterate ids when building membership values. - gis_edit_map widget: only re-create the MapLibre map when the geometry value actually changed (prevents zoom/pan loss and WebGL context churn on unrelated OWL patches).
Member
Author
|
Update: the four previously-deferred gemini-code-assist findings are now applied as well (per maintainer request):
Suites: spp_program_geofence 41/0, spp_gis 96/0. All gemini findings on this PR are now addressed. |
Contributor
Code Review — PR #273: geofence-based geographic targeting for programsReviewed the full Overview
Downstream impact (in-repo) — no breakage foundTwo in-repo modules
Correctness
Things to verify manually (not confirmed defects)
TestsStrong coverage: operator MultiPolygon/GeometryCollection + buffer + shapely-object paths; MapTiler controller (configured/placeholder/absent); migration remap/dedup/valid-survival/fresh-DB no-op; eligibility tiers, target-type, disabled exclusion, multi-geofence union, enroll/import/dedup, async threshold, cycle verification, preview success/error, archived-program skip; officer-role read/create; wizard geofence-vs-default manager selection. Minor test smells (non-blocking): Security
Conventions
VerdictClean, well-tested PR with no confirmed defects and no in-repo downstream breakage. The one substantive item worth a maintainer decision is the |
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
3 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.
Re-lands the scope described in reverted PR #76 (revert: #271), as part of splitting that PR into description-consistent pieces. This description incorporates the relevant content of #76 so reviewers do not need to open it; everything listed below is contained in THIS PR's diff.
Summary
spp_program_geofencemodule for geofence-based program targeting and eligibility management.spp_gisspatial operators to supportMultiPolygonandGeometryCollection.YOUR_MAPTILER_API_KEY_HEREtreated as unconfigured).Functional detail (from #76, verified against this diff)
spp_program_geofence(new module)geofence_idsandgeofence_count.spp.program.membership.manager.geofence) with:groupsrestriction avoids exposing items to users lacking read access).spp_gisST_GeomFromGeoJSON; forMultiPolygon/GeometryCollection,(geojson, distance)appliesST_Buffer(...)correctly (EPSG:3857 transform parity when SRID is 4326), with regression tests for the distance-buffer path on both complex types.uuidas feature id; newspp.gis.geofence.tagmodel replaces vocabulary-based geofence tags (+ACLs).Added on top of #76 (not in the original)
spp_gis 19.0.2.1.0): geofencetag_idschanges co-model fromspp.vocabularytospp.gis.geofence.tagover the same rel table. Release v19.0.2.0.0 (Biliran) ships the old schema, so a pre-migration parks legacy links (the FK swap would otherwise fail) and a post-migration recreates them as tag records. Fully literal, value-parameterized SQL (clears Semgrep/pylint-odoo SQL findings; GHAS alerts #1837–#1842 fixed). Regression tests: remap, dedup across geofences, valid-link survival, fresh-DB no-op._(); the map edit widget re-creates the MapLibre map only when the geometry value actually changed (prevents zoom/pan loss and WebGL context churn — a quick manual map-edit smoke test during review is recommended).action_open_geofences, and the MapTiler key controller (configured/placeholder/absent key).spp_gisDESCRIPTION now describes geofences, complex-geometry queries, and the OSM fallback;spp_program_geofencegains an initial HISTORY fragment; version bump + HISTORY entry forspp_gis; READMEs rendered by CI's pinned generator.Not in this PR (other pieces of #76, re-landed separately)
spp_hazard CAP severity (#274), spp_cel_domain (#275), spp_api_v2 (#276), PHL demo data (#277), infra/tooling (#278, merged), spp_metric_service (#279), spp_gis_report (#280), spp_api_v2_gis (#281), spp_mis_demo_v2 (#282).
Verification
./spp t spp_gis: 96 passed, 0 failed (includes 4 migration tests + 4 MapTiler controller tests)./spp t spp_program_geofence: 41 passed, 0 failed (includes archived-program regression test)