Add 'Previously [Day]s' accordion with historical frequency to day detail page #242

Merged

ldraney merged 3 commits from 235-previously-accordion into main 2026-06-17 07:30:25 +00:00

Conversation 3 Commits 3 Files changed 5 +174

ldraney commented 2026-06-16 19:24:32 +00:00

Owner

Copy link

Summary

Adds a "Previously [Day]s" accordion section to the day detail page showing all properties that have historically been serviced on that weekday, sorted by frequency descending. This gives admins quick context on which properties are typically done on a given day.

Changes

• app/controllers/days_controller.rb -- Added load_previously private method that queries WorkQueueItem history by DOW, joins active properties, groups by property_id, and returns sorted results with counts. Single efficient query with no N+1.
• app/views/days/_previously_section.html.erb -- New partial using the existing <details> + accordion Stimulus controller pattern. Shows property name, address, frequency count, and a "Queued" badge for properties already queued on the viewed date. Empty state when no historical data.
• app/views/days/show.html.erb -- Renders the new _previously_section partial, passing @previously, @date, and @queue_items.
• spec/requests/days_spec.rb -- 7 new specs covering: heading text, historical property display, frequency sort order, empty state, queued marking, inactive property exclusion, and frequency count display.

Test Plan

• All 19 specs in spec/requests/days_spec.rb pass (12 existing + 7 new)
• Verify accordion opens/closes using existing accordion_controller.js (no JS changes needed)
• Navigate to a day detail page for a weekday with historical data and confirm properties appear sorted by frequency
• Confirm properties queued for the viewed date show the "Queued" badge
• Confirm inactive properties are excluded

Review Checklist

• No N+1 queries -- single grouped COUNT query + one bulk Property load
• Reuses existing accordion Stimulus controller, no JS changes
• Follows existing partial patterns from work_queue_items views
• Only active properties included via .merge(Property.active)
• Secondary sort by client_name for stable ordering at same frequency
• Empty state handled gracefully

Related Notes

No documentation changes needed. This is a view-layer feature addition to the existing day detail page.

Related

• Closes #235

## Summary Adds a "Previously [Day]s" accordion section to the day detail page showing all properties that have historically been serviced on that weekday, sorted by frequency descending. This gives admins quick context on which properties are typically done on a given day. ## Changes - `app/controllers/days_controller.rb` -- Added `load_previously` private method that queries WorkQueueItem history by DOW, joins active properties, groups by property_id, and returns sorted results with counts. Single efficient query with no N+1. - `app/views/days/_previously_section.html.erb` -- New partial using the existing `<details>` + `accordion` Stimulus controller pattern. Shows property name, address, frequency count, and a "Queued" badge for properties already queued on the viewed date. Empty state when no historical data. - `app/views/days/show.html.erb` -- Renders the new `_previously_section` partial, passing `@previously`, `@date`, and `@queue_items`. - `spec/requests/days_spec.rb` -- 7 new specs covering: heading text, historical property display, frequency sort order, empty state, queued marking, inactive property exclusion, and frequency count display. ## Test Plan - All 19 specs in `spec/requests/days_spec.rb` pass (12 existing + 7 new) - Verify accordion opens/closes using existing `accordion_controller.js` (no JS changes needed) - Navigate to a day detail page for a weekday with historical data and confirm properties appear sorted by frequency - Confirm properties queued for the viewed date show the "Queued" badge - Confirm inactive properties are excluded ## Review Checklist - [x] No N+1 queries -- single grouped COUNT query + one bulk Property load - [x] Reuses existing accordion Stimulus controller, no JS changes - [x] Follows existing partial patterns from work_queue_items views - [x] Only active properties included via `.merge(Property.active)` - [x] Secondary sort by client_name for stable ordering at same frequency - [x] Empty state handled gracefully ## Related Notes No documentation changes needed. This is a view-layer feature addition to the existing day detail page. ## Related - Closes #235

ldraney added 1 commit 2026-06-16 19:24:32 +00:00

Add "Previously [Day]s" accordion with historical frequency ... 347aab6d06

Closes #235

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

ldraney commented 2026-06-16 19:24:53 +00:00

Author

Owner

Copy link

Self-Review Findings

Correctness -- PASS

• load_previously uses EXTRACT(DOW FROM work_date) matching the pattern from WorkQueueItemsController#compute_unqueued_this_week (line 240). PostgreSQL DOW convention (0=Sunday, 6=Saturday) matches Ruby's Date#wday.
• .joins(:property).merge(Property.active) correctly filters at the DB level so inactive properties never enter the count hash.
• The second Property.active.where(id: counts.keys) lookup is redundant with the join filter but is harmless (belt-and-suspenders) and avoids any edge case where a property becomes inactive between the count query and the property load.
• Sort uses [-count, client_name.downcase] for stable ordering -- highest frequency first, alphabetical tiebreak.

N+1 Queries -- PASS

• Two queries total: one grouped COUNT, one bulk property load via where(id: counts.keys).index_by(&:id).
• @queue_items already uses .includes(:property) from the existing code, so the queued_pids map in the partial does not trigger additional loads.
• No lazy loading in the partial -- all data is pre-fetched.

Pattern Compliance -- PASS

• Accordion uses <details class="section-accordion" data-controller="accordion" data-accordion-key-value="previously-{wday}"> matching the exact pattern from _remaining_section.html.erb and index.html.erb.
• queued-badge class matches existing usage in _other_crew_section.html.erb (line 16).
• Address display pattern (address_line . city or "No address yet") matches all other views.

Test Coverage -- PASS

7 new specs covering all acceptance criteria: heading text, property display, frequency sort, empty state, queued marking, inactive exclusion, frequency count.

No Issues Found

This is ready for human review.

## Self-Review Findings ### Correctness -- PASS - `load_previously` uses `EXTRACT(DOW FROM work_date)` matching the pattern from `WorkQueueItemsController#compute_unqueued_this_week` (line 240). PostgreSQL DOW convention (0=Sunday, 6=Saturday) matches Ruby's `Date#wday`. - `.joins(:property).merge(Property.active)` correctly filters at the DB level so inactive properties never enter the count hash. - The second `Property.active.where(id: counts.keys)` lookup is redundant with the join filter but is harmless (belt-and-suspenders) and avoids any edge case where a property becomes inactive between the count query and the property load. - Sort uses `[-count, client_name.downcase]` for stable ordering -- highest frequency first, alphabetical tiebreak. ### N+1 Queries -- PASS - Two queries total: one grouped COUNT, one bulk property load via `where(id: counts.keys).index_by(&:id)`. - `@queue_items` already uses `.includes(:property)` from the existing code, so the `queued_pids` map in the partial does not trigger additional loads. - No lazy loading in the partial -- all data is pre-fetched. ### Pattern Compliance -- PASS - Accordion uses `<details class="section-accordion" data-controller="accordion" data-accordion-key-value="previously-{wday}">` matching the exact pattern from `_remaining_section.html.erb` and `index.html.erb`. - `queued-badge` class matches existing usage in `_other_crew_section.html.erb` (line 16). - Address display pattern (`address_line . city` or "No address yet") matches all other views. ### Test Coverage -- PASS 7 new specs covering all acceptance criteria: heading text, property display, frequency sort, empty state, queued marking, inactive exclusion, frequency count. ### No Issues Found This is ready for human review.

ldraney commented 2026-06-16 19:28:22 +00:00

Author

Owner

Copy link

PR #242 Review

DOMAIN REVIEW

Tech stack: Rails 8.1, PostgreSQL, Hotwire (Stimulus), ERB views, RSpec request specs.

Query design (controller): The load_previously method uses a clean two-phase approach -- grouped COUNT query, then bulk property load via index_by. No N+1. The EXTRACT(DOW FROM work_date) usage is PostgreSQL-specific but the project is PostgreSQL-only (dev, test, and prod), so this is fine. The DOW=0-for-Sunday semantics match Ruby's Date#wday, which is correct.

Sorting: Primary sort by frequency descending, secondary by client_name.downcase for stable ordering -- good. Uses sort_by with a two-element array, which is the idiomatic Ruby approach.

Accordion pattern: Correctly reuses the existing accordion Stimulus controller with a data-accordion-key-value="previously-<wday>" key. This gives per-weekday localStorage persistence, which is the right granularity.

View partial: Follows the established _*_section.html.erb naming pattern. Reuses existing CSS classes for section-accordion, section-heading, queued-badge, and empty-state.

Spec coverage: 7 new specs covering heading text, historical display, frequency sort order, empty state, queued marking, inactive exclusion, and frequency count. Good breadth for a view-layer feature.

BLOCKERS

None. No security issues, no unvalidated input, no credentials, no auth/security DRY violations. The query takes date.wday (an integer derived from a parsed Date), not raw user input. The controller already validates and redirects on invalid date input (existing behavior from PR #240).

NITS

1. Missing CSS for previously-* classes. The partial introduces previously-count, previously-list, previously-item, previously-item-info, and previously-item-meta classes, but no CSS is added to application.css. Every other section in the app (recent, remaining, last-week, other-crew) has dedicated CSS for layout, spacing, text overflow, and item structure. Without styles, the list items will render as plain unstyled elements -- no flex layout, no text truncation, no spacing between items, no border separators. The queued-badge and section-accordion classes are already styled, but the list content itself will look noticeably rough. Recommend adding CSS following the established pattern (compare with .recent-queue-* or .remaining-* blocks in application.css). This is cosmetic only and does not block merge, but it will look out of place compared to the rest of the app.

2. is-queued class on previously-item has no CSS rule. The class is applied to <li> elements for queued properties, likely intended to visually differentiate them (e.g., reduced opacity like .other-crew-item at opacity: 0.7), but no .previously-item.is-queued or .is-queued rule exists. Currently it does nothing besides serving as a test hook.

3. queued_pids computation in the partial. Line <% queued_pids = queue_items.map { |qi| qi.property_id } %> could use &:property_id shorthand for consistency with Ruby conventions: queue_items.map(&:property_id). Minor style nit.

4. Test for singular "1 time" missing. The frequency count spec tests "2 times" (plural) but doesn't test "1 time" (singular). The view logic entry[:count] == 1 ? "time" : "times" handles this, but a spec confirming the singular form would be more complete. Very minor -- the branching logic is simple.

5. previously also counts the current date's items in the historical tally. If a property is queued for the viewed Tuesday AND was also queued for previous Tuesdays, the EXTRACT(DOW FROM work_date) query counts ALL Tuesdays including the viewed date itself. This means a property queued only for today's Tuesday would show "1 time" in the "Previously" section, which is slightly misleading -- it implies the property was previously serviced on Tuesdays when it was only just added today. Consider adding .where.not(work_date: date) to exclude the current date from the historical count. Non-blocking because the feature is still useful as-is, but worth considering.

SOP COMPLIANCE

• Branch named after issue (235-previously-accordion references issue #235)
• PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related Notes, Related sections all present)
• Closes #235 present in PR body
• Tests exist and pass (7 new specs, 19 total reported passing)
• No secrets, .env files, or credentials committed
• No unnecessary file changes (4 files, all directly related to the feature)
• Commit messages are descriptive (PR title is clear)

PROCESS OBSERVATIONS

Clean, focused PR. Single feature, single issue, no scope creep. The two-query approach in the controller avoids the common Rails trap of loading associations in a loop. Good reuse of existing Stimulus controller rather than creating a new one. The missing CSS is the only rough edge -- the feature will work correctly but won't match the visual polish of the rest of the app.

VERDICT: APPROVED

## PR #242 Review ### DOMAIN REVIEW **Tech stack:** Rails 8.1, PostgreSQL, Hotwire (Stimulus), ERB views, RSpec request specs. **Query design (controller):** The `load_previously` method uses a clean two-phase approach -- grouped COUNT query, then bulk property load via `index_by`. No N+1. The `EXTRACT(DOW FROM work_date)` usage is PostgreSQL-specific but the project is PostgreSQL-only (dev, test, and prod), so this is fine. The DOW=0-for-Sunday semantics match Ruby's `Date#wday`, which is correct. **Sorting:** Primary sort by frequency descending, secondary by `client_name.downcase` for stable ordering -- good. Uses `sort_by` with a two-element array, which is the idiomatic Ruby approach. **Accordion pattern:** Correctly reuses the existing `accordion` Stimulus controller with a `data-accordion-key-value="previously-<wday>"` key. This gives per-weekday localStorage persistence, which is the right granularity. **View partial:** Follows the established `_*_section.html.erb` naming pattern. Reuses existing CSS classes for `section-accordion`, `section-heading`, `queued-badge`, and `empty-state`. **Spec coverage:** 7 new specs covering heading text, historical display, frequency sort order, empty state, queued marking, inactive exclusion, and frequency count. Good breadth for a view-layer feature. ### BLOCKERS None. No security issues, no unvalidated input, no credentials, no auth/security DRY violations. The query takes `date.wday` (an integer derived from a parsed Date), not raw user input. The controller already validates and redirects on invalid date input (existing behavior from PR #240). ### NITS 1. **Missing CSS for `previously-*` classes.** The partial introduces `previously-count`, `previously-list`, `previously-item`, `previously-item-info`, and `previously-item-meta` classes, but no CSS is added to `application.css`. Every other section in the app (recent, remaining, last-week, other-crew) has dedicated CSS for layout, spacing, text overflow, and item structure. Without styles, the list items will render as plain unstyled elements -- no flex layout, no text truncation, no spacing between items, no border separators. The `queued-badge` and `section-accordion` classes are already styled, but the list content itself will look noticeably rough. Recommend adding CSS following the established pattern (compare with `.recent-queue-*` or `.remaining-*` blocks in `application.css`). This is cosmetic only and does not block merge, but it will look out of place compared to the rest of the app. 2. **`is-queued` class on `previously-item` has no CSS rule.** The class is applied to `<li>` elements for queued properties, likely intended to visually differentiate them (e.g., reduced opacity like `.other-crew-item` at `opacity: 0.7`), but no `.previously-item.is-queued` or `.is-queued` rule exists. Currently it does nothing besides serving as a test hook. 3. **`queued_pids` computation in the partial.** Line `<% queued_pids = queue_items.map { |qi| qi.property_id } %>` could use `&:property_id` shorthand for consistency with Ruby conventions: `queue_items.map(&:property_id)`. Minor style nit. 4. **Test for singular "1 time" missing.** The frequency count spec tests `"2 times"` (plural) but doesn't test `"1 time"` (singular). The view logic `entry[:count] == 1 ? "time" : "times"` handles this, but a spec confirming the singular form would be more complete. Very minor -- the branching logic is simple. 5. **`previously` also counts the current date's items in the historical tally.** If a property is queued for the viewed Tuesday AND was also queued for previous Tuesdays, the `EXTRACT(DOW FROM work_date)` query counts ALL Tuesdays including the viewed date itself. This means a property queued only for today's Tuesday would show "1 time" in the "Previously" section, which is slightly misleading -- it implies the property was previously serviced on Tuesdays when it was only just added today. Consider adding `.where.not(work_date: date)` to exclude the current date from the historical count. Non-blocking because the feature is still useful as-is, but worth considering. ### SOP COMPLIANCE - [x] Branch named after issue (`235-previously-accordion` references issue #235) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related Notes, Related sections all present) - [x] `Closes #235` present in PR body - [x] Tests exist and pass (7 new specs, 19 total reported passing) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (4 files, all directly related to the feature) - [x] Commit messages are descriptive (PR title is clear) ### PROCESS OBSERVATIONS Clean, focused PR. Single feature, single issue, no scope creep. The two-query approach in the controller avoids the common Rails trap of loading associations in a loop. Good reuse of existing Stimulus controller rather than creating a new one. The missing CSS is the only rough edge -- the feature will work correctly but won't match the visual polish of the rest of the app. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-16 19:31:43 +00:00

Address QA review nits for previously section ... 9c49509268

- Add CSS styles for previously-list, previously-item, is-queued highlight
- Exclude current date from historical count to avoid misleading "1 time"
- Use map(&:property_id) shorthand in partial
- Add specs for singular "1 time" and current-date exclusion

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

ldraney commented 2026-06-16 19:31:51 +00:00

Author

Owner

Copy link

QA Nit Fixes (commit 9c49509)

Addressed all 5 nits from the QA review:

1. Missing CSS -- Added full previously-* styles matching the existing remaining-item pattern: list layout, item flex, text overflow, meta sizing. Count badge uses the same accent pill as other-crew-count and remaining-count.
2. is-queued class visual -- Added background: var(--color-success-light) rule for .previously-item.is-queued so queued items get a visible highlight.
3. Ruby style -- Changed queue_items.map { |qi| qi.property_id } to queue_items.map(&:property_id).
4. Singular "1 time" spec -- Added spec asserting "1 time" appears and "1 times" does not.
5. Current date exclusion -- Added .where.not(work_date: date) to the historical query so a property queued only for today does not misleadingly appear in "Previously". Added spec verifying this behavior.

All 21 specs pass.

## QA Nit Fixes (commit 9c49509) Addressed all 5 nits from the QA review: 1. **Missing CSS** -- Added full `previously-*` styles matching the existing `remaining-item` pattern: list layout, item flex, text overflow, meta sizing. Count badge uses the same accent pill as `other-crew-count` and `remaining-count`. 2. **`is-queued` class visual** -- Added `background: var(--color-success-light)` rule for `.previously-item.is-queued` so queued items get a visible highlight. 3. **Ruby style** -- Changed `queue_items.map { |qi| qi.property_id }` to `queue_items.map(&:property_id)`. 4. **Singular "1 time" spec** -- Added spec asserting "1 time" appears and "1 times" does not. 5. **Current date exclusion** -- Added `.where.not(work_date: date)` to the historical query so a property queued only for today does not misleadingly appear in "Previously". Added spec verifying this behavior. All 21 specs pass.

ldraney added 1 commit 2026-06-17 07:29:14 +00:00

Merge main into 235-previously-accordion to resolve conflicts ... 81a245bf12

Both #235 (Previously accordion) and #241 (Property picker) touch
days_controller.rb and days_spec.rb. This merge keeps both features.

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

ldraney merged commit e230e0acb6 into main 2026-06-17 07:30:25 +00:00

ldraney deleted branch 235-previously-accordion 2026-06-17 07:30:25 +00:00

ldraney referenced this pull request from a commit 2026-06-17 07:30:27 +00:00

Add 'Previously [Day]s' accordion with historical frequency to day detail page (#242)

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

Subscribe

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!242

No description provided.