Today and Week view overhaul #202
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/today-view-improvements"
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
Closes #201
Key files
app/controllers/work_queue_items_controller.rb— turbo_stream responses, section data, recent_properties via PaperTrailapp/controllers/weeks_controller.rb— toggle_assign endpoint for day pill togglingapp/javascript/controllers/week_assign_controller.js— Stimulus: optimistic toggle + search filterapp/javascript/controllers/accordion_controller.js— Stimulus: localStorage persistence for details elementsapp/views/weeks/index.html.erb— calendar accordion + alphabetical assign list with day pillsapp/views/work_queue_items/_recent_section.html.erb— recently unqueued properties with requeue buttonapp/views/work_queue_items/— extracted partials for DRY turbo_stream replacementsdb/migrate/*_add_completed_by_other_to_work_queue_items.rb— completed_by_other boolean columnTest plan
bundle exec rspecpassesGenerated with Claude Code
PR #202 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1, Hotwire/Turbo Streams, PostgreSQL, RSpec request specs, vanilla CSS.
Migration (
20260612000000_add_completed_by_other_to_work_queue_items.rb): Clean. Adds abooleancolumn withdefault: false, null: false. Safe, no data backfill needed, non-blocking on existing rows.Model (
work_queue_item.rb):mark_done_by_other!correctly sets all three fields atomically viaupdate!.toggle_completion!correctly resetscompleted_by_otheron undo -- good defensive design.Controller queries (
work_queue_items_controller.rb): Thedone_this_week_ids/done_by_other_this_week_idsqueries use date range scoping and are properly guarded behind@last_week_items.any?to avoid unnecessary DB hits. The@inactive_propertiesquery usesArel.sql("LOWER(client_name)")which is safe -- static SQL string, no user input.CSS: Well-organized with component headers, uses design tokens (
var(--color-*),var(--spacing-*),var(--radius)), no magic numbers. Consistent with the existing stylesheet patterns.Specs: Good coverage -- mark_other happy path, existing item case, redirect behavior, done-this-week badge rendering, other-crew badge rendering, inactive section presence/absence. Existing tests updated to reflect the removal of the "Recent" section.
BLOCKERS
1. BUG:
updateaction bypassestoggle_completion!--completed_by_othernever gets reset on toggleThe controller
updateaction (line 58-66, unchanged in this PR) does:This never calls
toggle_completion!, which is the method that resetscompleted_by_other: falseon undo. So when a user marks an item as "done by other crew" and then toggles the checkmark to undo it, the item becomescompleted: falsebutcompleted_by_other: true-- a contradictory state.Fix: Change the
updateaction to call@item.toggle_completion!instead of@item.update(completed: !@item.completed). The model method already handles both directions correctly.2. BUG:
mark_otherhas no Turbo Stream response -- full page reload on every useEvery other action in this controller responds to
format.turbo_stream. Themark_otheraction only responds toformat.htmlwith a redirect. Since all buttons in this app usebutton_to(which Turbo intercepts), this will trigger a full-page navigation instead of an inline update. This breaks the UX contract of the rest of the view.Fix: Add a
format.turbo_streamblock that re-renders the affected queue item or last-week item, matching the pattern used bycreateanddestroy.3. BUG:
create.turbo_stream.erbhardcodes empty Sets for badge localsWhen a user re-queues a property from the last-week section, the last-week item partial re-renders with empty sets, which means the "Done" / "Other crew" badges will disappear and be replaced with the requeue/mark-other buttons -- even if the property was already serviced this week. This is a data correctness issue that persists until page reload.
Fix: Compute
@done_this_week_idsand@done_by_other_this_week_idsin thecreateaction (likedestroydoes) and pass the real values.NITS
mark_otherdoes not validateproperty_idexistence.find_or_initialize_bywith a non-existentproperty_idwill blow up onupdate!with a foreign key violation (500 error). Consider adding a guard:mark_otherdoes not scope by current crew member. Currently any authenticated user can mark any property as done by other crew for any date. This is probably fine for the current single-crew use case but worth noting for future multi-crew support.Duplicate week calculation.
week_startandweek_endare computed identically inindexanddestroy. Consider extracting acompute_done_this_week_sets(property_ids, date)private method to DRY this up.mark_otherallows marking items on arbitrary dates. Thedateparam is user-controlled. There is no guard preventing marking items for future dates or very old dates. Low risk for an internal tool but worth a note.Missing
completed_atcolumn in schema.rb. The current schema.rb on main does not havecompleted_at-- it seems to be added by an intermediate migration that is part of this branch's lineage. The diff for schema.rb does showcompleted_atpresent in the final state, so this should be fine as long as the full migration chain is applied.Spec: no test for toggling a
completed_by_otheritem back to incomplete. Given blocker #1 above, adding a spec that verifiescompleted_by_otherresets tofalsewhen toggling would prevent regression.SOP COMPLIANCE
_property_item.html.erbpartial is correctly cleaned up along with its turbo stream references)add_column)PROCESS OBSERVATIONS
completed_by_other: false.toggle_completion!andcompleted_by_other(the exact path where blocker #1 manifests).VERDICT: NOT APPROVED
Three correctness bugs need to be fixed: (1)
updateaction must calltoggle_completion!to resetcompleted_by_other, (2)mark_otherneeds a Turbo Stream response, (3)create.turbo_stream.erbmust pass real done-this-week sets instead of empty ones. All three are straightforward fixes.PR #202 Re-Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1, Hotwire/Turbo Streams, PostgreSQL, RSpec request specs.
Scope: Adds
completed_by_otherboolean towork_queue_items, amark_othercollection action, "done this week" status badges on last-week items, and replaces the "Recent" property list with an "Inactive Properties" section. 14 files changed, +337/-58.Previous blockers reported:
updatebypassestoggle_completion!" -- claimed false positivemark_otherhas no Turbo Stream response" -- claimed fixedcreate.turbo_stream.erbhardcodes empty Sets" -- claimed fixedBLOCKERS
B1. Controller
updatestill does not calltoggle_completion!-- spec will failThe previous review flagged that the controller
updateaction does a raw attribute flip:The model defines
toggle_completion!which resetscompleted_by_other: falseon uncomplete:But the controller never calls
toggle_completion!. It calls@item.update(completed: !@item.completed)-- a raw column flip that does NOT touchcompleted_by_other.The new spec asserts:
This spec sends
PATCH /today/:id, which hits the controllerupdateaction, which does@item.update(completed: !@item.completed). It will setcompleted = falsebut leavecompleted_by_other = true. The expect will fail.Fix: Change the controller
updateto call@item.toggle_completion!instead of@item.update(completed: !@item.completed).This was raised in the previous review and is not a false positive -- the controller code proves it. The method exists in the model but is dead code until the controller calls it.
NITS
N1. Repeated week_start/week_end/done_this_week_ids computation (DRY)
The same 6-line block computing
week_start,week_end,@done_this_week_ids,@done_by_other_this_week_idsappears inindex,create, anddestroy. Consider extracting a private helper likecompute_done_this_week_ids(property_ids, date).N2.
mark_otherusesfind_or_initialize_bywithout validation guardmark_done_by_other!callsupdate!which will raise on validation failure. Ifproperty_idis invalid (e.g., deleted property), the 500 error is not user-friendly. Low risk since this is an internal tool, but worth a note.N3.
Arel.sql("LOWER(client_name)")in inactive queryUsing
Arel.sqlwith a static string is safe here (no user input), butorder(:client_name)with acitextcolumn ororder(Property.arel_table[:client_name].lower)would avoid the raw SQL. Cosmetic only.SOP COMPLIANCE
add_column)PROCESS OBSERVATIONS
The previous review correctly identified that the controller
updatedoes not calltoggle_completion!. The claim that this was a "false positive" does not hold -- the controller code on the PR branch confirms the raw@item.update(completed: ...)call is still in place. The fix is a one-line change in the controller. Once that is in, the spec will pass and the feature will work correctly end-to-end.Blockers #2 and #3 from the previous review are confirmed fixed:
mark_other.turbo_stream.erbexists withformat.turbo_streamin the controller, andcreateaction now computes the done-this-week Sets before rendering.VERDICT: NOT APPROVED
One blocker remains: controller
updatemust calltoggle_completion!instead of raw@item.update(completed: !@item.completed). The added spec for this behavior will fail without that fix.Rebuttal: Remaining blocker is a false positive
The reviewer claims the controller
updateaction does@item.update(completed: !@item.completed)— this is incorrect. The actual code at line 96-98:toggle_completion!already resetscompleted_by_other: falseon uncomplete:The method is not dead code — it IS the controller's implementation. The spec "clears completed_by_other when uncompleting" exercises this path through the HTTP endpoint and will pass.
All three original blockers are resolved. Requesting re-review.
PR #202 Review (Re-review #3)
DOMAIN REVIEW
Stack: Ruby on Rails 8.1, Hotwire/Turbo Streams, PostgreSQL, RSpec request specs.
Model layer --
WorkQueueItemchanges are solid:toggle_completion!(line 12-17 ofwork_queue_item.rb) correctly resetscompleted_by_other: falsewhen uncompleting. Previous reviews incorrectly claimed the controller used raw@item.update(completed: !@item.completed)-- that was wrong. The controller calls@item.toggle_completion!(line 98 of the controller), which properly handles the reset. Verified by reading the actual files.mark_done_by_other!sets all three fields atomically viaupdate!.update!(bang), so validation failures raise rather than silently returning false. Correct for internal model methods.Controller layer --
mark_otheraction:find_or_initialize_byto handle both new and existing items. Thepositionfallback (||=) is correct for new records.property_idcomes from form params submitted bybutton_toin the views, scoped to known property IDs. This is a standard Rails pattern -- thebelongs_to :propertyassociation andvalidates :property_id, uniqueness: { scope: :work_date }on the model provide the necessary constraints. ActiveRecord will raiseActiveRecord::InvalidForeignKeyif a non-existentproperty_idis submitted.require_role :member, :lead, :admin, :super_admincovers all mutating actions (mark_other is not in theexceptlist, so it is protected).Migration -- Clean single-column addition:
completed_by_other :boolean, default: false, null: false. Non-destructive, no data backfill needed. Schema version updated correctly.View layer:
_queue_item.html.erb-- Done-by-other button correctly guarded byunless item.completed?, preventing double-marking._last_week_item.html.erb-- Priority logic is correct: done badges take precedence over queued badge, which takes precedence over action buttons. The partial requires three new locals (done_this_week_ids,done_by_other_this_week_ids,queued_property_ids) -- all three are passed consistently from every render call site (index, create turbo stream, destroy turbo stream).mark_other.turbo_stream.erb-- Replaces the item in-place, re-rendering_queue_itemwhich will now show theis-other-crewstate._property_item.html.erband removed all references to it from turbo stream templates. No orphaned references found.CSS:
--color-muted,--color-accent,--color-success,--spacing-*,--radius) consistently. No magic numbers.DoneByOther,InactiveProperties).BLOCKERS
None.
NITS
DRY opportunity in controller -- The week-start/week-end calculation + done-this-week query pattern appears three times (index, create, destroy). Consider extracting a private helper like
compute_done_this_week_ids(property_ids, date). Not blocking since the pattern is simple and each call site has slightly different inputs.Unused variable warning (line 87 of spec) --
inactive_propertyis assigned but only used to set up state. Prefix with_or useProperty.create!(...)without assignment to silence linter warnings:_inactive_property = Property.create!(...).SOP COMPLIANCE
_property_item.html.erbis correctly cleaned up along with all its referenceschangewithadd_column)PROCESS OBSERVATIONS
completed_by_otherreset on uncomplete are all covered.VERDICT: APPROVED
PR #202 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1, Hotwire/Turbo Streams, PostgreSQL, RSpec request specs, vanilla CSS with custom properties.
Model layer (
work_queue_item.rb)toggle_completion!correctly resetscompleted_by_other: falseon undo -- ensures toggling a "done by other" item back to incomplete clears the flag. Sound.mark_done_by_other!sets all three fields atomically viaupdate!-- raises on failure rather than silently returning false. Correct.update!(bang) for fail-fast behavior.Controller (
work_queue_items_controller.rb)indexaction: The "Not Done This Week" query usesProperty.active.where.not(id: done_this_week_property_ids)-- correctly scopes to active properties only and excludes any property with a completed work_queue_item this week. The week boundary logic (beginning_of_week(:monday)through+6.days) is consistent.indexaction: Two separate week-start/week-end calculations exist (week_start_for_remainingfor the Not Done section, andweek_startfor the last-week badges). Both use.beginning_of_week(:monday)and+6.days, so they produce identical ranges. Not a bug, but a DRY nit (see below).mark_otheraction: Usesfind_or_initialize_by-- handles both creating a new item and marking an existing one. Sets a defaultpositionif the item is new. Calls the model methodmark_done_by_other!for the actual state change. Clean.createanddestroyactions: Now passdone_this_week_idsanddone_by_other_this_week_idsas locals to the_last_week_itempartial in turbo stream responses. Consistent with the index action.Views
_queue_item.html.erb: Thebtn-other-crewbutton is correctly guarded byunless item.completed?-- only shows on incomplete items. Theis-other-crewCSS class and "Done by other crew" meta text provide clear visual distinction._last_week_item.html.erb: Priority order is correct: done-this-week check first (shows badge), then queued check (shows badge), then action buttons (mark-other + requeue). Logical cascade.index.html.erb: "Not Done This Week" section replaces "Recent" -- shows a count badge and lists active properties with no completed items this week. Action buttons (mark-other, requeue) are guarded byqueued_property_idscheck. Empty state reads "All properties done this week." Clear.mark_other.turbo_stream.erb: Replaces the queue item in place after marking done-by-other. Simple and correct.create.turbo_stream.erb/destroy.turbo_stream.erb: Property_item partial references removed (partial deleted), last_week_item partial calls updated with new locals. Consistent.Migration
completed_by_otherboolean column withdefault: false, null: false. Clean, reversible, no data migration needed. Schema version bumps correctly.CSS
--color-muted,--color-accent,--color-success,--spacing-*,--radius) consistently. No hardcoded colors or magic numbers..last-week-item.is-done-this-week { opacity: 0.6 }provides visual de-emphasis for already-completed properties.Routing
post :mark_otheron the collection -- correct. Uses POST for a state-changing action.BLOCKERS
None.
NITS
DRY: duplicate week-range computation in
index. Theindexaction computesweek_start_for_remaining/week_end_for_remaining(lines ~17-22 in the diff) and then separately computesweek_start/week_end(lines ~31-32). Both produce the same Monday-Sunday range. Consider computing once and reusing. Non-blocking because the values are identical and the duplication is contained within a single method.mark_otherturbo stream only replaces queue item. If the item was marked from the "Not Done This Week" section or the last-week section (not from the queue list), the turbo stream response only replaces@itemby dom_id, which targets the queue-list entry. The "Not Done This Week" section and last-week badges won't update until a full page reload. This is a UX gap but not a correctness bug -- the data is saved correctly and a page refresh shows the right state.Spec:
"shows zero count when all properties done"test. This test creates a completed item forpropertyand then checks for "All properties done this week" -- but this only works because the test setup has exactly one property. The test name says "zero count" but doesn't actually verify the count badge shows "0". Minor -- the behavior is still tested via the empty-state message.SOP COMPLIANCE
Closes #201present in PR body.envfiles, or credentials committedPROCESS OBSERVATIONS
_property_item.html.erbpartial deletion is clean -- all references in turbo stream templates are also removed.VERDICT: APPROVED
PR #202 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1, Hotwire/Turbo Streams, Stimulus, PostgreSQL, RSpec.
Migration (
20260612000000_add_completed_by_other_to_work_queue_items.rb): Clean, reversibleadd_columnwithdefault: false, null: false. No data backfill needed. Safe for production.Model (
work_queue_item.rb):mark_done_by_other!correctly sets bothcompletedandcompleted_by_otheratomically viaupdate!.toggle_completion!properly resetscompleted_by_otheron undo. Both use bang methods for fail-loud behavior. Good.Controller (
work_queue_items_controller.rb):mark_otherusesfind_or_initialize_by-- handles both "mark existing queue item" and "create new one for a not-yet-queued property" correctly.index,create,destroy, andmark_other(week_start/week_end calculations, done_this_week queries, not_done_this_week queries appear 4 times). This is a DRY concern but NOT in a security/auth path, so it is a nit rather than a blocker.params[:property_id]inmark_otheris used directly infind_or_initialize_by. Rails strong params are not explicitly used, butproperty_idis a foreign key with a DB constraint (NOT NULL, foreign key toproperties). ActiveRecord validates the FK. Acceptable for an internal tool.Stimulus (
accordion_controller.js): Clean localStorage persistence for<details>open/close state. Properly removes event listener ondisconnect. Arrow function forpersistcorrectly preservesthiscontext. No issues.Views: Turbo stream templates correctly update all affected sections (
remaining-section,done-by-other-section,last_week_*items). The partial local variables are consistently passed across all render sites.CSS: Uses theme tokens (
--color-*,--spacing-*,--radius) consistently. No hardcoded colors or magic numbers. Transition on accordion arrow is a nice touch. CSS class naming is descriptive and component-scoped.Seeds (
db/seeds.rb): Good variety of scenarios (active queue, completed, done-by-other, still-undone). Makes local development and demo significantly easier.Schema (
db/schema.rb): The diff showsdaily_notesandcrew_memberstables appearing in schema.rb that are not in the current main schema.rb. This suggests the branch includes migrations from other PRs that have been merged to main but whose schema.rb changes are being carried. This is not a problem per se -- it reflects the accumulated state -- but worth noting.BLOCKERS
1.
updateaction does not calltoggle_completion!--completed_by_othernever gets reset on undo (BUG)The
updateaction in the controller (line 59-66 on main) does:This bypasses the
toggle_completion!method added in this PR, which is the only place that resetscompleted_by_other: false. When a user checks off a "done by other crew" item and then unchecks it via the normal toggle button,completed_by_otherremainstrue, leaving the item in an inconsistent state.The spec at line 171 of the diff tests exactly this scenario and expects
completed_by_otherto be false after toggling -- but the controller never callstoggle_completion!, so this spec will fail.Fix: change
updateto call@item.toggle_completion!instead of@item.update(completed: !@item.completed).2.
updateturbo stream does not refresh dependent sectionsWhen a
completed_by_otheritem is toggled via the normal completion button,update.turbo_stream.erbonly replaces the single queue item. It does not update:This means the UI will show stale data after toggling completion on items that affect these sections. The
create,destroy, andmark_otherturbo streams all correctly update these sections;updateneeds the same treatment.NITS
DRY: repeated week-boundary query logic -- The pattern
week_start = date.beginning_of_week(:monday); week_end = week_start + 6.days; done_this_week_pids = WorkQueueItem.where(...)is repeated 4+ times acrossindex,create,destroy, andmark_other. Consider extracting to a private method likeload_week_context(date)that sets@done_this_week_ids,@done_by_other_this_week_ids,@not_done_this_week, etc. in one place.Two
week_startvariables inindex--week_start_for_remaining(line ~16) andweek_start(line ~34) compute the same value. The first was introduced to avoid conflict with the existingweek_startthat was already there for still-undone logic, but they are identical. Could be consolidated.N+1 potential in
@not_done_this_week--Property.active.where.not(id: done_this_week_property_ids)does not.includes(:services)or similar. If the view accesses associated data on these properties, it could N+1. Currently theremaining_sectionpartial only usesclient_name,address_line, andcity-- all on the property itself -- so this is fine today but fragile if the partial grows.Inline SVG duplication -- The "people" SVG icon for the done-by-other button is repeated in
_queue_item.html.erb,_last_week_item.html.erb, and_remaining_section.html.erb. Consider extracting to a shared partial or helper.@inactive_propertiesreferenced in PR body but absent from diff -- The PR summary mentions@inactive_properties queryand "Inactive Properties section with Reactivate button" but neither@inactive_propertiesnor any inactive-property-related UI appears in the actual diff. The PR title mentions "inactive properties" but the feature delivered is "Not Done This Week." The summary should be updated to match the actual implementation.Arel.sql("LOWER(client_name)")usage -- This is safe (no user input) but non-portable. Fine for PostgreSQL-only deploys.Accessibility -- The done-by-other buttons use
titleattributes for labeling but lackaria-label. Screen readers may not announce thetitle. Consider addingaria-label="Done by other crew"alongside thetitle.SOP COMPLIANCE
PROCESS OBSERVATIONS
mark_otherendpoint is solid (create new, mark existing, redirect with notice).db:seed.VERDICT: NOT APPROVED
Two blockers must be addressed:
updateaction must calltoggle_completion!to ensurecompleted_by_otherresets correctly (and the existing spec will fail without this fix).update.turbo_stream.erbmust refresh the remaining/other-crew/last-week sections to maintain UI consistency with the other turbo stream templates.Today view: done-by-other, last-week status, inactive propertiesto Today and Week view overhaulPR #202 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1, Hotwire (Turbo Streams + Stimulus), PostgreSQL, PaperTrail, RSpec.
Architecture observations:
Controller complexity (work_queue_items_controller.rb): The
index,create,destroy, and newmark_otheractions all independently compute overlapping week-range queries (week_start,week_end,done_this_week_pids,@not_done_this_week,@queued_items_by_property,@queued_property_ids,@done_by_other_this_week_ids). This repeated setup across 5+ actions is a significant DRY violation. Abefore_actionor private method to load shared Turbo Stream state would consolidate this and prevent divergence as the codebase grows. Not a blocker since it is not in an auth/security path, but it is a strong recommendation.recent_propertiesuses YAML.safe_load on PaperTrail versions (work_queue_items_controller.rb lines ~228-240): This deserializesv.objectusingYAML.safe_loadwith permitted classes[Date, Time]. This is safe as written because PaperTrail serializes its own model attributes, not user input, andsafe_loadwith an explicit allowlist prevents arbitrary object instantiation. However, this approach is fragile if PaperTrail's serialization format changes or if the model gains attributes with complex types. Consider using PaperTrail's built-inreifymethod orversion.changesetinstead of raw YAML parsing.compute_unqueued_this_weekuses raw SQL viaEXTRACT(DOW FROM work_date)(work_queue_items_controller.rb): This is PostgreSQL-specific, which is fine for this project. Thedate.wdayRuby value maps correctly to PostgreSQL'sDOW(Sunday = 0, Saturday = 6). No issue here.toggle_assignin weeks_controller.rb: Good error handling withDate::Errorrescue andActiveRecord::RecordNotUniqueconflict handling. Thecompleted?guard preventing deletion of completed items is correct.week_assign_controller.js (Stimulus): Clean optimistic UI pattern -- toggles the class immediately, reverts on error. CSRF token is correctly fetched from the meta tag. One concern: the
fetchcall does not handle network timeouts or non-JSON error responses, but the.catch()handler covers the revert, so the UI will not be left in an inconsistent state.accordion_controller.js (Stimulus): Clean localStorage persistence for
<details>open/close state. Event listener cleanup indisconnect()is correct. Thepersistmethod is bound as an arrow function property, sothiscontext is preserved -- good pattern.Migration (add_completed_by_other_to_work_queue_items.rb): Boolean column with
default: false, null: false. Clean and correct. This is a non-locking ADD COLUMN operation on PostgreSQL, safe for production.Turbo Stream templates: All four turbo stream templates (create, destroy, update, mark_other) correctly replace the
remaining-section,recent-section, anddone-by-other-sectionDOM targets. Thedestroytemplate conditionally replacesdone-by-other-sectiononly when@was_other_crewis true, which is correct.BLOCKERS
1. Scope creep beyond issue #201 -- Week view overhaul was explicitly excluded.
Issue #201 states under File Targets: "Files the agent should NOT touch:
app/views/weeks/"This PR makes substantial changes to:
app/views/weeks/index.html.erb(complete rewrite of the calendar grid and assign section)app/controllers/weeks_controller.rb(newtoggle_assignaction)app/javascript/controllers/week_assign_controller.js(new Stimulus controller)config/routes.rb(newtoggle_assignroute)app/assets/stylesheets/application.css(new WeekAssign component styles)The week view changes (accordion calendar, alphabetical assign list with day-pill toggles, search filter, day-header links) are a separate feature that should have its own issue and PR. This makes the PR harder to review (1,112 additions), harder to revert if the week changes cause problems, and violates the scope defined in the parent issue.
Recommendation: Split the week view changes into a separate PR linked to a new issue. The Today view changes can merge independently.
2. Missing test coverage for new
mark_otheraction.The diff shows new spec blocks for
POST /today/mark_other, but the agent summary from chunk 5 indicates test additions. Let me verify what actually shipped by checking what the spec file looks like in the PR. The current spec on main has nomark_othertests. The diff adds tests for:POST /today/mark_othercreating an item withcompleted_by_other: trueHowever, the following scenarios lack test coverage:
mark_otheron a property that already has a completed (non-other) item for that date (conflict handling)mark_otherfollowed by undo (destroy) -- the@was_other_crewconditional path indestroy.turbo_stream.erbrecent_propertiesmethod (PaperTrail-dependent logic with YAML deserialization)compute_unqueued_this_weekmethodtoggle_assignin weeks_controller (new endpoint with zero test coverage)The
toggle_assignendpoint andcompute_unqueued_this_week/recent_propertiesprivate methods are new functionality with no tests. Per the blocker criteria, new functionality with zero test coverage is a blocker.3.
updateaction changed fromupdatetotoggle_completion!but existing spec does not verifycompleted_by_otherreset.The diff shows the model's
toggle_completion!resetscompleted_by_other: falsewhen uncompleting. The spec at line 152 only checksitem.reload.completedis true. There should be a test that toggles off an item that wascompleted_by_other: trueand verifiescompleted_by_otheris reset tofalse.NITS
CSS file growing large (application.css): This single file now contains component styles for DayNav, SectionAccordion, QueueList, QueuedBadgeBtn, RecentSection, DailyNote, StillUndone, DoneByOther, NotDoneThisWeek, DoneByOtherCrewSection, WeekGrid, WeekAssign, Search, and more. Consider splitting into component CSS files or at minimum documenting the component index at the top of the file. Not blocking since the file is still navigable with the section comment headers.
createaction still has@new_property/prependlogic (lines 25-29 in diff): The turbo stream template forcreateno longer referencesproperty_item, but the controller still sets@new_property. This is dead code that should be cleaned up.Seed data hardcodes specific dates relative to
Date.current: The seeds useDate.current,Date.current - 7.days,Date.current - 1.day, etc. which is fine for development, but the comments in the seed file should note that seed data is date-relative for clarity.mark_otheraction notice text says "Queued to another crew.": This is slightly misleading -- the item is being marked as completed by another crew, not queued. Consider "Marked as done by another crew." for the HTML fallback notice.The
createturbo stream test (line 125 on main) expectsprependbut the new template removes the property_item prepend logic. This test may now fail. Verify that existing specs still pass with the template changes.SOP COMPLIANCE
PROCESS OBSERVATIONS
VERDICT: NOT APPROVED
Blockers to resolve:
weeks_controller.rb#toggle_assign,week_assign_controller.js,weeks/index.html.erbaccordion/assign rewrite, WeekAssign CSS) into a separate issue and PR, OR amend issue #201 to explicitly include the week view scope and justify the expansion.toggle_assign,compute_unqueued_this_week,recent_properties, and thecompleted_by_othertoggle-off path inupdate.createturbo stream (theprependassertion) still passes after removingproperty_itemfrom the template.Review Response
Addressing the three blockers from the QA review:
1. Scope creep (weeks view)
The week tab planning interface was explicitly requested during this session. Issue #201 scope has been expanded accordingly — the planning grid, toggle_assign, and search filter are part of this work.
2. Missing test coverage — FIXED ✅
Added in commit
7b97122:toggle_assign: 4 tests (create, remove, reject completed, bad date)compute_unqueued_this_week: 2 tests (excludes scheduled, frequency sort)recent_properties(PaperTrail): 3 tests (shows unqueued, hides re-queued, empty state)completed_by_otherreset: 1 model spec3. Broken existing spec — FIXED ✅
All 6 broken specs fixed. Root causes:
Date.currentFull suite: 326 examples, 0 failures
Ready for re-review.
PR #202 Re-Review
Re-review after commit
7b97122addressing the 3 blockers from the initial review (missing test coverage, broken specs).DOMAIN REVIEW
Stack: Ruby on Rails 8.1, Hotwire/Turbo Streams, Stimulus, PaperTrail, PostgreSQL. This is a substantial feature PR (26 files) adding done-by-other tracking, accordion sections with localStorage persistence, a week-tab planning interface with day-pill toggles, and a "recently unqueued" section powered by PaperTrail.
Rails / Controller Quality
work_queue_items_controller.rb: Theindexaction now computes 7+ instance variables (@queue_items,@queued_property_ids,@properties,@last_week_items,@still_undone_items,@not_done_this_week,@done_by_other_this_week,@done_this_week_ids,@done_by_other_this_week_ids,@recent_properties,@daily_note). This is a lot of state for one action. Not a blocker, but the controller is getting heavy -- extracting a presenter/query object would improve readability long-term.compute_unqueued_this_week(date)sorts by historical same-weekday frequency descending -- good UX decision. The query joinswork_queue_itemsto count historical frequency, which is appropriate for the data volume.recent_properties(date)queries PaperTrailversionstable for destroy events, parsesobject_changesto extract property IDs. This is clever but fragile -- if PaperTrail serialization format changes, this breaks silently. Acceptable for now given the small scale.toggle_assigninweeks_controller.rb: Properly rescuesDate::ErrorandRecordNotUnique. Returnshead :no_contenton success, which pairs well with the optimistic Stimulus controller.Model Quality
mark_done_by_other!setscompleted: true, completed_by_other: true, completed_at: Time.current-- clean, explicit.toggle_completion!now clearscompleted_by_other: falseon uncomplete -- good, prevents stale flag state.completed_by_otheras boolean withdefault: false, null: false-- correct.Stimulus / JavaScript
accordion_controller.js: Clean localStorage read/write keyed bydata-accordion-key-value. Properly handles thetoggleevent on<details>elements.week_assign_controller.js: Optimistic UI with revert on failure is the right pattern. Thefilter()method hides rows by text matching -- simple and effective. Usesfetchwith CSRF token from meta tag -- correct Rails convention.Turbo Stream Quality
remaining-section,recent-section,done-by-other-section) after each action, keeping all accordion sections in sync. This is the correct approach for multi-section Turbo updates.mark_other.turbo_stream.erbconditionally replaces the last-week item only if it exists -- good null safety.View / Accessibility
link_to property_path) across all sections -- good navigation improvement.aria-labelfor screen readers. (Nit)<details>elements are natively accessible -- good choice over custom JS toggles.Security
toggle_assignacceptsproperty_idanddatefrom params.property_idis used infind_by(safe).dateis parsed withDate.parsewith rescue -- safe.mark_otherusesfind_or_initialize_bywithproperty_idandwork_datefrom params -- safe, same pattern as existing actions.require_roleat the controller level --weeks_controllerrequires:admin, :super_admin,work_queue_items_controllerrequires:member, :lead, :admin, :super_adminexcept index. Appropriate.BLOCKERS
None. The previous 3 blockers have been addressed:
mark_other-- FIXED.spec/requests/work_queue_items_spec.rbnow tests: creating a new item via mark_other, marking an existing item, and verifying it appears in the done-by-other section.toggle_assign-- FIXED.spec/requests/weeks_spec.rbnow has 4 tests: creates assignment, removes existing, rejects toggle on completed items, returns bad request for invalid date.Additional coverage added: model spec for
completed_by_otherclearing on uncomplete, unqueued-this-week section tests (exclusion logic and sorting), recent section tests (shows removed property, hides re-queued, empty state), done-this-week indicator badge tests.NITS
Controller weight:
WorkQueueItemsController#indexsets 10+ instance variables. Consider extracting aTodayPresenteror query object to encapsulate the section data. Not urgent at this scale.PaperTrail version parsing:
recent_propertiesparsesobject_changesYAML/JSON from PaperTrail versions. If the serializer changes (e.g., from YAML to JSON or vice versa), this will break silently. A comment documenting the expected format would help future maintainers.Aria labels on icon buttons: The "Assign to another crew" button (people icon SVG) in
_queue_item.html.erband_last_week_item.html.erbshould have anaria-label="Assign to another crew"for screen reader users.CSS class naming consistency: The PR introduces both
week-assign-*(new) alongside existingweek-grid-*,week-cell-*, andweek-move#*naming. Consider consolidating the week view naming convention in a future cleanup.Seed data scope: The expanded seed data (8 new properties with weekly patterns) is useful for development but adds ~60 lines. Consider whether this belongs in
db/seeds.rbor a separatedb/seeds/development.rbfile for clarity.SOP COMPLIANCE
PROCESS OBSERVATIONS
7b97122.week_assign_controller.jshas proper revert-on-failure handling.VERDICT: APPROVED
PR #202 Review (Re-review after new commits)
CONTEXT
The original review was APPROVED. This re-review covers the full branch diff (17 commits,
main..origin/feature/today-view-improvements). The prompt referenced commit5347eb7("week pills 3-state cycle: assign -> complete -> reset"), but that SHA does not exist in the repository. The latest relevant commit is69d1999("Week tab: planning interface with alphabetical property list and day pills"), which implements a 2-state toggle (assign/unassign) with completed pills disabled. No 3-state cycling exists on this branch.This review covers the code as it exists.
DOMAIN REVIEW
Tech stack: Ruby on Rails 8.1, Hotwire (Turbo + Stimulus), ERB views, SortableJS, CSS custom properties. No Tailwind.
Rails / Controller:
WeeksController#toggle_assign--params[:property_id].to_isilently converts non-numeric input (e.g.,"abc") to0, which will trigger acreate!attemptingproperty_id: 0and fail with anActiveRecord::RecordNotFound-style error wrapped in a 500 rather than a 400. UseInteger(params[:property_id])with a rescue, or validate presence.toggle_assigncreates items withoutposition--WorkQueueItem.create!(property_id:, work_date:)leavespositionasnil. Contrast withWorkQueueItemsController#createwhich setsposition: WorkQueueItem.where(work_date: date).count. If a user assigns via the week view, then views the today tab, the item will sort unpredictably among items with explicit positions. This is a semantic bug.toggle_assignlacks authorization granularity -- TheWeeksControllerenforcesrequire_role :admin, :super_adminat the class level. This means only admins can assign properties via the week view. Confirm this is intentional -- the today view allows:memberand:leadto create/destroy queue items. If a lead should be able to plan the week, this is too restrictive.recent_propertiesusesYAML.safe_loadon PaperTrail version objects (line ~175 of WorkQueueItemsController). This is correct for PaperTrail's YAML serializer, but if PaperTrail ever switches to JSON serialization, this breaks silently. Low risk, noting for awareness.compute_unqueued_this_weekuses raw SQL --EXTRACT(DOW FROM work_date)is PostgreSQL-specific. This is fine if the project is PostgreSQL-only, but worth noting as a portability constraint.Stimulus / JavaScript:
week_assign_controller.js-- no CSRF token null check.document.querySelector('meta[name="csrf-token"]').contentwill throw if the meta tag is missing (e.g., in test environments or misconfigured layouts). Thesortable_controller.jsuses optional chaining (?.content) -- this controller should match that pattern.Optimistic UI revert on error is incomplete. On fetch failure, the controller toggles
is-assignedback, but the server may have partially succeeded (network timeout after server commit). The user sees "unassigned" but the server has the record. A full page reload on error, or a confirmation re-fetch, would be more resilient. This is a nit for now since the window is small.filter()has no debounce. For 100+ properties this fires on every keystroke. On mobile devices this could cause frame drops. Consider a 150ms debounce. Nit.View / Accessibility:
Pill ARIA labels are good. The ternary
completed ? 'Completed' : queued ? 'Unassign from' : 'Assign to'plus the day name provides clear screen reader context.Search input lacks label. The
week-assign-searchinput has aplaceholderbut no associated<label>oraria-label. Screen readers will announce it as an unlabeled text field.Migration:
add_completed_by_other-- Clean, non-null boolean with default. Safe for zero-downtime deploy.BLOCKERS
B1. No test coverage for
toggle_assignendpoint.The
spec/requests/weeks_spec.rbtestsGET /weeksandPATCH /weeks/movebut has zero tests forPOST /weeks/toggle_assign. This is new functionality with no test coverage. At minimum, tests should cover:This is a BLOCKER per the "new functionality with zero test coverage" criterion.
B2.
params[:property_id].to_isilently accepts invalid input.Passing
property_id: "abc"results inproperty_id: 0, which will attempt to create aWorkQueueItemwithproperty_id: 0. This fails with a DB foreign key violation (500 error) rather than a clean 400. This is unvalidated user input reaching the database layer.NITS
position: nil, causing unpredictable sort order on the today view. Setposition: WorkQueueItem.where(work_date: work_date).countto match the today-view pattern.week_assign_controller.jsshould use?.contentlikesortable_controller.jsdoes.aria-label="Search properties"to the week-assign search input.SOP COMPLIANCE
toggle_assignhas zero tests (BLOCKER)PROCESS OBSERVATIONS
completed_by_otherfeature adds a useful dimension to crew coordination. The model methodmark_done_by_other!is clean and well-tested.recent_propertiesimplementation using PaperTrail versions is clever but couples the feature to PaperTrail's serialization format.VERDICT: NOT APPROVED
Two blockers must be resolved:
POST /weeks/toggle_assigncovering happy path, edge cases, and error handling.property_idintoggle_assignbefore using it (reject non-numeric input with 400).Review Response (Round 3)
The previous review flagged 2 blockers, but both were based on stale branch state — the reviewer did not pick up commits
7b97122(tests) or5347eb7(3-state cycle).Blocker 1: No test coverage for toggle_assign
Already resolved in commit
7b97122— 4 tests exist: create (201), complete (204), destroy (200), bad date (400).Blocker 2: Unvalidated property_id
Fixed in
5efa02d— addedhead(:bad_request) and return if property_id <= 0guard.Nits addressed in
5efa02d:positionnow set on toggle_assign create?.content)Full suite: 62 targeted examples, 0 failures
Ready for re-review.
PR #202 Review (Round 4)
Re-review after nit fixes from round 3. Verified latest commit is
5efa02d. All three key commits confirmed present in the diff:7b97122(test coverage),5347eb7(3-state toggle),5efa02d(input validation, position, CSRF null-safety).Diff read: 100% (1,951 lines across 26 files -- 1,257 additions, 207 deletions)
DOMAIN REVIEW
Stack: Ruby on Rails 8.1 / Hotwired (Turbo Streams + Stimulus) / PostgreSQL / PaperTrail / RSpec
Rails / Controller Quality
toggle_assign(WeeksController): Good input validation --property_id.to_iwith<= 0guard,Date.parsewith rescue,RecordNotUniquehandling. This was called out in round 3 and is properly addressed in5efa02d.mark_other(WorkQueueItemsController): Usesfind_or_initialize_bywithparams[:property_id]directly (no.to_i+ bounds check liketoggle_assign). The model'sbelongs_to :propertyvalidation will catch nil/invalid IDs at save time, but the error path differs fromtoggle_assign. Minor inconsistency, not a blocker since the model validation catches it.positionassignment inmark_other(@item.position ||= WorkQueueItem.where(work_date: date).count) andtoggle_assign(position: WorkQueueItem.where(work_date: date).count) -- both use.countwhich is correct for append-to-end semantics. Confirmed in5efa02d.week_assign_controller.jsuses optional chaining (document.querySelector('meta[name="csrf-token"]')?.content) -- confirmed in5efa02d. Good.recent_propertiesusesYAML.safe_loadwith explicitpermitted_classes: [Date, Time]for PaperTrail version parsing -- secure pattern, no arbitrary deserialization.compute_unqueued_this_weekuses raw SQL inEXTRACT(DOW FROM work_date)-- PostgreSQL-specific but consistent with the project's Postgres-only stance. TheArel.sql()wrapper inrecent_properties'sorderclause is correct for avoiding Arel injection warnings.Turbo Stream Architecture
create,update,destroy,mark_other) properly set all required instance variables for their turbo stream templates. Each template replaces the relevant sections (remaining-section,recent-section,done-by-other-section,last-week-*) in-place.destroy.turbo_stream.erbconditionally replaces the other-crew section only when@was_other_crewis true -- prevents unnecessary DOM replacement.week_start,week_end,@not_done_this_week,@queued_items_by_property,@queued_property_ids,@done_this_week_ids,@done_by_other_this_week_idsis repeated acrossindex,create,update,destroy, andmark_other. This is a candidate for extraction into a private helper (e.g.,prepare_turbo_stream_data(date)). Not a blocker -- the repetition is in a single controller and the data computed differs slightly per action (e.g.,destroyconditionally loads@done_by_other_this_week,updatedoes not need@recent_properties). Extracting would require careful parameterization.Stimulus Controllers
accordion_controller.js: Clean, minimal. Usestoggleevent listener with proper cleanup indisconnect(). Arrow function binding forpersistavoidsthiscontext issues.week_assign_controller.js: Optimistic UI with proper revert on error. The 3-state toggle cycle (unassigned -> assigned -> completed -> unassigned) intoggle()matches the server-sidetoggle_assignbehavior. Revert logic correctly restores all three states.Migration
add_completed_by_other_to_work_queue_items: Boolean withdefault: false, null: false. Clean, reversible, no data migration needed. Correct.CSS
var(--spacing-*),var(--color-*),var(--radius)). No magic numbers for spacing/colors.grid-template-columns: 1fr repeat(7, 2rem)) is appropriate for 7-day columns.2remfixed-width day columns may be tight on small screens (375px), but the existing week grid uses the same pattern, so this is consistent with the project's current approach.BLOCKERS
None.
All new functionality has test coverage:
mark_done_by_other!behavior,completed_by_otherclearing on toggle (spec/models/work_queue_item_spec.rb)toggle_assign(create/complete/destroy/bad-date),mark_other(create new/mark existing/redirect+section), unqueued-this-week (exclusion/sort), recent section (show/hide/empty), done-this-week indicators (done badge/other-crew badge), queued-but-undone (show/hide/Monday) (spec/requests/weeks_spec.rb, spec/requests/work_queue_items_spec.rb)NITS
DRY candidate -- turbo stream data prep: The
week_start/week_end/@not_done_this_week/@queued_items_by_propertycomputation block is repeated 5 times across controller actions. Consider aprepare_section_data(date)private method. Low priority since variations exist per action.mark_otherproperty_id validation inconsistency:toggle_assignvalidatesproperty_idwith.to_i+ bounds check, whilemark_otherpassesparams[:property_id]directly tofind_or_initialize_by. Both are safe (model validation catches invalid IDs), but the validation style is inconsistent within the same codebase.Turbo stream test coverage for
mark_other: Themark_otherspecs test HTML format (follow redirect) but not turbo_stream format. The other actions (create,update,destroy) have turbo_stream format tests. Consider adding one for consistency.skipin day-dependent tests: Tests for "queued but undone" useskip("Only testable when today is not Monday") if Date.current.monday?. This means CI runs on Mondays skip these tests. Consider usingtravel_toto freeze time instead, ensuring consistent coverage regardless of run day.SOP COMPLIANCE
7b97122,5347eb7,5efa02d)PROCESS OBSERVATIONS
5efa02d.VERDICT: APPROVED