forgejo_admin/westside-contracts
1
0
Fork 0
Code Issues 2 Pull requests Projects Releases Packages Wiki Activity Actions

feat: girls travel contract — Prep Hoops Girls circuit #32

Merged
forgejo_admin merged 1 commit from 31-feat-girls-travel-contract-prep-hoops-gi into main 2026-03-28 19:26:30 +00:00
Conversation 1 Commits 1 Files changed 4 +214 -5
forgejo_admin commented 2026-03-28 19:20:44 +00:00
Owner
Copy link

Summary

  • Add Queens-specific contract path for Prep Hoops Girls circuit (5 tournaments, different from boys Power 32)
  • Roster flexibility language: teams subject to change, late additions lower per-player costs
  • Cost fields marked with bright yellow MARCUS TODO markers for admin review before go-live

Changes

  • src/routes/contract/[token]/+page.server.ts: pass isGirls flag derived from team name containing "Queens"
  • src/routes/contract/[token]/+page.svelte: full girls travel contract section — Prep Hoops Girls circuit schedule (Utah Girls Invitational Apr 10-11, Denver May 7-10, Mesa May 28-31, Utah Summer Classic Jun 27-28, Nike Vegas Jul 9-13), roster flexibility clause, cost/schedule placeholders for Marcus
  • src/routes/contract/[token]/sign/+server.ts: stamp 2026-spring-girls-travel-v1 contract version for Queens players
  • src/app.css: .marcus-todo bright yellow marker style (remove before go-live)

Test Plan

  • Queens player sees Prep Hoops Girls content, not Power 32
  • Kings player still sees Power 32 content (no regression)
  • Local player still sees local content
  • MARCUS TODO markers visible and unmissable in browser
  • Signing a Queens contract stamps 2026-spring-girls-travel-v1

Review Checklist

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

Related Notes

  • forgejo_admin/westside-contracts#31 — girls travel contract feature
  • project-westside-basketball — Westside Kings & Queens project

Closes #31

## Summary - Add Queens-specific contract path for Prep Hoops Girls circuit (5 tournaments, different from boys Power 32) - Roster flexibility language: teams subject to change, late additions lower per-player costs - Cost fields marked with bright yellow MARCUS TODO markers for admin review before go-live ## Changes - `src/routes/contract/[token]/+page.server.ts`: pass `isGirls` flag derived from team name containing "Queens" - `src/routes/contract/[token]/+page.svelte`: full girls travel contract section — Prep Hoops Girls circuit schedule (Utah Girls Invitational Apr 10-11, Denver May 7-10, Mesa May 28-31, Utah Summer Classic Jun 27-28, Nike Vegas Jul 9-13), roster flexibility clause, cost/schedule placeholders for Marcus - `src/routes/contract/[token]/sign/+server.ts`: stamp `2026-spring-girls-travel-v1` contract version for Queens players - `src/app.css`: `.marcus-todo` bright yellow marker style (remove before go-live) ## Test Plan - [ ] Queens player sees Prep Hoops Girls content, not Power 32 - [ ] Kings player still sees Power 32 content (no regression) - [ ] Local player still sees local content - [ ] MARCUS TODO markers visible and unmissable in browser - [ ] Signing a Queens contract stamps `2026-spring-girls-travel-v1` ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `forgejo_admin/westside-contracts#31` — girls travel contract feature - `project-westside-basketball` — Westside Kings & Queens project Closes #31
Add Queens-specific contract path with Prep Hoops Girls tournament
schedule (Utah Invitational, Denver, Mesa, Utah Summer Classic, Nike
Vegas). Includes roster flexibility language for late additions.
Cost fields marked with MARCUS TODO markers for admin review.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-28 19:22:31 +00:00
Author
Owner
Copy link

PR #32 Review

DOMAIN REVIEW

Tech stack: SvelteKit (TypeScript, Svelte 5 runes, server-side rendering, direct Postgres via pg pool, MinIO for signature uploads). 4 files changed, +214/-5.

Server logic (+page.server.ts)

  • isGirls flag derived from player.team_name.includes('Queens') -- consistent with the existing isLocal pattern using .includes('Local'). Clean and readable.
  • Parameterized SQL query unchanged -- no injection risk introduced.

Sign endpoint (+server.ts)

  • Contract version branching uses a clean ternary chain: isLocal ? local : isGirls ? girls-travel : boys-travel. Correct precedence -- a "Queens Local" team (if one ever existed) would get the local version, which is the right default since isLocal is checked first.
  • The isGirls detection is duplicated between +page.server.ts (line 44) and +server.ts (line 60) -- both do teamName.includes('Queens'). This is an existing pattern (same duplication exists for isLocal). Not a blocker since these are not auth/security paths, but worth noting as a DRY opportunity for a future refactor (extract a getContractType(teamName) helper into $lib/).

Svelte template (+page.svelte)

  • Branching logic {#if !isLocal && !isGirls} ... {:else if !isLocal && isGirls} ... {:else} is correct and exhaustive. All three contract types (boys travel, girls travel, local) are properly routed.
  • The girls contract section reuses existing CSS classes (contract-content, cost-table, tournament-card, payment-schedule, etc.) -- no new structural CSS needed beyond .marcus-todo. Good reuse.
  • MARCUS TODO markers are prominent and intentional placeholders. The PR body explicitly states "remove before go-live" and the CSS comment reinforces this. This is a staging/draft contract for admin review, not a production deploy.
  • The callout box was moved inside the boys travel contract closing div (it was previously shared with the girls block in the diff context). Both boys and girls now get their own callout box. Correct.

CSS (app.css)

  • .marcus-todo uses #fbbf24 (amber-400) -- high contrast against white/dark text. Intentionally temporary. Acceptable as a review marker.
  • border-radius: 4px is a magic number but consistent with other radius values in the file. Fine for a temporary style.

Accessibility

  • No new interactive elements added. Tournament tables and cost cards are presentational content.
  • The existing ARIA labels on the checkbox and signature pad remain unchanged.
  • No new images or icons without alt text.

Correctness observations

  • proratedFee calculation (Math.round(monthlyFee * 25 / 30 / 5) * 5) is reused in the girls contract's payment schedule (line 524). This is correct -- same proration logic applies to both programs.
  • The girls contract includes a "Roster Flexibility" section not present in the boys contract -- this is intentional per the PR description (actively building the girls program).

BLOCKERS

None.

Test coverage: This PR adds template/view logic (which contract content to show based on isGirls flag) and a new contract version string. The existing tests/validation.test.ts covers the shared signing validation logic, which is untouched by this PR. The new functionality is:

  1. A boolean flag derived from a string check (teamName.includes('Queens')) -- trivially correct, same pattern as existing isLocal.
  2. A ternary selecting a contract version string -- deterministic, no branching complexity.
  3. ~190 lines of static HTML template content.

No new user input paths, no new API endpoints, no new validation logic. The test plan in the PR body covers the integration scenarios (Queens sees girls content, Kings still sees boys, signing stamps correct version). This is appropriate for template-level changes. Not a blocker.

NITS

  1. DRY opportunity: The isLocal / isGirls detection logic is duplicated between +page.server.ts and +server.ts. Consider extracting a getContractType(teamName: string): 'local' | 'girls-travel' | 'boys-travel' helper into $lib/contract.ts. This would also make the ternary chain in +server.ts (lines 61-65) cleaner. Not blocking because it follows the existing pattern, but worth a follow-up ticket as the number of contract types grows.

  2. Repeated section structure: The boys and girls contracts share identical sections (Jersey, Communication, Rules of Conduct, Commitment, Callout Box). These could be extracted into Svelte snippets or components to reduce template duplication. Not blocking for a 2-variant system, but would matter at 3+ variants.

  3. style="display: block;" on contract-content divs: This inline style appears on all three contract branches (lines 203, 378, 569). If it is always display: block, it should be in the CSS class definition rather than repeated inline.

SOP COMPLIANCE

  • Branch named after issue (31-feat-girls-travel-contract-prep-hoops-gi references #31)
  • PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
  • Related references project slug (project-westside-basketball)
  • Closes #31 links to parent issue
  • No secrets committed
  • No unnecessary file changes (4 files, all scoped to the feature)
  • Commit messages are descriptive

PROCESS OBSERVATIONS

  • Deployment risk: Low. This is additive content -- new {:else if} branch alongside existing untouched branches. Boys and local contracts are structurally unchanged. The only shared code modification is adding isGirls to the data return and adding a comma after isLocal in destructuring.
  • Go-live gate: The MARCUS TODO markers are an explicit pre-go-live gate. Marcus must fill in cost/schedule data before this contract is sent to real families. The PR is safe to merge as a staging artifact, but the contract must not be linked to players until TODOs are resolved.
  • Follow-up scope: The DRY extraction (nit #1) and component extraction (nit #2) are candidates for a follow-up ticket if a third contract variant is ever needed.

VERDICT: APPROVED

## PR #32 Review ### DOMAIN REVIEW **Tech stack**: SvelteKit (TypeScript, Svelte 5 runes, server-side rendering, direct Postgres via `pg` pool, MinIO for signature uploads). 4 files changed, +214/-5. **Server logic (`+page.server.ts`)** - `isGirls` flag derived from `player.team_name.includes('Queens')` -- consistent with the existing `isLocal` pattern using `.includes('Local')`. Clean and readable. - Parameterized SQL query unchanged -- no injection risk introduced. **Sign endpoint (`+server.ts`)** - Contract version branching uses a clean ternary chain: `isLocal ? local : isGirls ? girls-travel : boys-travel`. Correct precedence -- a "Queens Local" team (if one ever existed) would get the local version, which is the right default since `isLocal` is checked first. - The `isGirls` detection is duplicated between `+page.server.ts` (line 44) and `+server.ts` (line 60) -- both do `teamName.includes('Queens')`. This is an existing pattern (same duplication exists for `isLocal`). Not a blocker since these are not auth/security paths, but worth noting as a DRY opportunity for a future refactor (extract a `getContractType(teamName)` helper into `$lib/`). **Svelte template (`+page.svelte`)** - Branching logic `{#if !isLocal && !isGirls}` ... `{:else if !isLocal && isGirls}` ... `{:else}` is correct and exhaustive. All three contract types (boys travel, girls travel, local) are properly routed. - The girls contract section reuses existing CSS classes (`contract-content`, `cost-table`, `tournament-card`, `payment-schedule`, etc.) -- no new structural CSS needed beyond `.marcus-todo`. Good reuse. - MARCUS TODO markers are prominent and intentional placeholders. The PR body explicitly states "remove before go-live" and the CSS comment reinforces this. This is a staging/draft contract for admin review, not a production deploy. - The callout box was moved inside the boys travel contract closing div (it was previously shared with the girls block in the diff context). Both boys and girls now get their own callout box. Correct. **CSS (`app.css`)** - `.marcus-todo` uses `#fbbf24` (amber-400) -- high contrast against white/dark text. Intentionally temporary. Acceptable as a review marker. - `border-radius: 4px` is a magic number but consistent with other radius values in the file. Fine for a temporary style. **Accessibility** - No new interactive elements added. Tournament tables and cost cards are presentational content. - The existing ARIA labels on the checkbox and signature pad remain unchanged. - No new images or icons without alt text. **Correctness observations** - `proratedFee` calculation (`Math.round(monthlyFee * 25 / 30 / 5) * 5`) is reused in the girls contract's payment schedule (line 524). This is correct -- same proration logic applies to both programs. - The girls contract includes a "Roster Flexibility" section not present in the boys contract -- this is intentional per the PR description (actively building the girls program). ### BLOCKERS None. **Test coverage**: This PR adds template/view logic (which contract content to show based on `isGirls` flag) and a new contract version string. The existing `tests/validation.test.ts` covers the shared signing validation logic, which is untouched by this PR. The new functionality is: 1. A boolean flag derived from a string check (`teamName.includes('Queens')`) -- trivially correct, same pattern as existing `isLocal`. 2. A ternary selecting a contract version string -- deterministic, no branching complexity. 3. ~190 lines of static HTML template content. No new user input paths, no new API endpoints, no new validation logic. The test plan in the PR body covers the integration scenarios (Queens sees girls content, Kings still sees boys, signing stamps correct version). This is appropriate for template-level changes. Not a blocker. ### NITS 1. **DRY opportunity**: The `isLocal` / `isGirls` detection logic is duplicated between `+page.server.ts` and `+server.ts`. Consider extracting a `getContractType(teamName: string): 'local' | 'girls-travel' | 'boys-travel'` helper into `$lib/contract.ts`. This would also make the ternary chain in `+server.ts` (lines 61-65) cleaner. Not blocking because it follows the existing pattern, but worth a follow-up ticket as the number of contract types grows. 2. **Repeated section structure**: The boys and girls contracts share identical sections (Jersey, Communication, Rules of Conduct, Commitment, Callout Box). These could be extracted into Svelte snippets or components to reduce template duplication. Not blocking for a 2-variant system, but would matter at 3+ variants. 3. **`style="display: block;"` on contract-content divs**: This inline style appears on all three contract branches (lines 203, 378, 569). If it is always `display: block`, it should be in the CSS class definition rather than repeated inline. ### SOP COMPLIANCE - [x] Branch named after issue (`31-feat-girls-travel-contract-prep-hoops-gi` references #31) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references project slug (`project-westside-basketball`) - [x] `Closes #31` links to parent issue - [x] No secrets committed - [x] No unnecessary file changes (4 files, all scoped to the feature) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment risk**: Low. This is additive content -- new `{:else if}` branch alongside existing untouched branches. Boys and local contracts are structurally unchanged. The only shared code modification is adding `isGirls` to the data return and adding a comma after `isLocal` in destructuring. - **Go-live gate**: The MARCUS TODO markers are an explicit pre-go-live gate. Marcus must fill in cost/schedule data before this contract is sent to real families. The PR is safe to merge as a staging artifact, but the contract must not be linked to players until TODOs are resolved. - **Follow-up scope**: The DRY extraction (nit #1) and component extraction (nit #2) are candidates for a follow-up ticket if a third contract variant is ever needed. ### VERDICT: APPROVED
forgejo_admin deleted branch 31-feat-girls-travel-contract-prep-hoops-gi 2026-03-28 19:26:30 +00:00
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
forgejo_admin/westside-contracts!32
No description provided.