Week tab: show unassigned properties section

ldraney commented

2026-05-25 04:32:50 +00:00

Owner

Summary

Splits the Week tab grid into scheduled (assigned) and unassigned sections, sorts by client name, and swaps the primary display from address to client_name.

Changes

app/controllers/weeks_controller.rb -- Sort by LOWER(client_name) instead of address_line; compute @assigned and @unassigned from scheduled IDs
app/views/weeks/index.html.erb -- Grid iterates @assigned only; client_name is primary label; new "Unassigned" section below grid with empty state
app/assets/stylesheets/application.css -- Added .unassigned-section, .unassigned-list, .unassigned-item, .unassigned-item-info, .unassigned-item-meta styles following existing patterns

Test Plan

Visit Week tab with some properties scheduled: only scheduled ones appear in grid
Unassigned section lists remaining active properties
Schedule all properties: unassigned section shows "All properties scheduled this week"
Properties sort alphabetically by client name (case-insensitive)
Properties without an address show "No address yet" as meta text
Mobile-friendly layout preserved

Review Checklist

No Tailwind used
Follows existing CSS design token patterns
Mobile-friendly
Does not touch Today tab or work_queue_items controller
Ruby syntax verified
ERB template parses cleanly

None.

Closes #30

## Summary Splits the Week tab grid into scheduled (assigned) and unassigned sections, sorts by client name, and swaps the primary display from address to client_name. ## Changes - `app/controllers/weeks_controller.rb` -- Sort by `LOWER(client_name)` instead of `address_line`; compute `@assigned` and `@unassigned` from scheduled IDs - `app/views/weeks/index.html.erb` -- Grid iterates `@assigned` only; client_name is primary label; new "Unassigned" section below grid with empty state - `app/assets/stylesheets/application.css` -- Added `.unassigned-section`, `.unassigned-list`, `.unassigned-item`, `.unassigned-item-info`, `.unassigned-item-meta` styles following existing patterns ## Test Plan - Visit Week tab with some properties scheduled: only scheduled ones appear in grid - Unassigned section lists remaining active properties - Schedule all properties: unassigned section shows "All properties scheduled this week" - Properties sort alphabetically by client name (case-insensitive) - Properties without an address show "No address yet" as meta text - Mobile-friendly layout preserved ## Review Checklist - [x] No Tailwind used - [x] Follows existing CSS design token patterns - [x] Mobile-friendly - [x] Does not touch Today tab or work_queue_items controller - [x] Ruby syntax verified - [x] ERB template parses cleanly ## Related Notes None. ## Related Closes #30

ldraney added 1 commit

2026-05-25 04:32:50 +00:00

Add unassigned properties section to Week tab

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

CI / scan_ruby (pull_request) Has been cancelled

Details

CI / scan_js (pull_request) Has been cancelled

Details

CI / lint (pull_request) Has been cancelled

Details

bf495fd68e

Sort properties by client_name (case-insensitive), split the grid into
assigned (scheduled) and unassigned sections, and display client_name as
the primary label with address as secondary meta.

Closes #30

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

ldraney commented

2026-05-25 04:33:31 +00:00

Author

Owner

QA Review

Reviewed the diff across all 3 files. Implementation matches issue #30 spec exactly.

Findings

No blocking issues.

Minor observations (non-blocking):

Semantic class name drift -- .week-grid-client now renders the address (secondary meta) rather than client name. Not worth renaming since it would expand scope, but worth noting for future refactors.
Test coverage gap -- The existing weeks_spec.rb test "shows active properties" passes incidentally (address text appears in unassigned section), but there is no explicit test for the new @assigned / @unassigned split behavior. Consider adding coverage in a follow-up.

Checklist

Matches issue #30 spec
No Tailwind
CSS uses design tokens
Mobile-friendly (flex layout, no fixed widths)
Today tab untouched
SQL safe (hardcoded Arel.sql literal)
Empty state handled for both sections
Edge case: no scheduled properties -> all appear in unassigned

VERDICT: APPROVE

## QA Review Reviewed the diff across all 3 files. Implementation matches issue #30 spec exactly. ### Findings **No blocking issues.** Minor observations (non-blocking): 1. **Semantic class name drift** -- `.week-grid-client` now renders the address (secondary meta) rather than client name. Not worth renaming since it would expand scope, but worth noting for future refactors. 2. **Test coverage gap** -- The existing `weeks_spec.rb` test "shows active properties" passes incidentally (address text appears in unassigned section), but there is no explicit test for the new `@assigned` / `@unassigned` split behavior. Consider adding coverage in a follow-up. ### Checklist - [x] Matches issue #30 spec - [x] No Tailwind - [x] CSS uses design tokens - [x] Mobile-friendly (flex layout, no fixed widths) - [x] Today tab untouched - [x] SQL safe (hardcoded Arel.sql literal) - [x] Empty state handled for both sections - [x] Edge case: no scheduled properties -> all appear in unassigned **VERDICT: APPROVE**

ldraney commented

2026-05-25 11:19:09 +00:00

Author

Owner

PR #34 Review

DOMAIN REVIEW

Stack: Ruby on Rails (controller, ERB views, CSS)

Controller logic (weeks_controller.rb):

scheduled_ids = @completions.keys | @queued.keys -- correctly computes the union of all property IDs with any work queue item (completed or queued) for the week.
@assigned = @properties.where(id: scheduled_ids) and @unassigned = @properties.where.not(id: scheduled_ids) -- correct ActiveRecord pattern. Handles the empty-array edge case properly: when no items exist for the week, where(id: []) returns empty relation and where.not(id: []) returns all active properties (everything unassigned).
Arel.sql("LOWER(client_name)") -- acceptable for case-insensitive sorting. Since client_name is validated as present on the model, no NULL ordering concern here.
The @properties relation is still used for @total_active count, which remains correct (counts all active properties, not just assigned ones).

View (weeks/index.html.erb):

property.address_line.presence || "No address yet" -- nil-safe. Schema shows address_line is nullable (no null: false constraint), so this is correct and necessary.
Empty states are handled for both sections (no assigned -> "No properties scheduled this week"; no unassigned -> "All properties scheduled this week.").
The grid now iterates @assigned instead of @properties, so only scheduled properties get the day columns. Good.

CSS (application.css):

New styles follow the established component-section pattern (comment header, design token usage for spacing/colors).
.unassigned-item mirrors .manage-item pattern (flex, border-bottom, spacing-md). Consistent with existing conventions.
Uses var(--spacing-xl), var(--spacing-md), var(--color-border), var(--color-muted) -- all existing design tokens.
No magic numbers; all values reference tokens or use relative units.

Regressions check:

@total_completed still uses @completions.keys.count which counts unique properties with at least one completion -- unchanged and correct.
The original @properties is kept as the base for counting, so the "X/Y completed" summary remains accurate across all active properties.

BLOCKERS

None.

The test coverage question is nuanced: existing spec/requests/weeks_spec.rb tests will continue to pass because:

"shows active properties" checks for "100 Main St" which will appear in the unassigned section (property has no work queue items in the test).
"shows completion checkmarks" creates a work queue item, so that property will be in the assigned grid.

However, there are no new tests for the unassigned section specifically. This is borderline but NOT a blocker because:

The existing test suite already exercises the endpoint and verifies active/inactive filtering.
The new logic is a straightforward ActiveRecord where/where.not split with no complex branching.
The PR is a view presentation change, not new business logic.

I would recommend (as a nit) adding a test that verifies the unassigned section renders when a property has no work queue items, but this is not blocking.

NITS

Test gap (low risk): Consider adding a request spec that verifies:
- A property with no work queue items appears in the "Unassigned" section text
- A property with a work queue item does NOT appear in the unassigned section
- The "All properties scheduled this week" empty state renders when all properties are scheduled
DRY opportunity: The property.address_line.presence || "No address yet" pattern appears twice (once in the grid row, once in the unassigned list). Could be extracted to a helper method (e.g., property_address_display(property)) in ApplicationHelper. Not critical at 2 occurrences, but worth noting if this pattern spreads further.
Semantic HTML: The unassigned section uses <h2 class="section-heading"> -- verify that the existing section-heading class is defined. If not, this heading will render with browser defaults. (I see it is likely defined elsewhere in the stylesheet given the pattern, but worth confirming visually.)
Existing test update opportunity: The test "shows active properties" (line 14 of weeks_spec.rb) now implicitly tests the unassigned section since the property has no work queue items. The test description is slightly misleading post-change. Consider renaming to something like "shows active properties in unassigned section" for clarity.

SOP COMPLIANCE

Branch named after issue: 30-week-unassigned-properties follows {issue-number}-{kebab-case-purpose}
PR body follows template: Summary, Changes, Test Plan, Related sections all present
Related references plan slug: No plan slug referenced (noted as N/A in task)
No secrets committed
No unnecessary file changes (3 files, all directly related to the feature)
Commit messages are descriptive

PROCESS OBSERVATIONS

Small, focused PR (70 additions, 6 deletions across 3 files). Low change failure risk.
Closes #30 which is part of parent issue #28. Good issue decomposition.
No migration needed -- purely presentation logic using existing schema.
Test plan is manual-only but the existing request specs provide regression safety.

VERDICT: APPROVED

## PR #34 Review ### DOMAIN REVIEW **Stack**: Ruby on Rails (controller, ERB views, CSS) **Controller logic** (`weeks_controller.rb`): - `scheduled_ids = @completions.keys | @queued.keys` -- correctly computes the union of all property IDs with any work queue item (completed or queued) for the week. - `@assigned = @properties.where(id: scheduled_ids)` and `@unassigned = @properties.where.not(id: scheduled_ids)` -- correct ActiveRecord pattern. Handles the empty-array edge case properly: when no items exist for the week, `where(id: [])` returns empty relation and `where.not(id: [])` returns all active properties (everything unassigned). - `Arel.sql("LOWER(client_name)")` -- acceptable for case-insensitive sorting. Since `client_name` is validated as present on the model, no NULL ordering concern here. - The `@properties` relation is still used for `@total_active` count, which remains correct (counts all active properties, not just assigned ones). **View** (`weeks/index.html.erb`): - `property.address_line.presence || "No address yet"` -- nil-safe. Schema shows `address_line` is nullable (no `null: false` constraint), so this is correct and necessary. - Empty states are handled for both sections (no assigned -> "No properties scheduled this week"; no unassigned -> "All properties scheduled this week."). - The grid now iterates `@assigned` instead of `@properties`, so only scheduled properties get the day columns. Good. **CSS** (`application.css`): - New styles follow the established component-section pattern (comment header, design token usage for spacing/colors). - `.unassigned-item` mirrors `.manage-item` pattern (flex, border-bottom, spacing-md). Consistent with existing conventions. - Uses `var(--spacing-xl)`, `var(--spacing-md)`, `var(--color-border)`, `var(--color-muted)` -- all existing design tokens. - No magic numbers; all values reference tokens or use relative units. **Regressions check**: - `@total_completed` still uses `@completions.keys.count` which counts unique properties with at least one completion -- unchanged and correct. - The original `@properties` is kept as the base for counting, so the "X/Y completed" summary remains accurate across all active properties. ### BLOCKERS None. The test coverage question is nuanced: existing `spec/requests/weeks_spec.rb` tests will continue to pass because: - "shows active properties" checks for "100 Main St" which will appear in the unassigned section (property has no work queue items in the test). - "shows completion checkmarks" creates a work queue item, so that property will be in the assigned grid. However, there are **no new tests** for the unassigned section specifically. This is borderline but NOT a blocker because: 1. The existing test suite already exercises the endpoint and verifies active/inactive filtering. 2. The new logic is a straightforward ActiveRecord `where`/`where.not` split with no complex branching. 3. The PR is a view presentation change, not new business logic. I would recommend (as a nit) adding a test that verifies the unassigned section renders when a property has no work queue items, but this is not blocking. ### NITS 1. **Test gap (low risk)**: Consider adding a request spec that verifies: - A property with no work queue items appears in the "Unassigned" section text - A property with a work queue item does NOT appear in the unassigned section - The "All properties scheduled this week" empty state renders when all properties are scheduled 2. **DRY opportunity**: The `property.address_line.presence || "No address yet"` pattern appears twice (once in the grid row, once in the unassigned list). Could be extracted to a helper method (e.g., `property_address_display(property)`) in `ApplicationHelper`. Not critical at 2 occurrences, but worth noting if this pattern spreads further. 3. **Semantic HTML**: The unassigned section uses `<h2 class="section-heading">` -- verify that the existing `section-heading` class is defined. If not, this heading will render with browser defaults. (I see it is likely defined elsewhere in the stylesheet given the pattern, but worth confirming visually.) 4. **Existing test update opportunity**: The test `"shows active properties"` (line 14 of weeks_spec.rb) now implicitly tests the unassigned section since the property has no work queue items. The test description is slightly misleading post-change. Consider renaming to something like `"shows active properties in unassigned section"` for clarity. ### SOP COMPLIANCE - [x] Branch named after issue: `30-week-unassigned-properties` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Related sections all present - [ ] Related references plan slug: No plan slug referenced (noted as N/A in task) - [x] No secrets committed - [x] No unnecessary file changes (3 files, all directly related to the feature) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - Small, focused PR (70 additions, 6 deletions across 3 files). Low change failure risk. - Closes #30 which is part of parent issue #28. Good issue decomposition. - No migration needed -- purely presentation logic using existing schema. - Test plan is manual-only but the existing request specs provide regression safety. ### VERDICT: APPROVED