fix: remove remaining hardcoded schedule blocks in fallback path

forgejo_admin commented

2026-04-12 20:16:52 +00:00

Contributor

Summary

PR #50's rebase left hardcoded BWill schedule blocks in all three fallback branches. This removes them and wires up the dynamic {#each practices} query in their place.

Changes

Replace boys travel hardcoded schedule (Kongo/West High/LCA) with {#each practices} loop
Replace girls travel hardcoded schedule (West High/LCA/Granger) with {#each practices} loop
Replace local hardcoded schedule (BWill Mon/Fri) with {#each practices} loop
Remove duplicate dynamic block from shared section (was rendering twice)

Test Plan

Load a 16U Local Queens contract (Nevaeh) — should show fallback message
Load a boys travel contract — should show practices from DB
Load a girls travel contract — should show practices from DB
Verify no "BWill" appears in any contract

Review Checklist

Build passes (npm run build)
No hardcoded schedule literals remain in fallback path
Dynamic block uses same CSS classes
Fallback message renders when no practices in DB

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

## Summary PR #50's rebase left hardcoded BWill schedule blocks in all three fallback branches. This removes them and wires up the dynamic `{#each practices}` query in their place. ## Changes - Replace boys travel hardcoded schedule (Kongo/West High/LCA) with `{#each practices}` loop - Replace girls travel hardcoded schedule (West High/LCA/Granger) with `{#each practices}` loop - Replace local hardcoded schedule (BWill Mon/Fri) with `{#each practices}` loop - Remove duplicate dynamic block from shared section (was rendering twice) ## Test Plan - [ ] Load a 16U Local Queens contract (Nevaeh) — should show fallback message - [ ] Load a boys travel contract — should show practices from DB - [ ] Load a girls travel contract — should show practices from DB - [ ] Verify no "BWill" appears in any contract ## Review Checklist - [x] Build passes (`npm run build`) - [x] No hardcoded schedule literals remain in fallback path - [x] Dynamic block uses same CSS classes - [x] Fallback message renders when no practices in DB ## Related Notes - Closes #49 - Follow-up to PR #50 - `story:WS-S28` — parent sees correct practice schedule

forgejo_admin added 1 commit

2026-04-12 20:16:52 +00:00

fix: remove remaining hardcoded schedule blocks in fallback path 9777a8ae0d

The rebase of PR #50 left hardcoded BWill schedule blocks in the
boys-travel, girls-travel, and local fallback branches. These
rendered before the dynamic {#each practices} block, showing stale
data on all non-config contracts. Replace all three with the dynamic
query and remove the duplicate shared-section block.

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

forgejo_admin commented

2026-04-12 20:19:23 +00:00

Author

Contributor

PR #51 Review

DOMAIN REVIEW

Stack: SvelteKit / TypeScript / server-rendered contract pages with direct Postgres queries.

What this PR does: PR #50 added a dynamic practices array from the DB and placed a shared {#each practices} block at the bottom of the template, but left three variant-specific hardcoded schedule blocks intact (boys travel at ~L342, girls travel at ~L422, local at ~L466). This PR replaces all three with the same dynamic {#each practices} loop and removes the now-duplicate shared block from the bottom. Net effect: -37 lines of hardcoded schedule data, +29 lines of dynamic rendering. Clean improvement.

Correctness: The replacement blocks correctly use practice.day_name, practice.location, practice.time_range, and conditionally render practice.notes -- matching the exact pattern established by PR #50's server-side data shaping. The {:else} fallback message is consistent across all three paths. The local variant nicely moves the "Season starts week of April 6" intro inside the {#if practices.length > 0} guard so it only shows when there are actual practices to display.

Duplicate shared block removal (L518-L529 deletion): Correct. PR #50 added this shared block outside the variant conditionals, which would have caused practice schedules to render twice (once in the variant block, once in the shared block). Removing it is the right call.

DRY observation: The same 7-line template fragment is now copy-pasted identically in three places (boys travel, girls travel, local). This is a nit, not a blocker -- the surrounding context (section headers, introductory text) differs enough per variant that extracting a Svelte snippet component would be a separate refactor. The PR description acknowledges this is a follow-up cleanup to PR #50, and keeping scope tight is correct.

No PracticeSchedule component references remain: The diff removes PracticeSchedule rows={[...]} calls. Grepping the codebase confirms no PracticeSchedule component exists (it was apparently inline prop syntax, not a separate component). No dead imports.

BLOCKERS

None.

This PR is a template-only change in a .svelte file. It replaces hardcoded HTML with dynamic rendering using data already fetched and shaped by PR #50's server-side code. No new functionality is introduced -- the practices data flow, DB query, and type definitions were all added in PR #50. The "new test coverage" blocker criterion does not apply to removing hardcoded content in favor of already-wired dynamic data.

NITS

DRY -- identical template block x3: The {#if practices.length > 0} / {#each} / {:else} block is repeated verbatim in boys-travel, girls-travel, and local paths. Consider extracting a PracticeScheduleBlock snippet or Svelte component in a future ticket. Not blocking because each variant's surrounding context differs.
Indentation consistency: The {#each} blocks use tabs consistently, which matches the rest of the file. No issues here.

SOP COMPLIANCE

Branch named after issue: 49-remove-hardcoded-schedule-fallback references issue #49
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related all present
Related references story: story:WS-S28 referenced; no plan slug (standalone fix, appropriate)
No secrets committed
No scope creep -- changes are strictly scoped to removing hardcoded schedules in fallback paths
Commit message is descriptive: fix: remove remaining hardcoded schedule blocks in fallback path

PROCESS OBSERVATIONS

This is a clean follow-up to PR #50 which missed the variant-specific blocks during its rebase. Good catch and quick turnaround.
The test plan is manual (load contracts, verify visually). This is acceptable for template-only changes in a contract rendering app with 12 existing validation tests. The dynamic data pipeline was tested in PR #50's scope.
Deployment risk is low: the data source (practices array) is already live from PR #50. This PR only changes which template paths consume it.

VERDICT: APPROVED

## PR #51 Review ### DOMAIN REVIEW **Stack**: SvelteKit / TypeScript / server-rendered contract pages with direct Postgres queries. **What this PR does**: PR #50 added a dynamic `practices` array from the DB and placed a shared `{#each practices}` block at the bottom of the template, but left three variant-specific hardcoded schedule blocks intact (boys travel at ~L342, girls travel at ~L422, local at ~L466). This PR replaces all three with the same dynamic `{#each practices}` loop and removes the now-duplicate shared block from the bottom. Net effect: -37 lines of hardcoded schedule data, +29 lines of dynamic rendering. Clean improvement. **Correctness**: The replacement blocks correctly use `practice.day_name`, `practice.location`, `practice.time_range`, and conditionally render `practice.notes` -- matching the exact pattern established by PR #50's server-side data shaping. The `{:else}` fallback message is consistent across all three paths. The local variant nicely moves the "Season starts week of April 6" intro inside the `{#if practices.length > 0}` guard so it only shows when there are actual practices to display. **Duplicate shared block removal** (L518-L529 deletion): Correct. PR #50 added this shared block outside the variant conditionals, which would have caused practice schedules to render twice (once in the variant block, once in the shared block). Removing it is the right call. **DRY observation**: The same 7-line template fragment is now copy-pasted identically in three places (boys travel, girls travel, local). This is a nit, not a blocker -- the surrounding context (section headers, introductory text) differs enough per variant that extracting a Svelte snippet component would be a separate refactor. The PR description acknowledges this is a follow-up cleanup to PR #50, and keeping scope tight is correct. **No `PracticeSchedule` component references remain**: The diff removes `PracticeSchedule rows={[...]}` calls. Grepping the codebase confirms no `PracticeSchedule` component exists (it was apparently inline prop syntax, not a separate component). No dead imports. ### BLOCKERS None. This PR is a template-only change in a `.svelte` file. It replaces hardcoded HTML with dynamic rendering using data already fetched and shaped by PR #50's server-side code. No new functionality is introduced -- the `practices` data flow, DB query, and type definitions were all added in PR #50. The "new test coverage" blocker criterion does not apply to removing hardcoded content in favor of already-wired dynamic data. ### NITS 1. **DRY -- identical template block x3**: The `{#if practices.length > 0} / {#each} / {:else}` block is repeated verbatim in boys-travel, girls-travel, and local paths. Consider extracting a `PracticeScheduleBlock` snippet or Svelte component in a future ticket. Not blocking because each variant's surrounding context differs. 2. **Indentation consistency**: The `{#each}` blocks use tabs consistently, which matches the rest of the file. No issues here. ### SOP COMPLIANCE - [x] Branch named after issue: `49-remove-hardcoded-schedule-fallback` references issue #49 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related all present - [x] Related references story: `story:WS-S28` referenced; no plan slug (standalone fix, appropriate) - [x] No secrets committed - [x] No scope creep -- changes are strictly scoped to removing hardcoded schedules in fallback paths - [x] Commit message is descriptive: `fix: remove remaining hardcoded schedule blocks in fallback path` ### PROCESS OBSERVATIONS - This is a clean follow-up to PR #50 which missed the variant-specific blocks during its rebase. Good catch and quick turnaround. - The test plan is manual (load contracts, verify visually). This is acceptable for template-only changes in a contract rendering app with 12 existing validation tests. The dynamic data pipeline was tested in PR #50's scope. - Deployment risk is low: the data source (`practices` array) is already live from PR #50. This PR only changes which template paths consume it. ### VERDICT: APPROVED