refactor: move spp.gis.geofence from spp_api_v2_gis to spp_gis#74
refactor: move spp.gis.geofence from spp_api_v2_gis to spp_gis#74
Conversation
Move the core geofence model to spp_gis so other modules can use geofences without pulling in the full API stack. Key changes: - New spp_gis/models/geofence.py with UUID field, core types (area_of_interest, custom), tag_ids, and GeoJSON methods - spp_hazard extends geofence with incident_id and hazard_zone type - spp_api_v2_gis slimmed to _inherit extension (service_area, targeting_area types + incident properties override) - Use uuid instead of DB id in GeoJSON properties - Use create_uid instead of custom created_by_id - Catch psycopg2.Error instead of bare Exception in area compute - Use self._table instead of hardcoded table name - Fix spp_gis category to OpenSPP/Core - Add spp_vocabulary, spp_registry to spp_gis depends - Add spp_gis to spp_hazard depends - Move ACL rows from spp_api_v2_gis to spp_gis - Split tests: core tests in spp_gis, extension tests in spp_api_v2_gis
- Batch _compute_area_sqkm into single SQL query (was N+1) - Use psycopg2.sql.Identifier for safe table name interpolation - Narrow exception catch in to_geojson to specific types - Prefetch tag_ids and create_uid in to_geojson_collection - Add spp_gis. prefix to geofence ACL model_id references - Remove redundant field guard in spp_api_v2_gis properties override
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #74 +/- ##
==========================================
+ Coverage 58.09% 65.22% +7.13%
==========================================
Files 188 309 +121
Lines 10797 18825 +8028
==========================================
+ Hits 6272 12279 +6007
- Misses 4525 6546 +2021
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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 is a significant and well-executed refactoring that moves the geofence model into the core spp_gis module. This is a great architectural improvement that will increase reusability. The new implementation includes several notable quality improvements, such as batching SQL queries to fix N+1 problems, using parameterized queries to prevent SQL injection, and prefetching data to optimize performance. My review identified one high-severity issue in spp_api_v2_gis related to unsafe access of a field from a dependent module, which could lead to runtime errors. I also have a suggestion to improve test coverage for the new GeoJSON properties.
spp_api_v2_gis/models/geofence.py
Outdated
| props["incident_id"] = self.incident_id.uuid if self.incident_id and hasattr(self.incident_id, "uuid") else None | ||
| props["incident_name"] = self.incident_id.name if self.incident_id else None |
There was a problem hiding this comment.
This implementation has two issues:
- It will raise an
AttributeErrorif thespp_hazardmodule is not installed, because theincident_idfield will not exist on the model. It's safer to usegetattrto access optional fields from other modules. - You're trying to access
self.incident_id.uuid, but thespp.hazard.incidentmodel does not have auuidfield. This will causeprops['incident_id']to always beNone.
I've suggested a fix that uses getattr for safety and falls back to using id for the incident. If the intent is to use UUIDs, a uuid field should be added to the spp.hazard.incident model.
| props["incident_id"] = self.incident_id.uuid if self.incident_id and hasattr(self.incident_id, "uuid") else None | |
| props["incident_name"] = self.incident_id.name if self.incident_id else None | |
| incident = getattr(self, "incident_id", None) | |
| props["incident_id"] = incident.id if incident else None | |
| props["incident_name"] = incident.name if incident else None |
| self.assertIsNone(props["incident_id"]) | ||
| self.assertIsNone(props["incident_name"]) |
There was a problem hiding this comment.
This test only covers the case where no incident is linked to the geofence. To make the test more robust and ensure properties are correctly serialized, please consider adding a test case where an incident is created and linked to the geofence. This would also help verify that the correct identifier (e.g., id or uuid) is being used.
…sing tests Bug fix: - spp_api_v2_gis _get_geojson_properties used incident_id.uuid but spp.hazard.incident has no uuid field; use .code instead New tests in spp_gis (11 added, 78 total): - UUID copy=False generates new UUID on copy - Rename to duplicate active name raises ValidationError - Reactivate archived record with duplicate name raises ValidationError - create_from_geojson accepts dict input (not just string) - Different geometry sizes produce different area_sqkm values - Empty recordset to_geojson_collection returns empty features - GeoJSON property values are correct (not just keys present) - Description defaults to empty string in properties - geofence_type defaults to "custom" when omitted - area_sqkm is strictly positive for known test polygon - Archived geofences excluded from default search New tests in spp_hazard (5 added, 38 total): - hazard_zone geofence type via selection_add - Geofence linked to actual hazard incident - incident_id ondelete set null behavior - hazard_zone type label in GeoJSON properties - Geofence without incident works fine New test in spp_api_v2_gis (1 added, 191 total): - Linked incident code and name appear in GeoJSON properties
…drims In Odoo 19, SQL constraint violations (models.Constraint, required=True) raise psycopg2.IntegrityError at the ORM level, not ValidationError. The conversion to ValidationError happens in the RPC layer, not during direct create() calls in tests. Fixes 5 pre-existing test failures: - spp_area_hdx: test_hdx_pcode_unique_constraint - spp_area_hdx: test_required_fields (source_id NOT NULL) - spp_area_hdx: test_unique_country_constraint - spp_drims: test_4w_wizard_requires_incident - spp_drims: test_urgency_state_resolved (missing resolution notes)
1 participant
Summary
Move the core
spp.gis.geofencemodel fromspp_api_v2_gistospp_gisso other modules can use geofences without pulling in the full API stack (spp_api_v2,spp_aggregation, etc.).spp_gis/models/geofence.py: Core geofence model with UUID field (replaces DB id in GeoJSON), core types (area_of_interest,custom),tag_ids, GeoJSON methods, andcreate_from_geojsonfactoryspp_hazard/models/geofence.py:_inheritextension addingincident_idandhazard_zonegeofence typespp_api_v2_gis/models/geofence.py: Now just an_inheritextension addingservice_area/targeting_areatypes and incident properties in GeoJSON outputspp_api_v2_gistospp_gisspp_gis, 4 extension tests inspp_api_v2_gisQuality improvements over original
_compute_area_sqkminto single SQL query (was N+1)psycopg2.sql.Identifierfor safe table name interpolation (was string formatting)to_geojsonto specific typesto_geojson_collectionto avoid N+1created_by_idfield (use Odoo's built-increate_uid)mail.thread/mail.activity.mixin(geofences don't need chatter)spp_vocabularyandspp_registrydependencies inspp_gisspp_giscategory toOpenSPP/CoreTest plan
./spp t spp_gispasses (67 tests, 0 failures)./spp t spp_hazardpasses (33 tests, 0 failures)./spp t spp_api_v2_gispasses (190 tests, 0 failures)