Add ServiceRequest model with status transition validation #178

Merged

ldraney merged 1 commit from 122-service-request-model into main 2026-06-08 05:02:38 +00:00

Conversation 2 Commits 1 Files changed 8 +394 -3

ldraney commented 2026-06-08 04:54:07 +00:00

Owner

Copy link

Summary

• Introduces the ServiceRequest model for tracking client package change and project requests through a validated status lifecycle
• Adds monthly_price to the Service model and updates seeds with pricing for Mowing, Weeding, and Edging & Trimming
• Implements status transition validation per the defined state graph (requested -> quoted -> paid -> scheduled -> completed, with decline exits)

Changes

• db/migrate/20260608020000_add_monthly_price_to_services.rb: adds monthly_price (decimal, precision 8, scale 2) column to services table
• db/migrate/20260608020001_create_service_requests.rb: creates service_requests table with FKs to properties and crew_members, indexes on status and request_type
• app/models/service_request.rb: new model with request_type/status inclusion validations, status transition validation, helper methods (transition_to, transition_to!, terminal?, allowed_transitions)
• app/models/property.rb: adds has_many :service_requests, dependent: :destroy
• app/models/crew_member.rb: adds has_many :service_requests, dependent: :destroy
• db/seeds.rb: updates service seeds with monthly prices (Mowing $35, Weeding $25, Edging & Trimming $20)
• db/schema.rb: auto-updated by migration
• spec/models/service_request_spec.rb: comprehensive specs covering validations, associations, all valid/invalid status transitions, terminal states, and helper methods

Test Plan

• Tests pass locally — bundle exec rspec: 278 examples, 0 failures
• Manual verification: migrations run cleanly, schema.rb reflects new tables and columns
• No regressions — all existing specs continue to pass

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, no user-visible workflow

Related Notes

• Closes #122
• ldraney/landscaping-assistant #122 — ServiceRequest model + migration
• Parent spike: #121

## Summary - Introduces the ServiceRequest model for tracking client package change and project requests through a validated status lifecycle - Adds `monthly_price` to the Service model and updates seeds with pricing for Mowing, Weeding, and Edging & Trimming - Implements status transition validation per the defined state graph (requested -> quoted -> paid -> scheduled -> completed, with decline exits) ## Changes - `db/migrate/20260608020000_add_monthly_price_to_services.rb`: adds `monthly_price` (decimal, precision 8, scale 2) column to services table - `db/migrate/20260608020001_create_service_requests.rb`: creates service_requests table with FKs to properties and crew_members, indexes on status and request_type - `app/models/service_request.rb`: new model with request_type/status inclusion validations, status transition validation, helper methods (transition_to, transition_to!, terminal?, allowed_transitions) - `app/models/property.rb`: adds `has_many :service_requests, dependent: :destroy` - `app/models/crew_member.rb`: adds `has_many :service_requests, dependent: :destroy` - `db/seeds.rb`: updates service seeds with monthly prices (Mowing $35, Weeding $25, Edging & Trimming $20) - `db/schema.rb`: auto-updated by migration - `spec/models/service_request_spec.rb`: comprehensive specs covering validations, associations, all valid/invalid status transitions, terminal states, and helper methods ## Test Plan - [x] Tests pass locally — `bundle exec rspec`: 278 examples, 0 failures - [x] Manual verification: migrations run cleanly, schema.rb reflects new tables and columns - [x] No regressions — all existing specs continue to pass ## Review Checklist - [ ] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive - [x] Feature flag needed? No — internal model change, no user-visible workflow ## Related Notes - Closes #122 - `ldraney/landscaping-assistant #122` — ServiceRequest model + migration - Parent spike: #121

ldraney added 1 commit 2026-06-08 04:54:07 +00:00

Add ServiceRequest model with status transition validation (#122) ... e4d8bcf60c

Introduces the ServiceRequest model for tracking client package change
and project requests through a validated lifecycle: requested -> quoted
-> paid -> scheduled -> completed, with decline exits from requested
and quoted states.

- Create service_requests table with FK to properties and crew_members
- Add monthly_price column to services table
- Implement status transition validation per the defined state graph
- Add has_many :service_requests to Property and CrewMember models
- Update seeds with prices for Mowing, Weeding, Edging & Trimming
- Add comprehensive specs for validations, associations, transitions

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

ldraney commented 2026-06-08 04:54:52 +00:00

Author

Owner

Copy link

QA Review -- PR #178

Checklist

• Migration correctness -- FKs to properties and crew_members with null: false, proper indexes on status and request_type, correct decimal precision for price (10,2) and monthly_price (8,2)
• Status transition graph -- VALID_TRANSITIONS hash matches the spec exactly: requested->{quoted,declined}, quoted->{paid,declined}, paid->{scheduled}, scheduled->{completed}, completed/declined->terminal
• Validation coverage -- request_type inclusion, status inclusion, price numericality (>=0, nil allowed), transition validation on status change
• Transition validation skips new records -- return if new_record? prevents false rejections when creating with a non-default status
• Association setup -- has_many :service_requests, dependent: :destroy on both Property and CrewMember
• Seeds -- prices for Mowing ($35), Weeding ($25), Edging & Trimming ($20) using idempotent find_or_create_by! + update!
• Test completeness -- validations, associations (both directions), every valid transition, every invalid transition per state, terminal state exhaustive check, transition_to/transition_to!/terminal?/allowed_transitions helpers
• No secrets or unrelated changes -- only the 8 targeted files
• Schema.rb -- auto-generated, matches migration output
• All 278 specs pass per PR body

Findings

No blocking issues found. Code is clean, well-structured, and matches every acceptance criterion from issue #122.

Minor note (non-blocking): transition_to sets the status before calling valid?, so on failure the object retains the invalid status in memory. This is standard Rails dirty-tracking behavior and the tests verify it. Callers should reload if they need to recover from a failed transition_to call. No action needed -- just documenting the design choice.

VERDICT: APPROVE

## QA Review -- PR #178 ### Checklist - [x] **Migration correctness** -- FKs to `properties` and `crew_members` with `null: false`, proper indexes on `status` and `request_type`, correct decimal precision for `price` (10,2) and `monthly_price` (8,2) - [x] **Status transition graph** -- `VALID_TRANSITIONS` hash matches the spec exactly: requested->{quoted,declined}, quoted->{paid,declined}, paid->{scheduled}, scheduled->{completed}, completed/declined->terminal - [x] **Validation coverage** -- request_type inclusion, status inclusion, price numericality (>=0, nil allowed), transition validation on status change - [x] **Transition validation skips new records** -- `return if new_record?` prevents false rejections when creating with a non-default status - [x] **Association setup** -- `has_many :service_requests, dependent: :destroy` on both Property and CrewMember - [x] **Seeds** -- prices for Mowing ($35), Weeding ($25), Edging & Trimming ($20) using idempotent `find_or_create_by!` + `update!` - [x] **Test completeness** -- validations, associations (both directions), every valid transition, every invalid transition per state, terminal state exhaustive check, `transition_to`/`transition_to!`/`terminal?`/`allowed_transitions` helpers - [x] **No secrets or unrelated changes** -- only the 8 targeted files - [x] **Schema.rb** -- auto-generated, matches migration output - [x] **All 278 specs pass** per PR body ### Findings No blocking issues found. Code is clean, well-structured, and matches every acceptance criterion from issue #122. **Minor note (non-blocking):** `transition_to` sets the status before calling `valid?`, so on failure the object retains the invalid status in memory. This is standard Rails dirty-tracking behavior and the tests verify it. Callers should `reload` if they need to recover from a failed `transition_to` call. No action needed -- just documenting the design choice. ### VERDICT: APPROVE

ldraney commented 2026-06-08 04:59:37 +00:00

Author

Owner

Copy link

PR #178 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, PostgreSQL, RSpec. Reviewed against Rails conventions, ActiveRecord migration best practices, and model validation patterns.

Migration correctness (2 migrations):

20260608020000_add_monthly_price_to_services.rb: Adds decimal, precision: 8, scale: 2 -- appropriate for monthly pricing (up to $999,999.99). Nullable is correct since existing services get backfilled via seeds. Clean.

20260608020001_create_service_requests.rb: Well-structured.

• Foreign keys with null: false on both property and crew_member -- correct, every request must belong to both.
• status has null: false, default: "requested" -- correct default for the initial state.
• price is nullable (precision: 10, scale: 2) -- appropriate since price is assigned at the "quoted" stage, not at creation.
• Indexes on status and request_type -- good for query filtering. FK indexes auto-created by t.references.
• stripe_payment_link, paid_at, completed_at all nullable -- correct for their lifecycle positions.

Model (service_request.rb):

The VALID_TRANSITIONS hash cleanly encodes the state graph:

requested -> [quoted, declined]
quoted    -> [paid, declined]
paid      -> [scheduled]
scheduled -> [completed]
completed -> []
declined  -> []

This matches the spec description (6 valid forward transitions, 2 decline exits, 2 terminal states).

The status_transition_is_valid callback correctly uses status_was to check the previous value and only runs on persisted records (return if new_record?). The if: :status_changed? guard avoids unnecessary validation on saves that don't touch status. Both are correct Rails patterns.

Associations:

• Property has_many :service_requests, dependent: :destroy -- correct.
• CrewMember has_many :service_requests, dependent: :destroy -- correct.
• ServiceRequest belongs_to :property / belongs_to :crew_member -- correct, matches FK constraints.
• No association on Service -- correct, since service_requests has no service_id FK.

Seeds: Idempotent via find_or_create_by! then update!. Always overwrites price on re-seed, which is the correct behavior for reference data.

No UI leakage: Confirmed. Only model, migration, seed, schema, and spec files touched. No controllers, views, routes, or JavaScript changed.

BLOCKERS

None.

NITS

1. Initial status not enforced at model level. The status_transition_is_valid callback has return if new_record?, which means ServiceRequest.create!(property: p, crew_member: c, request_type: "project", status: "completed") would succeed -- bypassing the entire state machine. The DB default handles the normal ServiceRequest.create! path (status defaults to "requested"), but an explicit status: "completed" on create is not rejected. Consider adding a validation like:

   validate :initial_status_must_be_requested, on: :create
   
   def initial_status_must_be_requested
     errors.add(:status, "must be 'requested' for new records") unless status == "requested"
   end
   

   Not a blocker because the DB default covers the happy path and no controller exists yet to accept user input, but this would harden the model before a controller is added.

2. terminal? duplicates knowledge from VALID_TRANSITIONS. The method hardcodes %w[completed declined] while this could be derived: VALID_TRANSITIONS.select { |_, v| v.empty? }.keys.include?(status). Low risk given the small, stable set, but the derivation would be self-maintaining if a new terminal state were added.

3. Missing spec: transition_to does not persist. The spec tests that transition_to returns true/false and sets the status, but does not assert that it does NOT save to the database. Adding expect(request).not_to be_changed after a reload (or expect(request.reload.status).to eq("requested") after a failed transition_to) would document the non-persisting contract explicitly.

4. Missing spec: initial status bypass. No test verifies what happens when creating a ServiceRequest with an explicit non-"requested" status. Whether you fix nit #1 or not, a spec documenting the current behavior would prevent accidental regressions.

5. crew_member naming on ServiceRequest. The FK is crew_member_id, implying the crew member assigned to handle the request. But a package_change request would typically be initiated by a client (who is also a CrewMember with role "client"). The naming is technically correct given the current data model, but a comment clarifying whether this represents the requester or the assignee would help future developers. Very minor.

SOP COMPLIANCE

• Branch named after issue: 122-service-request-model follows {issue-number}-{kebab-case-purpose}
• PR body follows template: Summary, Changes, Test Plan, Related sections all present
• Related references issue: Closes #122, parent spike #121 referenced
• No secrets committed: stripe_payment_link is a column definition, not an actual key/secret
• No unnecessary file changes: all 8 files directly serve the feature
• Commit messages are descriptive (per PR title)
• Test coverage present: 280-line spec with comprehensive transition matrix

PROCESS OBSERVATIONS

• Clean model-only PR with no scope creep. Good discipline separating model from controller/view layers.
• Test coverage is above average: all 6 valid transitions, all invalid transitions from each state, terminal state exhaustive checks, and helper method coverage. The spec-to-model line ratio (~5:1) reflects thorough testing.
• The update_column usage in specs to set up intermediate states is the correct pattern -- it bypasses validations intentionally to test transitions from arbitrary starting states.
• The monthly_price addition to services is well-scoped here since it supports the pricing context of service requests.
• Low change failure risk: model-only with no existing code dependencies on ServiceRequest.

VERDICT: APPROVED

Solid, well-tested model-only PR. The nits (especially #1 regarding initial status enforcement) should be addressed before a controller is added in a follow-up ticket, but are not blocking for a model-layer-only change. The state machine is correctly implemented, migrations are clean, and test coverage is comprehensive.

## PR #178 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, PostgreSQL, RSpec. Reviewed against Rails conventions, ActiveRecord migration best practices, and model validation patterns. **Migration correctness (2 migrations):** `20260608020000_add_monthly_price_to_services.rb`: Adds `decimal, precision: 8, scale: 2` -- appropriate for monthly pricing (up to $999,999.99). Nullable is correct since existing services get backfilled via seeds. Clean. `20260608020001_create_service_requests.rb`: Well-structured. - Foreign keys with `null: false` on both `property` and `crew_member` -- correct, every request must belong to both. - `status` has `null: false, default: "requested"` -- correct default for the initial state. - `price` is nullable (`precision: 10, scale: 2`) -- appropriate since price is assigned at the "quoted" stage, not at creation. - Indexes on `status` and `request_type` -- good for query filtering. FK indexes auto-created by `t.references`. - `stripe_payment_link`, `paid_at`, `completed_at` all nullable -- correct for their lifecycle positions. **Model (`service_request.rb`):** The `VALID_TRANSITIONS` hash cleanly encodes the state graph: ``` requested -> [quoted, declined] quoted -> [paid, declined] paid -> [scheduled] scheduled -> [completed] completed -> [] declined -> [] ``` This matches the spec description (6 valid forward transitions, 2 decline exits, 2 terminal states). The `status_transition_is_valid` callback correctly uses `status_was` to check the previous value and only runs on persisted records (`return if new_record?`). The `if: :status_changed?` guard avoids unnecessary validation on saves that don't touch status. Both are correct Rails patterns. **Associations:** - `Property has_many :service_requests, dependent: :destroy` -- correct. - `CrewMember has_many :service_requests, dependent: :destroy` -- correct. - `ServiceRequest belongs_to :property` / `belongs_to :crew_member` -- correct, matches FK constraints. - No association on `Service` -- correct, since `service_requests` has no `service_id` FK. **Seeds:** Idempotent via `find_or_create_by!` then `update!`. Always overwrites price on re-seed, which is the correct behavior for reference data. **No UI leakage:** Confirmed. Only model, migration, seed, schema, and spec files touched. No controllers, views, routes, or JavaScript changed. ### BLOCKERS None. ### NITS 1. **Initial status not enforced at model level.** The `status_transition_is_valid` callback has `return if new_record?`, which means `ServiceRequest.create!(property: p, crew_member: c, request_type: "project", status: "completed")` would succeed -- bypassing the entire state machine. The DB default handles the normal `ServiceRequest.create!` path (status defaults to "requested"), but an explicit `status: "completed"` on create is not rejected. Consider adding a validation like: ```ruby validate :initial_status_must_be_requested, on: :create def initial_status_must_be_requested errors.add(:status, "must be 'requested' for new records") unless status == "requested" end ``` Not a blocker because the DB default covers the happy path and no controller exists yet to accept user input, but this would harden the model before a controller is added. 2. **`terminal?` duplicates knowledge from `VALID_TRANSITIONS`.** The method hardcodes `%w[completed declined]` while this could be derived: `VALID_TRANSITIONS.select { |_, v| v.empty? }.keys.include?(status)`. Low risk given the small, stable set, but the derivation would be self-maintaining if a new terminal state were added. 3. **Missing spec: `transition_to` does not persist.** The spec tests that `transition_to` returns true/false and sets the status, but does not assert that it does NOT save to the database. Adding `expect(request).not_to be_changed` after a reload (or `expect(request.reload.status).to eq("requested")` after a failed `transition_to`) would document the non-persisting contract explicitly. 4. **Missing spec: initial status bypass.** No test verifies what happens when creating a ServiceRequest with an explicit non-"requested" status. Whether you fix nit #1 or not, a spec documenting the current behavior would prevent accidental regressions. 5. **`crew_member` naming on ServiceRequest.** The FK is `crew_member_id`, implying the crew member assigned to handle the request. But a `package_change` request would typically be initiated by a client (who is also a CrewMember with role "client"). The naming is technically correct given the current data model, but a comment clarifying whether this represents the requester or the assignee would help future developers. Very minor. ### SOP COMPLIANCE - [x] Branch named after issue: `122-service-request-model` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Related sections all present - [x] Related references issue: `Closes #122`, parent spike #121 referenced - [x] No secrets committed: `stripe_payment_link` is a column definition, not an actual key/secret - [x] No unnecessary file changes: all 8 files directly serve the feature - [x] Commit messages are descriptive (per PR title) - [x] Test coverage present: 280-line spec with comprehensive transition matrix ### PROCESS OBSERVATIONS - Clean model-only PR with no scope creep. Good discipline separating model from controller/view layers. - Test coverage is above average: all 6 valid transitions, all invalid transitions from each state, terminal state exhaustive checks, and helper method coverage. The spec-to-model line ratio (~5:1) reflects thorough testing. - The `update_column` usage in specs to set up intermediate states is the correct pattern -- it bypasses validations intentionally to test transitions from arbitrary starting states. - The `monthly_price` addition to `services` is well-scoped here since it supports the pricing context of service requests. - Low change failure risk: model-only with no existing code dependencies on ServiceRequest. ### VERDICT: APPROVED Solid, well-tested model-only PR. The nits (especially #1 regarding initial status enforcement) should be addressed before a controller is added in a follow-up ticket, but are not blocking for a model-layer-only change. The state machine is correctly implemented, migrations are clean, and test coverage is comprehensive.

ldraney merged commit 53910561e8 into main 2026-06-08 05:02:38 +00:00

ldraney deleted branch 122-service-request-model 2026-06-08 05:02:38 +00:00

ldraney referenced this pull request from a commit 2026-06-08 05:02:39 +00:00

Add ServiceRequest model with status transition validation (#178)

ldraney referenced this pull request 2026-06-08 05:10:38 +00:00

docs: service request architecture with mermaid diagrams #182

ldraney referenced this pull request 2026-06-13 22:06:54 +00:00

Bug: ServiceRequest missing decline transitions from quoted and accepted states #206

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

No description provided.