Add DayExclusion model and remove-from-list button

ldraney commented

2026-06-17 08:27:28 +00:00

Author

Owner

PR #245 Review

Parent issue: #236 -- Day detail page: DayExclusion model and remove-from-list button

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, Hotwire (Turbo Streams), PostgreSQL, RSpec

Model (DayExclusion)

Clean and minimal: belongs_to :property, validates presence of both fields, uniqueness of wday scoped to property_id. Good.
has_many :day_exclusions, dependent: :destroy on Property -- correct lifecycle management.

Migration

t.references :property, null: false, foreign_key: true -- proper FK with NOT NULL.
add_index :day_exclusions, [:property_id, :wday], unique: true -- matches the model uniqueness validation. DB-level enforcement is correct.

Controller (exclude action)

Follows the established pattern from add_to_queue exactly: parse_date -> find_property_or_reject -> error handling -> business logic -> respond_to with turbo_stream and HTML fallback. Consistent.
find_or_create_by(property_id: property.id, wday: @date.wday) -- idempotent by design. No race condition risk because the unique index will catch concurrent inserts and find_or_create_by handles that gracefully.
load_previously modification: adds 2 lines to reject excluded property IDs by wday. Scoped correctly -- exclusion on Tuesday (wday 2) does not affect Wednesday (wday 3).

Authorization

Controller-level require_role :admin, :super_admin on line 2 covers all actions including exclude. No separate auth logic needed. No DRY violation.

View changes

DOM IDs added: previously_item_<id> on each <li>, previously-count on the badge, previously-list on the <ul>. All needed for Turbo Stream targeting.
X button uses button_to (generates a form with CSRF token) -- correct for a POST action. Inline SVG keeps it dependency-free.
Turbo Stream template (exclude.turbo_stream.erb): removes the item by DOM ID, replaces the count badge. Clean.

Accessibility note (nit): The X button has a title attribute ("Remove from Tuesdays") but no aria-label. Screen readers may not announce the title. See nits below.

BLOCKERS

None.

Test coverage: 6 model specs + 12 request specs covering create, idempotency, filtering, cross-day isolation, turbo stream response, error handling (invalid property_id), and role access for all 5 roles (admin, super_admin, member, lead, client). This is thorough.
No unvalidated user input: property_id goes through find_property_or_reject (returns nil for bogus IDs), date goes through parse_date (returns nil for unparseable input). Both are handled gracefully.
No secrets or credentials committed.
No duplicated auth logic -- uses the class-level require_role mechanism.

NITS

wday range validation (low priority): The model validates presence but not range (0-6). In practice this is safe because @date.wday always produces 0-6, and the controller is the only writer. But a model-level inclusion: { in: 0..6 } validation would be a defensive-in-depth measure that documents the domain constraint. Not blocking.
excluded_ids.include?(pid) uses Array: In load_previously, excluded_ids is an Array and .include? is O(n). For the expected dataset size (handful of exclusions) this is fine. If the exclusion list ever grew large, converting to a Set would be trivial. Not blocking.
Accessibility -- aria-label on X button: The remove button has title: "Remove from #{day_name}s" but no aria-label. Adding aria-label: "Remove #{property.client_name} from #{day_name}s" would improve screen reader experience by identifying which property is being removed. Minor.
Empty state after all items excluded: If a user excludes every item in the Previously accordion, the list becomes empty but no empty-state message appears (the <ul> is simply empty). The original partial has an <% else %> branch for the empty state, but Turbo Stream removal does not trigger it. The count badge correctly shows 0, so this is cosmetic. Could be addressed in a follow-up.

SOP COMPLIANCE

PR body has: Summary, Changes, Test Plan, Related -- all present and detailed
Test Plan includes both automated and manual verification steps
No secrets committed
No unnecessary file changes -- all 10 changed files are directly related to the feature
Commit messages are descriptive (single commit, title matches PR)
Closes #236 present -- issue will auto-close on merge
Schema.rb updated to match migration

PROCESS OBSERVATIONS

Deployment risk: Low. New model and endpoint with no changes to existing behavior. The load_previously modification is additive (filter step). Rollback path is clean -- drop the table, revert the code.
Change scope: Tight. 209 additions, 4 deletions across exactly the files you would expect for a new model + controller action + view + tests. No scope creep.
Test quality: The cross-day isolation test ("exclusion on Tuesday does not affect Wednesday") is the kind of edge case spec that prevents real bugs. Well done.

VERDICT: APPROVED

## PR #245 Review **Parent issue:** #236 -- Day detail page: DayExclusion model and remove-from-list button ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1, Hotwire (Turbo Streams), PostgreSQL, RSpec **Model (`DayExclusion`)** - Clean and minimal: `belongs_to :property`, validates presence of both fields, uniqueness of `wday` scoped to `property_id`. Good. - `has_many :day_exclusions, dependent: :destroy` on `Property` -- correct lifecycle management. **Migration** - `t.references :property, null: false, foreign_key: true` -- proper FK with NOT NULL. - `add_index :day_exclusions, [:property_id, :wday], unique: true` -- matches the model uniqueness validation. DB-level enforcement is correct. **Controller (`exclude` action)** - Follows the established pattern from `add_to_queue` exactly: `parse_date` -> `find_property_or_reject` -> error handling -> business logic -> respond_to with turbo_stream and HTML fallback. Consistent. - `find_or_create_by(property_id: property.id, wday: @date.wday)` -- idempotent by design. No race condition risk because the unique index will catch concurrent inserts and `find_or_create_by` handles that gracefully. - `load_previously` modification: adds 2 lines to reject excluded property IDs by wday. Scoped correctly -- exclusion on Tuesday (wday 2) does not affect Wednesday (wday 3). **Authorization** - Controller-level `require_role :admin, :super_admin` on line 2 covers all actions including `exclude`. No separate auth logic needed. No DRY violation. **View changes** - DOM IDs added: `previously_item_<id>` on each `<li>`, `previously-count` on the badge, `previously-list` on the `<ul>`. All needed for Turbo Stream targeting. - X button uses `button_to` (generates a form with CSRF token) -- correct for a POST action. Inline SVG keeps it dependency-free. - Turbo Stream template (`exclude.turbo_stream.erb`): removes the item by DOM ID, replaces the count badge. Clean. **Accessibility note (nit):** The X button has a `title` attribute ("Remove from Tuesdays") but no `aria-label`. Screen readers may not announce the `title`. See nits below. ### BLOCKERS None. - Test coverage: 6 model specs + 12 request specs covering create, idempotency, filtering, cross-day isolation, turbo stream response, error handling (invalid property_id), and role access for all 5 roles (admin, super_admin, member, lead, client). This is thorough. - No unvalidated user input: `property_id` goes through `find_property_or_reject` (returns nil for bogus IDs), `date` goes through `parse_date` (returns nil for unparseable input). Both are handled gracefully. - No secrets or credentials committed. - No duplicated auth logic -- uses the class-level `require_role` mechanism. ### NITS 1. **`wday` range validation (low priority):** The model validates presence but not range (0-6). In practice this is safe because `@date.wday` always produces 0-6, and the controller is the only writer. But a model-level `inclusion: { in: 0..6 }` validation would be a defensive-in-depth measure that documents the domain constraint. Not blocking. 2. **`excluded_ids.include?(pid)` uses Array:** In `load_previously`, `excluded_ids` is an Array and `.include?` is O(n). For the expected dataset size (handful of exclusions) this is fine. If the exclusion list ever grew large, converting to a Set would be trivial. Not blocking. 3. **Accessibility -- `aria-label` on X button:** The remove button has `title: "Remove from #{day_name}s"` but no `aria-label`. Adding `aria-label: "Remove #{property.client_name} from #{day_name}s"` would improve screen reader experience by identifying which property is being removed. Minor. 4. **Empty state after all items excluded:** If a user excludes every item in the Previously accordion, the list becomes empty but no empty-state message appears (the `<ul>` is simply empty). The original partial has an `<% else %>` branch for the empty state, but Turbo Stream removal does not trigger it. The count badge correctly shows 0, so this is cosmetic. Could be addressed in a follow-up. ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and detailed - [x] Test Plan includes both automated and manual verification steps - [x] No secrets committed - [x] No unnecessary file changes -- all 10 changed files are directly related to the feature - [x] Commit messages are descriptive (single commit, title matches PR) - [x] `Closes #236` present -- issue will auto-close on merge - [x] Schema.rb updated to match migration ### PROCESS OBSERVATIONS - **Deployment risk:** Low. New model and endpoint with no changes to existing behavior. The `load_previously` modification is additive (filter step). Rollback path is clean -- drop the table, revert the code. - **Change scope:** Tight. 209 additions, 4 deletions across exactly the files you would expect for a new model + controller action + view + tests. No scope creep. - **Test quality:** The cross-day isolation test ("exclusion on Tuesday does not affect Wednesday") is the kind of edge case spec that prevents real bugs. Well done. ### VERDICT: APPROVED

2026-06-17 08:22:06 +00:00

Summary

Adds a DayExclusion model that lets admins permanently remove a property from a specific weekday's Previously accordion
An X button on each previously item posts to a new exclude endpoint, which creates the exclusion idempotently and removes the item via Turbo Stream
Excluded properties are filtered out of load_previously by wday, so exclusion on Tuesday does not affect Wednesday

Changes

db/migrate/20260617000000_create_day_exclusions.rb: new day_exclusions table with property_id (FK NOT NULL), wday (integer NOT NULL), unique index on [property_id, wday]
app/models/day_exclusion.rb: model with belongs_to :property, validates presence of property_id and wday, validates uniqueness of wday scoped to property_id
app/models/property.rb: added has_many :day_exclusions, dependent: :destroy
config/routes.rb: added POST /days/:date/exclude route
app/controllers/days_controller.rb: added exclude action using find_or_create_by for idempotency; updated load_previously to filter out excluded property_ids for the given wday
app/views/days/_previously_section.html.erb: added DOM IDs (previously_item_<id>, previously-count) and X button per item
app/views/days/exclude.turbo_stream.erb: removes the previously item and updates the count badge
spec/models/day_exclusion_spec.rb: 6 specs covering presence, uniqueness, associations
spec/requests/days_spec.rb: 12 new specs covering create, idempotency, filtering, cross-day isolation, turbo stream response, and role access

Test Plan

Tests pass locally — 55 examples, 0 failures
Manual verification: X button appears on each Previously item and removes it
Excluded property stays hidden on page reload
Excluding on Tuesday does not affect Wednesday's Previously list
No regressions in day detail page queue functionality

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive
Feature flag needed? No — internal model change with admin-only UI button, no new user-visible workflow

ldraney/landscaping-assistant #236 — the Forgejo issue this PR implements
project_landscaping_assistant — the project this work belongs to

Closes #236

## Summary - Adds a `DayExclusion` model that lets admins permanently remove a property from a specific weekday's Previously accordion - An X button on each previously item posts to a new `exclude` endpoint, which creates the exclusion idempotently and removes the item via Turbo Stream - Excluded properties are filtered out of `load_previously` by wday, so exclusion on Tuesday does not affect Wednesday ## Changes - `db/migrate/20260617000000_create_day_exclusions.rb`: new `day_exclusions` table with property_id (FK NOT NULL), wday (integer NOT NULL), unique index on [property_id, wday] - `app/models/day_exclusion.rb`: model with belongs_to :property, validates presence of property_id and wday, validates uniqueness of wday scoped to property_id - `app/models/property.rb`: added `has_many :day_exclusions, dependent: :destroy` - `config/routes.rb`: added `POST /days/:date/exclude` route - `app/controllers/days_controller.rb`: added `exclude` action using `find_or_create_by` for idempotency; updated `load_previously` to filter out excluded property_ids for the given wday - `app/views/days/_previously_section.html.erb`: added DOM IDs (`previously_item_<id>`, `previously-count`) and X button per item - `app/views/days/exclude.turbo_stream.erb`: removes the previously item and updates the count badge - `spec/models/day_exclusion_spec.rb`: 6 specs covering presence, uniqueness, associations - `spec/requests/days_spec.rb`: 12 new specs covering create, idempotency, filtering, cross-day isolation, turbo stream response, and role access ## Test Plan - [x] Tests pass locally — 55 examples, 0 failures - [ ] Manual verification: X button appears on each Previously item and removes it - [ ] Excluded property stays hidden on page reload - [ ] Excluding on Tuesday does not affect Wednesday's Previously list - [ ] No regressions in day detail page queue functionality ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — internal model change with admin-only UI button, no new user-visible workflow ## Related Notes - `ldraney/landscaping-assistant #236` — the Forgejo issue this PR implements - `project_landscaping_assistant` — the project this work belongs to Closes #236

ldraney added 1 commit

Add DayExclusion model and remove-from-list button on Previously accordion

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

CI / scan_ruby (pull_request) Has been cancelled

CI / scan_js (pull_request) Has been cancelled

CI / lint (pull_request) Has been cancelled

fd2a20a39d

Allows admins to permanently exclude a property from a specific weekday's
Previously section via an X button. Uses find_or_create_by for idempotency.
Turbo stream removes the item and updates the count badge in real time.

Closes #236

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