Skip to content

feat: replace measurement floats with traffic-light accessibility levels - #124

Merged
koinsaari merged 1 commit into
mainfrom
feat/accessibility-level-enum
Aug 2, 2026
Merged

feat: replace measurement floats with traffic-light accessibility levels#124
koinsaari merged 1 commit into
mainfrom
feat/accessibility-level-enum

Conversation

@koinsaari

Copy link
Copy Markdown
Contributor

Summary

  • Entrance/pathway/restroom/parking/elevator measurement fields (width, slope, turning radius, toilet seat height, distance, depth) now take a shared AccessibilityLevel enum (good/limited/no) instead of a raw float, submitted directly rather than measured.
  • internal/a11y audit-flag logic reads the enum directly; added LevelForDoorWidth/LevelForSlopePercent for raw-value conversion, threshold bands cited to SIA 500/ADA on the struct field doc comments.
  • internal/sources/osm now converts its two raw-value tags (width/door:width, incline) via those threshold functions. The remaining measurement fields have no OSM tag equivalent and stay submission-only.
  • api/openapi.yaml gets a new AccessibilityLevel schema (same pattern as existing DoorType/SurfaceType); generated code regenerated.
  • No DB migration — the affected columns are JSONB, unaffected by the field-type change.

🤖 Generated with Claude Code

@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Claude finished @koinsaari's task in 3m 52s —— View job


Review complete

  • Read REVIEW.md and CLAUDE.md for review guidelines and project context
  • Fetch PR diff and read changed files
  • Review against core invariants (pure data layer, deterministic AuditFlags, validation ordering, no HTTP framework)
  • Check SQL/GORM parameterization, JSONB Valuer/Scanner symmetry, test coverage
  • Post final review and set commit status

🔴 Important

Existing JSONB rows with numeric measurement values will fail to read after this change.

Before this PR, EntranceProps.Width, SlopePercent, RestroomProps.DoorWidth/TurningRadius/ToiletSeatHeight, ParkingProps.DistanceToEntrance/Width, and ElevatorProps.Width/Depth/DoorWidth were *float64 and directly client-submittable (openapi previously allowed raw numbers with a minimum/maximum). Any place already written through the API (or via prior OSM ingestion runs, if this pipeline ran before) has these as JSON numbers inside the entrance/restroom/parking/elevator JSONB columns.

These fields are now typed *AccessibilityLevel (a string), and pkg/models/accessibility.go:170 (scanJSONB) does a plain json.Unmarshal(b, dest) with no custom UnmarshalJSON on AccessibilityLevel to tolerate a legacy numeric value. Unmarshaling a JSON number into a Go string field returns an error, which GORM's Scan propagates through .Error on every First/Find call (internal/place/repository.go) — so reading any place that has a pre-existing numeric value in one of these seven fields will fail outright (like a 500 from the read path), not just silently drop that one field.

The PR body's claim "No DB migration — the affected columns are JSONB, unaffected by the field-type change" only holds for the column type (still jsonb); the JSON shape stored inside it changed in a breaking, non-backward-compatible way. This needs either:

  • a data backfill migration that rewrites existing numeric values in these 7 fields to good/limited/no using the same thresholds as LevelForDoorWidth/LevelForSlopePercent before this deploys, or
  • a custom UnmarshalJSON on AccessibilityLevel that accepts a legacy float and converts it via those thresholds, for backward compatibility.

Fix this →

🟡 Nit

  • internal/sources/osm/a11y_test.go — the subtest named "door:width takes precedence over width" (around line 213) actually asserts the opposite: with both tags set, parseMetres(tags["width"], tags["door:width"]) returns width first, so width wins (the subtest's own comment even says "good (width tag wins when present)"). The test name is inverted from what it verifies — worth renaming to avoid confusing future readers about tag precedence.

Summary

One blocking issue: the switch from raw float measurements to the good/limited/no enum changes the on-disk JSON shape of seven JSONB fields with no migration path for previously-persisted numeric values, contradicting the PR's "no migration needed" claim and risking read failures on existing data. Everything else is solid — the threshold functions (LevelForDoorWidth/LevelForSlopePercent) are correctly bounded and well-tested at their boundaries, the audit-flag engine changes remain deterministic and computed only from stored properties (no invariant violation there), OSM tag parsing (parseMetres/parsePercent) has good edge-case test coverage, and docs (internal/a11y/README.md, pkg/models/README.md, internal/sources/osm/README.md) were updated consistently with the code.


Note: I was unable to set the claude-code-review commit status (HTTP 403, "Resource not accessible by integration") — the GitHub App token doesn't currently have statuses: write permission. This review found an Important finding, so the intended status would have been state=failure.

Entrance/pathway/restroom/parking/elevator measurement fields (width,
slope, turning radius, etc.) now take an AccessibilityLevel enum
(good/limited/no) instead of a raw float, submitted directly rather
than measured. OSM ingestion still converts its two raw-value tags
(width/door:width, incline) via threshold tables cited to SIA 500/ADA;
the remaining measurement fields have no OSM equivalent and are
populated only through direct submission.
@koinsaari
koinsaari force-pushed the feat/accessibility-level-enum branch from a69eef6 to 9f6c703 Compare August 2, 2026 08:34
@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@koinsaari
koinsaari merged commit b9d6869 into main Aug 2, 2026
2 of 3 checks passed
@koinsaari
koinsaari deleted the feat/accessibility-level-enum branch August 2, 2026 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant