Make address optional, add from Today tab, GPS update on details #29

Merged

ldraney merged 4 commits from 26-optional-address into main 2026-05-25 04:14:40 +00:00

Conversation 1 Commits 4 Files changed 15 +318 -67

ldraney commented 2026-05-25 04:11:14 +00:00

Owner

Copy link

Summary

• Address no longer required when creating properties — just a name is enough
• Quick-add form on Today tab creates + queues a property in one step
• "Update Address" GPS button on property details page for setting/changing address on-site

Changes

• app/models/property.rb: removed validates :address_line, presence: true
• app/controllers/properties_controller.rb: resolve action handles missing address params
• app/controllers/work_queue_items_controller.rb: create action accepts client_name to make new property
• app/javascript/controllers/location_controller.js: save without GPS/address, graceful nil handling
• app/javascript/controllers/add_location_controller.js: new Stimulus controller for GPS address update
• app/views/properties/show.html.erb: Update Address button, nil-safe display
• app/views/properties/manage.html.erb: nil-safe address display
• app/views/work_queue_items/_queue_item.html.erb: name as primary, links to details
• app/views/work_queue_items/_property_item.html.erb: name as primary, nil-safe
• app/views/work_queue_items/index.html.erb: quick-add form, "Recent" heading
• app/views/work_queue_items/create.turbo_stream.erb: handle new property in Turbo Stream
• app/assets/stylesheets/application.css: styles for quick-add, detail-actions, btn-link
• .gitignore: added .playwright-mcp/ and *.png

Test Plan

• Tests pass locally
• Add a property with just a name from the Today tab quick-add
• Tap queue item to verify it links to details page
• On details, tap Update Address — GPS detects street, enter house number, save
• Existing properties with addresses still display correctly
• No regressions in deactivate, drag-reorder, or complete toggle

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive

Related Notes

• ldraney/landscaping-assistant #26 — optional address
• ldraney/landscaping-assistant #27 — add from Today tab
• landscaping-assistant — project

Closes #26
Closes #27

## Summary - Address no longer required when creating properties — just a name is enough - Quick-add form on Today tab creates + queues a property in one step - "Update Address" GPS button on property details page for setting/changing address on-site ## Changes - `app/models/property.rb`: removed `validates :address_line, presence: true` - `app/controllers/properties_controller.rb`: resolve action handles missing address params - `app/controllers/work_queue_items_controller.rb`: create action accepts `client_name` to make new property - `app/javascript/controllers/location_controller.js`: save without GPS/address, graceful nil handling - `app/javascript/controllers/add_location_controller.js`: new Stimulus controller for GPS address update - `app/views/properties/show.html.erb`: Update Address button, nil-safe display - `app/views/properties/manage.html.erb`: nil-safe address display - `app/views/work_queue_items/_queue_item.html.erb`: name as primary, links to details - `app/views/work_queue_items/_property_item.html.erb`: name as primary, nil-safe - `app/views/work_queue_items/index.html.erb`: quick-add form, "Recent" heading - `app/views/work_queue_items/create.turbo_stream.erb`: handle new property in Turbo Stream - `app/assets/stylesheets/application.css`: styles for quick-add, detail-actions, btn-link - `.gitignore`: added `.playwright-mcp/` and `*.png` ## Test Plan - [ ] Tests pass locally - [ ] Add a property with just a name from the Today tab quick-add - [ ] Tap queue item to verify it links to details page - [ ] On details, tap Update Address — GPS detects street, enter house number, save - [ ] Existing properties with addresses still display correctly - [ ] No regressions in deactivate, drag-reorder, or complete toggle ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `ldraney/landscaping-assistant #26` — optional address - `ldraney/landscaping-assistant #27` — add from Today tab - `landscaping-assistant` — project Closes #26 Closes #27

ldraney added 3 commits 2026-05-25 04:11:14 +00:00

Add deactivate button to properties on Today tab ... bf059a2cf1

X button on each property in All Properties list toggles it inactive.
Property is removed from the list via Turbo Stream since Today only
shows active properties. Subtle muted X turns red on hover.

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

Add playwright and screenshots to gitignore ... 543e101341

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

Make address optional, add properties from Today tab, GPS update on details ... 994d9faf2c

- Remove address_line presence validation so name-only properties work
- Quick-add form on Today tab creates property and queues it immediately
- Update Address button on details page uses GPS + house number
- Queue items link to property details
- Client name is primary display everywhere, address is secondary
- Rename "All Properties" section to "Recent"

Closes #26, closes #27

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

ldraney commented 2026-05-25 04:13:19 +00:00

Author

Owner

Copy link

PR #29 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 7 (Hotwire/Turbo Streams, Stimulus JS, ERB views). Reviewing for Rails controller safety, Stimulus correctness, view nil-safety, XSS risk, and Turbo Stream correctness.

Rails Controller Safety

1. properties_controller.rb#resolve (line 39-46): The else branch has dead code -- has_address is always false in this branch, so has_address ? "#{number} #{street}" : nil will always be nil. Harmless but confusing; should just be address_line: nil.

2. work_queue_items_controller.rb#create (line 15-16): Property.create! with only client_name will raise ActiveRecord::RecordInvalid on any validation failure and return a 500. There is no rescue. For a quick-add form this is low risk (only client_name presence is validated and we guard with params[:client_name].present?), but an empty-string-with-whitespace like " " would slip past the present? check but fail the model validation. Consider .strip before the check or a rescue with user-facing error.

3. properties_controller.rb#resolve: No strong parameters -- raw params[:street], params[:city], etc. are used directly. This is acceptable for a JSON API endpoint that only inserts into permitted model attributes via the model layer, but it bypasses Rails' conventional mass-assignment protection pattern. Not a blocker since each attribute is explicitly assigned, but worth noting.

Stimulus Controller Correctness

4. add_location_controller.js: Targets and values declared correctly. The save() method references this.street.road (line 62) which would throw if detect() was never called or geocoding failed. However, the formSection is hidden until geocoding succeeds, so user cannot reach save() without this.street being set. Safe in practice.

5. add_location_controller.js line 51: document.querySelector("meta[name='csrf-token']").content -- if the meta tag is missing (e.g., API-only page), this throws. Low risk for this app.

View Nil-Safety

6. toggle_active.turbo_stream.erb (lines 11, 34): Both the active and inactive branches still use <%= @property.address_line %> &middot; <%= @property.city %> WITHOUT the nil-safe pattern used everywhere else. When address_line is nil, this renders " · " (empty dot). This is inconsistent with the rest of the PR which uses property.address_line.present? ? ... : "No address yet". This is a bug for properties created without an address.

XSS Risk

7. location_controller.js renderRecent(): Uses textContent for all dynamic values (line 135, 139) -- safe, no innerHTML. The add_location_controller.js also uses textContent (lines 32-33). No XSS vectors found.

Turbo Stream Correctness

8. create.turbo_stream.erb: The @new_property instance variable is set in the controller and correctly used in the template. prepend "property-list" targets the correct DOM ID from index.html.erb line 50. The replace for existing properties targets property_#{id} which matches _property_item.html.erb line 1. Correct.

BLOCKERS

1. Broken test: spec/models/property_spec.rb lines 4-7 explicitly test validates :address_line, presence: true -- a validation that this PR removes. This test will now FAIL. The test must be updated to reflect that address_line is optional. Additionally, tests on lines 18, 30-31, and 39 create properties with address_line which is fine, but there should be a NEW test asserting that a property IS valid without an address_line. The existing test suite is now incorrect.

2. Nil-safety gap in toggle_active.turbo_stream.erb: Lines 11 and 34 still render @property.address_line without a nil guard. A property without an address that gets toggled active/inactive will render broken markup (" · "). This template was touched in this PR (it was refactored into the if/else split), so it should have received the same nil-safe treatment applied to every other template.

NITS

1. properties_controller.rb line 42: Dead ternary has_address ? "#{number} #{street}" : nil -- has_address is always false in that branch. Simplify to just nil.

2. work_queue_items/index.html.erb quick-add form: No client-side validation or required attribute on the input. Submitting an empty form will create a Property.create!(client_name: "") call that fails with 500 since model validates presence. Add required to the input for graceful UX.

3. The heading change from "All Properties" to "Recent" (index.html.erb line 32) may confuse users -- the list still shows ALL active properties ordered by updated_at, not a "recent" subset. Consider matching label to behavior.

4. CSS: btn-deactivate opacity 0.4 is very low for touch targets on mobile -- may be hard to discover. Consider at least 0.5 or a different affordance.

SOP COMPLIANCE

• Branch named after issue (26-optional-address matches issue #26)
• PR body follows template (Summary, Changes, Test Plan, Related)
• Related references plan slug -- no plan slug provided (noted as N/A by caller)
• No secrets committed
• Tests pass -- existing test will FAIL due to removed validation still being asserted
• No unnecessary file changes (.gitignore for playwright artifacts is reasonable)
• Commit messages are descriptive

PROCESS OBSERVATIONS

• This PR bundles three issues (#26 optional address, #27 add from Today tab, plus a GPS update feature). The scope is manageable but ideally each would be its own PR for cleaner revert paths.
• The test plan is manual-only (checkboxes). Given the validation change, automated test updates are essential.
• The toggle_active.turbo_stream.erb refactor (if/else split + turbo_stream.remove) appears related to issue #24 scope (deactivate from Today tab), which is scope creep into this PR. It works correctly for that feature but expands the review surface.

VERDICT: NOT APPROVED

Two blockers must be addressed:

1. Fix spec/models/property_spec.rb -- remove the address_line presence test, add a test for valid property without address_line.
2. Apply nil-safe address display in toggle_active.turbo_stream.erb (both branches, lines 11 and 34) to match the pattern used in all other templates.

## PR #29 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails 7 (Hotwire/Turbo Streams, Stimulus JS, ERB views). Reviewing for Rails controller safety, Stimulus correctness, view nil-safety, XSS risk, and Turbo Stream correctness. **Rails Controller Safety** 1. `properties_controller.rb#resolve` (line 39-46): The `else` branch has dead code -- `has_address` is always `false` in this branch, so `has_address ? "#{number} #{street}" : nil` will always be `nil`. Harmless but confusing; should just be `address_line: nil`. 2. `work_queue_items_controller.rb#create` (line 15-16): `Property.create!` with only `client_name` will raise `ActiveRecord::RecordInvalid` on any validation failure and return a 500. There is no rescue. For a quick-add form this is low risk (only `client_name` presence is validated and we guard with `params[:client_name].present?`), but an empty-string-with-whitespace like `" "` would slip past the `present?` check but fail the model validation. Consider `.strip` before the check or a rescue with user-facing error. 3. `properties_controller.rb#resolve`: No strong parameters -- raw `params[:street]`, `params[:city]`, etc. are used directly. This is acceptable for a JSON API endpoint that only inserts into permitted model attributes via the model layer, but it bypasses Rails' conventional mass-assignment protection pattern. Not a blocker since each attribute is explicitly assigned, but worth noting. **Stimulus Controller Correctness** 4. `add_location_controller.js`: Targets and values declared correctly. The `save()` method references `this.street.road` (line 62) which would throw if `detect()` was never called or geocoding failed. However, the `formSection` is hidden until geocoding succeeds, so user cannot reach `save()` without `this.street` being set. Safe in practice. 5. `add_location_controller.js` line 51: `document.querySelector("meta[name='csrf-token']").content` -- if the meta tag is missing (e.g., API-only page), this throws. Low risk for this app. **View Nil-Safety** 6. `toggle_active.turbo_stream.erb` (lines 11, 34): Both the active and inactive branches still use `<%= @property.address_line %> &middot; <%= @property.city %>` WITHOUT the nil-safe pattern used everywhere else. When `address_line` is nil, this renders " &middot; " (empty dot). This is inconsistent with the rest of the PR which uses `property.address_line.present? ? ... : "No address yet"`. This is a bug for properties created without an address. **XSS Risk** 7. `location_controller.js` `renderRecent()`: Uses `textContent` for all dynamic values (line 135, 139) -- safe, no innerHTML. The `add_location_controller.js` also uses `textContent` (lines 32-33). No XSS vectors found. **Turbo Stream Correctness** 8. `create.turbo_stream.erb`: The `@new_property` instance variable is set in the controller and correctly used in the template. `prepend "property-list"` targets the correct DOM ID from `index.html.erb` line 50. The `replace` for existing properties targets `property_#{id}` which matches `_property_item.html.erb` line 1. Correct. ### BLOCKERS 1. **Broken test**: `spec/models/property_spec.rb` lines 4-7 explicitly test `validates :address_line, presence: true` -- a validation that this PR removes. This test will now FAIL. The test must be updated to reflect that address_line is optional. Additionally, tests on lines 18, 30-31, and 39 create properties with `address_line` which is fine, but there should be a NEW test asserting that a property IS valid without an address_line. The existing test suite is now incorrect. 2. **Nil-safety gap in `toggle_active.turbo_stream.erb`**: Lines 11 and 34 still render `@property.address_line` without a nil guard. A property without an address that gets toggled active/inactive will render broken markup (" &middot; "). This template was touched in this PR (it was refactored into the if/else split), so it should have received the same nil-safe treatment applied to every other template. ### NITS 1. `properties_controller.rb` line 42: Dead ternary `has_address ? "#{number} #{street}" : nil` -- `has_address` is always false in that branch. Simplify to just `nil`. 2. `work_queue_items/index.html.erb` quick-add form: No client-side validation or `required` attribute on the input. Submitting an empty form will create a `Property.create!(client_name: "")` call that fails with 500 since model validates presence. Add `required` to the input for graceful UX. 3. The heading change from "All Properties" to "Recent" (index.html.erb line 32) may confuse users -- the list still shows ALL active properties ordered by `updated_at`, not a "recent" subset. Consider matching label to behavior. 4. CSS: `btn-deactivate` opacity 0.4 is very low for touch targets on mobile -- may be hard to discover. Consider at least 0.5 or a different affordance. ### SOP COMPLIANCE - [x] Branch named after issue (`26-optional-address` matches issue #26) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [ ] Related references plan slug -- no plan slug provided (noted as N/A by caller) - [x] No secrets committed - [ ] Tests pass -- existing test will FAIL due to removed validation still being asserted - [x] No unnecessary file changes (`.gitignore` for playwright artifacts is reasonable) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - This PR bundles three issues (#26 optional address, #27 add from Today tab, plus a GPS update feature). The scope is manageable but ideally each would be its own PR for cleaner revert paths. - The test plan is manual-only (checkboxes). Given the validation change, automated test updates are essential. - The `toggle_active.turbo_stream.erb` refactor (if/else split + turbo_stream.remove) appears related to issue #24 scope (deactivate from Today tab), which is scope creep into this PR. It works correctly for that feature but expands the review surface. ### VERDICT: NOT APPROVED Two blockers must be addressed: 1. Fix `spec/models/property_spec.rb` -- remove the address_line presence test, add a test for valid property without address_line. 2. Apply nil-safe address display in `toggle_active.turbo_stream.erb` (both branches, lines 11 and 34) to match the pattern used in all other templates.

ldraney added 1 commit 2026-05-25 04:14:11 +00:00

Fix QA blockers: update spec and nil-safe toggle_active template ... 36748d2ce5

- Update property_spec to assert address_line is optional (not required)
- Apply nil-safe address display pattern to toggle_active turbo stream

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

ldraney merged commit 1a1a2a6529 into main 2026-05-25 04:14:40 +00:00

ldraney deleted branch 26-optional-address 2026-05-25 04:14:40 +00:00

ldraney referenced this pull request from a commit 2026-05-25 04:14:41 +00:00

Make address optional, add from Today tab, GPS update on details (#29)

ldraney referenced this pull request 2026-05-25 04:17:39 +00:00

Week tab: show unassigned properties, add to day, move between days [PARENT] #28

ldraney referenced this pull request 2026-05-25 04:17:51 +00:00

Week tab: show unassigned properties section #30

ldraney referenced this pull request 2026-05-25 04:18:26 +00:00

Week tab: show unassigned properties, add to day, move between days [PARENT] #28

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!29

No description provided.