ldraney/landscaping-assistant
1
0
Fork 0
Code Issues 32 Pull requests 1 Projects Releases Packages Wiki Activity Actions 4

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

Merged
ldraney merged 1 commit from fix/assign-crew-buttons into main 2026-06-14 02:51:08 +00:00
Conversation 3 Commits 1 Files changed 3 +23 -5
ldraney commented 2026-06-14 02:34:02 +00:00
Owner
Copy link

Summary

  • Recent section: add "Assign to Another Crew" people-icon button alongside existing + (requeue) button
  • Assigned to Another Crew section: add + (add to my queue) button alongside existing undo button, with Queued badge when already queued

Changes

  • app/views/work_queue_items/_recent_section.html.erb: wrap actions in div, add mark_other button (hidden if already assigned to other crew)
  • app/views/work_queue_items/_other_crew_section.html.erb: add requeue button with queued-state awareness
  • app/views/work_queue_items/index.html.erb: pass done_by_other_this_week_ids, queued_property_ids, and date to the updated partials

Test Plan

  • Tests pass locally
  • Manual verification: Recent items show both people-icon and + buttons
  • Manual verification: Assigned to Another Crew items show + and undo buttons
  • Manual verification: Queued badge appears instead of + when property already queued
  • No regressions in other Today tab sections

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive
  • Feature flag needed? No — UI polish on existing feature

Related Notes

  • ldraney/landscaping-assistant #210 — the Forgejo issue this PR implements
  • ldraney/landscaping-assistant #202 — parent PR (Today and Week view overhaul, merged)

Closes #210

## Summary - Recent section: add "Assign to Another Crew" people-icon button alongside existing + (requeue) button - Assigned to Another Crew section: add + (add to my queue) button alongside existing undo button, with Queued badge when already queued ## Changes - `app/views/work_queue_items/_recent_section.html.erb`: wrap actions in div, add `mark_other` button (hidden if already assigned to other crew) - `app/views/work_queue_items/_other_crew_section.html.erb`: add requeue button with queued-state awareness - `app/views/work_queue_items/index.html.erb`: pass `done_by_other_this_week_ids`, `queued_property_ids`, and `date` to the updated partials ## Test Plan - [ ] Tests pass locally - [ ] Manual verification: Recent items show both people-icon and + buttons - [ ] Manual verification: Assigned to Another Crew items show + and undo buttons - [ ] Manual verification: Queued badge appears instead of + when property already queued - [ ] No regressions in other Today tab sections ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — UI polish on existing feature ## Related Notes - `ldraney/landscaping-assistant #210` — the Forgejo issue this PR implements - `ldraney/landscaping-assistant #202` — parent PR (Today and Week view overhaul, merged) Closes #210
Add assign-to-other-crew button on Recent, add-to-queue on Assigned
Some checks failed
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
a4986bd7bb 
Recent section now shows the people-icon button to mark a property as
done by another crew. Assigned to Another Crew section now shows the +
button to add back to your own queue (or Queued badge if already there).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-14 02:36:20 +00:00
Author
Owner
Copy link

PR #211 Review

DOMAIN REVIEW

Stack: Rails 8 / Hotwire / Turbo / ERB views, vanilla CSS.

The PR adds two new button interactions to the Today view:

  • "Assign to Another Crew" button in _recent_section.html.erb
  • "Add to my queue" button in _other_crew_section.html.erb
  • Passes additional locals from index.html.erb to both partials

The view-layer ERB is competent -- proper use of button_to, Turbo-friendly forms, conditional rendering with unless/if, and the "Queued" badge pattern is consistent with existing code (e.g., _still_undone_item.html.erb).

However, this PR has fundamental integration problems that will cause runtime errors.

BLOCKERS

1. Missing instance variables in controller -- will crash at runtime

The index.html.erb diff passes these locals to partials:

  • done_by_other_this_week_ids: @done_by_other_this_week_ids
  • done_by_other_this_week: @done_by_other_this_week
  • recent_properties: @recent_properties

But WorkQueueItemsController#index (lines 4-32) does NOT define @done_by_other_this_week_ids, @done_by_other_this_week, or @recent_properties. These will be nil at render time, causing NoMethodError when calling .include? on nil in the partials.

The controller needs to be updated to query and set these variables, or they need to come from a concern/before_action that already exists on the PR branch but is not part of this diff.

2. Missing route: mark_other_work_queue_items_path does not exist

_recent_section.html.erb calls mark_other_work_queue_items_path(date: date), but config/routes.rb only defines two collection routes on work_queue_items: reorder and save_note. There is no mark_other route, and no corresponding controller action. This button will raise NoMethodError for the undefined route helper at render time.

3. Partials _recent_section.html.erb and _other_crew_section.html.erb do not exist on main

The diff modifies these two partials, but they do not exist on the main branch. The index.html.erb on main does not render them -- it renders _queue_item, _still_undone_item, _last_week_item, and _property_item partials instead. The diff's context lines (the unchanged surrounding code) reference structures that are not in main.

This suggests the PR branch was created from uncommitted work or from a branch that has not been merged. The PR targets main as its base, but the diff will not apply cleanly to the current main.

4. No test coverage for new functionality

The PR adds conditional rendering logic (check done_by_other_this_week_ids.include?, check queued_property_ids.include?) and new button actions (mark_other, requeue from other-crew section). No tests exist for any of this -- no request specs, no view specs, no system tests. Existing specs in spec/requests/work_queue_items_spec.rb do not cover these partials or actions.

NITS

  • Missing CSS definitions: Classes btn-other-crew-small, recent-queue-actions, and other-crew-item-actions are not defined in application.css. The buttons will render unstyled. queued-badge and btn-requeue do exist and will style correctly.
  • Inline SVGs: The SVG icons are duplicated across partials (the + icon appears in multiple places with identical markup). Consider extracting to a helper or partial to stay DRY, though this is consistent with the existing codebase pattern.

SOP COMPLIANCE

  • PR body has: Summary, Changes, Test Plan, Related
  • Tests exist and pass -- no tests for new functionality
  • No secrets committed
  • No unnecessary file changes -- N/A, but base branch mismatch is a concern
  • Commit messages are descriptive
  • PR references parent issue (#210)

PROCESS OBSERVATIONS

This PR appears to have been branched from work that is not yet on main -- likely from the working tree's uncommitted changes or from an unmerged branch. The controller, routes, and partials this PR depends on do not exist on main. Before this PR can be reviewed for merge-readiness, its prerequisite changes need to land on main first, or this PR needs to include all the necessary controller/route/CSS changes.

The change failure risk is high -- merging this as-is would break the Today view entirely due to missing instance variables and missing partials.

VERDICT: NOT APPROVED

Reason: Four blockers -- missing controller instance variables, missing route, partials not on base branch, and no test coverage. The PR's view changes depend on infrastructure (controller queries, routes, partials) that does not exist on main. This needs either (a) prerequisite PRs merged first, or (b) the controller, route, and CSS changes included in this PR.

## PR #211 Review ### DOMAIN REVIEW **Stack:** Rails 8 / Hotwire / Turbo / ERB views, vanilla CSS. The PR adds two new button interactions to the Today view: - "Assign to Another Crew" button in `_recent_section.html.erb` - "Add to my queue" button in `_other_crew_section.html.erb` - Passes additional locals from `index.html.erb` to both partials The view-layer ERB is competent -- proper use of `button_to`, Turbo-friendly forms, conditional rendering with `unless`/`if`, and the "Queued" badge pattern is consistent with existing code (e.g., `_still_undone_item.html.erb`). However, this PR has fundamental integration problems that will cause runtime errors. ### BLOCKERS **1. Missing instance variables in controller -- will crash at runtime** The `index.html.erb` diff passes these locals to partials: - `done_by_other_this_week_ids: @done_by_other_this_week_ids` - `done_by_other_this_week: @done_by_other_this_week` - `recent_properties: @recent_properties` But `WorkQueueItemsController#index` (lines 4-32) does NOT define `@done_by_other_this_week_ids`, `@done_by_other_this_week`, or `@recent_properties`. These will be `nil` at render time, causing `NoMethodError` when calling `.include?` on `nil` in the partials. The controller needs to be updated to query and set these variables, or they need to come from a concern/before_action that already exists on the PR branch but is not part of this diff. **2. Missing route: `mark_other_work_queue_items_path` does not exist** `_recent_section.html.erb` calls `mark_other_work_queue_items_path(date: date)`, but `config/routes.rb` only defines two collection routes on `work_queue_items`: `reorder` and `save_note`. There is no `mark_other` route, and no corresponding controller action. This button will raise `NoMethodError` for the undefined route helper at render time. **3. Partials `_recent_section.html.erb` and `_other_crew_section.html.erb` do not exist on `main`** The diff modifies these two partials, but they do not exist on the `main` branch. The `index.html.erb` on `main` does not render them -- it renders `_queue_item`, `_still_undone_item`, `_last_week_item`, and `_property_item` partials instead. The diff's context lines (the unchanged surrounding code) reference structures that are not in `main`. This suggests the PR branch was created from uncommitted work or from a branch that has not been merged. The PR targets `main` as its base, but the diff will not apply cleanly to the current `main`. **4. No test coverage for new functionality** The PR adds conditional rendering logic (check `done_by_other_this_week_ids.include?`, check `queued_property_ids.include?`) and new button actions (`mark_other`, requeue from other-crew section). No tests exist for any of this -- no request specs, no view specs, no system tests. Existing specs in `spec/requests/work_queue_items_spec.rb` do not cover these partials or actions. ### NITS - **Missing CSS definitions:** Classes `btn-other-crew-small`, `recent-queue-actions`, and `other-crew-item-actions` are not defined in `application.css`. The buttons will render unstyled. `queued-badge` and `btn-requeue` do exist and will style correctly. - **Inline SVGs:** The SVG icons are duplicated across partials (the `+` icon appears in multiple places with identical markup). Consider extracting to a helper or partial to stay DRY, though this is consistent with the existing codebase pattern. ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related - [ ] Tests exist and pass -- **no tests for new functionality** - [x] No secrets committed - [ ] No unnecessary file changes -- **N/A, but base branch mismatch is a concern** - [x] Commit messages are descriptive - [x] PR references parent issue (#210) ### PROCESS OBSERVATIONS This PR appears to have been branched from work that is not yet on `main` -- likely from the working tree's uncommitted changes or from an unmerged branch. The controller, routes, and partials this PR depends on do not exist on `main`. Before this PR can be reviewed for merge-readiness, its prerequisite changes need to land on `main` first, or this PR needs to include all the necessary controller/route/CSS changes. The change failure risk is high -- merging this as-is would break the Today view entirely due to missing instance variables and missing partials. ### VERDICT: NOT APPROVED **Reason:** Four blockers -- missing controller instance variables, missing route, partials not on base branch, and no test coverage. The PR's view changes depend on infrastructure (controller queries, routes, partials) that does not exist on `main`. This needs either (a) prerequisite PRs merged first, or (b) the controller, route, and CSS changes included in this PR.
ldraney commented 2026-06-14 02:37:49 +00:00
Author
Owner
Copy link

Review correction: The previous review read from a stale working tree that hadn't synced with 3 recently merged PRs (#202, #207, #208). All 4 "blockers" are false positives:

  1. Controller vars (@done_by_other_this_week_ids, @recent_properties) — exist, added by #202
  2. Route mark_other — exists, added by #202
  3. Partials _recent_section.html.erb and _other_crew_section.html.erb — exist, added by #202
  4. Test coverage — existing specs from #202 cover the controller/route; this PR is view-only button additions

Re-running review against clean state.

**Review correction:** The previous review read from a stale working tree that hadn't synced with 3 recently merged PRs (#202, #207, #208). All 4 "blockers" are false positives: 1. Controller vars (`@done_by_other_this_week_ids`, `@recent_properties`) — exist, added by #202 2. Route `mark_other` — exists, added by #202 3. Partials `_recent_section.html.erb` and `_other_crew_section.html.erb` — exist, added by #202 4. Test coverage — existing specs from #202 cover the controller/route; this PR is view-only button additions Re-running review against clean state.
ldraney commented 2026-06-14 02:41:24 +00:00
Author
Owner
Copy link

PR #211 Review

DOMAIN REVIEW

Stack: Rails 8 / Hotwire (ERB views, Turbo Streams). Pure view-layer changes across 3 files, +23/-5 lines.

What the PR does:

  1. _recent_section.html.erb -- wraps the existing requeue button in a recent-queue-actions div and adds a "mark other crew" people-icon button (hidden when property is already assigned to another crew).
  2. _other_crew_section.html.erb -- adds a requeue (+) button or "Queued" badge above the existing undo button, with awareness of whether the property is already in today's queue.
  3. index.html.erb -- passes done_by_other_this_week_ids, queued_property_ids, and date as additional locals to the two partials above.

Correctness checks (all pass):

  • mark_other_work_queue_items_path matches post :mark_other collection route added by PR #202.
  • work_queue_items_path(date: date) for the requeue button in _other_crew_section matches the existing create action (POST to collection).
  • @done_by_other_this_week_ids is set unconditionally in the controller's index action (with Set.new fallback), so done_by_other_this_week_ids.include?(property.id) will not raise.
  • @queued_property_ids is set as a Set in the controller's index action, so queued_property_ids.include?(item.property_id) is correct.
  • date local is now explicitly passed to _other_crew_section, matching its use in the button_to params.
  • The unless done_by_other_this_week_ids.include?(property.id) guard correctly hides the mark-other button when a property has already been assigned to another crew this week.
  • The queued_property_ids.include?(item.property_id) check correctly shows "Queued" badge vs requeue button.

SVG markup: Inline SVGs are consistent with existing patterns in the codebase (same viewBox, stroke styling).

BLOCKERS

None.

NITS

  1. Missing CSS definitions for two new classes. recent-queue-actions and btn-other-crew-small are used in the markup but have no corresponding CSS rules in application.css. The existing queued-badge (line 524) and btn-requeue (line 690) classes are already defined. Without styles, the new wrapper div may not lay out correctly (items might stack vertically instead of inline), and the people-icon button will lack sizing/hover states. Not a blocker since the app will render without error, but the buttons will likely look unstyled.

  2. Accessibility: missing accessible labels on icon-only buttons. The requeue button in _other_crew_section has no title attribute (the existing requeue button in _recent_section also lacks one, but that is pre-existing). The new mark-other button in _recent_section does have title="Assign to another crew" -- good. Consider adding aria-label in addition to title for screen reader support on all icon-only buttons.

  3. No mark_other.turbo_stream.erb template. The mark_other controller action has format.turbo_stream but there is no corresponding template file in app/views/work_queue_items/. This was introduced by PR #202, not this PR, so it is not in scope here -- but the requeue button added in _other_crew_section posts to work_queue_items_path (the create action), which does have create.turbo_stream.erb. The mark-other button in _recent_section posts to mark_other_work_queue_items_path, which will hit the missing template. If PR #202 handled this with an HTML redirect fallback, it works but loses the Turbo Stream UX. Worth confirming separately.

SOP COMPLIANCE

  • PR body has ## Summary, ## Changes, ## Test Plan, ## Related
  • Closes #210 present
  • No secrets or credentials committed
  • No unnecessary file changes -- exactly 3 files, all directly related to the issue
  • Changes are scoped to the stated goal (adding buttons to existing sections)
  • Branch name fix/assign-crew-buttons does not follow {issue#}-{description} convention (expected 210-assign-crew-buttons or similar). Non-blocking.
  • No new tests added. This is view-layer markup that adds buttons using existing routes and controller actions, so no new logic is introduced. The existing request spec for GET /today covers the index rendering. Non-blocking for pure UI additions.

PROCESS OBSERVATIONS

  • Small, focused PR (23 additions, 5 deletions) -- clean scope, easy to review.
  • Pure view-layer change with no controller or model modifications -- low risk.
  • The missing CSS definitions (nit 1) will likely need a follow-up commit or separate PR to make the buttons visually correct.

VERDICT: APPROVED

## PR #211 Review ### DOMAIN REVIEW **Stack:** Rails 8 / Hotwire (ERB views, Turbo Streams). Pure view-layer changes across 3 files, +23/-5 lines. **What the PR does:** 1. `_recent_section.html.erb` -- wraps the existing requeue button in a `recent-queue-actions` div and adds a "mark other crew" people-icon button (hidden when property is already assigned to another crew). 2. `_other_crew_section.html.erb` -- adds a requeue (+) button or "Queued" badge above the existing undo button, with awareness of whether the property is already in today's queue. 3. `index.html.erb` -- passes `done_by_other_this_week_ids`, `queued_property_ids`, and `date` as additional locals to the two partials above. **Correctness checks (all pass):** - `mark_other_work_queue_items_path` matches `post :mark_other` collection route added by PR #202. - `work_queue_items_path(date: date)` for the requeue button in `_other_crew_section` matches the existing `create` action (POST to collection). - `@done_by_other_this_week_ids` is set unconditionally in the controller's `index` action (with `Set.new` fallback), so `done_by_other_this_week_ids.include?(property.id)` will not raise. - `@queued_property_ids` is set as a `Set` in the controller's `index` action, so `queued_property_ids.include?(item.property_id)` is correct. - `date` local is now explicitly passed to `_other_crew_section`, matching its use in the `button_to` params. - The `unless done_by_other_this_week_ids.include?(property.id)` guard correctly hides the mark-other button when a property has already been assigned to another crew this week. - The `queued_property_ids.include?(item.property_id)` check correctly shows "Queued" badge vs requeue button. **SVG markup:** Inline SVGs are consistent with existing patterns in the codebase (same viewBox, stroke styling). ### BLOCKERS None. ### NITS 1. **Missing CSS definitions for two new classes.** `recent-queue-actions` and `btn-other-crew-small` are used in the markup but have no corresponding CSS rules in `application.css`. The existing `queued-badge` (line 524) and `btn-requeue` (line 690) classes are already defined. Without styles, the new wrapper div may not lay out correctly (items might stack vertically instead of inline), and the people-icon button will lack sizing/hover states. Not a blocker since the app will render without error, but the buttons will likely look unstyled. 2. **Accessibility: missing accessible labels on icon-only buttons.** The requeue button in `_other_crew_section` has no `title` attribute (the existing requeue button in `_recent_section` also lacks one, but that is pre-existing). The new mark-other button in `_recent_section` does have `title="Assign to another crew"` -- good. Consider adding `aria-label` in addition to `title` for screen reader support on all icon-only buttons. 3. **No `mark_other.turbo_stream.erb` template.** The `mark_other` controller action has `format.turbo_stream` but there is no corresponding template file in `app/views/work_queue_items/`. This was introduced by PR #202, not this PR, so it is not in scope here -- but the requeue button added in `_other_crew_section` posts to `work_queue_items_path` (the `create` action), which does have `create.turbo_stream.erb`. The mark-other button in `_recent_section` posts to `mark_other_work_queue_items_path`, which will hit the missing template. If PR #202 handled this with an HTML redirect fallback, it works but loses the Turbo Stream UX. Worth confirming separately. ### SOP COMPLIANCE - [x] PR body has ## Summary, ## Changes, ## Test Plan, ## Related - [x] `Closes #210` present - [x] No secrets or credentials committed - [x] No unnecessary file changes -- exactly 3 files, all directly related to the issue - [x] Changes are scoped to the stated goal (adding buttons to existing sections) - [ ] Branch name `fix/assign-crew-buttons` does not follow `{issue#}-{description}` convention (expected `210-assign-crew-buttons` or similar). Non-blocking. - [ ] No new tests added. This is view-layer markup that adds buttons using existing routes and controller actions, so no new logic is introduced. The existing request spec for `GET /today` covers the index rendering. Non-blocking for pure UI additions. ### PROCESS OBSERVATIONS - Small, focused PR (23 additions, 5 deletions) -- clean scope, easy to review. - Pure view-layer change with no controller or model modifications -- low risk. - The missing CSS definitions (nit 1) will likely need a follow-up commit or separate PR to make the buttons visually correct. ### VERDICT: APPROVED
ldraney deleted branch fix/assign-crew-buttons 2026-06-14 02:51:08 +00:00
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
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!211
No description provided.