Move Assigned to Another Crew to bottom, remove Schedule section

ldraney commented

2026-06-14 12:10:44 +00:00

Owner

Summary

Moves "Assigned to Another Crew" items to the bottom of the Today queue
Removes the read-only Schedule accordion from Week view
Adds 4-state assign pill cycle (empty → assigned → completed → other-crew → destroyed)
Removes the move action (drag items between days)
Rebased onto current main to resolve conflicts with PR #213

Supersedes PR #215.

Changes

weeks_controller.rb — add toggle_assign action with 4-state cycle, remove move action
weeks/index.html.erb — remove Schedule accordion, keep upload button (flag-gated)
week_assign_controller.js — handle other-crew state in pill toggle
week_move_controller.js — deleted (move action removed)
application.css — add other-crew pill styles, remove Schedule grid styles
today/index.html.erb — move other-crew section to bottom
routes.rb — remove move route, add toggle_assign
Specs updated for all changes

Test Plan

Week view loads without Schedule accordion
Assign pill cycles through all 4 states
Today view shows other-crew items at bottom
Upload button still hidden (schedule_digest flag OFF)

Review Checklist

Conflict resolution preserves both PR #213 and #215 intent
No code lost from either PR

Supersedes #215. Closes #214

## Summary - Moves "Assigned to Another Crew" items to the bottom of the Today queue - Removes the read-only Schedule accordion from Week view - Adds 4-state assign pill cycle (empty → assigned → completed → other-crew → destroyed) - Removes the move action (drag items between days) - Rebased onto current main to resolve conflicts with PR #213 Supersedes PR #215. ## Changes - `weeks_controller.rb` — add `toggle_assign` action with 4-state cycle, remove `move` action - `weeks/index.html.erb` — remove Schedule accordion, keep upload button (flag-gated) - `week_assign_controller.js` — handle other-crew state in pill toggle - `week_move_controller.js` — deleted (move action removed) - `application.css` — add other-crew pill styles, remove Schedule grid styles - `today/index.html.erb` — move other-crew section to bottom - `routes.rb` — remove move route, add toggle_assign - Specs updated for all changes ## Test Plan - [ ] Week view loads without Schedule accordion - [ ] Assign pill cycles through all 4 states - [ ] Today view shows other-crew items at bottom - [ ] Upload button still hidden (schedule_digest flag OFF) ## Review Checklist - [x] Conflict resolution preserves both PR #213 and #215 intent - [x] No code lost from either PR ## Related Notes Supersedes #215. Closes #214

ldraney added 8 commits

2026-06-14 12:10:44 +00:00

Move Assigned to Another Crew to bottom accordion, remove Schedule section from Week view bdac2e81c6

The Schedule grid duplicated what Assign Properties already shows. Day-letter
links now live on the Assign Properties header instead. Removes the move action,
week_move_controller, and all associated CSS.

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

4-state assign pill cycle: queued → completed → other crew → unassign 4a25f180f6

Replaces the 3-state toggle with a full cycle: empty (day letter) →
queued (blue) → completed (green checkmark) → other crew (amber people
icon) → destroy (back to empty). Fixes the red hover on queued pills —
now green to hint the next state is "complete", not "delete".

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

Fix missing done_by_other_this_week_ids in turbo stream partials 089f7d9630

Both create and destroy turbo streams render the recent_section partial
without passing the required local, causing NameError on queue actions.

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

Fix missing locals in turbo stream renders of other_crew_section e27422c3d8

mark_other and destroy turbo streams rendered other_crew_section without
queued_property_ids and date, causing NameError on queue actions.

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

Reclaim other-crew items when re-queuing from Assigned to Another Crew 86cc263c2e

Clicking "+" on an other-crew property now flips it back to queued instead
of hitting the unique constraint. The other_crew_section re-renders via
turbo stream so the item disappears from that list.

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

Make Queued badge clickable to unqueue from Last [Day] accordion 88ae072685

The static "Queued" badge in last-week items is now a button that
destroys the today queue item and re-renders with action buttons.
Also passes queued_items_by_property to all last_week_item renders.

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

Make Another Crew badge clickable to undo in Last [Day] accordion 8302e03c11

The static "Another crew" badge is now a delete button that removes the
other-crew assignment, re-rendering with queue/other-crew action buttons.
Mirrors the clickable Queued badge behavior.

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

Remove hover styles from badge buttons — touchscreen app

ci/woodpecker/push/woodpecker Pipeline failed

Details

ci/woodpecker/pr/woodpecker Pipeline failed

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

673c2422a5

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

ldraney commented

2026-06-14 12:13:45 +00:00

Author

Owner

PR #222 Review

DOMAIN REVIEW

Stack: Ruby on Rails 7 (ERB views, Stimulus JS, Turbo Streams, CSS custom properties). This is a server-rendered Rails app with Hotwire for partial-page updates.

Architecture assessment: The 4-state pill cycle (empty -> assigned -> completed -> other-crew -> destroyed) is cleanly implemented. The Stimulus controller refactor from boolean flags (wasAssigned, wasCompleted) to a #state(btn) helper that returns a string is a measurable improvement in readability and correctness. The extracted CHECK_SVG and OTHER_CREW_SVG constants eliminate inline SVG duplication.

Controller logic (weeks_controller.rb): The toggle_assign action correctly differentiates completed_by_other? -> destroy, completed? -> mark_done_by_other!, unassigned -> toggle_completion!, and missing -> create. The state machine ordering matters and is correct.

Model fitness: WorkQueueItem has mark_done_by_other! and toggle_completion! (verified), and completed_by_other is a boolean column so Rails auto-generates the ? predicate. No model changes needed for the new states.

CSS: Removed ~190 lines of dead .week-grid-* and .week-move / .day-picker styles. The new .is-other-crew and .is-completed:hover pill styles use design tokens (--color-warning, --color-danger) with hardcoded fallbacks (#f59e0b, #fef3c7). Fallbacks are acceptable as defensive defaults for the CSS custom property system.

Schedule removal: Clean. The 70-line <details> accordion, the @assigned/@unassigned partition logic, and the @item_ids hash are all removed together. No orphaned references remain.

Move action removal: week_move_controller.js deleted, move route removed from routes.rb, move action removed from weeks_controller.rb, and all PATCH /weeks/move specs removed. Clean teardown.

BLOCKERS

1. Missing @done_by_other_items_by_property in mark_other action -- will cause NoMethodError

The PR adds done_by_other_items_by_property: @done_by_other_items_by_property to the _last_week_item render call in mark_other.turbo_stream.erb (line 10 in diff). However, the mark_other action in work_queue_items_controller.rb is NOT modified by this PR. It never sets @done_by_other_items_by_property.

In Rails, unset instance variables evaluate to nil. The _last_week_item partial (post-PR) calls done_by_other_items_by_property[item.property_id] -- invoking [] on nil raises NoMethodError: undefined method '[]' for nil.

Affected flow: User clicks "Assign to another crew" on a Last Week item -> Turbo Stream response renders _last_week_item partial -> crash.

Fix: Add this to the mark_other action, after existing @done_by_other_this_week_ids assignment:

done_by_other_rows = WorkQueueItem
  .where(work_date: week_start..week_end, completed_by_other: true)
  .pluck(:property_id, :id)
@done_by_other_items_by_property = done_by_other_rows.to_h
@done_by_other_this_week_ids = done_by_other_rows.map(&:first).to_set

(This also consolidates the duplicate query into one, matching the pattern used in create, destroy, and index.)

Similarly: @queued_items_by_property is set in mark_other (line 177) but the PR also passes queued_items_by_property to _last_week_item in mark_other.turbo_stream.erb. This one IS set, so no crash. But confirm mark_other.turbo_stream.erb line 10 matches.

2. Missing @done_by_other_items_by_property in mark_other for _other_crew_section -- secondary crash path

The mark_other.turbo_stream.erb diff also passes queued_property_ids: @queued_property_ids, date: @date to _other_crew_section (line 6). These ARE set in the controller. No issue here. But if future changes to _other_crew_section reference done_by_other_items_by_property, same pattern applies. Currently safe.

NITS

Unused local in _recent_section renders: The PR passes done_by_other_items_by_property to _recent_section in create.turbo_stream.erb and destroy.turbo_stream.erb, but the _recent_section partial only uses done_by_other_this_week_ids (a Set of property IDs), not the items-by-property hash. The extra local is harmless but misleading. Consider removing it from those render calls to keep the interface clean.
DRY opportunity -- done_by_other_rows pattern: The two-step pattern (pluck rows, derive both Set and Hash) is now repeated in 4 controller locations: index, create, destroy, and the fix needed for mark_other. Consider extracting a private helper:

def done_by_other_lookups(date_range)
  rows = WorkQueueItem.where(work_date: date_range, completed_by_other: true).pluck(:property_id, :id)
  [rows.map(&:first).to_set, rows.to_h]
end

Render parameter lines are getting long. The _last_week_item render calls now have 7 locals. This is approaching the point where a presenter object or a locals: hash variable would improve readability.
Missing @done_by_other_this_week in create Turbo Stream: The PR adds a turbo_stream.replace "done-by-other-section" block in create.turbo_stream.erb that renders _other_crew_section with done_by_other_this_week: @done_by_other_this_week. The create action in the PR diff does set @done_by_other_this_week (added lines in the diff). Confirmed safe.

SOP COMPLIANCE

PR body has Summary, Changes, Test Plan, Related
Test Plan includes manual verification steps
No secrets or credentials committed
No .env files committed
Commit messages are descriptive
Changes are scoped to issue #214
Specs updated: new tests for other-crew state, removed tests for deleted move action
PR correctly notes it supersedes #215 and closes #214

PROCESS OBSERVATIONS

Net -252 lines (172 added, 424 deleted) -- healthy removal of dead feature code (Schedule grid, move action)
The 4-state cycle adds genuine UX value without adding complexity to the data model (reuses existing completed_by_other column)
This PR rebases cleanly onto main after PR #213 merge, as noted in the body. Good conflict management.
Deployment risk is low: all changes are in UI/controller layer with no migrations. Rollback is a revert.

VERDICT: NOT APPROVED

One BLOCKER: the mark_other action does not set @done_by_other_items_by_property, which will cause a NoMethodError when the "Assign to another crew" button is used on a Last Week item and the Turbo Stream response tries to render _last_week_item. Fix that controller action, and this is ready to merge.

## PR #222 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 7 (ERB views, Stimulus JS, Turbo Streams, CSS custom properties). This is a server-rendered Rails app with Hotwire for partial-page updates. **Architecture assessment:** The 4-state pill cycle (empty -> assigned -> completed -> other-crew -> destroyed) is cleanly implemented. The Stimulus controller refactor from boolean flags (`wasAssigned`, `wasCompleted`) to a `#state(btn)` helper that returns a string is a measurable improvement in readability and correctness. The extracted `CHECK_SVG` and `OTHER_CREW_SVG` constants eliminate inline SVG duplication. **Controller logic (weeks_controller.rb):** The `toggle_assign` action correctly differentiates `completed_by_other?` -> destroy, `completed?` -> `mark_done_by_other!`, unassigned -> `toggle_completion!`, and missing -> create. The state machine ordering matters and is correct. **Model fitness:** `WorkQueueItem` has `mark_done_by_other!` and `toggle_completion!` (verified), and `completed_by_other` is a boolean column so Rails auto-generates the `?` predicate. No model changes needed for the new states. **CSS:** Removed ~190 lines of dead `.week-grid-*` and `.week-move` / `.day-picker` styles. The new `.is-other-crew` and `.is-completed:hover` pill styles use design tokens (`--color-warning`, `--color-danger`) with hardcoded fallbacks (`#f59e0b`, `#fef3c7`). Fallbacks are acceptable as defensive defaults for the CSS custom property system. **Schedule removal:** Clean. The 70-line `<details>` accordion, the `@assigned`/`@unassigned` partition logic, and the `@item_ids` hash are all removed together. No orphaned references remain. **Move action removal:** `week_move_controller.js` deleted, `move` route removed from `routes.rb`, `move` action removed from `weeks_controller.rb`, and all `PATCH /weeks/move` specs removed. Clean teardown. ### BLOCKERS **1. Missing `@done_by_other_items_by_property` in `mark_other` action -- will cause NoMethodError** The PR adds `done_by_other_items_by_property: @done_by_other_items_by_property` to the `_last_week_item` render call in `mark_other.turbo_stream.erb` (line 10 in diff). However, the `mark_other` action in `work_queue_items_controller.rb` is NOT modified by this PR. It never sets `@done_by_other_items_by_property`. In Rails, unset instance variables evaluate to `nil`. The `_last_week_item` partial (post-PR) calls `done_by_other_items_by_property[item.property_id]` -- invoking `[]` on `nil` raises `NoMethodError: undefined method '[]' for nil`. **Affected flow:** User clicks "Assign to another crew" on a Last Week item -> Turbo Stream response renders `_last_week_item` partial -> crash. **Fix:** Add this to the `mark_other` action, after existing `@done_by_other_this_week_ids` assignment: ```ruby done_by_other_rows = WorkQueueItem .where(work_date: week_start..week_end, completed_by_other: true) .pluck(:property_id, :id) @done_by_other_items_by_property = done_by_other_rows.to_h @done_by_other_this_week_ids = done_by_other_rows.map(&:first).to_set ``` (This also consolidates the duplicate query into one, matching the pattern used in `create`, `destroy`, and `index`.) **Similarly:** `@queued_items_by_property` is set in `mark_other` (line 177) but the PR also passes `queued_items_by_property` to `_last_week_item` in `mark_other.turbo_stream.erb`. This one IS set, so no crash. But confirm `mark_other.turbo_stream.erb` line 10 matches. **2. Missing `@done_by_other_items_by_property` in `mark_other` for `_other_crew_section` -- secondary crash path** The `mark_other.turbo_stream.erb` diff also passes `queued_property_ids: @queued_property_ids, date: @date` to `_other_crew_section` (line 6). These ARE set in the controller. No issue here. But if future changes to `_other_crew_section` reference `done_by_other_items_by_property`, same pattern applies. Currently safe. ### NITS 1. **Unused local in `_recent_section` renders:** The PR passes `done_by_other_items_by_property` to `_recent_section` in `create.turbo_stream.erb` and `destroy.turbo_stream.erb`, but the `_recent_section` partial only uses `done_by_other_this_week_ids` (a Set of property IDs), not the items-by-property hash. The extra local is harmless but misleading. Consider removing it from those render calls to keep the interface clean. 2. **DRY opportunity -- `done_by_other_rows` pattern:** The two-step pattern (pluck rows, derive both Set and Hash) is now repeated in 4 controller locations: `index`, `create`, `destroy`, and the fix needed for `mark_other`. Consider extracting a private helper: ```ruby def done_by_other_lookups(date_range) rows = WorkQueueItem.where(work_date: date_range, completed_by_other: true).pluck(:property_id, :id) [rows.map(&:first).to_set, rows.to_h] end ``` 3. **Render parameter lines are getting long.** The `_last_week_item` render calls now have 7 locals. This is approaching the point where a presenter object or a `locals:` hash variable would improve readability. 4. **Missing `@done_by_other_this_week` in `create` Turbo Stream:** The PR adds a `turbo_stream.replace "done-by-other-section"` block in `create.turbo_stream.erb` that renders `_other_crew_section` with `done_by_other_this_week: @done_by_other_this_week`. The `create` action in the PR diff does set `@done_by_other_this_week` (added lines in the diff). Confirmed safe. ### SOP COMPLIANCE - [x] PR body has Summary, Changes, Test Plan, Related - [x] Test Plan includes manual verification steps - [x] No secrets or credentials committed - [x] No `.env` files committed - [x] Commit messages are descriptive - [x] Changes are scoped to issue #214 - [x] Specs updated: new tests for other-crew state, removed tests for deleted move action - [x] PR correctly notes it supersedes #215 and closes #214 ### PROCESS OBSERVATIONS - Net -252 lines (172 added, 424 deleted) -- healthy removal of dead feature code (Schedule grid, move action) - The 4-state cycle adds genuine UX value without adding complexity to the data model (reuses existing `completed_by_other` column) - This PR rebases cleanly onto main after PR #213 merge, as noted in the body. Good conflict management. - Deployment risk is low: all changes are in UI/controller layer with no migrations. Rollback is a revert. ### VERDICT: NOT APPROVED One BLOCKER: the `mark_other` action does not set `@done_by_other_items_by_property`, which will cause a `NoMethodError` when the "Assign to another crew" button is used on a Last Week item and the Turbo Stream response tries to render `_last_week_item`. Fix that controller action, and this is ready to merge.