Move Assigned to Another Crew to bottom, remove Schedule section #215
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "move-other-crew-bottom-remove-schedule"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
week_move_controller.js, move action/route, ~350 lines of dead CSSChanges
app/views/work_queue_items/index.html.erb: movedother_crew_sectionrender from position 2 to afterremaining_sectionapp/views/weeks/index.html.erb: removed Schedule<details>block (69 lines), converted day-label<div>to<a>linking towork_queue_items_path(date: day)app/controllers/weeks_controller.rb: removedmoveaction,@assigned,@unassigned,@item_ids; simplified pluck to drop:idapp/assets/stylesheets/application.css: removed.week-grid-*,.week-cell-*,.day-picker-*CSS; added.week-assign-day-linkstylesapp/javascript/controllers/week_move_controller.js: deleted (no remaining references)config/routes.rb: removedpatch :moveroutespec/requests/weeks_spec.rb: removed move specs, updated Schedule-dependent assertionsTest Plan
bundle exec rspec spec/requests/weeks_spec.rb— 17 examples, 0 failuresReview Checklist
Related Notes
landscaping-assistant— project this affectsCloses #214
PR #215 Review
DOMAIN REVIEW
Stack: Rails 8 / ERB / Stimulus / RSpec request specs / vanilla CSS
Controller (
app/controllers/weeks_controller.rb)moveaction and its three instance variables (@assigned,@unassigned,@item_ids) are cleanly removed. The remainingindexaction correctly drops:idfrom thepluckcall since no code consumes item IDs anymore. Thetoggle_assignaction is untouched and correct.pluckstill avoids loading full AR objects.View (
app/views/weeks/index.html.erb)<details>block is fully removed. The day-letter labels in the Assign Properties header are correctly converted from<div>to<a>linking towork_queue_items_path(date: day). Theis-todayclass is preserved for today-highlighting.View (
app/views/work_queue_items/index.html.erb)other_crew_sectionpartial render moves from afterrecent_sectionto afterremaining_section, placing it at the bottom of the today accordion stack. The partial call signature (locals) is identical before and after -- no risk of missing variables.Routes (
config/routes.rb)patch :moveremoved from weeks collection routes. Onlypost :toggle_assignremains. Clean.JS (
app/javascript/controllers/week_move_controller.js)week-moveorweek_moveanywhere in the codebase (verified via grep across app/, config/, spec/).CSS (
app/assets/stylesheets/application.css).week-grid-*,.week-cell-*,.day-picker-*rules removed. The new.week-assign-day-linkstyles are added in the correct location under the WeekAssign component section. The old.week-grid-day-linkselector in the link-reset rule block (line ~447) is correctly replaced with.week-assign-day-link..week-cell-wrapper,.week-cell-btn,.day-picker,.day-picker-btnrules removed. These belonged exclusively to the deletedweek_move_controller.jsinteraction.Specs (
spec/requests/weeks_spec.rb)PATCH /weeks/movedescribe block (4 examples) is fully removed -- correct, since the route and action no longer exist.is-completedCSS class instead of the removed "Completed" aria-label text. This correctly matches the view output at line 85 ofweeks/index.html.erbwhich rendersis-completedon completed pills.week-assign-day-link-- matches the new view output at line 69.Dangling reference audit: Grep across the entire codebase found zero remaining references to
week_move,week-move,move_weeks_path,day-picker,week-grid,week-cell,@item_ids,@assigned, or@unassignedin the weeks context. Cleanup is thorough.BLOCKERS
None.
moveaction's input parsing is gone, not modified)NITS
Day-link
link_toon one line (line 69 ofweeks/index.html.erb): Thelink_toblock and its content are compressed onto a single line. This works but is harder to scan than the multi-line format used elsewhere in the file. Cosmetic only.Spec description wording (line 23 of
weeks_spec.rb):"shows completion state for completed items"is slightly vague -- it's really testing that theis-completedCSS class appears on the pill. Consider"renders is-completed class for completed items". Non-blocking.SOP COMPLIANCE
PROCESS OBSERVATIONS
rendercall position change.VERDICT: APPROVED
PR #215 Review (Re-review after second commit)
DOMAIN REVIEW
Tech stack: Rails 8, Stimulus JS, ERB templates, RSpec request specs, CSS (no preprocessor).
State machine alignment (4-state pill cycle):
The JS client and Rails controller implement the same 4-state cycle. Traced through both sides:
toggle_assign)is-assignedcreate!-> 201is-completed+ check SVGtoggle_completion!-> 204is-other-crew+ people SVGmark_done_by_other!-> 204is-other-crew+ day letterdestroy!-> 200Both sides are aligned. The
#state()private method in JS cleanly maps CSS classes to state names, and#revert()correctly restores any prior state on fetch failure. Good refactor from the previous boolean flags (wasAssigned/wasCompleted) to a singleprevstate enum.Model methods (
WorkQueueItem):toggle_completion!correctly resetscompleted_by_other: falsewhen toggling back to incomplete. No stale flag risk.mark_done_by_other!sets bothcompleted: trueandcompleted_by_other: truewith timestamp. Correct.completed_by_other?beforecompleted?in the if-chain, so the more specific state is matched first. Correct ordering.Controller (
weeks_controller.rb):@completions,@queued,@done_by_other) correctly checkscompleted && completed_by_otherfirst, preventing items from landing in both@completionsand@done_by_other.@total_completedonly counts@completions.keys(own-crew completions), excluding other-crew. This is the right business metric for "our crew's progress."@item_ids,@assigned,@unassigned,moveaction,scheduled_ids. No orphaned references.CSS:
is-other-crewpill styles (amber warning palette) are consistent with the existing pill pattern (is-assigned,is-completed).is-assignedpills changed from red (--color-danger) to green (--color-success). This makes sense: hovering on a queued item previews the "complete" action (green), not "destroy" (red). Good UX fix.is-completedpreviews amber (other-crew transition). Hover onis-other-crewpreviews red (destroy). Both match the next state in the cycle.--color-warning(#f59e0b) and--color-warning-light(#fef3c7): these are fine as defensive CSS, but the fallbacks are hardcoded hex values. Non-blocking, but worth confirming these custom properties are defined in the theme.JavaScript (
week_assign_controller.js):CHECK_SVGandOTHER_CREW_SVG. Eliminates duplication betweentoggle()and#revert(). Clean.is-${state}producesis-other-crewfor the "other-crew" state string. Matches CSS class. Verified.#state,#revert) use native ES2022 private syntax. Consistent with the project's JS baseline (importmaps + no transpiler for private fields means this requires modern browser support, but the project already uses this pattern).Deleted code:
week_move_controller.js(38 lines),moveaction (18 lines),patch :moveroute, ~350 lines of dead CSS (.week-grid-*,.week-cell-*,.day-picker-*). All cleanly removed with no orphaned references in templates or routes.ERB template (
weeks/index.html.erb):other_crewvariable is correctly derived from@done_by_otherand rendered with mutually exclusive CSS classes.work_queue_items_path(date: day)to navigate to the daily view.Work queue item index (
work_queue_items/index.html.erb):other_crew_sectionrender moved from position 2 (after recent) to afterremaining_section. Layout change only, no logic change. Correct.BLOCKERS
None.
NITS
CSS fallback hex values (
#f59e0b,#fef3c7): If--color-warningand--color-warning-lightare defined in the theme, the fallbacks are dead code. If they are NOT defined, these hardcoded values bypass the theme system. Worth adding them to the theme variables if missing, or removing the fallbacks if present.Completion count excludes other-crew:
@total_completedcounts only own-crew completions. This is probably the right metric, but the label just says "completed" with no qualifier. If a user marks 3 items as "other crew," the count might be confusing. Consider "3/10 completed by us" or similar if users report confusion. Non-blocking.SOP COMPLIANCE
is-completedandis-other-crew)Closes #214presentPROCESS OBSERVATIONS
completed_by_other/is-other-crewpath.VERDICT: APPROVED
PR #215 Review
DOMAIN REVIEW
Stack: Rails 7 / Turbo Streams / Stimulus / CSS. Reviewing for controller correctness, partial local consistency, Stimulus state machine integrity, and dead-code cleanup.
What this PR does well:
moveaction,week_move_controller.js, and ~350 lines of orphaned CSS. No remaining references tomove_weeks_pathorweek-move#togglein production code.#state()helper and#revert()private methods are a clear improvement over the old boolean flags._other_crew_sectionis correctly moved from position 2 (between Recent and Last Week) to the bottom (after Remaining) inindex.html.erb.other-crewstate and day-label links. Coverage for the newmark_done_by_other!transition and theother-crewdestroy transition is present.BLOCKERS
1. WeeksController#toggle_assign state machine does not match the JS cycle (CORRECTNESS BUG)
The Stimulus controller (
week_assign_controller.js) implements a 4-state cycle:But the PR-branch
toggle_assignaction (per the diff) implements:The priority ordering is correct --
completed_by_other?is checked beforecompleted?, which matters becausecompleted_by_otheritems also havecompleted: true. This is actually fine. No bug here on second analysis.However, there IS a subtle issue:
toggle_completion!on the model doesupdate!(completed: false, completed_at: nil, completed_by_other: false)when the item is already completed. Buttoggle_assignonly callstoggle_completion!in theelsebranch (when!completed? && !completed_by_other?), so it will only ever toggle fromcompleted: falsetocompleted: true. This is correct.Withdrawing this blocker -- the state machine is sound upon full trace.
2.
createaction:@done_by_other_this_weeknot set, butcreate.turbo_stream.erbrendersother_crew_section(RUNTIME ERROR)The PR diff adds this to
create.turbo_stream.erb:But the
createaction inwork_queue_items_controller.rb(per the diff) does NOT set@done_by_other_this_week. It sets@done_by_other_this_week_idsand@done_by_other_items_by_property, but NOT the ActiveRecord collection@done_by_other_this_weekthat the partial iterates over with.each.This will cause a NoMethodError (
undefined method 'any?' for nil:NilClass) when creating a work queue item via the Today view, because_other_crew_section.html.erbline 2 callsdone_by_other_this_week.any?.The
mark_otheranddestroyactions both set@done_by_other_this_weekcorrectly. Thecreateaction was missed.Fix: Add to the
createaction's success path:3.
destroyaction:@done_by_other_items_by_propertynot set, butdestroy.turbo_stream.erbpasses it to_last_week_item(RUNTIME ERROR)The PR diff changes
destroy.turbo_stream.erbto passdone_by_other_items_by_property: @done_by_other_items_by_propertyandqueued_items_by_property: @queued_items_by_propertyto the_last_week_itempartial.The
destroyaction on main sets@queued_items_by_property(line 132), but does NOT set@done_by_other_items_by_property. The PR diff forwork_queue_items_controller.rbadds@done_by_other_items_by_propertyto thecreate,update(toggle), andmark_otherpaths, but the destroy action is not in the diff at all -- meaning it is unchanged and still lacks@done_by_other_items_by_property.The
_last_week_itempartial (per the PR diff) now rendersbutton_tousingdone_by_other_items_by_property[item.property_id]to build the destroy path. If@done_by_other_items_by_propertyis nil, this will raise a NoMethodError when destroying a queued item that has a last-week counterpart.Fix: Add to the
destroyaction:4.
mark_otheraction:@done_by_other_items_by_propertyand@queued_items_by_propertynot set for_last_week_item(RUNTIME ERROR)Same pattern. The
mark_other.turbo_stream.erbdiff passesdone_by_other_items_by_propertyandqueued_items_by_propertyto_last_week_item, but themark_otheraction on the controller does not set@done_by_other_items_by_property. It does set@queued_items_by_property(line 177), but not the other.Fix: Add to the
mark_otheraction:(Or use the same
done_by_other_rowspattern.)NITS
CSS fallback values: Several new CSS rules use inline fallbacks like
var(--color-warning, #f59e0b). If--color-warningis defined in the theme, the fallback is dead code and adds maintenance debt. If it is NOT defined, consider adding it to the theme variables for consistency rather than hardcoding hex values in multiple places.Duplicated
done_by_other_rowsquery pattern: The same query (WorkQueueItem.where(..., completed_by_other: true).pluck(:property_id, :id)) appears in at least 4 controller actions after this PR. Consider extracting a helper method.Unused local in
_recent_section: The PR passesdone_by_other_items_by_propertyto_recent_section, but the partial does not use this variable. It only usesdone_by_other_this_week_ids. The extra local is harmless but misleading to future readers.Long render lines: The turbo_stream template render calls are getting very long (7+ locals). Consider using a locals hash variable or a view object to keep these readable.
SOP COMPLIANCE
PROCESS OBSERVATIONS
This is a net-negative PR (-252 lines). Removing dead features and simplifying the UI is valuable. The scope is well-bounded: move a section, remove a section, migrate links.
The new 4-state toggle cycle (empty/assigned/completed/other-crew) in the week view is a real improvement over the previous destroy-on-complete behavior. However, the state machine expansion touched enough turbo_stream templates that the variable-passing got ahead of the controller assignments.
VERDICT: NOT APPROVED
Three runtime errors from missing instance variables in the
create,destroy, andmark_otheractions. These will crash the app on common user flows (adding to queue, removing from queue, marking as other crew) whenever the turbo_stream response tries to re-render_last_week_itemor_other_crew_section. Fix those, then the PR is good to merge.PR #215 Review
DOMAIN REVIEW
Stack: Rails 7 / Hotwired (Turbo Streams + Stimulus) / vanilla CSS
What this PR does well:
The PR cleanly removes ~350 lines of dead code (the
moveaction,week_move_controller.js, ~100 lines of.week-grid-*/.week-cell-*/.day-picker-*CSS, and thepatch :moveroute). The Schedule accordion is removed from the Week view, and day-letter labels in Assign Properties are promoted to<a>links to the daily work queue -- a clean navigation pattern.The core design change is extending the Week Assign pill from a 3-state cycle (empty -> assigned -> completed -> destroyed) to a 4-state cycle (empty -> assigned -> completed -> other-crew -> destroyed). This is implemented consistently across:
toggle_assign): checkscompleted_by_other?first, thencompleted?, thentoggle_completion!, then creates. State transitions match the JS.week_assign_controller.js):#state()reads classes in the correct priority order (is-other-crew>is-completed>is-assigned>empty). Optimistic UI with#revert()on failure..is-other-crewpill styles + sensible hover colors (assigned hover = green for "complete next", completed hover = warning for "mark other crew", other-crew hover = danger for "remove").mark_done_by_other!andtoggle_completion!both exist and handle state correctly.Re-queue from other-crew state (create action): The
existingcheck findscompleted_by_other: trueitems and resets them rather than hitting a uniqueness violation. This is a good edge case handled correctly.Partial locals consistency: All four render sites for
_other_crew_sectionnow passqueued_property_idsanddate. All render sites for_last_week_itemnow passqueued_items_by_propertyanddone_by_other_items_by_property. This actually fixes a latent bug onmainwheremark_other.turbo_stream.erbanddestroy.turbo_stream.erbwere rendering_other_crew_sectionwithout those locals.Today view reordering: Moving the
other_crew_sectionrender from between Recent and Last Week to afterremaining_sectionputs it at the bottom as intended.BLOCKERS
None.
NITS
Missing hover state for
.queued-badge-btn: The PR removes.queued-badge-btn:hover(which had danger colors) but the_last_week_itempartial now renders abutton_towith classqueued-badge queued-badge-btn. This button has no hover feedback. The new.done-badge-btnclass only setsborderandcursor. Consider adding hover styles for both badge buttons so users get visual feedback on interactive elements.DRY opportunity --
done_by_other_rowspattern: The 3-line pattern (done_by_other_rows = ...pluck(:property_id, :id)/@done_by_other_this_week_ids = ...map(&:first).to_set/@done_by_other_items_by_property = ...to_h) is repeated 4 times across the controller (index,create,destroy,mark_other). A private helper likecompute_done_by_other_lookups(week_start, week_end)would reduce repetition. Non-blocking since this is a data-fetch pattern, not auth/security logic.Hardcoded fallback color:
var(--color-warning, #f59e0b)appears 4 times in the new CSS. If--color-warningis defined in the theme, the fallback is redundant. If it is not yet defined, adding it to the CSS custom properties block would be cleaner than inline fallbacks.SVG duplication: The other-crew SVG (users icon) is defined as a JS constant (
OTHER_CREW_SVGinweek_assign_controller.js) and also inline in the ERB template (weeks/index.html.erb). If the icon ever changes, both locations need updating.SOP COMPLIANCE
PROCESS OBSERVATIONS
assigned -> completed,completed -> other-crew, andother-crew -> destroyedtransitions.VERDICT: APPROVED
Pull request closed