fix(spp_api_v2_gis): repair aborted-transaction fallbacks and geofence/incident scope actions

[jeremi]

Fixes five independent defects in spp_api_v2_gis's spatial-query and access-scope code, all found while auditing the module for the aborted-transaction bug class.

query_statistics, query_statistics_batch and query_proximity each retry a failed coordinate-based lookup with an area-based fallback on the same database cursor. Three of the four fallback paths ran outside any savepoint, so a genuine database error in the coordinate attempt left the transaction aborted, and the fallback (plus every later query on that cursor) failed with InFailedSqlTransaction instead of degrading gracefully.

A fourth, pre-existing bug bound SQL parameters out of order when an is_group filter was present, feeding a GeoJSON string into a numeric column position.

Separately, routers/geofence.py gated create/delete on has_scope("gis", "geofence"), but geofence was never added as a selectable client scope action, so no client could ever be granted that scope short of gis:all. The same gap existed for incident.

11 files changed, 647 insertions(+), 34 deletions(-) versus 19.0.

What it removes, renames, or breaks

Nothing. All fixes are additive: a savepoint around an existing query, a parameter-order correction, and two new selectable scope-action values.

The manifest bumps 19.0.2.0.0 → 19.0.2.0.1 only so Odoo re-registers the widened Selection for scope actions. No field is added, removed, or retyped.

Two related items are explicitly out of scope here and handled elsewhere: _compute_statistics still has its own unprotected raw-SQL sites, and a broader inventory found roughly 20 further aborted-transaction-shaped sites across the codebase. Please do not read this branch as having closed the bug class, only its own four sites.

Migrations

Manifest bump only, no migration script. selection_add widens the allowed set of action values; every stored value stays valid and there is no SQL constraint on the column, so the bump exists solely to trigger re-registration on -u.

Known gap: nobody has exercised this specific 19.0.2.0.0 → 19.0.2.0.1 upgrade end to end against a database that already holds spp.api.client.scope rows. The "no migration needed" conclusion rests on the reasoning above, not on an executed upgrade.

⚠️ Merge-order constraint

This PR must merge before #281, or spp_api_v2_gis regresses in a way that cannot be recovered.

ref	spp_api_v2_gis version
19.0	19.0.2.0.0
this PR	19.0.2.0.1
#281	19.0.3.0.0

If any part of #281 merges first, this PR then drives the manifest backwards from 19.0.3.x to 19.0.2.0.1. Odoo still performs the upgrade, because a lower version is still a different version, but the dispatch window installed < V <= manifest becomes empty for every conceivable V from that point on, so no migration for this module can ever run again until some future bump climbs back above 19.0.3.1.0. Nothing warns, the module keeps working, and the next schema change it ships silently does not migrate. That is precisely the failure that forced the #76 revert.

Merging in the documented order costs nothing: 2.0.0 → 2.0.1 → 3.0.0, monotonic throughout. If for any reason this PR ends up merging second, re-bump its manifest above whatever is on 19.0 at that moment, rather than merging as-is.

Second, semantic constraint: #281 independently fixed the same parameter-order bug but carries zero savepoints. A merge that resolves in #281's favour silently reintroduces both aborted-transaction paths. Both sides look correct in isolation and the conflict is semantic rather than textual, so this one needs a human check at merge time. The scope-action fix is unaffected; models/api_client_scope.py is byte-identical across the branches.

Verification

./spp test spp_api_v2_gis on this branch at 0a11b276, read from the run log rather than the console summary:

0 failed, 0 error(s) of 209 tests

12 of those 209 skipped, and they are worth naming rather than hiding behind the green:

Skipped test	Reason given
TestCatalogService.test_get_catalog_report_driven_data_layer	No geo field available for data layer creation
TestCatalogService.test_get_catalog_with_data_layers	No data layer available (spp.area polygon field not found)
TestLayersService.test_build_layer_styling	No data layer available
TestLayersService.test_fetch_layer_features_limit	No data layer available
TestLayersService.test_fetch_layer_features_without_geometry	No data layer available
TestLayersService.test_get_data_layer_as_geojson	No data layer available (spp.area polygon field not found)
TestOGCService.test_get_collection_by_layer_id	No data layer available
TestOGCService.test_get_collections_contains_data_layers	No data layer available (spp.area polygon field not found)
TestOGCService.test_model_driven_data_layer_collection_has_no_qml_link	No data layer available
TestOGCService.test_report_driven_data_layer_collection_has_qml_link	No geo field available for data layer creation
TestOGCHTTP.test_qml_for_model_driven_data_layer_returns_404	No geo field available for model-driven layer
TestOGCHTTP.test_qml_for_report_driven_data_layer_returns_200	No geo field available for data layer creation

These skips are pre-existing on 19.0 and not introduced by this branch, but they are the bad kind: each one skips itself precisely when the geo field it exists to exercise is missing, so the suite goes green in exactly the situation the test was written to catch. Converting this family from skipTest to hard assertions is tracked separately and is expected to land after #281, which is what makes the geo fields reliably present. Flagging it here so nobody reads 0 failed on this module as full coverage of the data-layer paths.

Historical context for the fixes themselves: the coordinate savepoint fix took the query-parameter-order test class from 2 failed, 9 error(s) of 14 tests to 0 failed, 0 error(s) of 4 (the other 10 were unaffected by this bug), and the full suite from 2 failed, 10 error(s) of 205 tests to clean.

All four savepoints use cr.savepoint(flush=False). This is a deliberate deviation from the default flushing behaviour, with the rationale inline at each site: it keeps unrelated pending ORM writes out of the rollback. Worth a reviewer's second look, because it means only the raw SQL inside the with block is protected, not any not-yet-flushed ORM writes made earlier in the same request.

Assumptions

• The geofence and incident scope actions were added as part of this fix rather than split into a separate PR, on the grounds that the gate without the selectable value is not shippable on its own. Flagged here in case you would rather they had been left alone.
• All 7 commits carry Signed-off-by:.

---

Update 2026-08-24 (review fix-ups, pushed by @gonzalesedwin1123)

Four commits landed on this branch addressing the expert-review findings; they supersede some statements above:

• The batch is now actually immune to a poisoned iteration. The original savepoints only covered the spatial legs; a database error inside _compute_statistics (per-geometry or summary) could still abort the transaction for the rest of the batch. Each batch iteration and the batch summary now run inside their own savepoint; a summary failure degrades to an empty summary instead of discarding the per-geometry results. (_compute_statistics's own internals remain out of scope, but its failures can no longer poison the cursor for later geometries.)
• Statistics failures now propagate. The coordinate-attempt try was narrowed to the savepoint itself, so a statistics failure is no longer mislabelled "Coordinate-based query failed" and silently retried through the area fallback.
• The new coordinate tests are safe under the full SP-MIS stack (ci-full): the test column is created with ADD COLUMN IF NOT EXISTS and the _fields widening is skipped when spp_registrant_gis already provides the real field. Previously setUpClass died with DuplicateColumn there.
• Test client ids use uuid4 instead of id(scopes) (CPython address reuse caused a real unique-constraint collision), and stale docstrings describing the pre-fix behaviour were reworded.
• The incident scope action is forward-preparation for the incidents API in feat(spp_api_v2_gis): OGC processes/jobs + incidents API (re-land from #76) #281, which gates on has_scope("gis", "incident") and carries a byte-identical models/api_client_scope.py; DESCRIPTION.md now documents it as reserved.

Verification after the fix-ups: 214 tests, 0 failed, 0 errors (same 12 pre-existing skips), both standalone and with spp_registrant_gis co-installed (-i spp_api_v2_gis,spp_registrant_gis).

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.48%. Comparing base (4123d6e) to head (ab3c28f).

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #375      +/-   ##
==========================================
- Coverage   75.65%   69.48%   -6.17%     
==========================================
  Files         546      133     -413     
  Lines       36563    12148   -24415     
==========================================
- Hits        27660     8441   -19219     
+ Misses       8903     3707    -5196     

Flag	Coverage Δ
spp_analytics	?
spp_api_v2_change_request	?
spp_api_v2_cycles	?
spp_api_v2_data	?
spp_api_v2_entitlements	?
spp_api_v2_gis	74.60% <100.00%> (+3.02%)	⬆️
spp_api_v2_programs	?
spp_api_v2_service_points	?
spp_api_v2_simulation	?
spp_attachment_av_scan	?
spp_attendance	?
spp_base_common	91.07% <ø> (ø)
spp_case_entitlements	?
spp_case_programs	?
spp_cel_load_testing	?
spp_change_request_v2	?
spp_claim_169	?
spp_cr_type_assign_program	?
spp_dci	?
spp_dci_client	?
spp_dci_client_compliance	?
spp_dci_client_crvs	?
spp_dci_client_dr	?
spp_dci_client_ibr	?
spp_dci_compliance	?
spp_dci_demo	94.28% <ø> (ø)
spp_dci_indicators	?
spp_dci_server	?
spp_dci_server_social	?
spp_drims	?
spp_drims_sl_demo	?
spp_farmer_registry_cr	?
spp_farmer_registry_demo	?
spp_grm_cel	?
spp_hazard	?
spp_key_management	?
spp_mis_demo_v2	70.38% <ø> (ø)
spp_oauth	?
spp_program_geofence	?
spp_programs	65.53% <ø> (ø)
spp_registrant_gis	?
spp_registry	87.22% <ø> (+0.07%)	⬆️
spp_security	69.56% <ø> (ø)
spp_starter_sp_mis	?
spp_studio_change_requests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_api_v2_gis/models/api_client_scope.py	100.00% <100.00%> (ø)
spp_api_v2_gis/services/spatial_query_service.py	94.24% <100.00%> (+19.01%)	⬆️

... and 414 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c9d6923846

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Guard the test-only coordinates column creation

When these tests run in a database where spp_registrant_gis is already installed, this unconditional ALTER TABLE raises PostgreSQL DuplicateColumn because that module defines the same res_partner.coordinates column. This occurs in combined installations such as spp_mis_demo_v2, which depends on both modules, and prevents the GIS API tests from running; check for the field/column first or use ADD COLUMN IF NOT EXISTS.

AGENTS.md reference: AGENTS.md:L107-L115

Useful? React with 👍 / 👎.

[gonzalesedwin1123]

Expert review — spatial-query + scope fixes

All four claimed fixes are real and correctly implemented, and I could reproduce the PR's verification exactly (209 tests, 0 failed, 0 errors, the same 12 pre-existing skips). Requesting changes for one must-fix: a new test that errors deterministically in ci-full's SP-MIS job. Happy to approve once that lands.

What I verified as correct

• Param-order fix (_query_by_coordinates): the SQL places the where_clause placeholders before ST_GeomFromGeoJSON(%s); pre-fix code bound the GeoJSON into the p.is_group = %s position (invalid input syntax for type boolean → aborted transaction). filter_params + [geometry_json] matches the SQL text. Checked _query_by_area, _proximity_by_coordinates, _proximity_by_area — no other instances of the bug class remain in the module.
• Savepoints: Odoo's Savepoint rolls back then re-raises into the existing except, so the fallback stays reachable; flush=False is the right call here — the guarded code is pure raw SQL, and a flushing savepoint's rollback would cr.clear() the caller's unflushed ORM cache.
• Scope fix: has_scope requires the exact action or all, and geofence was indeed never selectable — gis:all was the only key to the create/delete endpoints. The change narrows required privilege; no ACL widening.
• incident action: nothing on 19.0 checks has_scope(*, "incident") today, but #281 adds endpoints gated on exactly that scope and carries a byte-identical models/api_client_scope.py — so this is legitimate forward-prep. Please say so explicitly in the PR description (content-must-match-description rule) and document both new actions in readme/DESCRIPTION.md.
• Cross-PR warnings: both check out. #281 carries zero savepoints and its own param-order fix, so the merge-order and semantic-conflict warnings are accurate. #375 first keeps versions monotonic (2.0.0 → 2.0.1 → 3.0.0).
• Version/HISTORY/README: bump correct (nothing on 19.0 touched the module since the branch point), OCA-style fragment, CI-generated README consistent.
• Tests additive: 557 lines added, 0 removed; no existing test weakened.

Must fix

tests/test_spatial_query_coordinates.py:48 — hard failure in ci-full (SP-MIS job).

cls.env.cr.execute("ALTER TABLE res_partner ADD COLUMN coordinates geometry(Point, 4326)")

spp_mis_demo_v2 depends directly on spp_registrant_gis, which defines the real res.partner.coordinates GeoPointField, and ci-full.yml runs --test-tags for every spp_* module with tests on that stack. Reproduced locally with -i spp_api_v2_gis,spp_registrant_gis:

psycopg2.errors.DuplicateColumn: column "coordinates" of relation "res_partner" already exists
ERROR: setUpClass (…test_spatial_query_coordinates.TestQueryByCoordinates)
→ 0 failed, 1 error(s) of 205 tests   (vs 209/0/0 standalone — the 4 tests vanish)

Per-PR CI is green only because the per-module job installs spp_api_v2_gis alone.

Fix (precedent: spp_gis_report/tests/test_migration_boolean_dimensions.py:40):

cls.env.cr.execute("ALTER TABLE res_partner ADD COLUMN IF NOT EXISTS coordinates geometry(Point, 4326)")

and make declared_coordinates_field a no-op when "coordinates" in partner_cls._fields, so the MappingProxyType swap never shadows the real, set-up field.

Should fix

1. Batch-immunity claim is broader than the code (spatial_query_service.py:55-95). Three _compute_statistics calls remain unguarded: lines 148 and 169 (inside the batch loop body) and line 95 (batch summary — not even in a try). A DB error there still aborts the transaction, and the next iteration's SAVEPOINT raises InFailedSqlTransaction — the exact failure mode the PR fixes elsewhere. Either wrap each batch iteration (plus the summary call) in a savepoint, or soften the description's batch claim; you already declare _compute_statistics out of scope, but the batch-immunity wording overpromises.
2. _compute_statistics sits inside the coordinate-attempt try (spatial_query_service.py:136-155): a statistics failure logs the misleading "Coordinate-based query failed" warning and triggers a redundant area query + second stats computation. Pre-existing, but this PR edits these exact lines — narrowing the try to the savepoint block would fix it.
3. HISTORY understates the change (readme/HISTORY.md): both bullets mention only the coordinate queries, but two of the four savepoints wrap the area fallbacks — the part that actually keeps query_statistics_batch alive. Add a bullet, then apply CI's README-regen diff.
4. Stale comments describing the pre-fix world: tests/test_batch_query.py:249-250 and tests/test_spatial_query_fallback.py:180-182 say the area fallback "runs unguarded" (no longer true); test_spatial_query_fallback.py:196-199 assumes spp_registrant_gis is never installed (false in ci-full — the coordinate leg does hit ST_Buffer(NaN) there; the test still passes either way). Reword to state the invariant, not the old bug.

Suggestions (non-blocking)

• test_spatial_query_fallback.py:225: radius_km=float("nan") slips past radius_km <= 0 and relies on PostGIS's ST_Buffer finite-distance check — deterministic today but version-brittle; patching a failing _proximity_by_area (like the sibling tests) would be self-documenting. Keeping the NaN case as an extra is fine.
• test_api_client_scope.py:30-48: fixture setup duplicates test_statistics_endpoint.py almost verbatim — worth a tests/common.py mixin.
• test_api_client_scope.py:34-35: id(scopes) isn't a stable unique key (address reuse); use a counter or uuid4().hex.
• test_batch_query.py:306: the service's own warnings go unasserted — wrap in assertLogs(SERVICE_LOGGER, "WARNING") like the fallback tests, so expected errors are captured rather than just muted.
• spatial_query_service.py:164-166: lone f-string _logger.info among otherwise lazy %s calls.
• models/api_client_scope.py:20-26: the new actions join the global action selection, so combos like individual:geofence become storable. Consider an @api.constrains restricting them to resource == "gis", or at least a comment marking them GIS-only.
• Theoretical, nothing reachable today: spp_api_v2/models/api_client.py get_allowed_fields returns all fields if any scope row for a resource lacks field_filter_ids, so once multiple gis:* rows are practical, an unfiltered gis:geofence row could accidentally lift a field restriction on gis:read. No GIS router calls it — flagging for awareness only.

Verification

Check	Status	Notes
Naming conventions	PASS	follows existing selection_add/ondelete pattern
Security/ACL	PASS	privilege narrowed, no new sudo, no PII in logs, nosec justifications accurate
Odoo 19 compat	PASS	savepoint + selection_add semantics verified
Tests pass	PASS standalone / FAIL in ci-full co-install	209/0/0, 12 pre-existing skips match the description; must-fix above
Tests additive	PASS	0 lines removed
Manifest/HISTORY/README	PASS	HISTORY wording gap noted above
UI patterns	N/A	no view changes

[gonzalesedwin1123]

Approved on re-review

All findings from the previous review are resolved as of 1c3683fa; CI is fully green (pre-commit, all test jobs, CodeQL/Semgrep/gitleaks, codecov patch + project).

Must-fix — resolved. The coordinate tests now use ADD COLUMN IF NOT EXISTS and skip the _fields widening when spp_registrant_gis provides the real field. Verified in the ci-full-equivalent scenario (-i spp_api_v2_gis,spp_registrant_gis): 214 tests, 0 failed, 0 errors — previously that run died in setUpClass with DuplicateColumn.

Should-fixes — resolved:

• Batch iterations and the batch summary now run inside their own savepoints, so the description's batch-immunity claim is actually true: a statistics DB failure on one geometry no longer poisons the rest of the batch, and a summary failure degrades to an empty summary instead of discarding the per-geometry results. Both covered by new TDD tests that fail on the pre-fix code.
• The coordinate-attempt try blocks were narrowed to the savepoint, so statistics failures propagate instead of being mislabelled and retried through the area fallback (also test-covered).
• HISTORY now names the area-fallback/batch/summary savepoints and the propagation fix; DESCRIPTION documents gis:incident as reserved for the incidents API (#281, which gates on has_scope("gis", "incident") with a byte-identical scope model file).
• Stale pre-fix docstrings reworded to state the invariants.

Bonus: the id(scopes) client-id pattern flagged as a suggestion turned out to be a live bug — CPython address reuse produced a real unique-constraint collision in test_statistics_endpoint once neighbouring tests shifted allocation patterns. Both occurrences now use uuid4.

Suite is 209 → 214 tests (5 added, none removed or weakened), green standalone and co-installed.

Reminder for the merger: this PR must merge before #281 (version monotonicity 2.0.0 → 2.0.1 → 3.0.0), and #281's later rebase must preserve these savepoints by hand — it currently carries none, and the conflict is semantic, not textual.