Add crew member list and profile views for admin oversight #146
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "117-crew-tab"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Changes
db/migrate/20260607030548_create_crew_members.rb: create_crew_members table (name, keycloak_username, role, active boolean) with unique index on keycloak_usernameapp/models/crew_member.rb: CrewMember model with ROLES constant, validations, active/by_name scopesapp/controllers/crew_controller.rb: replaced placeholder with real index (active members) and show (member profile) actionsapp/views/crew/index.html.erb: member list with name, role badge, active status, linked to showapp/views/crew/show.html.erb: member profile with name, role, status, keycloak username, created dateconfig/routes.rb: added :show to crew resourcesapp/assets/stylesheets/application.css: crew list, role badges (color-coded per role), status indicators, profile layoutlib/tasks/crew.rake: idempotent crew:sync task seeding 5 test users (follows feature_flags.rake pattern)spec/requests/crew_spec.rb: index + show specs with role-based access coveragespec/requests/role_access_spec.rb: added crew show route tests for all rolesspec/models/crew_member_spec.rb: validations, scopes, defaultsTest Plan
bundle exec rspecvia docker-compose)/crewshows active members with role badges (admin/super_admin only)/crew/:idprofile pagerake crew:syncseeds test data idempotentlyReview Checklist
Related Notes
ldraney/landscaping-assistant #117-- Crew tab admin oversightQA Review -- PR #146
Checklist
create_crew_memberswith null constraints, unique index onkeycloak_username, defaultactive: trueactiveandby_namescopesrequire_role :admin, :super_admininherited, index queriesCrewMember.active.by_name, show usesfind<dl>profile layout, role badge, status, keycloak username, created date:showadded to crew resources!important, no#idselectors -- follows~/ror-css-guidefind_or_create_by!matchingfeature_flags.rakepattern, 5 users covering all rolesCloses #117present in PR bodyObservations (non-blocking)
RecordNotFoundrescue inshow-- Rails returns a 404 by default for missing records in production, which is acceptable for admin-only pages. No action needed.role-badge-super_adminCSS class uses an underscore in the class name (frommember.role). This works but is unconventional for CSS. Non-blocking since it's generated from the model constant and consistent throughout.VERDICT: PASS
Clean implementation matching all acceptance criteria from #117. No blocking issues found.
c7c010b5ba7f3e66d8a4PR #146 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.0, RSpec request specs, vanilla CSS with design tokens, Rake tasks.
Migration (
db/migrate/20260607030548_create_crew_members.rb):name,keycloak_username,role,activewithnull: falseconstraints anddefault: trueonactive.keycloak_username-- correct, matches model uniqueness validation.db/schema.rbin the diff does NOT include the newcrew_memberstable. This is expected if the PR branch was not rebased after running the migration, but it means the schema.rb update is missing from the diff. The migration file itself is correct, but schema.rb must be updated before merge (runrails db:migrateand commit the result). This is a blocker -- Rails uses schema.rb fordb:schema:loadin CI and fresh setups.Model (
app/models/crew_member.rb):client, member, lead, admin, super_admin. Correct.keycloak_username. Correct and backed by DB index.activeandby_namescopes are clean and appropriate.has_manyoraccepts_nested_attributes_for, and the controller never callscreate/updatefrom params.Controller (
app/controllers/crew_controller.rb):require_role :admin, :super_adminat class level -- gates both index and show. Correct.index: usesCrewMember.active.by_name-- correct, only shows active members.show: usesCrewMember.find(params[:id])-- this raisesActiveRecord::RecordNotFoundfor missing IDs, which Rails maps to 404 by default. Acceptable. Note: this allows admins to view inactive members via direct URL (by ID), which is tested and intentional per the specit "can view inactive members".Views:
index.html.erb: Iterates@crew_members, shows name, role badge, and active status. Empty state handled with@crew_members.any?guard. Useslink_to crew_path(member)for navigation. All values are output via<%= %>(ERB auto-escapes by default). No XSS risk.show.html.erb: Displays member profile as a<dl>grid. Back link usescrew_index_path. Shows role, status, keycloak username, and created date. Clean. Usesstrftimefor date formatting -- acceptable.class="role-badge role-badge-<%= member.role %>"pattern with design-token-based CSS. No inline styles.Routes (
config/routes.rb):only: [:index]toonly: [:index, :show]. Correct and minimal.CSS (
app/assets/stylesheets/application.css):.crew-placeholder(no longer used). Good cleanup..crew-item(removes old padding/color/font-size, adds border-bottom only). Clean..crew-item-link,.crew-item-info,.crew-item-meta,.crew-item-status,.crew-profile,.role-badge,.role-badge-{role},.status-indicator,.status-indicator-{active|inactive}. All use design tokens. No magic numbers. No!important. No#idselectors. No Tailwind.CSS conflict with PR #148 (IMPORTANT):
PR #148 removed a duplicate
.role-badgefrom the Person CSS section. On current main,.role-badgeis referenced inperson/index.html.erb(line 19) but has NO CSS definition -- it is unstyled. This PR introduces a.role-badgedefinition in the Crew CSS section. After rebasing onto current main, this will be the ONLY.role-badgedefinition, and it will correctly style both the Person view and the Crew views. This is actually a fix for the post-PR-#148 regression. No conflict -- this is additive and welcome.Rake task (
lib/tasks/crew.rake):find_or_create_by!(keycloak_username:)with a block -- idempotent for creates. Correct pattern, matchesfeature_flags.rake.lucas-super-admin,lucas-admin,lucas-lead,lucas-crew,lucas-client. Matches the requirement.find_or_create_by!will NOT updatenameorroleon subsequent runs if the record already exists. This is consistent withfeature_flags.rakebehavior but worth documenting.Specs:
spec/models/crew_member_spec.rb: Covers all validations (presence of name, keycloak_username, role; uniqueness of keycloak_username; role inclusion with invalid value AND all valid values), both scopes (active, by_name), and defaults (active: true). Thorough.spec/requests/crew_spec.rb: Covers index and show for dev mode (no Keycloak) and Keycloak mode. Tests admin access, super_admin access, member denial, lead denial, client denial, active/inactive filtering, and member profile display. Comprehensive.spec/requests/role_access_spec.rb: Adds/crew/:idtests for all role categories (unauthenticated, dev mode, member, lead, admin, super_admin, client). Matches the existing pattern for/crewindex. Thorough.BLOCKERS
db/schema.rbupdate. The migration file is present but the diff does not include a schema.rb change. The current schema.rb on main (version2026_06_07_011440) does not containcrew_members. After running the migration, the schema.rb must be committed with the new table definition. Without this,rails db:schema:load(used in CI and fresh deploys) will not create thecrew_memberstable. Runrails db:migrateand commit the resultingdb/schema.rb.NITS
border-radius: 1remon badges -- The.role-badgeand.status-indicatorboth useborder-radius: 1remas a literal value rather than a design token. The existing--radius: 8pxtoken is for standard corners; a pill radius is a different concept. Consider adding a--radius-pill: 1remtoken for consistency, or accept the literal value as an intentional one-off for pill shapes. Low priority.border-radius: 4pxon.crew-profile code-- Same concern: literal4pxinstead of a token. The rest of the codebase usesvar(--radius)(which is8px). Minor inconsistency.Rake task does not update existing records -- If a crew member's role or name changes in Keycloak,
crew:syncwill not reflect the change becausefind_or_create_by!only sets attributes in the creation block. This is consistent withfeature_flags.rakebehavior but could surprise someone. Consider documenting this limitation or usingfind_or_initialize_by+update!if future-proofing is desired. Not blocking for MVP.Show page date format --
@crew_member.created_at.strftime("%B %d, %Y")produces "June 06, 2026" (with leading zero on day). Consider%-dfor "June 6, 2026" if that is preferred. Cosmetic.SOP COMPLIANCE
117-crew-tabmatches issue #117)!important, no#idselectorsPROCESS OBSERVATIONS
.role-badgeCSS story across PRs #145, #147, #148, and #146 is worth tracking: PR #148 removed the duplicate from Person, and this PR reintroduces the single canonical definition in the Crew section. After rebase, the Person view's.role-badgeusage will be styled again. This is the right outcome but was not explicitly called out in the PR body.crew:synctask will need a correspondinginitContainerentry in the deployment config (similar tofeature_flags:syncper issue #140) when this goes to production. Not blocking for this PR but worth a follow-up ticket.VERDICT: NOT APPROVED
One blocker: the
db/schema.rbupdate is missing from the diff. Runrails db:migrateon the branch, commit the updated schema.rb, and push. Everything else is clean and well-executed.PR #146 Review
DOMAIN REVIEW
Tech stack: Ruby on Rails 8.0, ERB views, vanilla CSS, RSpec request specs, PostgreSQL.
Model (
app/models/crew_member.rb)ROLESconstant, presence/uniqueness/inclusion validations, two scopes. No issues.frozen_string_literalpragma present. Good.Controller (
app/controllers/crew_controller.rb)require_role :admin, :super_adminapplied at class level -- correctly gates bothindexandshow. Consistent with the existingTAB_ROLEShash inApplicationController(line 82:crew: %w[admin super_admin]).showusesCrewMember.find(params[:id])-- this follows the same pattern asPropertiesController#show(barefind, relying on Rails default 404 forRecordNotFound). Consistent.indexscopes toactive.by_name-- only active members shown in the list. Good. Show action intentionally allows viewing inactive members by direct ID, which the test covers explicitly.Migration (
db/migrate/20260607030548_create_crew_members.rb)null: false.activedefaults totrue. Unique index onkeycloak_username. Clean.schema.rbreflects the migration correctly (verified lines 45-53).Views
index.html.erb: Empty state handled (No crew members yet). Role badge usesrole-badge role-badge-<%= member.role %>pattern with dynamic class. Status indicator uses ternary for active/inactive. Properly links tocrew_path(member).show.html.erb: Usescrew_index_pathfor back link (correct forresources :crewrouting).strftime("%B %d, %Y")for date formatting. Displays keycloak_username in a<code>tag.CSS (
application.css).role-badgeCSS which was removed in PR #148. Confirmed: Person view (person/index.html.erbline 19) uses.role-badgewithout modifier classes, and the base.role-badgestyle in the Crew section provides the default (gray background/muted text). The Person view will inherit this base style correctly. No duplication..status-indicator-active,.status-indicator-inactive) follow the same pill badge pattern as.flag-statusin the FlagsTable section. Slightly duplicative in structure but different semantics. Acceptable.--spacing-*,--color-*,--radius). No magic numbers exceptborder-radius: 1remon pills (intentional full-round) andborder-radius: 4pxon code blocks (matches existing pattern in.flags-table code).Rake task (
lib/tasks/crew.rake)feature_flags.rakepattern exactly:find_or_create_by!with keycloak_username as the unique key, block sets remaining attributes. Idempotent -- re-running does not duplicate or update existing records (by design:find_or_create_by!only runs the block for new records).Routes (
config/routes.rb):showtoresources :crew. Minimal, correct change.BLOCKERS
None.
NITS
Rake task will not update existing records.
find_or_create_by!only executes the block for new records. If you later need to change a seeded user's name or role, re-runningrake crew:syncwill not apply the update. This matches thefeature_flags.rakepattern intentionally, but worth documenting as a conscious choice. Consider whetherfind_or_initialize_by+update!would be better for a "sync" task (sync implies updating to match the source of truth, not just creating missing records). Non-blocking -- current behavior is correct for initial seeding.keycloak_usernamedisplayed on show page. This is an internal Keycloak identifier visible to admin/super_admin only, so not a security concern. But if crew members eventually get non-admin views, this should be hidden.No
RecordNotFoundrescue forshow. If an admin visits/crew/99999, they get Rails' default 404 page. This is consistent withPropertiesController#showso it is not a divergence, but a user-friendly "Member not found" page could be a future improvement.View template:
member.active?ternary is repeated. Thestatus-indicatorspan withmember.active? ? 'active' : 'inactive'andmember.active? ? "Active" : "Inactive"appears in both index and show views. A helper method could DRY this up if more views use it later. Non-blocking for two usages.SOP COMPLIANCE
117-crew-tabmaps to issue #117)PROCESS OBSERVATIONS
/crew/:idfor all role permutations. This is well above the minimum bar.rake crew:syncmust be run post-deploy to seed data (or the empty state message appears, which is a clean fallback)..role-badgeclass. PR #148 cleanup is validated -- no duplicate definitions remain.db/schema.rbshowsversion: 2026_06_07_030548matching the migration filename. The schema file appears to be committed with changes (git status showsM db/schema.rb), which is correct practice for Rails.VERDICT: APPROVED