Review: ServiceRequest model + migration (re-review)

Verdict: APPROVED

Previous Findings -- Resolution

• [FIXED] user_id column ambiguity -- Changed to crew_member_id (FK to crew_members). Verified: crew_members table exists in committed schema with columns: name, keycloak_username, role, active. The Constraints section explicitly states "crew_member_id references crew_members table (clients are crew_members with role=client)". Clean resolution.
• [FIXED] Status transition rules missing -- Explicit Status Transition Graph section added with six valid transitions: requested->quoted, requested->declined, quoted->paid, quoted->declined, paid->scheduled, scheduled->completed. Terminal states documented (declined, completed). "No other transitions are valid" is stated. An agent can implement this unambiguously.
• [FIXED] Feature Flag section missing -- Section added with value "None". Correct for an internal model change with no UI.

Template Completeness

• [x] Type -- Feature
• [x] Lineage -- child of spike #121
• [x] Repo -- ldraney/landscaping-assistant
• [x] User Story
• [x] Context
• [x] File Targets -- 7 targets listed
• [x] Acceptance Criteria -- 5 criteria
• [x] Status Transition Graph -- NEW, addresses previous gap
• [x] Test Expectations
• [x] Constraints
• [x] Feature Flag -- "None" (addresses previous gap)
• [x] Checklist
• [x] Related

Traceability

• [x] story:auth label -- auth story
• [x] story note verified -- "auth" entry found in project-landscaping-assistant user-stories table
• [x] arch:rails-app label
• [ ] arch note MISSING -- [SCOPE] No arch-rails-app note exists in pal-e-docs. Carried forward from first review. Non-blocking for this ticket.
• [x] Forgejo issue -- ldraney/landscaping-assistant#122, state: open

File Targets

• [x] db/migrate/*_create_service_requests.rb (new) -- OK, no existing ServiceRequest migration. No ServiceRequest references anywhere in codebase.
• [x] db/migrate/*_add_monthly_price_to_services.rb (new) -- OK, services table exists in schema. No monthly_price column present. ERD in docs confirms this addition.
• [x] app/models/service_request.rb (new) -- OK, no existing model or references in codebase.
• [x] app/models/property.rb -- verified exists. Has has_many :property_services, has_many :services, has_many :work_queue_items. Adding has_many :service_requests is straightforward.
• [x] app/models/crew_member.rb -- verified exists. Has validations for name, keycloak_username, role. Adding has_many :service_requests is straightforward. crew_members table confirmed in committed schema.
• [x] db/seeds.rb -- verified exists. Currently seeds 3 services (Edging & Trimming, Mowing, Weeding) without prices via find_or_create_by!. Adding monthly_price will need idempotent update pattern.
• [x] spec/models/service_request_spec.rb (new) -- OK, spec/models/ directory exists with 8 existing model specs. Pattern is consistent.

Repo Placement

Dependencies

• #121 (parent spike) -- done (board item 1346 in done column). No blocker.
• crew_members table -- exists in committed schema (migration 20260607030548). CrewMember model confirmed with ROLES including "client". No blocker.
• Stripe -- stripe_payment_link is a plain string column (URL). No Stripe gem dependency. Separate ticket #125 handles integration. No blocker.
• Downstream -- #123 (Client request UI) and #124 (Admin request management) depend on this model. Both in backlog, not blocked.
• #176 (Property detail page: Projects section) -- explicitly says "depends on #122". In backlog. Correctly ordered.

Acceptance Criteria

• AC1 (ServiceRequest model with columns) -- testable. All columns clearly specified with types. crew_member_id FK is now unambiguous.
• AC2 (Service gains monthly_price) -- testable, clear.
• AC3 (Seeds updated with prices) -- testable, three service names match existing seeds.
• AC4 (Status transitions per graph) -- now fully specified with 6 valid transitions and 2 terminal states. Agent can implement deterministically.
• AC5 (Model specs) -- testable, clear.

Blast Radius

• Property model -- adding has_many :service_requests is low risk. No existing associations conflict.
• CrewMember model -- adding has_many :service_requests is low risk. No existing associations on this model.
• Service model -- adding monthly_price column is low risk. No existing code reads a price from services.
• Seeds -- uses find_or_create_by!. Will need idempotent update for prices (e.g., find_or_create_by!(name:).tap { |s| s.update!(monthly_price:) }). Ticket should note this but it is not blocking.
• No downstream breakage risk -- new model with no UI. Existing tests unaffected.

Decomposition Assessment

Recommendations

• [SCOPE] Create architecture note arch-rails-app in pal-e-docs (carried forward, non-blocking).