ldraney/landscaping-assistant
1
0
Fork 0
Code Issues 32 Pull requests 1 Projects Releases Packages Wiki Activity Actions 4

Add request specs for properties CRUD and resolve endpoint #53

Merged
ldraney merged 1 commit from 51-add-request-specs-for-properties-crud-an into main 2026-06-01 06:51:00 +00:00
Conversation 2 Commits 1 Files changed 1 +239
ldraney commented 2026-05-29 23:41:20 +00:00
Owner
Copy link

Summary

Adds 22 request specs covering the properties controller endpoints that had zero test coverage: show, edit, update, and the resolve endpoint. All existing specs preserved. Full suite passes at 79 examples, 0 failures.

Changes

  • spec/requests/properties_spec.rb -- Extended with specs for:
    • GET /properties/:id (6 specs): status, address, client name, active status, special notes, services
    • GET /properties/:id/edit (3 specs): status, form content, service checkboxes
    • PATCH /properties/:id (5 specs): success redirect with :see_other, service assignment update, validation failure re-render, JSON success, JSON error
    • POST /properties/resolve (8 specs): create new with address, find existing (no count change), client-name-only create, TBD default, service assignment, preserve existing client_name, special_notes, empty params default

Test Plan

  • bundle exec rspec spec/requests/properties_spec.rb -- 28 examples, 0 failures
  • bundle exec rspec -- 79 examples, 0 failures
  • CI pipeline will run both lint and test steps

Review Checklist

  • Tests pass locally
  • No app code changes (test-only)
  • Follows existing spec patterns (Property.create!, let!, describe blocks)
  • No new dependencies added

Related Notes

None -- test-only change, no docs or SOPs affected.

Related

Closes #51

## Summary Adds 22 request specs covering the properties controller endpoints that had zero test coverage: show, edit, update, and the resolve endpoint. All existing specs preserved. Full suite passes at 79 examples, 0 failures. ## Changes - `spec/requests/properties_spec.rb` -- Extended with specs for: - `GET /properties/:id` (6 specs): status, address, client name, active status, special notes, services - `GET /properties/:id/edit` (3 specs): status, form content, service checkboxes - `PATCH /properties/:id` (5 specs): success redirect with `:see_other`, service assignment update, validation failure re-render, JSON success, JSON error - `POST /properties/resolve` (8 specs): create new with address, find existing (no count change), client-name-only create, TBD default, service assignment, preserve existing client_name, special_notes, empty params default ## Test Plan - `bundle exec rspec spec/requests/properties_spec.rb` -- 28 examples, 0 failures - `bundle exec rspec` -- 79 examples, 0 failures - CI pipeline will run both lint and test steps ## Review Checklist - [x] Tests pass locally - [x] No app code changes (test-only) - [x] Follows existing spec patterns (Property.create!, let!, describe blocks) - [x] No new dependencies added ## Related Notes None -- test-only change, no docs or SOPs affected. ## Related Closes #51
Add request specs for show, edit, update, and resolve endpoints
Some checks failed
ci/woodpecker/push/woodpecker Pipeline was successful
Details
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
CI / scan_ruby (pull_request) Has been cancelled
Details
CI / scan_js (pull_request) Has been cancelled
Details
CI / lint (pull_request) Has been cancelled
Details
a8b1c6cf69 
Cover the properties CRUD gaps identified in testing strategy audit:
show (content rendering), edit (form + services), update (success
redirect, validation failure, JSON responses), and resolve (create new,
find existing, client-name-only, service assignment, defaults).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-05-29 23:41:58 +00:00
Author
Owner
Copy link

QA Review

Scope: 1 file changed, +239 lines (test-only)

Acceptance Criteria Check

Criterion Status
GET /properties/:id returns 200 with property content Covered (6 specs)
GET /properties/:id/edit returns 200 with form Covered (3 specs)
POST /properties creates a property and redirects Skipped -- controller has no create action (route exists but action is missing)
POST /properties with invalid params returns unprocessable_entity Skipped -- same reason
PATCH /properties/:id updates and redirects Covered
PATCH /properties/:id with invalid params re-renders edit Covered
POST /properties/resolve with street + number finds or creates Covered
POST /properties/resolve with existing address returns existing Covered
POST /properties/resolve without street creates with client_name only Covered
POST /properties/resolve with service_ids assigns services Covered
All existing specs continue to pass Verified (79 examples, 0 failures)

Findings

No blocking issues.

  • Style matches existing patterns in work_queue_items_spec.rb and uploads_spec.rb (let!, Property.create!, Service.find_or_create_by!, describe blocks per action)
  • No app code touched, no new dependencies
  • Both resolve paths tested: find_or_create_by! (existing match) and create! (no street)
  • Update specs cover both HTML (see_other redirect) and JSON respond_to branches
  • The POST /properties create specs are correctly omitted -- the route is defined in routes.rb but the controller has no create action. This is a pre-existing gap in the app code, not a test gap.

VERDICT: APPROVE

## QA Review **Scope:** 1 file changed, +239 lines (test-only) ### Acceptance Criteria Check | Criterion | Status | |-----------|--------| | `GET /properties/:id` returns 200 with property content | Covered (6 specs) | | `GET /properties/:id/edit` returns 200 with form | Covered (3 specs) | | `POST /properties` creates a property and redirects | Skipped -- controller has no `create` action (route exists but action is missing) | | `POST /properties` with invalid params returns unprocessable_entity | Skipped -- same reason | | `PATCH /properties/:id` updates and redirects | Covered | | `PATCH /properties/:id` with invalid params re-renders edit | Covered | | `POST /properties/resolve` with street + number finds or creates | Covered | | `POST /properties/resolve` with existing address returns existing | Covered | | `POST /properties/resolve` without street creates with client_name only | Covered | | `POST /properties/resolve` with service_ids assigns services | Covered | | All existing specs continue to pass | Verified (79 examples, 0 failures) | ### Findings **No blocking issues.** - Style matches existing patterns in `work_queue_items_spec.rb` and `uploads_spec.rb` (`let!`, `Property.create!`, `Service.find_or_create_by!`, describe blocks per action) - No app code touched, no new dependencies - Both resolve paths tested: `find_or_create_by!` (existing match) and `create!` (no street) - Update specs cover both HTML (`see_other` redirect) and JSON `respond_to` branches - The `POST /properties` create specs are correctly omitted -- the route is defined in `routes.rb` but the controller has no `create` action. This is a pre-existing gap in the app code, not a test gap. ### VERDICT: APPROVE
ldraney commented 2026-05-29 23:45:58 +00:00
Author
Owner
Copy link

PR #53 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, RSpec request specs, PostgreSQL. No app code changes -- test-only PR.

Controller fidelity: I read app/controllers/properties_controller.rb line by line and traced every test assertion against the actual controller logic. The tests are accurate:

  • find_or_create_by! uses (address_line, city, state, zip_code) as the composite lookup key. The "finds an existing property by address" test correctly constructs a matching address and asserts no count change + updated client_name. Verified against controller lines 22-37.
  • The "preserves existing client_name when resolving without one" test correctly relies on params[:client_name].presence || property.client_name (controller line 34). Sending no client_name param means .presence returns nil, fallback to original. Correct.
  • The TBD default tests match controller lines 28 and 40 (params[:client_name].presence || "TBD"). Correct.
  • Service assignment test correctly exercises controller lines 49-51. Correct.
  • The special_notes test exercises the create-path block (controller line 29). Correct.

No create action -- resolve IS the creation path: Confirmed. The controller has no create method. The resolve endpoint handles both find-or-create (with address) and create-only (without address). The tests correctly cover both branches. The routes file (config/routes.rb line 6) declares :create in the resources block but no controller action exists -- this is a pre-existing latent issue, not in scope for this PR.

Turbo Drive 303 convention: The PATCH /properties/:id success path correctly asserts have_http_status(:see_other), matching docs/hotwire.md requirements and controller line 69. The resolve endpoint returns JSON (not a redirect), so 303 is not applicable there. Correct.

Validation behavior: The update failure tests (lines 123-128, 141-148) correctly rely on the validates :client_name, presence: true model validation (app/models/property.rb line 8). Sending client_name: "" triggers unprocessable_entity. Both HTML and JSON error paths are covered.

Test style: Matches existing patterns in spec/requests/work_queue_items_spec.rb and spec/requests/uploads_spec.rb -- uses let! setup, Property.create! (no factories), describe blocks per endpoint, Service.find_or_create_by! for test data. Consistent.

BLOCKERS

None.

NITS

  1. Missing edge case: number present, street blank (controller line 18). When number is present but street is blank/nil, has_address evaluates to false and falls through to the else branch, creating a new property with address_line: nil (line 42, since has_address is false). This is a valid code path in production (user types a number before GPS resolves a street). A test like post "/properties/resolve", params: { number: "123" } would confirm the behavior. Low risk since the fallback is safe, but worth adding for completeness.

  2. Missing edge case: same address, different city (controller line 22-26). find_or_create_by! includes city in the lookup key, so "123 Oak Ave" in Austin and "123 Oak Ave" in Dallas are distinct properties. No test covers this. Again low risk, but it documents an important business rule.

  3. Double property.reload on lines 108-109. Could be property.reload once, then check both attributes. Extremely minor -- readability is fine as-is.

  4. PR body says 22 specs, later says 28 examples (6 pre-existing + 22 new = 28). The body text is slightly inconsistent -- the Summary says "22 request specs" while Test Plan says "28 examples, 0 failures". Both are correct but could be clearer that 28 is the total for the file. Trivial.

SOP COMPLIANCE

  • Branch named after issue: 51-add-request-specs-for-properties-crud-an follows {issue-number}-{kebab-case-purpose} convention
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related section references plan slug -- says "None -- test-only change" which is acceptable for test-only PRs, though ideally would reference the parent issue #51 (which it does via "Closes #51")
  • Tests exist and pass (28 examples, 0 failures per PR body)
  • No secrets, .env files, or credentials committed
  • No unnecessary file changes -- single file changed, test-only, 0 deletions from existing code
  • Commit scope is clean -- no app code modifications

PROCESS OBSERVATIONS

This PR fills a real gap -- the properties controller had the most complex endpoint in the app (resolve with find-or-create, conditional updates, service assignment) and zero test coverage. Adding 22 specs for show/edit/update/resolve significantly reduces change failure risk for future work on these endpoints. The test patterns are consistent with the rest of the suite, which keeps maintenance cost low.

The only gap worth noting for future work: the resolve endpoint has no error handling for find_or_create_by! or create! failures -- if a database constraint fails, it will raise a 500. That is a pre-existing controller issue, not a test gap in this PR.

VERDICT: APPROVED

## PR #53 Review ### DOMAIN REVIEW **Stack**: Ruby on Rails 8.1, RSpec request specs, PostgreSQL. No app code changes -- test-only PR. **Controller fidelity**: I read `app/controllers/properties_controller.rb` line by line and traced every test assertion against the actual controller logic. The tests are accurate: - `find_or_create_by!` uses `(address_line, city, state, zip_code)` as the composite lookup key. The "finds an existing property by address" test correctly constructs a matching address and asserts no count change + updated client_name. Verified against controller lines 22-37. - The "preserves existing client_name when resolving without one" test correctly relies on `params[:client_name].presence || property.client_name` (controller line 34). Sending no `client_name` param means `.presence` returns nil, fallback to original. Correct. - The TBD default tests match controller lines 28 and 40 (`params[:client_name].presence || "TBD"`). Correct. - Service assignment test correctly exercises controller lines 49-51. Correct. - The `special_notes` test exercises the create-path block (controller line 29). Correct. **No `create` action -- `resolve` IS the creation path**: Confirmed. The controller has no `create` method. The `resolve` endpoint handles both find-or-create (with address) and create-only (without address). The tests correctly cover both branches. The routes file (`config/routes.rb` line 6) declares `:create` in the `resources` block but no controller action exists -- this is a pre-existing latent issue, not in scope for this PR. **Turbo Drive 303 convention**: The `PATCH /properties/:id` success path correctly asserts `have_http_status(:see_other)`, matching `docs/hotwire.md` requirements and controller line 69. The `resolve` endpoint returns JSON (not a redirect), so 303 is not applicable there. Correct. **Validation behavior**: The update failure tests (lines 123-128, 141-148) correctly rely on the `validates :client_name, presence: true` model validation (`app/models/property.rb` line 8). Sending `client_name: ""` triggers unprocessable_entity. Both HTML and JSON error paths are covered. **Test style**: Matches existing patterns in `spec/requests/work_queue_items_spec.rb` and `spec/requests/uploads_spec.rb` -- uses `let!` setup, `Property.create!` (no factories), `describe` blocks per endpoint, `Service.find_or_create_by!` for test data. Consistent. ### BLOCKERS None. ### NITS 1. **Missing edge case: number present, street blank** (controller line 18). When `number` is present but `street` is blank/nil, `has_address` evaluates to false and falls through to the `else` branch, creating a new property with `address_line: nil` (line 42, since `has_address` is false). This is a valid code path in production (user types a number before GPS resolves a street). A test like `post "/properties/resolve", params: { number: "123" }` would confirm the behavior. Low risk since the fallback is safe, but worth adding for completeness. 2. **Missing edge case: same address, different city** (controller line 22-26). `find_or_create_by!` includes `city` in the lookup key, so `"123 Oak Ave" in Austin` and `"123 Oak Ave" in Dallas` are distinct properties. No test covers this. Again low risk, but it documents an important business rule. 3. **Double `property.reload` on lines 108-109**. Could be `property.reload` once, then check both attributes. Extremely minor -- readability is fine as-is. 4. **PR body says 22 specs, later says 28 examples** (6 pre-existing + 22 new = 28). The body text is slightly inconsistent -- the Summary says "22 request specs" while Test Plan says "28 examples, 0 failures". Both are correct but could be clearer that 28 is the total for the file. Trivial. ### SOP COMPLIANCE - [x] Branch named after issue: `51-add-request-specs-for-properties-crud-an` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related section references plan slug -- says "None -- test-only change" which is acceptable for test-only PRs, though ideally would reference the parent issue #51 (which it does via "Closes #51") - [x] Tests exist and pass (28 examples, 0 failures per PR body) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- single file changed, test-only, 0 deletions from existing code - [x] Commit scope is clean -- no app code modifications ### PROCESS OBSERVATIONS This PR fills a real gap -- the properties controller had the most complex endpoint in the app (`resolve` with find-or-create, conditional updates, service assignment) and zero test coverage. Adding 22 specs for show/edit/update/resolve significantly reduces change failure risk for future work on these endpoints. The test patterns are consistent with the rest of the suite, which keeps maintenance cost low. The only gap worth noting for future work: the `resolve` endpoint has no error handling for `find_or_create_by!` or `create!` failures -- if a database constraint fails, it will raise a 500. That is a pre-existing controller issue, not a test gap in this PR. ### VERDICT: APPROVED
ldraney deleted branch 51-add-request-specs-for-properties-crud-an 2026-06-01 06:51:01 +00:00
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
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!53
No description provided.