Week tab: move property between days #35

Merged

ldraney merged 1 commit from 32-week-move-property-between-days into main

2026-05-25 11:20:22 +00:00

ldraney commented

2026-05-25 04:34:52 +00:00

Owner

Summary

Adds a button-based day picker to the Week tab that lets users move a queued or completed property from one day to another within the current week. Tap a cell to reveal day buttons, tap a target day to move.

Changes

config/routes.rb — Added patch :move collection route to weeks resource
app/controllers/weeks_controller.rb — Added move action with uniqueness error handling; extended index to pluck item IDs for the view
app/views/weeks/index.html.erb — Wrapped queued/completed cells in a Stimulus-controlled wrapper with day-picker buttons; added flash message rendering
app/javascript/controllers/week_move_controller.js — New Stimulus controller: toggle picker, close on outside click
app/assets/stylesheets/application.css — Styles for day-picker, day-picker buttons (44px touch targets), flash messages
spec/requests/weeks_spec.rb — Added 5 new specs covering move success, duplicate rejection, invalid date handling, and picker rendering

Test Plan

docker compose run --rm -e RAILS_ENV=test web bundle exec rspec — 39 examples, 0 failures
Manual: navigate to Week tab, tap a queued/completed cell, verify day buttons appear, tap a different day, confirm item moves
Verify error flash when moving to a day that already has the property queued

Review Checklist

No Tailwind used
44px minimum touch targets on day-picker buttons
Button-based (no drag-and-drop), mobile-friendly
Follows existing Stimulus and Turbo patterns
Handles uniqueness violation gracefully with flash alert
Does not modify work_queue_items_controller.rb
All 39 tests pass

None.

Closes #32

## Summary Adds a button-based day picker to the Week tab that lets users move a queued or completed property from one day to another within the current week. Tap a cell to reveal day buttons, tap a target day to move. ## Changes - `config/routes.rb` — Added `patch :move` collection route to weeks resource - `app/controllers/weeks_controller.rb` — Added `move` action with uniqueness error handling; extended index to pluck item IDs for the view - `app/views/weeks/index.html.erb` — Wrapped queued/completed cells in a Stimulus-controlled wrapper with day-picker buttons; added flash message rendering - `app/javascript/controllers/week_move_controller.js` — New Stimulus controller: toggle picker, close on outside click - `app/assets/stylesheets/application.css` — Styles for day-picker, day-picker buttons (44px touch targets), flash messages - `spec/requests/weeks_spec.rb` — Added 5 new specs covering move success, duplicate rejection, invalid date handling, and picker rendering ## Test Plan - `docker compose run --rm -e RAILS_ENV=test web bundle exec rspec` — 39 examples, 0 failures - Manual: navigate to Week tab, tap a queued/completed cell, verify day buttons appear, tap a different day, confirm item moves - Verify error flash when moving to a day that already has the property queued ## Review Checklist - [x] No Tailwind used - [x] 44px minimum touch targets on day-picker buttons - [x] Button-based (no drag-and-drop), mobile-friendly - [x] Follows existing Stimulus and Turbo patterns - [x] Handles uniqueness violation gracefully with flash alert - [x] Does not modify work_queue_items_controller.rb - [x] All 39 tests pass ## Related Notes None. ## Related Closes #32

ldraney added 1 commit

2026-05-25 04:34:52 +00:00

Add move property between days on Week tab (#32 )

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

CI / scan_ruby (pull_request) Has been cancelled

Details

CI / scan_js (pull_request) Has been cancelled

Details

CI / lint (pull_request) Has been cancelled

Details

bb9b5320f8

Button-based day picker for moving queued/completed items between
days of the week. Tap a cell to reveal M-T-W-T-F-S-S buttons,
tap a day to move. Handles uniqueness violations gracefully.

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

ldraney commented

2026-05-25 04:35:36 +00:00

Author

Owner

QA Review

Scope: 6 files, +259/-9 lines. New move action on WeeksController, Stimulus controller for day-picker UI, CSS, tests.

Findings

No blocking issues found.

Minor observations (informational, not blocking):

Flash messages are view-scoped -- Flash rendering was added only to weeks/index.html.erb. If other views later need flash support, consider moving to the layout. Fine for now since only weeks#move produces flashes.
Day-picker overflow on small screens -- The picker renders 7 buttons in a row (absolute positioned, right-aligned). On a narrow mobile viewport with a property in the leftmost columns, the picker could overflow off-screen to the left. Low risk since the week grid cells are near the right side of the viewport anyway, but worth a manual check at 375px.
button_to generates a full <form> per day button -- This means 7 forms per cell with an item. For a grid with many properties, this increases DOM size. Acceptable for a landscaping business scale (dozens of properties, not thousands).

Checklist

No Tailwind
44px touch targets (2.75rem min-width/height)
Button-based, mobile-friendly
Follows existing Stimulus/Turbo patterns
Uniqueness violation handled (both validation + DB constraint)
work_queue_items_controller.rb untouched
39 tests pass, including 5 new specs for the move action
Branch naming correct: 32-week-move-property-between-days

VERDICT: APPROVE

## QA Review **Scope:** 6 files, +259/-9 lines. New `move` action on WeeksController, Stimulus controller for day-picker UI, CSS, tests. ### Findings **No blocking issues found.** Minor observations (informational, not blocking): 1. **Flash messages are view-scoped** -- Flash rendering was added only to `weeks/index.html.erb`. If other views later need flash support, consider moving to the layout. Fine for now since only weeks#move produces flashes. 2. **Day-picker overflow on small screens** -- The picker renders 7 buttons in a row (absolute positioned, right-aligned). On a narrow mobile viewport with a property in the leftmost columns, the picker could overflow off-screen to the left. Low risk since the week grid cells are near the right side of the viewport anyway, but worth a manual check at 375px. 3. **`button_to` generates a full `<form>` per day button** -- This means 7 forms per cell with an item. For a grid with many properties, this increases DOM size. Acceptable for a landscaping business scale (dozens of properties, not thousands). ### Checklist - [x] No Tailwind - [x] 44px touch targets (2.75rem min-width/height) - [x] Button-based, mobile-friendly - [x] Follows existing Stimulus/Turbo patterns - [x] Uniqueness violation handled (both validation + DB constraint) - [x] `work_queue_items_controller.rb` untouched - [x] 39 tests pass, including 5 new specs for the move action - [x] Branch naming correct: `32-week-move-property-between-days` **VERDICT: APPROVE**

ldraney commented

2026-05-25 11:18:49 +00:00

Author

Owner

PR #35 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8, Hotwire (Stimulus + Turbo), vanilla CSS with design tokens, RSpec request specs.

Controller (weeks_controller.rb - move action):

Good: ActiveRecord::RecordNotUnique rescue handles race conditions where the uniqueness validation passes but the DB constraint catches a concurrent insert.
Good: Date::Error rescue handles malformed target_date params.
Good: Uses find(params[:work_queue_item_id]) which raises ActiveRecord::RecordNotFound (returns 404) if the item ID is tampered with. Safe for a single-user app without auth.
Minor: The item.update call will trigger model validation (validates :property_id, uniqueness: { scope: :work_date }), so the RecordNotUnique rescue is a belt-and-suspenders approach -- correct.

Stimulus controller (week_move_controller.js):

Good: Proper lifecycle -- connect() registers the listener, disconnect() cleans it up. No memory leaks.
Good: event.stopPropagation() in toggle() prevents the outside-click handler from immediately re-closing.
Good: Closes other open pickers before opening a new one.
The move() action closes the picker but the actual navigation happens via button_to form submission -- the picker closure is cosmetic UX polish. Correct pattern.

Route:

patch :move on collection is appropriate -- the item ID is passed as a param rather than nested in the URL, which is fine for a single action.

View:

Cells without items render nothing (the if/elsif block falls through to empty div). No crash risk.
button_to generates a form with CSRF token automatically. Safe against CSRF.
aria-label="Move <%= property.address_line %> from <%= day.strftime('%A') %>" -- good accessibility on the toggle button.
Day picker buttons use single-letter labels (M, T, W, etc.) which is readable for a 7-day week context.

CSS:

Uses design tokens consistently (--spacing-*, --color-*, --radius) throughout.
Touch targets: .day-picker-btn uses min-width: 2.75rem / min-height: 2.75rem (44px) -- meets mobile touch target requirements.
z-index: 50 is a reasonable value for a dropdown; no z-index wars evident.

Tests:

5 new specs covering: move success, flash message verification, duplicate rejection, invalid date, and picker rendering in GET.
Tests cover happy path, validation error path, and malformed input.

BLOCKERS

None.

NITS

Hardcoded color in .flash-alert: background: #fef2f2 is a raw hex value. The design token system has --color-success-light for the notice flash but no --color-danger-light token. Suggest adding --color-danger-light: #fef2f2; to :root and using var(--color-danger-light) for consistency.
DRY opportunity in view: The day-picker markup is duplicated for both the completed and queued branches (lines ~43-59 and ~61-77 in the diff). These are identical except for the inner icon. Consider extracting a partial like _week_cell_with_picker.html.erb that accepts the icon content as a block or local. Non-blocking since it is two instances, not a proliferation.
itemId value declared but unused: The Stimulus controller declares static values = { itemId: Number } but never references this.itemIdValue. The item ID is embedded in the button_to form action URL instead. The value declaration is dead code -- remove it or use it for something.
No constraint on target_date range: The move action accepts any valid date, not just dates within the current week. A user could theoretically move an item to a date months away. For a single-user app this is low-risk, but a guard like unless (@start_date..@end_date).cover?(target_date) would tighten the API.

SOP COMPLIANCE

Branch named after issue: 32-week-move-property-between-days follows {issue-number}-{kebab-case-purpose}
PR body has: Summary, Changes, Test Plan, Related
Related references plan slug -- no plan slug exists (noted by requester as intentional)
No secrets committed
No unnecessary file changes (all 6 files are directly relevant to the feature)
Commit messages are descriptive

PROCESS OBSERVATIONS

This is a well-scoped PR: one feature, one controller action, one Stimulus controller, matching tests. Low change failure risk.
Test plan includes both automated (39 passing specs) and manual verification steps.
The PR correctly closes #32 via the Related section.
Deployment frequency: small incremental feature, safe to ship.

VERDICT: APPROVED

## PR #35 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8, Hotwire (Stimulus + Turbo), vanilla CSS with design tokens, RSpec request specs. **Controller (`weeks_controller.rb` - move action):** - Good: `ActiveRecord::RecordNotUnique` rescue handles race conditions where the uniqueness validation passes but the DB constraint catches a concurrent insert. - Good: `Date::Error` rescue handles malformed `target_date` params. - Good: Uses `find(params[:work_queue_item_id])` which raises `ActiveRecord::RecordNotFound` (returns 404) if the item ID is tampered with. Safe for a single-user app without auth. - Minor: The `item.update` call will trigger model validation (`validates :property_id, uniqueness: { scope: :work_date }`), so the `RecordNotUnique` rescue is a belt-and-suspenders approach -- correct. **Stimulus controller (`week_move_controller.js`):** - Good: Proper lifecycle -- `connect()` registers the listener, `disconnect()` cleans it up. No memory leaks. - Good: `event.stopPropagation()` in `toggle()` prevents the outside-click handler from immediately re-closing. - Good: Closes other open pickers before opening a new one. - The `move()` action closes the picker but the actual navigation happens via `button_to` form submission -- the picker closure is cosmetic UX polish. Correct pattern. **Route:** - `patch :move` on collection is appropriate -- the item ID is passed as a param rather than nested in the URL, which is fine for a single action. **View:** - Cells without items render nothing (the `if/elsif` block falls through to empty div). No crash risk. - `button_to` generates a form with CSRF token automatically. Safe against CSRF. - `aria-label="Move <%= property.address_line %> from <%= day.strftime('%A') %>"` -- good accessibility on the toggle button. - Day picker buttons use single-letter labels (M, T, W, etc.) which is readable for a 7-day week context. **CSS:** - Uses design tokens consistently (`--spacing-*`, `--color-*`, `--radius`) throughout. - Touch targets: `.day-picker-btn` uses `min-width: 2.75rem` / `min-height: 2.75rem` (44px) -- meets mobile touch target requirements. - `z-index: 50` is a reasonable value for a dropdown; no z-index wars evident. **Tests:** - 5 new specs covering: move success, flash message verification, duplicate rejection, invalid date, and picker rendering in GET. - Tests cover happy path, validation error path, and malformed input. ### BLOCKERS None. ### NITS 1. **Hardcoded color in `.flash-alert`:** `background: #fef2f2` is a raw hex value. The design token system has `--color-success-light` for the notice flash but no `--color-danger-light` token. Suggest adding `--color-danger-light: #fef2f2;` to `:root` and using `var(--color-danger-light)` for consistency. 2. **DRY opportunity in view:** The day-picker markup is duplicated for both the completed and queued branches (lines ~43-59 and ~61-77 in the diff). These are identical except for the inner icon. Consider extracting a partial like `_week_cell_with_picker.html.erb` that accepts the icon content as a block or local. Non-blocking since it is two instances, not a proliferation. 3. **`itemId` value declared but unused:** The Stimulus controller declares `static values = { itemId: Number }` but never references `this.itemIdValue`. The item ID is embedded in the `button_to` form action URL instead. The value declaration is dead code -- remove it or use it for something. 4. **No constraint on target_date range:** The `move` action accepts any valid date, not just dates within the current week. A user could theoretically move an item to a date months away. For a single-user app this is low-risk, but a guard like `unless (@start_date..@end_date).cover?(target_date)` would tighten the API. ### SOP COMPLIANCE - [x] Branch named after issue: `32-week-move-property-between-days` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has: Summary, Changes, Test Plan, Related - [ ] Related references plan slug -- no plan slug exists (noted by requester as intentional) - [x] No secrets committed - [x] No unnecessary file changes (all 6 files are directly relevant to the feature) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - This is a well-scoped PR: one feature, one controller action, one Stimulus controller, matching tests. Low change failure risk. - Test plan includes both automated (39 passing specs) and manual verification steps. - The PR correctly closes #32 via the Related section. - Deployment frequency: small incremental feature, safe to ship. ### VERDICT: APPROVED