feat: geofence-based geographic targeting for programs (re-land from #76)#273
feat: geofence-based geographic targeting for programs (re-land from #76)#273gonzalesedwin1123 wants to merge 2 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.
| def _import_registrants(self, new_beneficiaries, state="draft", do_count=False): | ||
| _logger.info("spp_program_geofence: Importing %s beneficiaries", len(new_beneficiaries)) | ||
| self.env["spp.program.membership"].create( | ||
| [{"program_id": self.program_id.id, "partner_id": b.id, "state": state} for b in new_beneficiaries] | ||
| ) |
There was a problem hiding this comment.
In Odoo, when validating a Many2one relation in asynchronous or background tasks (such as _import_registrants which is queued via delayable), you should explicitly check if the referenced record is active (e.g., not self.program_id.active) in addition to checking its existence. A Many2one field still dereferences a deactivated record as a truthy recordset, which can bypass simple existence checks if the record is deactivated after the task is queued.
Additionally, iterating over new_beneficiaries.ids in the list comprehension is significantly more efficient than iterating over the new_beneficiaries recordset directly, as it avoids instantiating singleton record objects for each iteration.
| def _import_registrants(self, new_beneficiaries, state="draft", do_count=False): | |
| _logger.info("spp_program_geofence: Importing %s beneficiaries", len(new_beneficiaries)) | |
| self.env["spp.program.membership"].create( | |
| [{"program_id": self.program_id.id, "partner_id": b.id, "state": state} for b in new_beneficiaries] | |
| ) | |
| def _import_registrants(self, new_beneficiaries, state="draft", do_count=False): | |
| self.ensure_one() | |
| if not self.program_id or not self.program_id.active: | |
| _logger.warning("spp_program_geofence: Program is inactive or deleted, skipping import.") | |
| return | |
| _logger.info("spp_program_geofence: Importing %s beneficiaries", len(new_beneficiaries)) | |
| self.env["spp.program.membership"].create( | |
| [{"program_id": self.program_id.id, "partner_id": partner_id, "state": state} for partner_id in new_beneficiaries.ids] | |
| ) |
References
- In Odoo, when validating a Many2one relation in asynchronous or background tasks, explicitly check if the referenced record is active (e.g.,
not record.active) in addition to checking its existence. A Many2one field still dereferences a deactivated record as a truthy recordset, which can bypass simple existence checks if the record is deactivated after the task is queued.
| onPatched(() => { | ||
| this.defaultZoom = this.map.getZoom(); | ||
| this.renderMap(); | ||
| this.onLoadMap(); | ||
| this.addDrawInteraction(); | ||
| }); |
There was a problem hiding this comment.
Re-creating the entire MapLibre map instance on every patch/update is extremely inefficient. It causes map flickering, resets the map state (such as user zoom/pan), and can rapidly exhaust the browser's WebGL context limit, leading to context loss errors.
Instead, you should check if the geometry value has actually changed, and only re-render the map or update the draw features when a real change occurs. (Note: Remember to also initialize this._lastValue = this.props.record.data[this.props.name]; in onMounted to prevent unnecessary re-renders on the first patch).
onPatched(() => {
const currentValue = this.props.record.data[this.props.name];
if (currentValue !== this._lastValue) {
this._lastValue = currentValue;
this.defaultZoom = this.map.getZoom();
this.renderMap();
this.addDrawInteraction();
}
});| tier2_domain = base_domain + [ | ||
| ("area_id", "in", matching_areas.ids), | ||
| ("id", "not in", tier1.ids), | ||
| ] |
There was a problem hiding this comment.
Using ("id", "not in", tier1.ids) in the search domain is a performance anti-pattern in Odoo when tier1 contains a large number of records. This generates a heavy SQL query with a massive NOT IN clause.
Since the method returns tier1 | tier2 at the end, and Odoo's recordset union operator (|) automatically handles deduplication in Python extremely efficiently, you can safely remove the not in filter from the database query.
tier2_domain = base_domain + [
("area_id", "in", matching_areas.ids),
]| existing_partner_ids = set(self.program_id.get_beneficiaries().mapped("partner_id").ids) | ||
| new_beneficiaries = new_beneficiaries.filtered(lambda r: r.id not in existing_partner_ids) |
There was a problem hiding this comment.
Filtering a recordset using a Python lambda with not in on a set of IDs is less efficient and less idiomatic than using Odoo's built-in recordset subtraction operator (-).
You can simplify this to a single subtraction operation, which is faster and cleaner.
| existing_partner_ids = set(self.program_id.get_beneficiaries().mapped("partner_id").ids) | |
| new_beneficiaries = new_beneficiaries.filtered(lambda r: r.id not in existing_partner_ids) | |
| existing_partners = self.program_id.get_beneficiaries().mapped("partner_id") | |
| new_beneficiaries -= existing_partners |
| program.message_post(body=f"Import of {len(new_beneficiaries)} beneficiaries started.") | ||
| program.write({"is_locked": True, "locked_reason": "Importing beneficiaries"}) |
There was a problem hiding this comment.
These user-facing log messages and lock reasons are hardcoded strings and should be wrapped in _() to support internationalization (i18n).
| program.message_post(body=f"Import of {len(new_beneficiaries)} beneficiaries started.") | |
| program.write({"is_locked": True, "locked_reason": "Importing beneficiaries"}) | |
| program.message_post(body=_("Import of %s beneficiaries started.") % len(new_beneficiaries)) | |
| program.write({"is_locked": True, "locked_reason": _("Importing beneficiaries")}) |
| _logger.exception("Geofence eligibility preview failed for manager %s", self.id) | ||
| self.preview_count = 0 | ||
| self.preview_error = "Preview failed. Check the server logs for details." | ||
| return { | ||
| "type": "ir.actions.client", | ||
| "tag": "display_notification", | ||
| "params": { | ||
| "title": "Preview Complete", | ||
| "message": f"{self.preview_count} registrants match the current geofences.", | ||
| "sticky": False, | ||
| "type": "success" if not self.preview_error else "warning", |
There was a problem hiding this comment.
These user-facing notification titles, messages, and preview errors are hardcoded strings and should be wrapped in _() to support internationalization (i18n).
self.preview_error = _("Preview failed. Check the server logs for details.")
return {
"type": "ir.actions.client",
"tag": "display_notification",
"params": {
"title": _("Preview Complete"),
"message": _("%s registrants match the current geofences.") % self.preview_count,
"sticky": False,
"type": "success" if not self.preview_error else "warning",
},
}![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| Tag = env["spp.gis.geofence.tag"] | ||
| aux = sql.Identifier(AUX_TABLE) | ||
|
|
||
| cr.execute(sql.SQL("SELECT DISTINCT vocab_name FROM {aux}").format(aux=aux)) |
| tag_ids_by_name[name] = tag.id | ||
|
|
||
| restored = 0 | ||
| cr.execute(sql.SQL("SELECT geofence_id, vocab_name FROM {aux}").format(aux=aux)) |
| ) | ||
| restored += cr.rowcount | ||
|
|
||
| cr.execute(sql.SQL("DROP TABLE {aux}").format(aux=aux)) |
| cr.execute( | ||
| sql.SQL( | ||
| """ | ||
| CREATE TABLE IF NOT EXISTS {aux} ( | ||
| geofence_id integer NOT NULL, | ||
| vocab_name varchar NOT NULL | ||
| ) | ||
| """ | ||
| ).format(aux=aux) | ||
| ) |
| cr.execute( | ||
| sql.SQL( | ||
| """ | ||
| INSERT INTO {aux} (geofence_id, vocab_name) | ||
| SELECT rel.geofence_id, | ||
| COALESCE(v.name->>'en_US', (SELECT t.value FROM jsonb_each_text(v.name) t LIMIT 1)) | ||
| FROM spp_gis_geofence_tag_rel rel | ||
| JOIN spp_vocabulary v ON v.id = rel.tag_id | ||
| WHERE true {valid_clause} | ||
| """ | ||
| ).format(aux=aux, valid_clause=valid_clause) | ||
| ) |
| cr.execute( | ||
| sql.SQL( | ||
| """ | ||
| DELETE FROM spp_gis_geofence_tag_rel rel | ||
| USING spp_vocabulary v | ||
| WHERE v.id = rel.tag_id {valid_clause} | ||
| """ | ||
| ).format(valid_clause=valid_clause) | ||
| ) |
…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
2 participants
Re-lands the scope described in reverted PR #76 (revert: #271), as part of splitting that PR into description-consistent pieces.
Summary
spp_program_geofencemodule for geofence-based program targeting and eligibility management (Tier 1 coordinate intersection, Tier 2 area-intersection fallback, program configuration UI, creation wizard).spp_gisspatial operators to supportMultiPolygonandGeometryCollection, including correctST_Bufferdistance behavior.spp.gis.geofence.tagmodel replaces vocabulary-based geofence tags.Added vs the original PR
spp_gis 19.0.2.1.0): the geofencetag_idsco-model changes 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. Regression tests included (remap, dedup across geofences, valid-link survival, fresh-DB no-op).Verification
./spp t spp_gis: 92 passed, 0 failed (includes 4 new migration tests)./spp t spp_program_geofence: 31 passed, 0 failed