fix: config path uses dynamic practices from practice_schedules table

forgejo_admin commented

2026-04-12 21:47:34 +00:00

Contributor

Summary

Config-driven contracts used data.config.practices (from teams.contract_config JSONB) instead of the dynamic practice_schedules table query. Now both paths use the same source of truth.

Changes

Replace data.config.practices in Practice Schedule rendering with {#each practices} dynamic loop
Replace data.config.practices in fee description practice count with practices array
Add fallback message when no practices exist in DB

Test Plan

Load config-path contract (e.g. 17U Select Kings player) — schedule comes from practice_schedules table
Load fallback-path contract — schedule comes from practice_schedules table
Team with no practices in DB — shows fallback message
Fee description shows correct practice count from DB

Review Checklist

Build passes (npm run build)
Zero references to config.practices remain in template
No database changes — code only

Closes #52
story:WS-S28 — parent sees correct practice schedule
Follow-up to PR #50, PR #51

## Summary Config-driven contracts used `data.config.practices` (from `teams.contract_config` JSONB) instead of the dynamic `practice_schedules` table query. Now both paths use the same source of truth. ## Changes - Replace `data.config.practices` in Practice Schedule rendering with `{#each practices}` dynamic loop - Replace `data.config.practices` in fee description practice count with `practices` array - Add fallback message when no practices exist in DB ## Test Plan - [ ] Load config-path contract (e.g. 17U Select Kings player) — schedule comes from `practice_schedules` table - [ ] Load fallback-path contract — schedule comes from `practice_schedules` table - [ ] Team with no practices in DB — shows fallback message - [ ] Fee description shows correct practice count from DB ## Review Checklist - [x] Build passes (`npm run build`) - [x] Zero references to `config.practices` remain in template - [x] No database changes — code only ## Related Notes - Closes #52 - `story:WS-S28` — parent sees correct practice schedule - Follow-up to PR #50, PR #51

forgejo_admin added 1 commit

2026-04-12 21:47:34 +00:00

fix: config path uses dynamic practices from practice_schedules table de6b3c771d

The config-driven rendering path was using data.config.practices
(from teams.contract_config JSONB) instead of the dynamic practices
query from the practice_schedules table. This meant config-path
contracts ignored the DB and showed whatever was baked into the
JSON config.

Now both the config path and fallback path use the same dynamic
practices array from practice_schedules. The fee description
practice count also derives from the dynamic data.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-04-12 21:49:31 +00:00

Author

Contributor

PR #53 Review

DOMAIN REVIEW

Stack: SvelteKit / TypeScript / CSS (westside-contracts)

This PR replaces data.config.practices (static JSONB config) with the dynamic practices array (from practice_schedules table) in the data-driven contract rendering path. One file changed, 11 additions, 2 deletions. Scope is tight and matches issue #52.

Semantic shift -- p.optional to p.notes: The old code used an explicit p.optional boolean to distinguish required vs. optional practices. The new code uses p.notes as a proxy: practices with notes are treated as optional, practices without notes are treated as required. This drives two behaviors:

Fee description count: practices.filter(p => !p.notes).length counts "required" practices
Fee description suffix: practices.some(p => p.notes) triggers "plus optional sessions"

If any practice has notes that are NOT about optionality (e.g., "Bring shin guards"), it will be miscounted as optional. Verify that the practice_schedules.notes column is used exclusively for optional indicators in the current data. If it is general-purpose, this needs a dedicated is_optional column or explicit filter.

Inline rendering replaces PracticeSchedule component: The <PracticeSchedule> component is replaced with an inline {#each} block. The generated HTML uses the same CSS classes (schedule-row, schedule-day, schedule-detail, schedule-optional) that exist in app.css and match the hardcoded fallback paths. Structurally correct.

Fallback: Good -- empty practices array shows "Practice schedule will be communicated by your coach." rather than an empty section.

Accessibility: The schedule rows use <div> + <span> without semantic table markup or ARIA roles. This matches the existing pattern in the fallback paths, so not a regression. Non-blocking.

BLOCKERS

None. The notes-as-optional-proxy is a correctness concern but not a security or data integrity blocker -- it depends on how the column is actually used in the database. If notes is only ever "optional" or null, the logic is correct.

NITS

Repeated filter expression: practices.filter(p => !p.notes).length appears twice in the FeeSection practiceDescription string (once for the count, once for the plural suffix). Consider extracting to a const requiredCount variable for clarity and DRY.
PracticeSchedule component orphan: If the <PracticeSchedule> component is no longer used anywhere after this change, it should be removed in a follow-up to avoid dead code.
Hardcoded fallback paths still exist: Lines 323-333, 490-500, 589-596 still have hardcoded schedule rows in the non-config rendering paths. The PR body says "Zero references to config.practices remain" which is true, but the broader goal of dynamic schedules everywhere is incomplete. Not in scope for this PR (tracked by #49/#50/#51), but worth noting.

SOP COMPLIANCE

Branch named after issue: 52-config-path-use-dynamic-practices
PR body follows template (Summary, Changes, Test Plan, Related)
Related references story (story:WS-S28) and prior PRs
Related references plan slug -- no plan slug provided (user indicated none exists)
No secrets committed
No scope creep -- single file, tightly scoped to issue #52
Commit message is descriptive

PROCESS OBSERVATIONS

This is the third PR in a series (#50, #51, #53) migrating from hardcoded/config practices to DB-driven practices. The incremental approach keeps each PR small and reviewable. The test plan is manual (load contract, verify schedule source) which is appropriate for template rendering changes. The repo has 12 tests covering validation logic but no component/rendering tests -- this is a known gap, not introduced by this PR.

VERDICT: APPROVED

## PR #53 Review ### DOMAIN REVIEW **Stack**: SvelteKit / TypeScript / CSS (westside-contracts) This PR replaces `data.config.practices` (static JSONB config) with the dynamic `practices` array (from `practice_schedules` table) in the data-driven contract rendering path. One file changed, 11 additions, 2 deletions. Scope is tight and matches issue #52. **Semantic shift -- `p.optional` to `p.notes`**: The old code used an explicit `p.optional` boolean to distinguish required vs. optional practices. The new code uses `p.notes` as a proxy: practices with notes are treated as optional, practices without notes are treated as required. This drives two behaviors: 1. Fee description count: `practices.filter(p => !p.notes).length` counts "required" practices 2. Fee description suffix: `practices.some(p => p.notes)` triggers "plus optional sessions" If any practice has notes that are NOT about optionality (e.g., "Bring shin guards"), it will be miscounted as optional. Verify that the `practice_schedules.notes` column is used exclusively for optional indicators in the current data. If it is general-purpose, this needs a dedicated `is_optional` column or explicit filter. **Inline rendering replaces PracticeSchedule component**: The `<PracticeSchedule>` component is replaced with an inline `{#each}` block. The generated HTML uses the same CSS classes (`schedule-row`, `schedule-day`, `schedule-detail`, `schedule-optional`) that exist in `app.css` and match the hardcoded fallback paths. Structurally correct. **Fallback**: Good -- empty practices array shows "Practice schedule will be communicated by your coach." rather than an empty section. **Accessibility**: The schedule rows use `<div>` + `<span>` without semantic table markup or ARIA roles. This matches the existing pattern in the fallback paths, so not a regression. Non-blocking. ### BLOCKERS None. The `notes`-as-optional-proxy is a correctness concern but not a security or data integrity blocker -- it depends on how the column is actually used in the database. If `notes` is only ever "optional" or null, the logic is correct. ### NITS 1. **Repeated filter expression**: `practices.filter(p => !p.notes).length` appears twice in the `FeeSection` practiceDescription string (once for the count, once for the plural suffix). Consider extracting to a `const requiredCount` variable for clarity and DRY. 2. **PracticeSchedule component orphan**: If the `<PracticeSchedule>` component is no longer used anywhere after this change, it should be removed in a follow-up to avoid dead code. 3. **Hardcoded fallback paths still exist**: Lines 323-333, 490-500, 589-596 still have hardcoded schedule rows in the non-config rendering paths. The PR body says "Zero references to `config.practices` remain" which is true, but the broader goal of dynamic schedules everywhere is incomplete. Not in scope for this PR (tracked by #49/#50/#51), but worth noting. ### SOP COMPLIANCE - [x] Branch named after issue: `52-config-path-use-dynamic-practices` - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related references story (`story:WS-S28`) and prior PRs - [ ] Related references plan slug -- no plan slug provided (user indicated none exists) - [x] No secrets committed - [x] No scope creep -- single file, tightly scoped to issue #52 - [x] Commit message is descriptive ### PROCESS OBSERVATIONS This is the third PR in a series (#50, #51, #53) migrating from hardcoded/config practices to DB-driven practices. The incremental approach keeps each PR small and reviewable. The test plan is manual (load contract, verify schedule source) which is appropriate for template rendering changes. The repo has 12 tests covering validation logic but no component/rendering tests -- this is a known gap, not introduced by this PR. ### VERDICT: APPROVED