Fix accuracy gaps in feature flags doc before #130 implementation

ldraney commented

2026-06-07 02:05:18 +00:00

Owner

Summary

Fix "four roles" → "five roles" (Super Admin added in PR #136)
Fix lucas-admin → lucas-super-admin test account name
Add table_exists? guard to test before(:each) hook

Changes

docs/feature-flags.md: three accuracy fixes to align with current auth state before #130 implementation

Test Plan

Specs pass (doc-only, no code impact)
Cross-check against issue #130 AC — all three discrepancies resolved

Review Checklist

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

ldraney/landscaping-assistant #130 — the implementation ticket this doc supports
project-landscaping-assistant — landscaping assistant project
Doc accuracy fix only — does NOT close #130

## Summary - Fix "four roles" → "five roles" (Super Admin added in PR #136) - Fix `lucas-admin` → `lucas-super-admin` test account name - Add `table_exists?` guard to test `before(:each)` hook ## Changes - `docs/feature-flags.md`: three accuracy fixes to align with current auth state before #130 implementation ## Test Plan - [ ] Specs pass (doc-only, no code impact) - [ ] Cross-check against issue #130 AC — all three discrepancies resolved ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `ldraney/landscaping-assistant #130` — the implementation ticket this doc supports - `project-landscaping-assistant` — landscaping assistant project - Doc accuracy fix only — does NOT close #130

ldraney added 1 commit

2026-06-07 02:05:18 +00:00

Fix accuracy gaps in feature flags doc before #130 implementation

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

0f081a8aae

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

ldraney commented

2026-06-07 02:07:26 +00:00

Author

Owner

PR #142 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails documentation (Markdown). Verified all three fixes against source-of-truth files.

Fix 1 -- "four roles" to "five roles" (line 27)
Correct. docs/keycloak-setup.md line 31 lists the landscaping realm with roles = ["admin", "lead", "member", "client", "super_admin"] (five). The Terraform tfvars at line 77 confirms the same. ApplicationController#TAB_ROLES references all five including super_admin.

Fix 2 -- lucas-admin to lucas-super-admin (line 324)
Correct. docs/keycloak-setup.md line 142 lists lucas-super-admin as the test account with admin + super_admin roles. lucas-admin is a separate test account with only the admin role (line 143). The original text conflated the two accounts.

Fix 3 -- table_exists? guard in before(:each) (lines 384-389)
Correct Rails idiom. ActiveRecord::Base.connection.table_exists?(:feature_flags) is the standard ActiveRecord introspection method for checking table existence. The next unless pattern cleanly skips the hook body for specs that run before the feature_flags migration has been applied (e.g., model specs for unrelated models in CI where migration order matters). The removed comment (# Clear cache so tests see the enabled state) was redundant since Rails.cache.clear is self-documenting.

Line 324 -- "alongside the existing four roles"
This phrasing is correct as written. The sentence describes super_admin being created alongside the other four roles (client, member, lead, admin). "Four" here refers to the pre-existing roles, not the total count. No fix needed.

Broader staleness scan of docs/feature-flags.md:

current_user_has_role?("admin") usage in code examples (lines 116, 218, 273) -- matches the actual method signature in ApplicationController line 56-59. Correct.
require_super_admin controller method (line 272-274) uses current_user_has_role?("super_admin") -- matches the require_role DSL pattern from PR #136. The doc shows a manual before_action rather than the require_role :super_admin class-level DSL. Both work, but this is a deliberate choice (returns 404 vs redirect) so it is correct as documented.
helper_method :feature_enabled? (line 98) -- matches the existing helper_method pattern in ApplicationController line 8-9.
The current_user session hash model is correctly reflected. The doc does not treat current_user as an ActiveRecord model anywhere.
Five test users table in keycloak-setup.md lines 140-147 is consistent with the doc's references.

Out-of-scope observation: README.md lines 14-21 list only four roles (Client, Crew Member, Crew Lead, Admin) and do not mention Super Admin. This is a pre-existing gap, not introduced by this PR. Consider a follow-up to add Super Admin to the README roles table.

BLOCKERS

None. All three fixes are factually correct and align with source-of-truth files.

NITS

The diff removes the comment # Clear cache so tests see the enabled state from the test hook. While the comment was arguably redundant, having Rails.cache.clear without any comment in a before(:each) block may confuse future readers about why the cache is cleared there (it is specifically to ensure tests see the freshly-enabled flag state, not just a generic cleanup). Optional: keep a shorter comment like # Flags just enabled above; bust stale cache. Very minor -- not blocking.

SOP COMPLIANCE

Branch named after issue (docs/feature-flags-accuracy-fixes -- doc fix branch, no parent issue number but reasonable for a standalone doc fix)
PR body follows template (Summary, Changes, Test Plan, Related sections all present)
Related section references issue #130 and project slug
No secrets committed
No unnecessary file changes (single file, three targeted fixes)
Commit messages are descriptive

PROCESS OBSERVATIONS

Low-risk doc-only change. No code impact, no deployment risk.
Good practice: fixing doc accuracy before implementation (#130) prevents the implementation from being built on stale assumptions.
The README roles table gap (missing Super Admin) should be tracked separately.

VERDICT: APPROVED

## PR #142 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails documentation (Markdown). Verified all three fixes against source-of-truth files. **Fix 1 -- "four roles" to "five roles" (line 27)** Correct. `docs/keycloak-setup.md` line 31 lists the `landscaping` realm with `roles = ["admin", "lead", "member", "client", "super_admin"]` (five). The Terraform tfvars at line 77 confirms the same. `ApplicationController#TAB_ROLES` references all five including `super_admin`. **Fix 2 -- `lucas-admin` to `lucas-super-admin` (line 324)** Correct. `docs/keycloak-setup.md` line 142 lists `lucas-super-admin` as the test account with `admin` + `super_admin` roles. `lucas-admin` is a separate test account with only the `admin` role (line 143). The original text conflated the two accounts. **Fix 3 -- `table_exists?` guard in `before(:each)` (lines 384-389)** Correct Rails idiom. `ActiveRecord::Base.connection.table_exists?(:feature_flags)` is the standard ActiveRecord introspection method for checking table existence. The `next unless` pattern cleanly skips the hook body for specs that run before the feature_flags migration has been applied (e.g., model specs for unrelated models in CI where migration order matters). The removed comment (`# Clear cache so tests see the enabled state`) was redundant since `Rails.cache.clear` is self-documenting. **Line 324 -- "alongside the existing four roles"** This phrasing is correct as written. The sentence describes super_admin being created alongside the other four roles (client, member, lead, admin). "Four" here refers to the pre-existing roles, not the total count. No fix needed. **Broader staleness scan of `docs/feature-flags.md`:** - `current_user_has_role?("admin")` usage in code examples (lines 116, 218, 273) -- matches the actual method signature in `ApplicationController` line 56-59. Correct. - `require_super_admin` controller method (line 272-274) uses `current_user_has_role?("super_admin")` -- matches the `require_role` DSL pattern from PR #136. The doc shows a manual `before_action` rather than the `require_role :super_admin` class-level DSL. Both work, but this is a deliberate choice (returns 404 vs redirect) so it is correct as documented. - `helper_method :feature_enabled?` (line 98) -- matches the existing `helper_method` pattern in `ApplicationController` line 8-9. - The `current_user` session hash model is correctly reflected. The doc does not treat `current_user` as an ActiveRecord model anywhere. - Five test users table in `keycloak-setup.md` lines 140-147 is consistent with the doc's references. **Out-of-scope observation**: `README.md` lines 14-21 list only four roles (Client, Crew Member, Crew Lead, Admin) and do not mention Super Admin. This is a pre-existing gap, not introduced by this PR. Consider a follow-up to add Super Admin to the README roles table. ### BLOCKERS None. All three fixes are factually correct and align with source-of-truth files. ### NITS 1. The diff removes the comment `# Clear cache so tests see the enabled state` from the test hook. While the comment was arguably redundant, having `Rails.cache.clear` without any comment in a `before(:each)` block may confuse future readers about *why* the cache is cleared there (it is specifically to ensure tests see the freshly-enabled flag state, not just a generic cleanup). Optional: keep a shorter comment like `# Flags just enabled above; bust stale cache`. Very minor -- not blocking. ### SOP COMPLIANCE - [x] Branch named after issue (`docs/feature-flags-accuracy-fixes` -- doc fix branch, no parent issue number but reasonable for a standalone doc fix) - [x] PR body follows template (Summary, Changes, Test Plan, Related sections all present) - [x] Related section references issue #130 and project slug - [x] No secrets committed - [x] No unnecessary file changes (single file, three targeted fixes) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - Low-risk doc-only change. No code impact, no deployment risk. - Good practice: fixing doc accuracy before implementation (#130) prevents the implementation from being built on stale assumptions. - The README roles table gap (missing Super Admin) should be tracked separately. ### VERDICT: APPROVED