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

Add auth user stories: roles, phases, and data model impact #114

Closed
ldraney wants to merge 2 commits from docs/user-stories-auth into main
pull from: docs/user-stories-auth
merge into: ldraney:main
Conversation 2 Commits 2 Files changed 1 +279
ldraney commented 2026-06-06 03:13:33 +00:00
Owner
Copy link

Summary

  • Documents multi-user auth model with four incremental delivery phases
  • Defines three roles (viewer, lead, admin) grounded in real users (Lucas, Devin, future crew)
  • Flags per-user queue as a key data model change and includes permission matrix

Changes

  • docs/user-stories-auth.md: New file with actor definitions, 12 user stories across 4 phases, role permission matrix, user interaction diagrams, data model impact, and open questions

Test Plan

  • Review role definitions match real-world crew structure
  • Confirm phase ordering makes sense for incremental delivery
  • Verify data model impact section captures all breaking changes
  • Answer open questions at the bottom before ticketing

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related Notes

  • ldraney/landscaping-assistant #107 -- parent auth issue
  • Closes #113
## Summary - Documents multi-user auth model with four incremental delivery phases - Defines three roles (viewer, lead, admin) grounded in real users (Lucas, Devin, future crew) - Flags per-user queue as a key data model change and includes permission matrix ## Changes - `docs/user-stories-auth.md`: New file with actor definitions, 12 user stories across 4 phases, role permission matrix, user interaction diagrams, data model impact, and open questions ## Test Plan - [ ] Review role definitions match real-world crew structure - [ ] Confirm phase ordering makes sense for incremental delivery - [ ] Verify data model impact section captures all breaking changes - [ ] Answer open questions at the bottom before ticketing ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `ldraney/landscaping-assistant #107` -- parent auth issue - Closes #113
Add auth user stories: roles, phases, and data model impact
Some checks are pending
ci/woodpecker/push/woodpecker Pipeline was successful
Details
CI / scan_ruby (pull_request) Waiting to run
Details
CI / scan_js (pull_request) Waiting to run
Details
CI / lint (pull_request) Waiting to run
Details
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
81c24b2291 
Documents the multi-user auth model with four incremental phases
(login → read-only crew → team leads → admin oversight) and
per-user queue implications.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-06 03:14:59 +00:00
Author
Owner
Copy link

PR #114 Review

DOMAIN REVIEW

This is a documentation-only PR adding docs/user-stories-auth.md (283 lines, 0 deletions). No code changes. The domain review focuses on documentation quality, accuracy against the existing codebase, and completeness as a planning artifact.

Documentation quality: Strong. The document is well-structured with clear actor definitions, phased user stories with acceptance criteria, a permission matrix, Mermaid diagrams (ER, flow, Gantt-style delivery), and open questions. Each user story follows a consistent format with ID, narrative, and acceptance criteria.

Accuracy against existing codebase: Verified.

  • The doc states "The current QueueItem model has no user association -- it's a single shared queue." This is confirmed correct. The actual model is WorkQueueItem (see app/models/work_queue_item.rb), which has belongs_to :property with a uniqueness constraint on [work_date, property_id] and no user/username column. The per-user queue change flagged in the doc is a genuine breaking change.
  • The Property model fields in the ER diagram (client_name, address_line, special_notes, active) match the actual db/schema.rb Property table.
  • The document correctly identifies PropertyService and Upload as models that exist in the codebase and would need PaperTrail tracking.

Model naming inconsistency (nit): The doc's ER diagram uses QUEUE_ITEM with a username field, but the existing model is called WorkQueueItem. The doc should either use the actual model name or note the rename. This matters because implementers will look at this doc and need to know whether a rename is intended.

Mermaid diagrams: All four diagrams (actor graph, interaction diagram, ER diagram, delivery order) are syntactically valid and semantically meaningful. The ER diagram correctly shows the new KEYCLOAK_USER entity and its relationship to queue items.

BLOCKERS

None. This is a documentation-only PR with no code changes. The BLOCKER criteria (test coverage, input validation, secrets, DRY auth violations) do not apply.

NITS

  1. Model name mismatch: The ER diagram uses QUEUE_ITEM but the existing model is WorkQueueItem. Consider using the actual name or adding a note like "renamed from WorkQueueItem" if a rename is planned.

  2. PR body references wrong parent: The PR body says "Related Notes: ldraney/landscaping-assistant #107 -- parent auth issue" but also "Closes #113". The task prompt identifies #113 as the parent issue, and #107 is the broader auth epic. The Related section should be clearer about which is the parent issue vs. the epic. This is minor since both references are valid and useful.

  3. Phase reference tags: The delivery diagram uses #107a, #107b, #107c, #107d as phase identifiers, but these don't correspond to actual Forgejo issues. Consider noting these are future ticket placeholders, or remove the tags until tickets are created.

  4. ROADMAP.md consistency: The existing docs/ROADMAP.md line 183 lists "Keycloak auth (multi-user, crew member accounts)" under "Future (Not Scoped)". Once this user stories doc lands, consider updating ROADMAP.md to reference the new doc or mark auth as "scoped" with a link.

SOP COMPLIANCE

  • Branch named after issue -- Branch is docs/user-stories-auth. This uses a docs/ prefix convention rather than {issue-number}-{kebab-case-purpose} (would be 113-user-stories-auth). Minor deviation from SOP but the intent is clear and the docs/ prefix is reasonable for a documentation-only branch.
  • PR body follows template -- Has Summary, Changes, Test Plan, Review Checklist, Related Notes sections.
  • Related references plan slug -- No plan slug provided (confirmed: "No plan slug" per task prompt). Not a blocker for a documentation PR.
  • No secrets committed -- Confirmed. Single markdown file, no credentials.
  • No unnecessary file changes -- Single file, tightly scoped to the issue.
  • Commit messages are descriptive -- Recent commits: "Add auth user stories: roles, phases, and data model impact" and "Fix review findings: tests, N+1, ARIA, error handling, dead CSS" (the latter from a prior PR on the branch's base).

PROCESS OBSERVATIONS

  • This is a well-structured planning artifact. The phased delivery approach (login-only first, then read-only crew, then write access, then admin oversight) is sound incremental delivery. Each phase is independently deployable as stated.
  • The open questions section is a good practice -- it surfaces decisions that need human input before implementation begins.
  • The per-user queue migration is correctly identified as the key breaking change. The current WorkQueueItem uniqueness constraint [work_date, property_id] would need to become [work_date, property_id, username] or similar.
  • Change failure risk: Low. Documentation-only, no deployment impact.

VERDICT: APPROVED

## PR #114 Review ### DOMAIN REVIEW This is a documentation-only PR adding `docs/user-stories-auth.md` (283 lines, 0 deletions). No code changes. The domain review focuses on documentation quality, accuracy against the existing codebase, and completeness as a planning artifact. **Documentation quality**: Strong. The document is well-structured with clear actor definitions, phased user stories with acceptance criteria, a permission matrix, Mermaid diagrams (ER, flow, Gantt-style delivery), and open questions. Each user story follows a consistent format with ID, narrative, and acceptance criteria. **Accuracy against existing codebase**: Verified. - The doc states "The current `QueueItem` model has no user association -- it's a single shared queue." This is confirmed correct. The actual model is `WorkQueueItem` (see `app/models/work_queue_item.rb`), which has `belongs_to :property` with a uniqueness constraint on `[work_date, property_id]` and no user/username column. The per-user queue change flagged in the doc is a genuine breaking change. - The Property model fields in the ER diagram (client_name, address_line, special_notes, active) match the actual `db/schema.rb` Property table. - The document correctly identifies PropertyService and Upload as models that exist in the codebase and would need PaperTrail tracking. **Model naming inconsistency (nit)**: The doc's ER diagram uses `QUEUE_ITEM` with a `username` field, but the existing model is called `WorkQueueItem`. The doc should either use the actual model name or note the rename. This matters because implementers will look at this doc and need to know whether a rename is intended. **Mermaid diagrams**: All four diagrams (actor graph, interaction diagram, ER diagram, delivery order) are syntactically valid and semantically meaningful. The ER diagram correctly shows the new `KEYCLOAK_USER` entity and its relationship to queue items. ### BLOCKERS None. This is a documentation-only PR with no code changes. The BLOCKER criteria (test coverage, input validation, secrets, DRY auth violations) do not apply. ### NITS 1. **Model name mismatch**: The ER diagram uses `QUEUE_ITEM` but the existing model is `WorkQueueItem`. Consider using the actual name or adding a note like "renamed from WorkQueueItem" if a rename is planned. 2. **PR body references wrong parent**: The PR body says "Related Notes: ldraney/landscaping-assistant #107 -- parent auth issue" but also "Closes #113". The task prompt identifies #113 as the parent issue, and #107 is the broader auth epic. The Related section should be clearer about which is the parent issue vs. the epic. This is minor since both references are valid and useful. 3. **Phase reference tags**: The delivery diagram uses `#107a`, `#107b`, `#107c`, `#107d` as phase identifiers, but these don't correspond to actual Forgejo issues. Consider noting these are future ticket placeholders, or remove the tags until tickets are created. 4. **ROADMAP.md consistency**: The existing `docs/ROADMAP.md` line 183 lists "Keycloak auth (multi-user, crew member accounts)" under "Future (Not Scoped)". Once this user stories doc lands, consider updating ROADMAP.md to reference the new doc or mark auth as "scoped" with a link. ### SOP COMPLIANCE - [x] Branch named after issue -- Branch is `docs/user-stories-auth`. This uses a `docs/` prefix convention rather than `{issue-number}-{kebab-case-purpose}` (would be `113-user-stories-auth`). **Minor deviation** from SOP but the intent is clear and the `docs/` prefix is reasonable for a documentation-only branch. - [x] PR body follows template -- Has Summary, Changes, Test Plan, Review Checklist, Related Notes sections. - [ ] Related references plan slug -- No plan slug provided (confirmed: "No plan slug" per task prompt). Not a blocker for a documentation PR. - [x] No secrets committed -- Confirmed. Single markdown file, no credentials. - [x] No unnecessary file changes -- Single file, tightly scoped to the issue. - [x] Commit messages are descriptive -- Recent commits: "Add auth user stories: roles, phases, and data model impact" and "Fix review findings: tests, N+1, ARIA, error handling, dead CSS" (the latter from a prior PR on the branch's base). ### PROCESS OBSERVATIONS - This is a well-structured planning artifact. The phased delivery approach (login-only first, then read-only crew, then write access, then admin oversight) is sound incremental delivery. Each phase is independently deployable as stated. - The open questions section is a good practice -- it surfaces decisions that need human input before implementation begins. - The per-user queue migration is correctly identified as the key breaking change. The current `WorkQueueItem` uniqueness constraint `[work_date, property_id]` would need to become `[work_date, property_id, username]` or similar. - Change failure risk: Low. Documentation-only, no deployment impact. ### VERDICT: APPROVED
Fix review nits: correct model name, remove placeholder tags
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
46115f4e63 
- QUEUE_ITEM → WORK_QUEUE_ITEM to match actual WorkQueueItem model
- Remove #107a-d placeholder tags from delivery order diagram

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-06 03:15:50 +00:00
Author
Owner
Copy link

Fixed review nits in 46115f4:

  • QUEUE_ITEM → WORK_QUEUE_ITEM to match actual WorkQueueItem model name
  • Removed #107a-d placeholder tags from delivery order diagram (not real issue numbers)

Remaining nits (not fixing):

  • Branch naming convention (docs/ prefix vs 113- prefix) — cosmetic, not worth rebasing
  • ROADMAP.md staleness — tracked separately, will update after this doc is approved and auth tickets are scoped
Fixed review nits in 46115f4: - `QUEUE_ITEM` → `WORK_QUEUE_ITEM` to match actual `WorkQueueItem` model name - Removed `#107a-d` placeholder tags from delivery order diagram (not real issue numbers) Remaining nits (not fixing): - Branch naming convention (`docs/` prefix vs `113-` prefix) — cosmetic, not worth rebasing - ROADMAP.md staleness — tracked separately, will update after this doc is approved and auth tickets are scoped
ldraney closed this pull request 2026-06-06 17:11:00 +00:00
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

Pull request closed

Please reopen this pull request to perform a merge.
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!114
No description provided.