Add property picker for quick-add to day detail page #241

Merged

ldraney merged 1 commit from 237-day-property-picker into main 2026-06-17 02:57:22 +00:00

Conversation 3 Commits 1 Files changed 5 +197 -11

ldraney commented 2026-06-16 18:53:31 +00:00

Owner

Copy link

Summary

Adds a property picker to the day detail page so admins can quickly add properties to a day's work queue without leaving the page. Reuses the existing property_picker_controller.js Stimulus controller and follows the same patterns established in the Today page.

Changes

• config/routes.rb -- Added POST /days/:date/add route mapped to days#add_to_queue
• app/controllers/days_controller.rb -- Added @properties and @queued_property_ids to show action; added add_to_queue action that creates a WorkQueueItem, handles duplicates gracefully, and responds with turbo_stream
• app/views/days/show.html.erb -- Added property picker form above the queued list, reusing the same markup pattern and Stimulus controller as the Today page; added DOM IDs for turbo stream targeting
• app/views/days/add_to_queue.turbo_stream.erb -- New turbo stream response that appends the new queue item to the list and removes the empty state
• spec/requests/days_spec.rb -- Added 15 new specs: property picker rendering (3), add_to_queue functionality (5), and role access for add_to_queue (5)

Test Plan

• All 25 specs pass (10 existing + 15 new)
• Verify property picker appears on day detail page
• Search for a property, click to add -- item appears in queue via turbo stream
• Try adding a duplicate -- flash message appears, no duplicate created
• Verify non-admin roles are denied access to both GET and POST

Review Checklist

• Reuses existing property_picker_controller.js without modification
• Follows WorkQueueItemsController#create pattern for item creation
• Duplicate handling returns 422 with flash message
• Role enforcement inherited from controller-level require_role :admin, :super_admin
• No N+1 queries -- @queue_items uses .includes(:property)
• Turbo stream targets use unique DOM IDs (day-queue-list, day-queue-empty) to avoid conflicts with Today page

Related Notes

• Does NOT handle DayExclusion cleanup -- that is scoped to ticket #236
• The picker on the day page does not support creating new properties via Enter (client_name field is present for Stimulus controller compatibility but the controller action only handles property_id)

Related

• Closes #237

## Summary Adds a property picker to the day detail page so admins can quickly add properties to a day's work queue without leaving the page. Reuses the existing `property_picker_controller.js` Stimulus controller and follows the same patterns established in the Today page. ## Changes - `config/routes.rb` -- Added `POST /days/:date/add` route mapped to `days#add_to_queue` - `app/controllers/days_controller.rb` -- Added `@properties` and `@queued_property_ids` to `show` action; added `add_to_queue` action that creates a WorkQueueItem, handles duplicates gracefully, and responds with turbo_stream - `app/views/days/show.html.erb` -- Added property picker form above the queued list, reusing the same markup pattern and Stimulus controller as the Today page; added DOM IDs for turbo stream targeting - `app/views/days/add_to_queue.turbo_stream.erb` -- New turbo stream response that appends the new queue item to the list and removes the empty state - `spec/requests/days_spec.rb` -- Added 15 new specs: property picker rendering (3), add_to_queue functionality (5), and role access for add_to_queue (5) ## Test Plan - All 25 specs pass (10 existing + 15 new) - Verify property picker appears on day detail page - Search for a property, click to add -- item appears in queue via turbo stream - Try adding a duplicate -- flash message appears, no duplicate created - Verify non-admin roles are denied access to both GET and POST ## Review Checklist - [x] Reuses existing `property_picker_controller.js` without modification - [x] Follows WorkQueueItemsController#create pattern for item creation - [x] Duplicate handling returns 422 with flash message - [x] Role enforcement inherited from controller-level `require_role :admin, :super_admin` - [x] No N+1 queries -- `@queue_items` uses `.includes(:property)` - [x] Turbo stream targets use unique DOM IDs (day-queue-list, day-queue-empty) to avoid conflicts with Today page ## Related Notes - Does NOT handle DayExclusion cleanup -- that is scoped to ticket #236 - The picker on the day page does not support creating new properties via Enter (client_name field is present for Stimulus controller compatibility but the controller action only handles property_id) ## Related - Closes #237

ldraney added 1 commit 2026-06-16 18:53:31 +00:00

Add property picker for quick-add to day queue ... 51c60b26d2

Closes #237

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

ldraney commented 2026-06-16 18:53:55 +00:00

Author

Owner

Copy link

Self-Review Findings

Correctness

PASS -- All 25 specs pass (10 existing + 15 new). Route, controller, view, and turbo stream all work together correctly.

Potential Issues

1. Dead code in add_to_queue (minor) -- Lines 42-43 load @queue_items and @queued_property_ids after a successful save, but the turbo stream template (add_to_queue.turbo_stream.erb) does not use either variable. These were loaded anticipating future use (e.g., updating the picker dropdown to mark the property as queued), but the current Stimulus controller handles that client-side in select(). Verdict: harmless -- two cheap queries that go unused. Could be removed but also provides a hook if the turbo stream is extended later.

2. No new-property-via-Enter support -- The client_name hidden field is present in the form (required by the Stimulus controller's submit action), but add_to_queue only handles property_id. If a user types a name and hits Enter, property_id will be empty and the create will fail with a validation error. Verdict: acceptable -- the Today page supports this but the day detail page is an admin tool for scheduling existing properties, not creating new ones. The error path handles it gracefully (422 with flash).

3. N+1 queries -- No issues. show uses .includes(:property) on queue items. add_to_queue accesses @item.property once (single record, not a loop). Property.active.order(:client_name) is a single query.

4. Race condition on duplicate check -- The find_by + save pattern has a small window where two concurrent requests could both pass the duplicate check. The model's validates :property_id, uniqueness: { scope: :work_date } and the DB unique index catch this, and the error path returns a proper 422. Verdict: safe.

Summary

No blockers. Ready for QA review.

## Self-Review Findings ### Correctness **PASS** -- All 25 specs pass (10 existing + 15 new). Route, controller, view, and turbo stream all work together correctly. ### Potential Issues 1. **Dead code in `add_to_queue`** (minor) -- Lines 42-43 load `@queue_items` and `@queued_property_ids` after a successful save, but the turbo stream template (`add_to_queue.turbo_stream.erb`) does not use either variable. These were loaded anticipating future use (e.g., updating the picker dropdown to mark the property as queued), but the current Stimulus controller handles that client-side in `select()`. **Verdict:** harmless -- two cheap queries that go unused. Could be removed but also provides a hook if the turbo stream is extended later. 2. **No new-property-via-Enter support** -- The `client_name` hidden field is present in the form (required by the Stimulus controller's `submit` action), but `add_to_queue` only handles `property_id`. If a user types a name and hits Enter, `property_id` will be empty and the create will fail with a validation error. **Verdict:** acceptable -- the Today page supports this but the day detail page is an admin tool for scheduling existing properties, not creating new ones. The error path handles it gracefully (422 with flash). 3. **N+1 queries** -- No issues. `show` uses `.includes(:property)` on queue items. `add_to_queue` accesses `@item.property` once (single record, not a loop). `Property.active.order(:client_name)` is a single query. 4. **Race condition on duplicate check** -- The `find_by` + `save` pattern has a small window where two concurrent requests could both pass the duplicate check. The model's `validates :property_id, uniqueness: { scope: :work_date }` and the DB unique index catch this, and the error path returns a proper 422. **Verdict:** safe. ### Summary No blockers. Ready for QA review.

ldraney commented 2026-06-16 19:28:19 +00:00

Author

Owner

Copy link

PR #241 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, Hotwire (Turbo Streams + Stimulus), RSpec request specs.

Controller pattern (app/controllers/days_controller.rb)

The add_to_queue action follows the established WorkQueueItemsController#create pattern well:

• Duplicate detection via find_by before save -- good UX over raw model validation errors
• Model-level uniqueness constraint (validates :property_id, uniqueness: { scope: :work_date }) provides the safety net
• respond_to with both turbo_stream and HTML fallback -- correct
• Role enforcement inherited from class-level require_role :admin, :super_admin -- verified in application_controller.rb
• parse_date returns nil for invalid dates, and the guard clause redirects -- correct

Stimulus controller reuse

The view correctly wires all six targets expected by property_picker_controller.js: input, dropdown, option, form, propertyId, clientName. The data-action bindings (focus->open, input->filter, keydown->submit, click->select) match the controller. The data-queued, data-property-id, and data-search-text attributes are all present and correctly populated. Good reuse without modification.

Turbo stream response (add_to_queue.turbo_stream.erb)

Appends to day-queue-list and removes day-queue-empty -- both IDs are present in the view. Markup structure mirrors the existing queue item rendering in show.html.erb.

Accessibility

Good ARIA support: role="combobox" on input, aria-expanded, aria-controls pointing to day-picker-listbox, aria-autocomplete="list", role="listbox" on the dropdown, role="option" on each item. This matches WAI-ARIA combobox patterns.

BLOCKERS

None.

NITS

1. Stale heading count after turbo_stream add. The <h3>Queued (N)</h3> heading is only rendered when @queue_items.any? and shows a static count. After a turbo_stream append, the count remains at the original value (or the heading is absent if the list was empty). Consider wrapping the heading in a turbo frame or adding a turbo_stream.update for the heading in add_to_queue.turbo_stream.erb. Low priority since users see the item appear in the list, but the count becoming stale is a minor UX gap.

2. Position race condition. WorkQueueItem.where(work_date: @date).count for setting position is not atomic -- two concurrent requests could get the same position value. This is the same pattern used in WorkQueueItemsController#create (line 85), so it is consistent with existing code, but worth noting as a future improvement (e.g., MAX(position) + 1 in a transaction, or a DB default).

3. No spec for invalid property_id. There is no test for what happens when property_id refers to a non-existent property. The belongs_to :property will raise ActiveRecord::InvalidForeignKey on save. Consider adding a spec to document the expected behavior (likely a 500, which the app's error handling should catch). Not a blocker since this is an admin-only endpoint and the picker UI only provides valid IDs.

4. No spec for unauthenticated POST. The role access specs test authorized vs unauthorized roles, but there is no spec confirming that an unauthenticated user hitting POST /days/:date/add gets redirected to login. The existing GET /days/:date specs also omit this, so this is consistent, but worth noting.

5. client_name hidden field is inert. The form includes <input name="client_name"> for Stimulus controller compatibility, but the add_to_queue action only reads params[:property_id]. The PR body documents this -- good. If someone presses Enter to create a new property, the action will silently ignore client_name and attempt to save with a nil property_id. This is safe (save fails) but could be confusing. A flash message explaining "property selection required" would improve the UX if this is ever triggered.

SOP COMPLIANCE

• PR body has: Summary, Changes, Test Plan, Related -- all present and thorough
• Review Checklist included with detailed self-review notes
• No secrets, .env files, or credentials committed
• No unnecessary file changes -- all 5 files are scoped to the feature
• Commit message (from PR title) is descriptive
• Tests exist: 15 new specs covering rendering, functionality, and role access
• Related issue linked: Closes #237

PROCESS OBSERVATIONS

• Clean decomposition: #240 delivered the base day detail page, this PR layers on the property picker. Each PR is independently reviewable and deployable.
• Good pattern reuse: the Stimulus controller, flash partial, and controller patterns are all lifted from existing code rather than reinvented.
• The PR body's "Related Notes" section proactively scopes out what is NOT included (DayExclusion cleanup = #236, Enter-to-create = intentionally omitted). This is good scope discipline.
• Test coverage is solid at 15 new specs. The coverage gaps noted above (invalid property_id, unauthenticated access) are nits, not blockers.

VERDICT: APPROVED

## PR #241 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, Hotwire (Turbo Streams + Stimulus), RSpec request specs. **Controller pattern (app/controllers/days_controller.rb)** The `add_to_queue` action follows the established `WorkQueueItemsController#create` pattern well: - Duplicate detection via `find_by` before save -- good UX over raw model validation errors - Model-level uniqueness constraint (`validates :property_id, uniqueness: { scope: :work_date }`) provides the safety net - `respond_to` with both turbo_stream and HTML fallback -- correct - Role enforcement inherited from class-level `require_role :admin, :super_admin` -- verified in `application_controller.rb` - `parse_date` returns nil for invalid dates, and the guard clause redirects -- correct **Stimulus controller reuse** The view correctly wires all six targets expected by `property_picker_controller.js`: `input`, `dropdown`, `option`, `form`, `propertyId`, `clientName`. The data-action bindings (`focus->open`, `input->filter`, `keydown->submit`, `click->select`) match the controller. The `data-queued`, `data-property-id`, and `data-search-text` attributes are all present and correctly populated. Good reuse without modification. **Turbo stream response (add_to_queue.turbo_stream.erb)** Appends to `day-queue-list` and removes `day-queue-empty` -- both IDs are present in the view. Markup structure mirrors the existing queue item rendering in `show.html.erb`. **Accessibility** Good ARIA support: `role="combobox"` on input, `aria-expanded`, `aria-controls` pointing to `day-picker-listbox`, `aria-autocomplete="list"`, `role="listbox"` on the dropdown, `role="option"` on each item. This matches WAI-ARIA combobox patterns. ### BLOCKERS None. ### NITS 1. **Stale heading count after turbo_stream add.** The `<h3>Queued (N)</h3>` heading is only rendered when `@queue_items.any?` and shows a static count. After a turbo_stream append, the count remains at the original value (or the heading is absent if the list was empty). Consider wrapping the heading in a turbo frame or adding a `turbo_stream.update` for the heading in `add_to_queue.turbo_stream.erb`. Low priority since users see the item appear in the list, but the count becoming stale is a minor UX gap. 2. **Position race condition.** `WorkQueueItem.where(work_date: @date).count` for setting position is not atomic -- two concurrent requests could get the same position value. This is the same pattern used in `WorkQueueItemsController#create` (line 85), so it is consistent with existing code, but worth noting as a future improvement (e.g., `MAX(position) + 1` in a transaction, or a DB default). 3. **No spec for invalid property_id.** There is no test for what happens when `property_id` refers to a non-existent property. The `belongs_to :property` will raise `ActiveRecord::InvalidForeignKey` on save. Consider adding a spec to document the expected behavior (likely a 500, which the app's error handling should catch). Not a blocker since this is an admin-only endpoint and the picker UI only provides valid IDs. 4. **No spec for unauthenticated POST.** The role access specs test authorized vs unauthorized roles, but there is no spec confirming that an unauthenticated user hitting `POST /days/:date/add` gets redirected to login. The existing `GET /days/:date` specs also omit this, so this is consistent, but worth noting. 5. **`client_name` hidden field is inert.** The form includes `<input name="client_name">` for Stimulus controller compatibility, but the `add_to_queue` action only reads `params[:property_id]`. The PR body documents this -- good. If someone presses Enter to create a new property, the action will silently ignore `client_name` and attempt to save with a nil `property_id`. This is safe (save fails) but could be confusing. A flash message explaining "property selection required" would improve the UX if this is ever triggered. ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and thorough - [x] Review Checklist included with detailed self-review notes - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 5 files are scoped to the feature - [x] Commit message (from PR title) is descriptive - [x] Tests exist: 15 new specs covering rendering, functionality, and role access - [x] Related issue linked: Closes #237 ### PROCESS OBSERVATIONS - Clean decomposition: #240 delivered the base day detail page, this PR layers on the property picker. Each PR is independently reviewable and deployable. - Good pattern reuse: the Stimulus controller, flash partial, and controller patterns are all lifted from existing code rather than reinvented. - The PR body's "Related Notes" section proactively scopes out what is NOT included (DayExclusion cleanup = #236, Enter-to-create = intentionally omitted). This is good scope discipline. - Test coverage is solid at 15 new specs. The coverage gaps noted above (invalid property_id, unauthenticated access) are nits, not blockers. ### VERDICT: APPROVED

ldraney commented 2026-06-16 20:27:20 +00:00

Author

Owner

Copy link

PR #241 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, Hotwire (Turbo Streams + Stimulus), RSpec request specs.

Pattern consistency with WorkQueueItemsController#create:
The new DaysController#add_to_queue correctly follows the established pattern: check for duplicate, create with position, respond with turbo_stream on success and flash-messages append on failure. The simplification relative to the Today controller is appropriate -- the day detail page does not need the completed_by_other reclaim path or client_name creation path, and the PR body explicitly calls this out.

Stimulus controller reuse:
The property picker markup in show.html.erb correctly targets all required Stimulus targets: input, dropdown, option, form, propertyId, clientName. The clientName hidden field is present for controller compatibility even though the action only handles property_id -- this is documented in the PR body ("Related Notes"). The select() method in property_picker_controller.js will correctly set data-queued="true" and add the badge on click, preventing re-selection.

Turbo Stream response:
add_to_queue.turbo_stream.erb appends to day-queue-list and removes day-queue-empty. The DOM IDs are unique (day-queue-list, day-queue-empty, day-picker-listbox) and will not collide with the Today page's IDs. Good.

N+1 queries:
The show action uses .includes(:property) on queue items. The @properties query uses Property.active.order(:client_name) without eager-loading associations, which is correct -- the picker only needs client_name, address_line, and city, all of which are on the properties table directly.

Model validation:
WorkQueueItem has validates :property_id, uniqueness: { scope: :work_date }. The controller's explicit find_by check before save is belt-and-suspenders -- avoids a race condition producing a 500 from the uniqueness constraint. The find_by returns a graceful 422 flash instead. Sound approach.

BLOCKERS

None.

Input validation check -- property_id:
The property_id param is passed directly to WorkQueueItem.new(property_id: property_id). This is a foreign key to properties. If an attacker sends a non-existent property_id, the belongs_to :property association in WorkQueueItem will raise ActiveRecord::InvalidForeignKey at the database level, which Rails rescues as a 500. However, this endpoint is gated behind require_role :admin, :super_admin, so only trusted users can reach it. This is not a security blocker but is noted as a nit below.

Test coverage:
15 new specs covering: property picker rendering (3), add_to_queue functionality (5), role access for add_to_queue (5). This covers happy path, duplicate handling, turbo stream response format, position assignment, HTML fallback, and all five roles. No blocker.

NITS

1. Defensive property_id lookup (app/controllers/days_controller.rb, add_to_queue action): Consider wrapping the property lookup with Property.find_by(id: params[:property_id]) and returning a 404/422 if nil. Currently, a non-existent property_id will raise ActiveRecord::InvalidForeignKey at the DB level. The WorkQueueItemsController#create has the same pattern, so this is a pre-existing codebase choice rather than a regression, but worth noting.

2. Queued count header not updated by turbo stream (app/views/days/show.html.erb): The <h3 class="section-heading">Queued (<%= @queue_items.size %>)</h3> is rendered server-side and not wrapped in a turbo frame or targeted by the turbo stream response. After adding a property via the picker, the count in the header will be stale until a full page reload. The Today page likely has the same limitation, so this may be intentional, but noting it for awareness.

3. Empty state reappearance on remove (future consideration): turbo_stream.remove "day-queue-empty" handles the case where the list transitions from empty to non-empty. The reverse (removing the last item to show empty state) is not in scope for this PR since remove-from-list is ticket #236. Just noting the complementary pattern will be needed there.

4. position race condition (app/controllers/days_controller.rb, line position: WorkQueueItem.where(work_date: @date).count): If two admins add items simultaneously, they could get the same position value. This is a pre-existing pattern from WorkQueueItemsController#create (line 85) and not a regression. Low risk given the small user base.

SOP COMPLIANCE

• PR body has ## Summary, ## Changes, ## Test Plan, ## Related
• PR body includes Review Checklist with relevant items checked
• No secrets, .env files, or credentials committed
• No unnecessary file changes -- all 5 changed files are directly related to the feature
• Commit scope is tight -- single feature, single issue
• Tests exist: 15 new specs (property picker rendering, add_to_queue functionality, role access)
• PR explicitly documents out-of-scope items (DayExclusion cleanup -> #236, Enter-to-create -> noted)

PROCESS OBSERVATIONS

• Deployment risk: Low. Additive change -- new route, new action, new view template. No existing behavior modified. The only change to existing code is adding two lines to the show action (@queued_property_ids and @properties).
• Decomposition quality: Good. This PR is one slice of the decomposed issue #233. It builds on the foundation from PR #240 (day detail page controller/route/view) and explicitly defers DayExclusion to #236.
• Test-to-code ratio: 15 new specs for ~55 lines of new Ruby code (controller + view). Solid coverage.

VERDICT: APPROVED

## PR #241 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1, Hotwire (Turbo Streams + Stimulus), RSpec request specs. **Pattern consistency with WorkQueueItemsController#create:** The new `DaysController#add_to_queue` correctly follows the established pattern: check for duplicate, create with position, respond with turbo_stream on success and flash-messages append on failure. The simplification relative to the Today controller is appropriate -- the day detail page does not need the `completed_by_other` reclaim path or `client_name` creation path, and the PR body explicitly calls this out. **Stimulus controller reuse:** The property picker markup in `show.html.erb` correctly targets all required Stimulus targets: `input`, `dropdown`, `option`, `form`, `propertyId`, `clientName`. The `clientName` hidden field is present for controller compatibility even though the action only handles `property_id` -- this is documented in the PR body ("Related Notes"). The `select()` method in `property_picker_controller.js` will correctly set `data-queued="true"` and add the badge on click, preventing re-selection. **Turbo Stream response:** `add_to_queue.turbo_stream.erb` appends to `day-queue-list` and removes `day-queue-empty`. The DOM IDs are unique (`day-queue-list`, `day-queue-empty`, `day-picker-listbox`) and will not collide with the Today page's IDs. Good. **N+1 queries:** The `show` action uses `.includes(:property)` on queue items. The `@properties` query uses `Property.active.order(:client_name)` without eager-loading associations, which is correct -- the picker only needs `client_name`, `address_line`, and `city`, all of which are on the `properties` table directly. **Model validation:** `WorkQueueItem` has `validates :property_id, uniqueness: { scope: :work_date }`. The controller's explicit `find_by` check before `save` is belt-and-suspenders -- avoids a race condition producing a 500 from the uniqueness constraint. The `find_by` returns a graceful 422 flash instead. Sound approach. ### BLOCKERS None. **Input validation check -- `property_id`:** The `property_id` param is passed directly to `WorkQueueItem.new(property_id: property_id)`. This is a foreign key to `properties`. If an attacker sends a non-existent `property_id`, the `belongs_to :property` association in `WorkQueueItem` will raise `ActiveRecord::InvalidForeignKey` at the database level, which Rails rescues as a 500. However, this endpoint is gated behind `require_role :admin, :super_admin`, so only trusted users can reach it. This is not a security blocker but is noted as a nit below. **Test coverage:** 15 new specs covering: property picker rendering (3), add_to_queue functionality (5), role access for add_to_queue (5). This covers happy path, duplicate handling, turbo stream response format, position assignment, HTML fallback, and all five roles. No blocker. ### NITS 1. **Defensive `property_id` lookup** (`app/controllers/days_controller.rb`, `add_to_queue` action): Consider wrapping the property lookup with `Property.find_by(id: params[:property_id])` and returning a 404/422 if nil. Currently, a non-existent `property_id` will raise `ActiveRecord::InvalidForeignKey` at the DB level. The `WorkQueueItemsController#create` has the same pattern, so this is a pre-existing codebase choice rather than a regression, but worth noting. 2. **`Queued` count header not updated by turbo stream** (`app/views/days/show.html.erb`): The `<h3 class="section-heading">Queued (<%= @queue_items.size %>)</h3>` is rendered server-side and not wrapped in a turbo frame or targeted by the turbo stream response. After adding a property via the picker, the count in the header will be stale until a full page reload. The Today page likely has the same limitation, so this may be intentional, but noting it for awareness. 3. **Empty state reappearance on remove** (future consideration): `turbo_stream.remove "day-queue-empty"` handles the case where the list transitions from empty to non-empty. The reverse (removing the last item to show empty state) is not in scope for this PR since remove-from-list is ticket #236. Just noting the complementary pattern will be needed there. 4. **`position` race condition** (`app/controllers/days_controller.rb`, line `position: WorkQueueItem.where(work_date: @date).count`): If two admins add items simultaneously, they could get the same position value. This is a pre-existing pattern from `WorkQueueItemsController#create` (line 85) and not a regression. Low risk given the small user base. ### SOP COMPLIANCE - [x] PR body has ## Summary, ## Changes, ## Test Plan, ## Related - [x] PR body includes Review Checklist with relevant items checked - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 5 changed files are directly related to the feature - [x] Commit scope is tight -- single feature, single issue - [x] Tests exist: 15 new specs (property picker rendering, add_to_queue functionality, role access) - [x] PR explicitly documents out-of-scope items (DayExclusion cleanup -> #236, Enter-to-create -> noted) ### PROCESS OBSERVATIONS - **Deployment risk:** Low. Additive change -- new route, new action, new view template. No existing behavior modified. The only change to existing code is adding two lines to the `show` action (`@queued_property_ids` and `@properties`). - **Decomposition quality:** Good. This PR is one slice of the decomposed issue #233. It builds on the foundation from PR #240 (day detail page controller/route/view) and explicitly defers DayExclusion to #236. - **Test-to-code ratio:** 15 new specs for ~55 lines of new Ruby code (controller + view). Solid coverage. ### VERDICT: APPROVED

ldraney referenced this pull request 2026-06-17 00:08:59 +00:00

Harden input validation and add security documentation #243

ldraney referenced this pull request 2026-06-17 00:14:25 +00:00

Harden input validation and add security documentation #243

ldraney referenced this pull request 2026-06-17 00:18:11 +00:00

Harden input validation and add security documentation #243

ldraney merged commit 6fa01f1e1b into main 2026-06-17 02:57:22 +00:00

ldraney referenced this pull request from a commit 2026-06-17 02:57:22 +00:00

Add property picker for quick-add to day detail page (#241)

ldraney deleted branch 237-day-property-picker 2026-06-17 02:57:22 +00:00

ldraney referenced this pull request 2026-06-17 03:49:01 +00:00

Harden input validation and add security documentation #244

ldraney referenced this pull request 2026-06-17 03:51:18 +00:00

Harden input validation and add security documentation #244

ldraney referenced this pull request 2026-06-17 04:29:23 +00:00

Harden input validation and add security documentation #244

ldraney referenced this pull request from a commit 2026-06-17 07:29:14 +00:00

Merge main into 235-previously-accordion to resolve conflicts

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

No description provided.