Properties tab: proximity-based default sort order #207

Merged

ldraney merged 3 commits from 205-proximity-sort into main 2026-06-14 02:27:55 +00:00

Conversation 2 Commits 3 Files changed 5 +286 -6

ldraney commented 2026-06-13 22:11:34 +00:00

Owner

Copy link

Summary

• Replace alphabetical property sorting with nearest-neighbor greedy traversal using Haversine distance
• Geolocated properties chain from the centroid (or ENV-configurable base point); properties without coordinates sort alphabetically at the end
• No migrations, no new dependencies

Changes

• app/models/property.rb: Added Property.by_proximity class method with Haversine distance and nearest-neighbor chain algorithm. Supports optional scope parameter and ENV overrides (PROXIMITY_BASE_LAT, PROXIMITY_BASE_LNG).
• app/controllers/properties_controller.rb: Changed manage action from LOWER(client_name) sort to Property.by_proximity.
• app/controllers/weeks_controller.rb: Changed index action to Property.by_proximity. Updated @assigned/@unassigned filtering to Array select/reject since by_proximity returns an Array.
• spec/models/property_spec.rb: 9 new tests covering nearest-neighbor chain correctness, coords-before-no-coords ordering, alphabetical fallback, scope support, edge cases, ENV override.
• spec/requests/properties_spec.rb: 2 new tests verifying geolocated properties render before non-geolocated and proximity chain renders in manage view.

Test Plan

• Tests pass locally -- 319 examples, 0 failures
• Manual verification of property list order on manage and weeks pages
• No regressions in work queue item ordering (untouched)

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive
• Feature flag needed? No -- internal sort order change, no new user-visible workflow or external integration

Related Notes

• Closes #205
• landscaping-assistant -- the project this work belongs to

## Summary - Replace alphabetical property sorting with nearest-neighbor greedy traversal using Haversine distance - Geolocated properties chain from the centroid (or ENV-configurable base point); properties without coordinates sort alphabetically at the end - No migrations, no new dependencies ## Changes - `app/models/property.rb`: Added `Property.by_proximity` class method with Haversine distance and nearest-neighbor chain algorithm. Supports optional scope parameter and ENV overrides (`PROXIMITY_BASE_LAT`, `PROXIMITY_BASE_LNG`). - `app/controllers/properties_controller.rb`: Changed `manage` action from `LOWER(client_name)` sort to `Property.by_proximity`. - `app/controllers/weeks_controller.rb`: Changed `index` action to `Property.by_proximity`. Updated `@assigned`/`@unassigned` filtering to Array `select`/`reject` since `by_proximity` returns an Array. - `spec/models/property_spec.rb`: 9 new tests covering nearest-neighbor chain correctness, coords-before-no-coords ordering, alphabetical fallback, scope support, edge cases, ENV override. - `spec/requests/properties_spec.rb`: 2 new tests verifying geolocated properties render before non-geolocated and proximity chain renders in manage view. ## Test Plan - [x] Tests pass locally -- 319 examples, 0 failures - [ ] Manual verification of property list order on manage and weeks pages - [ ] No regressions in work queue item ordering (untouched) ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No -- internal sort order change, no new user-visible workflow or external integration ## Related Notes - Closes #205 - `landscaping-assistant` -- the project this work belongs to

ldraney added 1 commit 2026-06-13 22:11:34 +00:00

Properties tab: proximity-based default sort order (#205) ... ef444fe404

Replace alphabetical sorting with nearest-neighbor greedy traversal
using Haversine distance. Properties with lat/lng coordinates are
chained starting from the centroid (or ENV-configurable base point),
and properties without coordinates sort alphabetically at the end.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-13 22:13:18 +00:00

Author

Owner

Copy link

PR #207 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, RSpec, PostgreSQL (float columns for lat/lng)

Algorithm correctness (Property.by_proximity)

The Haversine implementation is textbook-correct. The nearest-neighbor greedy chain is a sound approach for this use case (route-like ordering for a landscaping crew). The O(n^2) complexity is explicitly documented and appropriate -- the comment says "fine for n <= 200" which aligns with a landscaping business's property count.

Good decisions:

• private_class_method on all helper methods -- clean public API surface
• ENV override for base point (PROXIMITY_BASE_LAT/PROXIMITY_BASE_LNG) -- allows depot-based routing without code changes
• Centroid fallback when no ENV is set -- sensible default
• to_a early and returning Array -- honest about the fact that ordering can't be expressed as SQL, avoids leaking a fake relation
• client_name.to_s.downcase handles nil client_name gracefully in the fallback sort

Return type change (ActiveRecord::Relation -> Array)

This is the most architecturally significant change. by_proximity returns an Array, which means:

• @properties.where(...) in WeeksController correctly changed to select/reject on Array
• @properties.count correctly changed to .size
• The manage view iterates with .each which works on both Array and Relation -- no view changes needed

One place to watch: @properties.any? in manage.html.erb (line 24 of that template) works on Arrays, so no issue there.

Scope parameter design

Property.by_proximity(Property.active) and Property.by_proximity(Property.includes(:services)) -- the scope parameter is clean. It calls .to_a on whatever relation is passed, so eager loading via includes will execute before the in-memory sort. This is correct.

WeeksController .to_set optimization

Converting scheduled_ids to a Set before the select/reject loop is a good performance choice -- O(1) lookups instead of O(n) on each property.

BLOCKERS

None.

NITS

1. Duplicated Haversine in spec (spec/models/property_spec.rb lines ~218-224): The test file defines its own haversine helper method that duplicates the model's implementation. This is intentional (the spec independently verifies the greedy property), so it's not a DRY violation in the security sense. However, consider extracting the Haversine formula to a shared concern or utility module if it's ever needed elsewhere (geocoding, distance display). Low priority.

2. ENV coupling: proximity_base_point reads ENV directly rather than through Rails configuration. For a Rails 8.1 app, Rails.application.config or Rails.application.credentials would be more conventional. However, ENV is fine for this use case (container-deployed, no sensitive data). Not blocking.

3. No guard on latitude/longitude type: The latitude and longitude columns are float in the schema. The code checks p.latitude.present? which handles nil, but a property with latitude: 0.0 would pass the check (0.0 is present in Ruby). Latitude 0, longitude 0 is a valid point (Gulf of Guinea), but for a Utah landscaping business, a property at 0,0 is almost certainly bad data. This is an edge case not worth blocking on, but worth noting for future data validation.

4. Request spec assertion weakness (spec/requests/properties_spec.rb, "renders proximity-sorted properties by nearest-neighbor chain"): The test creates 3 properties on the same longitude and asserts all 3 appear in the response body, but does not assert the actual order. The comment says "the three should appear contiguously" but the expect only checks that all 3 positions are non-nil. Consider asserting middle_pos < south_pos (or whichever order the centroid-based chain produces) to make this test actually verify ordering.

5. Model method count: Property is accumulating class methods (by_proximity, proximity_base_point, haversine, to_rad, nearest_neighbor_chain). If more geo functionality is added later, consider extracting to a Geocodable concern. Not needed now with 5 methods, all private_class_method.

SOP COMPLIANCE

• PR body has Summary, Changes, Test Plan, Related sections
• Test Plan includes local test run (319 examples, 0 failures)
• No secrets committed -- ENV vars are config references, not values
• No unnecessary file changes -- all 5 files directly serve the feature
• Commit messages are descriptive (single-commit PR from title)
• No .env files or credentials in diff
• Closes #205 referenced in PR body

PROCESS OBSERVATIONS

• Deployment risk: LOW. Pure application-layer sort change. No migrations, no schema changes, no new dependencies. Rollback is trivial (revert the single commit).
• Change failure risk: LOW. The Array return type is the only behavioral change that could surface bugs, and the controller changes properly account for it. Views iterate with .each which is polymorphic.
• Test coverage: GOOD. 9 model specs covering the algorithm (happy path, edge cases, ENV override, scope support, determinism, greedy property verification) plus 2 request specs for integration. The model specs are particularly thorough -- the "valid nearest-neighbor chain" test independently verifies the greedy invariant at each step.

VERDICT: APPROVED

## PR #207 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1, RSpec, PostgreSQL (float columns for lat/lng) **Algorithm correctness (Property.by_proximity)** The Haversine implementation is textbook-correct. The nearest-neighbor greedy chain is a sound approach for this use case (route-like ordering for a landscaping crew). The O(n^2) complexity is explicitly documented and appropriate -- the comment says "fine for n <= 200" which aligns with a landscaping business's property count. Good decisions: - `private_class_method` on all helper methods -- clean public API surface - ENV override for base point (`PROXIMITY_BASE_LAT`/`PROXIMITY_BASE_LNG`) -- allows depot-based routing without code changes - Centroid fallback when no ENV is set -- sensible default - `to_a` early and returning Array -- honest about the fact that ordering can't be expressed as SQL, avoids leaking a fake relation - `client_name.to_s.downcase` handles nil client_name gracefully in the fallback sort **Return type change (ActiveRecord::Relation -> Array)** This is the most architecturally significant change. `by_proximity` returns an Array, which means: - `@properties.where(...)` in WeeksController correctly changed to `select`/`reject` on Array - `@properties.count` correctly changed to `.size` - The manage view iterates with `.each` which works on both Array and Relation -- no view changes needed One place to watch: `@properties.any?` in `manage.html.erb` (line 24 of that template) works on Arrays, so no issue there. **Scope parameter design** `Property.by_proximity(Property.active)` and `Property.by_proximity(Property.includes(:services))` -- the scope parameter is clean. It calls `.to_a` on whatever relation is passed, so eager loading via `includes` will execute before the in-memory sort. This is correct. **WeeksController `.to_set` optimization** Converting `scheduled_ids` to a Set before the `select`/`reject` loop is a good performance choice -- O(1) lookups instead of O(n) on each property. ### BLOCKERS None. ### NITS 1. **Duplicated Haversine in spec** (`spec/models/property_spec.rb` lines ~218-224): The test file defines its own `haversine` helper method that duplicates the model's implementation. This is intentional (the spec independently verifies the greedy property), so it's not a DRY violation in the security sense. However, consider extracting the Haversine formula to a shared concern or utility module if it's ever needed elsewhere (geocoding, distance display). Low priority. 2. **ENV coupling**: `proximity_base_point` reads ENV directly rather than through Rails configuration. For a Rails 8.1 app, `Rails.application.config` or `Rails.application.credentials` would be more conventional. However, ENV is fine for this use case (container-deployed, no sensitive data). Not blocking. 3. **No guard on latitude/longitude type**: The `latitude` and `longitude` columns are `float` in the schema. The code checks `p.latitude.present?` which handles nil, but a property with `latitude: 0.0` would pass the check (0.0 is present in Ruby). Latitude 0, longitude 0 is a valid point (Gulf of Guinea), but for a Utah landscaping business, a property at 0,0 is almost certainly bad data. This is an edge case not worth blocking on, but worth noting for future data validation. 4. **Request spec assertion weakness** (`spec/requests/properties_spec.rb`, "renders proximity-sorted properties by nearest-neighbor chain"): The test creates 3 properties on the same longitude and asserts all 3 appear in the response body, but does not assert the actual order. The comment says "the three should appear contiguously" but the `expect` only checks that all 3 positions are non-nil. Consider asserting `middle_pos < south_pos` (or whichever order the centroid-based chain produces) to make this test actually verify ordering. 5. **Model method count**: `Property` is accumulating class methods (`by_proximity`, `proximity_base_point`, `haversine`, `to_rad`, `nearest_neighbor_chain`). If more geo functionality is added later, consider extracting to a `Geocodable` concern. Not needed now with 5 methods, all `private_class_method`. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Test Plan includes local test run (319 examples, 0 failures) - [x] No secrets committed -- ENV vars are config references, not values - [x] No unnecessary file changes -- all 5 files directly serve the feature - [x] Commit messages are descriptive (single-commit PR from title) - [x] No .env files or credentials in diff - [x] Closes #205 referenced in PR body ### PROCESS OBSERVATIONS - **Deployment risk: LOW.** Pure application-layer sort change. No migrations, no schema changes, no new dependencies. Rollback is trivial (revert the single commit). - **Change failure risk: LOW.** The Array return type is the only behavioral change that could surface bugs, and the controller changes properly account for it. Views iterate with `.each` which is polymorphic. - **Test coverage: GOOD.** 9 model specs covering the algorithm (happy path, edge cases, ENV override, scope support, determinism, greedy property verification) plus 2 request specs for integration. The model specs are particularly thorough -- the "valid nearest-neighbor chain" test independently verifies the greedy invariant at each step. ### VERDICT: APPROVED

ldraney commented 2026-06-14 02:00:18 +00:00

Author

Owner

Copy link

PR #207 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, RSpec, PostgreSQL (float columns for lat/lng).

Algorithm correctness (Property.by_proximity):

The Haversine implementation and nearest-neighbor greedy traversal are correct. The to_rad helper, the Haversine formula, and the chain logic all follow standard implementations. The O(n^2) complexity is explicitly documented as acceptable for n <= 200, which is reasonable for a landscaping business.

Partition logic: records.partition { |p| p.latitude.present? && p.longitude.present? } correctly handles nil vs present coordinates. The client_name.to_s.downcase fallback handles nil client names safely.

ENV override: proximity_base_point correctly parses ENV vars with to_f and falls back to centroid. Both paths are tested.

Array vs Relation change in WeeksController: The switch from @properties.where(...) to @properties.select/reject is necessary because by_proximity returns an Array. The view template only calls .any?, .each, and attribute accessors on @assigned/@unassigned, so the Array interface is fully compatible. The .count to .size change is also correct (Array has size, not count -- well, it does have count too, but size is idiomatic for Arrays and avoids confusion with AR count which issues a SQL query).

Encapsulation: private_class_method is used correctly to hide proximity_base_point, haversine, to_rad, and nearest_neighbor_chain. Good practice.

BLOCKERS

None found.

• No new functionality without tests: 9 model specs + 2 request specs cover the algorithm, edge cases, ENV override, scope parameter, and integration rendering.
• No unvalidated user input: ENV vars are parsed with to_f (safe; invalid strings become 0.0, which is a valid coordinate).
• No secrets or credentials in code.
• No DRY violations in auth/security paths.

NITS

1. ENV var documentation (nit): PROXIMITY_BASE_LAT and PROXIMITY_BASE_LNG are not documented in .env.example. Consider adding them with a comment explaining they are optional overrides for the proximity sort base point.

2. Haversine duplication in test (nit): spec/models/property_spec.rb duplicates the Haversine formula as a private test helper. This is actually fine for test independence -- the test should not call the production method it is verifying -- but worth noting that if the formula ever changes, the test helper would need updating too. The duplication is intentional and correct here.

3. scheduled_ids to Set (nit): The change from scheduled_ids = @completions.keys | @queued.keys to .to_set is a nice performance improvement for the include? lookups that follow. Good change.

4. No migration needed (confirmed): latitude and longitude columns already exist as float in the schema. No migration required. Confirmed.

SOP COMPLIANCE

• Branch named after issue: 205-proximity-sort references issue #205
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
• Closes #205 present in Related Notes section
• Tests exist: 9 model specs + 2 request specs (11 new tests total)
• No secrets committed
• No unnecessary file changes: 5 files changed, all directly related to the proximity sort feature
• Commit messages: single PR, descriptive title
• No .env files or credentials committed

PROCESS OBSERVATIONS

• Clean, well-scoped PR: only sort-order logic, no scope creep.
• Test plan is thorough: covers the algorithm correctness (greedy nearest-neighbor verification), edge cases (no coords, single property, all-no-coords), ENV override, scope parameter, and integration rendering.
• The Array return type is a deliberate trade-off documented in the method comment. Any future code that chains AR scopes after by_proximity will get a NoMethodError, but this is by design and the current callers handle it correctly.
• Deployment risk is low: internal sort order change, no new user-facing workflow, no migration, no new dependencies.

VERDICT: APPROVED

## PR #207 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, RSpec, PostgreSQL (float columns for lat/lng). **Algorithm correctness (Property.by_proximity):** The Haversine implementation and nearest-neighbor greedy traversal are correct. The `to_rad` helper, the Haversine formula, and the chain logic all follow standard implementations. The O(n^2) complexity is explicitly documented as acceptable for n <= 200, which is reasonable for a landscaping business. **Partition logic:** `records.partition { |p| p.latitude.present? && p.longitude.present? }` correctly handles nil vs present coordinates. The `client_name.to_s.downcase` fallback handles nil client names safely. **ENV override:** `proximity_base_point` correctly parses ENV vars with `to_f` and falls back to centroid. Both paths are tested. **Array vs Relation change in WeeksController:** The switch from `@properties.where(...)` to `@properties.select/reject` is necessary because `by_proximity` returns an Array. The view template only calls `.any?`, `.each`, and attribute accessors on `@assigned`/`@unassigned`, so the Array interface is fully compatible. The `.count` to `.size` change is also correct (Array has `size`, not `count` -- well, it does have `count` too, but `size` is idiomatic for Arrays and avoids confusion with AR `count` which issues a SQL query). **Encapsulation:** `private_class_method` is used correctly to hide `proximity_base_point`, `haversine`, `to_rad`, and `nearest_neighbor_chain`. Good practice. ### BLOCKERS None found. - No new functionality without tests: 9 model specs + 2 request specs cover the algorithm, edge cases, ENV override, scope parameter, and integration rendering. - No unvalidated user input: ENV vars are parsed with `to_f` (safe; invalid strings become 0.0, which is a valid coordinate). - No secrets or credentials in code. - No DRY violations in auth/security paths. ### NITS 1. **ENV var documentation** (nit): `PROXIMITY_BASE_LAT` and `PROXIMITY_BASE_LNG` are not documented in `.env.example`. Consider adding them with a comment explaining they are optional overrides for the proximity sort base point. 2. **Haversine duplication in test** (nit): `spec/models/property_spec.rb` duplicates the Haversine formula as a private test helper. This is actually fine for test independence -- the test should not call the production method it is verifying -- but worth noting that if the formula ever changes, the test helper would need updating too. The duplication is intentional and correct here. 3. **`scheduled_ids` to Set** (nit): The change from `scheduled_ids = @completions.keys | @queued.keys` to `.to_set` is a nice performance improvement for the `include?` lookups that follow. Good change. 4. **No migration needed** (confirmed): latitude and longitude columns already exist as `float` in the schema. No migration required. Confirmed. ### SOP COMPLIANCE - [x] Branch named after issue: `205-proximity-sort` references issue #205 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] `Closes #205` present in Related Notes section - [x] Tests exist: 9 model specs + 2 request specs (11 new tests total) - [x] No secrets committed - [x] No unnecessary file changes: 5 files changed, all directly related to the proximity sort feature - [x] Commit messages: single PR, descriptive title - [x] No .env files or credentials committed ### PROCESS OBSERVATIONS - Clean, well-scoped PR: only sort-order logic, no scope creep. - Test plan is thorough: covers the algorithm correctness (greedy nearest-neighbor verification), edge cases (no coords, single property, all-no-coords), ENV override, scope parameter, and integration rendering. - The Array return type is a deliberate trade-off documented in the method comment. Any future code that chains AR scopes after `by_proximity` will get a NoMethodError, but this is by design and the current callers handle it correctly. - Deployment risk is low: internal sort order change, no new user-facing workflow, no migration, no new dependencies. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-14 02:07:31 +00:00

Merge branch 'main' into 205-proximity-sort c6e1e03bf6

ldraney added 1 commit 2026-06-14 02:20:47 +00:00

Merge branch 'main' into 205-proximity-sort e590754501

ldraney merged commit ae62821196 into main 2026-06-14 02:27:55 +00:00

ldraney deleted branch 205-proximity-sort 2026-06-14 02:27:55 +00:00

ldraney referenced this pull request from a commit 2026-06-14 02:27:56 +00:00

Properties tab: proximity-based default sort order (#207)

ldraney referenced this pull request 2026-06-14 02:37:49 +00:00

Add assign-to-crew and add-to-queue buttons on Recent and Assigned sections #211

ldraney referenced this pull request 2026-06-15 12:20:20 +00:00

Properties tab: add sort toggle (alphabetical / proximity) #225

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!207

No description provided.