feat(registrant_gis): show latitude/longitude inputs on registrant form (#1143) - #324
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #324 +/- ##
==========================================
- Coverage 74.88% 68.60% -6.29%
==========================================
Files 515 123 -392
Lines 35099 12412 -22687
==========================================
- Hits 26284 8515 -17769
+ Misses 8815 3897 -4918 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
QA round 1 on OP#1143: entering an impossible latitude or longitude threw an Odoo error and left the record unrecoverable. The value was accepted and stored, and the MapTiler widget then failed projecting the point — "Cannot read properties of undefined (reading '0')" out of unproject/pointCoordinate. It fired on mouseover, so it recurred every time the form was opened and the bad value could not be corrected. Constrain latitude to [-90, 90] and longitude to [-180, 180]. The write is refused, so nothing out of range is ever stored and the widget is never handed a point it cannot project; the user sees an ordinary validation message naming the field and the offending value, with the form still in front of them. Constrained on `coordinates` as well as the two inputs, so a point arriving from an import or written directly by another module is checked on the same terms as one typed into the form. The bounds are the poles and the antimeridian inclusive — those are real places. Does not clean up rows saved before this existed. An out-of-range point already on file still breaks the widget until it is corrected or the database is reset, which the QA note covers. OP#1143
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
One mechanical blocker; everything substantive is already right, including things done before they could be asked for.
What I verified:
- The range constraint is exactly what this needed — and the story behind it (QA round 1: an impossible latitude was stored, the MapTiler widget threw while projecting it on every reopen, and the record could not be corrected) justifies both the constraint and its shape. Applying it to
coordinatesas well means imports and other modules face the same rule as the typed inputs — the import test proves it. This is the same guard #393 added for POD GPS, done here unprompted, with better tests: boundaries accepted (poles, antimeridian), both signs refused, and the recoverability test pinning that a refused write leaves the last good value intact — which was the actual harm. - The sync design is sound: compute from the point (x=lon, y=lat), inverse rebuilds it,
(0, 0)documented as the unset sentinel — safe in this domain — and the defensivegeom_typecheck keeps a non-point geometry from breaking the compute.digits=(9, 6)gives ~0.1 m precision and fits ±180. Thereadonly="disabled"on the new inputs matches the existing field's pattern on both forms. - Because they're real fields, importability-by-name comes free, as the PR says.
Blocker — version bump
spp_registrant_gis stays at 19.0.2.0.0 and ships in 2026.08. It matters functionally here: the two new stored columns and the view changes need -u spp_registrant_gis, and the views are the user-facing fix — without an upgrade, nobody sees the inputs.
One thing worth saying so nobody adds an unnecessary migration: no backfill script is needed. New stored computed fields are auto-computed when their columns are created during upgrade, so existing registrants' latitude/longitude populate from coordinates on their own — the bump is the complete upgrade story (unlike the #416 case, where existing columns changed meaning).
With the bump + HISTORY entry, this is an approve. This module is independent of the spp_drims queue, so no renumbering coordination applies.
The review's one blocker: 19.0.2.1.0 with its changelog entry. Functionally required — the two new stored columns and the view changes need an upgrade, and the views are the user-facing fix. No backfill script: new stored computes are populated when their columns are created during upgrade, so existing registrants get their latitude and longitude from coordinates on their own.
|
Thanks — bump added and merged as Version bump — And thank you for stating the no-backfill reasoning explicitly — that is the kind of thing that otherwise gets "fixed" by someone adding a migration to be safe. Recorded it in the commit message too, so the next person reading the bump sees why nothing else was needed: new stored computes populate when their columns are created, unlike the #416 case where existing columns changed meaning.
|
2 participants
Why is this change needed?
On the registry group/individual form, GPS Coordinates only render as the MapTiler map widget. Without MapTiler configured you cannot set coordinates by clicking the map, so the values are effectively un-enterable when creating a group via the registry (OP#1143). Only Excel import (via headers) could set them.
How was the change implemented?
gis_latitude/gis_longitude) onres.partner, kept in sync with the existingcoordinatespoint:coordinates(x = longitude, y = latitude),coordinatesheader).New unit tests
test_lat_long_computed_from_coordinates— settingcoordinatespopulates the lat/long inputs.test_coordinates_built_from_lat_long— typing lat/long rebuilds thecoordinatespoint (x=lon, y=lat).test_lat_long_empty_when_no_coordinates— no point ⇒ inputs read 0.Unit tests executed by the author
spp_registrant_gissuite: 0 failed, 0 error(s) of 7 tests (fresh DB,--stop-after-init).How to test manually
8.5, Longitude124.75, save → the map marker moves to that point.Related links
https://projects.acn.fr/work_packages/1143