Fix accepted → declined transition and update service-requests doc #209

Merged

ldraney merged 2 commits from 206-fix-decline-transitions into main 2026-06-14 02:07:29 +00:00

Conversation 1 Commits 2 Files changed 3 +6 -5

ldraney commented 2026-06-13 22:18:58 +00:00

Owner

Copy link

Summary

• Add declined to VALID_TRANSITIONS["accepted"] so customers can back out after accepting a quote
• Update docs/service-requests.md to reflect that accepted is implemented (was incorrectly noted as "planned for #179")
• Update spec: accepted → declined assertion changed from "rejects" to "allows"

Note: quoted → declined was already implemented — initial bug report over-stated the scope. Only accepted → declined was missing.

Changes

• app/models/service_request.rb: added "declined" to VALID_TRANSITIONS["accepted"]
• spec/models/service_request_spec.rb: changed accepted → declined test from "rejects" to "allows"
• docs/service-requests.md: updated status note to list all 7 statuses including accepted, updated ER diagram status field

Test Plan

• bundle exec rspec spec/models/service_request_spec.rb — 52 examples, 0 failures
• Verify no other specs rely on accepted → declined being blocked
• No regressions in service request transition logic

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 new UI surface

Related Notes

• ldraney/landscaping-assistant #206 — the Forgejo issue this PR fixes
• ldraney/landscaping-assistant #122 — ServiceRequest model (parent ticket)
• ldraney/landscaping-assistant #176 — Property detail projects display (downstream)

Closes #206

## Summary - Add `declined` to `VALID_TRANSITIONS["accepted"]` so customers can back out after accepting a quote - Update `docs/service-requests.md` to reflect that `accepted` is implemented (was incorrectly noted as "planned for #179") - Update spec: `accepted → declined` assertion changed from "rejects" to "allows" **Note:** `quoted → declined` was already implemented — initial bug report over-stated the scope. Only `accepted → declined` was missing. ## Changes - `app/models/service_request.rb`: added `"declined"` to `VALID_TRANSITIONS["accepted"]` - `spec/models/service_request_spec.rb`: changed `accepted → declined` test from "rejects" to "allows" - `docs/service-requests.md`: updated status note to list all 7 statuses including `accepted`, updated ER diagram status field ## Test Plan - [x] `bundle exec rspec spec/models/service_request_spec.rb` — 52 examples, 0 failures - [ ] Verify no other specs rely on `accepted → declined` being blocked - [ ] No regressions in service request transition logic ## 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 new UI surface ## Related Notes - `ldraney/landscaping-assistant #206` — the Forgejo issue this PR fixes - `ldraney/landscaping-assistant #122` — ServiceRequest model (parent ticket) - `ldraney/landscaping-assistant #176` — Property detail projects display (downstream) Closes #206

ldraney added 1 commit 2026-06-13 22:18:58 +00:00

Fix accepted → declined transition and update service-requests doc ... 8cc332d421

The accepted state had no decline exit, locking customers in after
accepting a quote. Also corrects docs that listed accepted as planned
when the model already implements it.

Closes #206

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

ldraney commented 2026-06-13 22:20:13 +00:00

Author

Owner

Copy link

PR #209 Review

DOMAIN REVIEW

Stack: Ruby on Rails model + RSpec + Markdown docs. State machine pattern using a VALID_TRANSITIONS hash with custom validation.

Model change (service_request.rb):
The fix is correct. Adding "declined" to VALID_TRANSITIONS["accepted"] fills the only gap in the decline path. Every other non-terminal state (requested, quoted) already allows declined as an exit. The accepted state was the sole exception, meaning a customer who accepted a quote had no way to back out without admin intervention at the database level. The fix is minimal and precisely scoped.

Spec change (service_request_spec.rb):
The existing test at line 189 is correctly flipped from expect(request).not_to be_valid to expect(request).to be_valid. The test description is updated to match. This is the right approach -- the spec already existed to assert the old behavior, so flipping the assertion is cleaner than deleting and re-adding.

Doc change (service-requests.md):
The stale note claiming accepted was "planned for #179" is correctly updated to reflect that it is implemented. The ER diagram status field now lists all 7 statuses.

BLOCKERS

None.

NITS

1. Mermaid state diagram missing accepted --> declined arrow. The stateDiagram-v2 block (lines 10-22 of docs/service-requests.md) does not include an accepted --> declined transition. The PR adds the transition to the model and updates the ER diagram status enum, but the state diagram still omits this edge. Since the doc's purpose is to visualize the state machine, this is an inconsistency worth fixing. Consider adding accepted --> declined : Client backs out between lines 18 and 19.

2. PR body test plan has unchecked items. Two test plan items are marked [ ] -- "Verify no other specs rely on accepted -> declined being blocked" and "No regressions in service request transition logic." These appear to be manual verification items. If they have been verified, check them off before merge for traceability.

SOP COMPLIANCE

• PR body has ## Summary, ## Changes, ## Test Plan, ## Related
• No secrets committed
• No unnecessary file changes -- all 3 files are directly relevant to the fix
• Commit messages are descriptive (branch name 206-fix-decline-transitions is clear)
• Tests exist and cover the change (spec flipped from reject to allow)
• Closes #206 present in PR body

PROCESS OBSERVATIONS

• Small, focused bug fix (3 files, +5/-5). Good change failure risk profile.
• The PR body note explaining that quoted -> declined was already working and only accepted -> declined was missing is helpful context for future readers.
• Test coverage for the state machine is thorough -- every state has explicit transition specs for both allowed and rejected paths, plus exhaustive terminal state checks.

VERDICT: APPROVED

## PR #209 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails model + RSpec + Markdown docs. State machine pattern using a `VALID_TRANSITIONS` hash with custom validation. **Model change (service_request.rb):** The fix is correct. Adding `"declined"` to `VALID_TRANSITIONS["accepted"]` fills the only gap in the decline path. Every other non-terminal state (`requested`, `quoted`) already allows `declined` as an exit. The `accepted` state was the sole exception, meaning a customer who accepted a quote had no way to back out without admin intervention at the database level. The fix is minimal and precisely scoped. **Spec change (service_request_spec.rb):** The existing test at line 189 is correctly flipped from `expect(request).not_to be_valid` to `expect(request).to be_valid`. The test description is updated to match. This is the right approach -- the spec already existed to assert the old behavior, so flipping the assertion is cleaner than deleting and re-adding. **Doc change (service-requests.md):** The stale note claiming `accepted` was "planned for #179" is correctly updated to reflect that it is implemented. The ER diagram status field now lists all 7 statuses. ### BLOCKERS None. ### NITS 1. **Mermaid state diagram missing `accepted --> declined` arrow.** The `stateDiagram-v2` block (lines 10-22 of `docs/service-requests.md`) does not include an `accepted --> declined` transition. The PR adds the transition to the model and updates the ER diagram status enum, but the state diagram still omits this edge. Since the doc's purpose is to visualize the state machine, this is an inconsistency worth fixing. Consider adding `accepted --> declined : Client backs out` between lines 18 and 19. 2. **PR body test plan has unchecked items.** Two test plan items are marked `[ ]` -- "Verify no other specs rely on `accepted -> declined` being blocked" and "No regressions in service request transition logic." These appear to be manual verification items. If they have been verified, check them off before merge for traceability. ### SOP COMPLIANCE - [x] PR body has ## Summary, ## Changes, ## Test Plan, ## Related - [x] No secrets committed - [x] No unnecessary file changes -- all 3 files are directly relevant to the fix - [x] Commit messages are descriptive (branch name `206-fix-decline-transitions` is clear) - [x] Tests exist and cover the change (spec flipped from reject to allow) - [x] `Closes #206` present in PR body ### PROCESS OBSERVATIONS - Small, focused bug fix (3 files, +5/-5). Good change failure risk profile. - The PR body note explaining that `quoted -> declined` was already working and only `accepted -> declined` was missing is helpful context for future readers. - Test coverage for the state machine is thorough -- every state has explicit transition specs for both allowed and rejected paths, plus exhaustive terminal state checks. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-13 22:20:38 +00:00

Add accepted → declined arrow to mermaid state diagram ... 61b9637c6a

QA review nit: the state diagram was missing the transition arrow
that the model and ER diagram already include.

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

ldraney merged commit ab7258739f into main 2026-06-14 02:07:29 +00:00

ldraney deleted branch 206-fix-decline-transitions 2026-06-14 02:07:29 +00:00

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

Fix accepted → declined transition and update service-requests doc (#209)

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

No description provided.