# Tournament skill display — zombie-field reads and the three skill calculators

**Date:** July 29, 2026
**Status:** **§1 site 1 and §2 IMPLEMENTED** on the working tree — not linted, not tested, not
committed. §3, §4 and §1 sites 2-4 remain proposals. See §5 for what landed and what did not.
Committing together with the gender-colour change from `TOURNAMENT_ROSTER_GENDER_DISPLAY.md` — same
screen, same two files, all three are "the roster builder displayed a value read from the wrong place."
**Origin:** two findings noticed while implementing the gender colours, plus Andrew's question —
*"when does `Team::calculateSkillLevelFromPlayers()` happen?"* The answer is §3 and it reframes the rest.
**Andrew's operating model, which decides what matters here:**

> Season: players self-score 1-5, the board reviews and sets an admin skill level before each season,
> teams are reshuffled for fair equal play. **Tournament: captains build their own teams, so the board
> does not need player-level skill.** The board assigns a skill *per team* on the Teams page, and the
> schedule builder uses those team-level numbers to match teams closely.

**Bottom line: the pipeline Andrew designed works.** The schedule builder reads the board's Assigned
Skill straight from the entity and nothing below can corrupt it. Everything in this document is a
display bug or dead code — but one of them is a write-on-page-load that stores a fabricated number.

---

## 1. Finding 2 — `Registration.self_score` no longer exists. Four sites still read it.

### What happened

`ccsoccer_update_9048()` deleted `self_score` from Registration and moved self-assessment to the User
entity as `field_self_score`. The Registration entity records this in a comment where the field used to
be (`Registration.php:151-153`):

```php
// NOTE: self_score was removed from Registration and moved to User entity
// as field_self_score. Self-assessment is a player attribute, not per-registration.
// See update hook ccsoccer_update_9048().
```

Four sites never got the memo. All four are guarded by `hasField('self_score')`, which returns FALSE
for a field that is not in the entity's definitions — so **nothing crashes, nothing logs, and every one
of them silently returns its default.**

| # | Site | Reads | Consequence |
|---|---|---|---|
| 1 | `Form/TournamentRosterBuilderForm.php:354-358` | `$reg->get('self_score')` | Every player card on the tournament roster builder shows skill **3** |
| 2 | `Controller/TournamentController.php:819-823` | same | `calculateTeamSkillLevel()` returns exactly **3.00** for every team — and **saves it** (see §3) |
| 3 | `Entity/Team.php:556-562` | same | Second fallback inside `calculateSkillLevelFromPlayers()`; dead branch inside a nearly-dead method |
| 4 | `Controller/SeasonController.php:591-594` | same | A **season** admin screen's self-score column renders blank |

This is the same class of bug as the July 20 `field_has_jersey` finding: a field was removed, the
writers were updated, the readers were not, and `hasField()` turned a fatal into silence.

**Site 4 is on the season side and is not part of the tournament story.** Worth confirming what that
column looks like today — it is the only one of the four where the correct value is actually wanted for
its own sake.

### Checkout is not the problem — it collects self-score correctly

Worth stating because the obvious first guess is wrong. `PlayerInfoPane::buildPaneForm()` (103-117)
presents skill as **required** 1-5 radios for tournaments as well as seasons, pre-filled from
`User.field_self_score`, and stores it back on the user. The data exists. The tournament roster builder
is looking in a place it stopped living.

### The fix

The correct read is the one the season builder already uses —
`TeamBalancerService::getPlayerSkill($registration, $player)` (`:484-504`):

1. admin-assigned `User.field_skill_level`, if 1-5
2. self-assessed `User.field_self_score`, if 1-5
3. default 3

**Correction to an earlier draft of this document:** `getPlayerSkill()` was *not* made public by the
gender-colour change — only `isPlayerWoman()` and `getPlayerPrefersGoalie()` were. It was still
`protected` and had to be widened as part of this work. The injection was already in place, though, so
site 1 was still a one-line call:

```php
// TournamentRosterBuilderForm.php, replacing 354-358
$skill = $this->teamBalancer->getPlayerSkill($reg, $player);
```

**Do this even though Andrew does not need player skill on this board.** The badge has to render
something, and — the reason it cannot simply be removed — **that badge is what carries the gender
colour.** `.player-card.player-female .skill-badge` is the red circle. Drop the badge and women lose it,
leaving only the new left stripe. So the choice is "render a true number" or "render a false one", and
the true one costs a line.

Sites 2 and 3: see §3, which changes what should be done with them.

Site 4: same one-line shape, but `SeasonController` does not currently have the balancer injected —
check before assuming, and if it means adding a constructor argument for one column, it may be better
batched with other season-admin work.

**Scale note:** `getPlayerSkill()` returns 1-5 with **no conversion**, which per Andrew is correct —
`field_skill_level` has only ever been used as 1-5 in practice, despite the field description. See §4.

---

## 2. Finding 1 — the footer showed the wrong quantity

> **Superseded, July 29 (later) — read §2a first.** This section diagnosed the JS overwriting the
> footer's Assigned Skill and recommended removing the overwrite. That was implemented, then
> **reversed** once the skill data was correct: Andrew decided the footer should show the *average
> player skill* all along, which makes the JS recalculation right rather than wrong. The diagnosis below
> is still accurate about the old behaviour and explains why the number was always `3.00`; the
> prescription is replaced by **§2a**.

**Confirmed simple, confirmed harmless to data, worth doing while the file is open.**

`TournamentRosterBuilderForm::buildTeamColumn()` (689-696) renders the board's Assigned Skill — or `-`
when unset — into `.avg-skill`:

```php
$admin_skill_display = $team['admin_skill'] !== NULL ? number_format((float) $team['admin_skill'], 1) : '-';
…
'<span class="stat avg-skill" title="Admin Skill">' . $admin_skill_display . '</span>' .
```

Then `updateTeamStats()` in `js/tournament-roster-builder.js` (836-852) overwrites that same span on
every drag with an average of the player cards' `data-skill`.

**Because of Finding 2, every card's `data-skill` is 3** — so the computed average is not merely a
different metric, it is **always exactly `3.00`**. Drag one player and a team the board rated 4.5 starts
reporting 3.00. Refresh and it is correct again.

**It cannot corrupt anything.** It rewrites text in a `<span>`. The move endpoint posts no skill value,
and the schedule builder reads `admin_skill_level` from the entity, never the DOM.

### The change

Delete the `avgSkillEl` lookup and its write, leaving age alone:

```js
        // Recalculate average age. Skill is NOT recalculated here: the
        // .avg-skill span holds the board's team-level Assigned Skill
        // (admin_skill_level), set on the Tournament Teams page — not an
        // average of player skills. Overwriting it made a team the board
        // rated 4.5 report 3.00 after the first drag.
        let totalAge = 0;
        players.forEach(player => {
          totalAge += parseInt(player.dataset.age) || 30;
        });

        const avgAgeEl = teamEl.querySelector('.team-footer .avg-age');
        if (avgAgeEl && count > 0) {
          avgAgeEl.textContent = (totalAge / count).toFixed(1);
        }
```

`totalSkill` and `avgSkillEl` become unused and go with it. Net effect: four lines removed, one comment
added. **Tournament JS only — do not touch `js/roster-builder.js`**, where the season board's
`.avg-skill` genuinely *is* a computed player average and must keep updating.

---

## 2a. What was actually built — the footer shows average player skill

**Andrew, July 29 (after §1 landed):** now that the skill data is real, the roster builder should show
the team's **average player skill**. The schedule builder keeps using the board's Assigned Skill from the
Tournament Teams page. Seeing the true roster average *before* setting the Assigned Skill is what helps —
it informs the rating, and it helps decide where free agents should go.

That inverts §2's prescription. The overwrite was not the bug; **the PHP rendering the wrong quantity
was.** Two sides now agree that `.avg-skill` means "average player skill", so recalculating on drag is
correct — and the tournament board ends up reading exactly like the season board, which is the direction
worth travelling given how much of this codebase is two paths for one rule.

### Changes

**PHP — `TournamentRosterBuilderForm::getTournamentRosterState()`**

`total_skill` was already accumulated per team (`:442`) and `avg_skill` was already initialised
(`:328`) — it was simply never computed, with a comment saying *"for age only - skill uses admin_skill"*.
So the per-team average was one line beside the existing age calculation:

```php
$team['stats']['avg_skill'] = number_format($team['stats']['total_skill'] / $team['stats']['player_count'], 2);
```

**`number_format()`, not `round()`** — deliberately. `round(2.80, 2)` renders as `2.8` while the JS's
`toFixed(2)` gives `2.80`, so the value would visibly change format on the first drag. That is the same
class of PHP/JS disagreement this whole section is about, and the season board still has it latent
(`TeamBalancerService:1186` uses `round()`); it is invisible there only because the current averages
happen to have two significant decimals.

The `else` branch renders `-` for an empty roster, so PHP and JS agree there too.

**PHP — footer markup** now renders `avg_skill` with `title="Avg Player Skill"`. `$admin_skill_display`
is gone. `$team['admin_skill']` is still populated in the team data array (`:311, :321`) and is now
unused by this form — left in place deliberately, since it is cheap and any future "show both" would
want it.

**PHP — the sidebar.** `roster_average_skill` was averaging the **teams' Assigned Skill values**
(2.0+4.0+4.5+4.0+1.5+1.0+3.0+2.5+2.0+3.5 = 28 ÷ 10 = the `2.8` in Andrew's screenshot). Per Andrew it is
now the average across **all players**, matching the season board (`TeamBalancerService:1143`) and
matching the average age rendered directly beneath it, which was already over all players. `$total_skill`
and `$players_with_skill` were already accumulated (`:423, :425`), so this was also a small change.

**A useful property of that choice:** the all-players average is **drag-invariant** — moving a player
between columns cannot change it, because every registration counts whether assigned or not. So the
sidebar never needs JS updating, and neither JS file touches it. Under the old
average-of-assigned-skills definition it would have been stale after any Teams-page edit.

**JS — `updateTeamStats()`** recalculates both averages again, with `toFixed(2)` to match the PHP, and
writes `-` when the last player leaves a column instead of stranding a stale average or dividing by zero.

### Net effect on the three numbers

| Where | Before | After |
|---|---|---|
| Team footer, middle stat | Assigned Skill (`4.5`), replaced by `3.00` on first drag | Average player skill (`3.67`), live on drag, `-` when empty |
| Sidebar "Roster Avg Skill" | Average of the teams' Assigned Skills (`2.8`) | Average player skill across all tournament players |
| Teams page "Assigned Skill" | board's value | **unchanged** — still the only place it is set, still what the schedule builder matches on |

---

## 3. Andrew's question — when does `Team::calculateSkillLevelFromPlayers()` run?

**Almost never. And the reason why is a worse bug than the one you asked about.**

### It has exactly one caller, behind a condition that is almost always false

`TournamentScheduleGeneratorService.php:534-538`:

```php
if ($team->getCalculatedSkillLevel() === NULL) {
  $team->calculateSkillLevelFromPlayers();
  $team->save();
}
```

That is the only call site in the entire module. So it runs when the schedule builder loads **and**
`calculated_skill_level` is NULL on that team.

### But something else fills that field in first — on every Teams page load

`TournamentController::teamsPage()` — the page at `/admin/ccsoccer/tournament/1/teams`, where you set
Assigned Skill — calls a **different, third** calculator and persists it, inside the render loop
(682-689):

```php
$calculated_skill = $this->calculateTeamSkillLevel($team, $registration_storage);
if ($calculated_skill !== NULL) {
  $team->set('calculated_skill_level', $calculated_skill);
  $team->save();
}
```

So by the time you open the Schedule Builder, `calculated_skill_level` is non-NULL for every team that
has players, and `Team::calculateSkillLevelFromPlayers()` is skipped. **Its `/2` halving is dormant** —
it can only fire for a team that has players and whose Teams page has not been loaded since those
players arrived. On your current tournament, never.

### Three calculators, and the one that actually runs is the broken one

| | Method | Reads | Runs when | Result today |
|---|---|---|---|---|
| **A** | `TeamBalancerService::getPlayerSkill()` | `User.field_skill_level` → `User.field_self_score` → 3 | Season roster builder, Suggest Rosters | ✅ correct |
| **B** | `Team::calculateSkillLevelFromPlayers()` | `User.field_skill_level` **÷ 2** → dead `Registration.self_score` → 3 | Schedule builder, only if `calculated_skill_level` is NULL | 💤 dormant (C beats it to the field) |
| **C** | `TournamentController::calculateTeamSkillLevel()` | dead `Registration.self_score` **only** → 3 | **Every Teams page load**, and it saves | ⚠️ returns exactly `3.00`, always |

**C is the one that runs, and it is the worst of the three.** It has no `field_skill_level` fallback at
all — its only source is the field that no longer exists. So `calculated_skill_level` on every
tournament team is currently a stored, fabricated `3.00`.

### Why this has not hurt you

`getSkillLevel()` (`Team.php:412-423`) prefers the admin value and only falls back to the calculated one:

```php
$admin_level = $this->get('admin_skill_level')->value;
if (!empty($admin_level)) {
  return (float) $admin_level;
}
$calculated = $this->get('calculated_skill_level')->value;
```

Because the board assigns a skill to **every** team — all ten on summer classic 2026 — the fallback is
never reached and the schedule builder matches on your real numbers. Your screenshots confirm it: Ball
Hogs 4.5, ASS FC 4.0, Gorilla Warfare 1.0 on the Teams page, identical in the schedule grid.

**The exposure is a forgotten assignment.** Clear one team's Assigned Skill, or create a team after the
skills pass, and the schedule builder silently treats it as **3.0** — a plausible-looking mid-range
number that is not a measurement of anything. It will not warn, because a stored `3.00` is
indistinguishable from a real calculation.

### Recommendation

**Preferred: delete B and C, and make the fallback honest.** For a tournament, a team-level average of
player self-scores is not a number the board wants — captains pick the teams and the board rates the
result. A calculator whose output is always 3.00 is worse than no calculator, because it looks like
data.

1. **Delete `TournamentController::calculateTeamSkillLevel()`** and the save block at 682-689. This also
   removes **an entity write on a GET request for every team on every page load** — a side effect worth
   losing on its own, and one that dirties caches and makes the Teams page slower the more teams exist.
2. **Delete `Team::calculateSkillLevelFromPlayers()`** and the conditional call in
   `TournamentScheduleGeneratorService:534-538`. That takes the `/2` scale bug with it, so §4 becomes a
   documentation-only change.
3. **Make the missing case visible instead of guessing.** `getSkillLevel() ?? 3.0` at
   `TournamentScheduleGeneratorService:543` is where an unassigned team becomes a silent 3.0. Keep the
   default so nothing crashes, but have the schedule builder **name the teams it had to guess for** —
   a warning listing them, in the same place it already reports team and field counts. That converts a
   silent mis-match into a prompt to go set the number.
4. Consider whether `calculated_skill_level` should be **removed from the Team entity** once nothing
   writes it. If it stays, an update hook should NULL the fabricated `3.00` values so a future reader
   does not mistake them for measurements. **Leave the field in place for now** — removing a base field
   needs its own update hook and a check for other readers, and this proposal is already three files.

**Lighter alternative if deleting feels too broad:** point C at
`TeamBalancerService::getPlayerSkill()` (now public) so the fallback is at least real, and leave B
deleted or fixed. This keeps the current shape and fixes the number, but it also keeps the write-on-GET
and keeps a metric the board has said it does not use. Andrew's call.

---

## 4. Finding 3 — resolved by Andrew, one doc fix left

**Andrew, July 29:** D7 migrated players only ever had 1-5. The board reviews and sets admin skill
before each season. New registrants self-score 1-5 only. The old site's *"Admin-set skill level (1-10)"*
label was never implemented as 1-10.

That settles it, and it confirms the code is right where it counts:

- `TeamBalancerService::getPlayerSkill()` treating `field_skill_level` as **1-5 with no conversion is
  correct**. The season balancer is fine. No query needed, nothing to repair.
- Both admin write paths already clamp to 1-5 (`PlayerSkillController:110-116`,
  `PlayerAdminController:448-453`), so nothing can introduce a 6-10 value going forward.
- `MigrateCommands.php:447` writes `(int) $row->obs_skill` unclamped, but per Andrew the source was 1-5,
  so this is historical only.

**Two stale artefacts remain, neither urgent:**

1. **`Team::calculateSkillLevelFromPlayers()`'s `/2`** (`Team.php:543-547`) is now definitively wrong,
   not merely inconsistent. On a 1-5 field it compresses the whole scale into 1-3 — `1→1, 2→1, 3→2,
   4→2, 5→3`. **Dormant** (§3) and **removed entirely** if §3's recommendation is taken, which is the
   cleanest resolution.
2. **The field description lies.** `config/sync/field.field.user.user.field_skill_level.yml:14` says
   *"Admin-set skill level (1-10)."* That sentence is what sent me looking for a bug, and it will
   mislead the next reader the same way. Fix it to `1-5`.

⚠ **That is a config change, so it does not ride along with a code-only deploy.** It needs
`drush cex` → commit → `cim` on TEST and PROD, and per the July 27 notes `drush config:status` should be
checked before any `cex` (expect the three known `media_library` display entries as the only drift).
Worth batching with the next config change rather than doing alone.

---

## 5. Status and sequencing

None of this was urgent — the board's numbers reach the schedule builder correctly today.

| Order | Item | Size | Status |
|---|---|---|---|
| 1 | §1 site 1 — `getPlayerSkill()` on the roster builder card | 1 line + visibility | ✅ **DONE** |
| 2 | §2a — footer + sidebar show average player skill; JS recalculates | ~15 lines | ✅ **DONE** (supersedes §2) |
| 3 | §3 — delete calculators B and C, add the unassigned-team warning | ~60 lines removed, 1 added | ⬜ proposal — own commit |
| 4 | §4 item 2 — field description 1-10 → 1-5 | 1 line | ⬜ proposal — **config**, batch with the next `cex` |
| 5 | §1 site 4 — `SeasonController` self-score column | 1 line + maybe injection | ⬜ proposal — season screen, separate work |

### What was changed for items 1 and 2

| File | Change |
|---|---|
| `src/Service/TeamBalancerService.php` | `getPlayerSkill()` `protected` → `public`; docblock corrected — it claimed *"Self-score (registration)"*, which has not been true since update 9048 — and now records the 1-5-no-conversion decision from §4 |
| `src/Form/TournamentRosterBuilderForm.php` | `Registration.self_score` read (354-358) → `getPlayerSkill()`; per-team `avg_skill` computed with `number_format(…, 2)` and `-` for empty rosters; footer renders it with `title="Avg Player Skill"`; sidebar `roster_average_skill` switched from average-of-Assigned-Skills to average across all players |
| `js/tournament-roster-builder.js` | `updateTeamStats()` recalculates average skill again, `toFixed(2)` to match the PHP, `-` on an empty roster |

Injection needed no work — `ccsoccer.team_balancer` was already added to the form's constructor and
`create()` by the gender-colour change, so item 2 really was one line plus a visibility keyword.

`js/roster-builder.js` **deliberately untouched** — verified it still computes and writes `.avg-skill`
on drag (925-947), which is correct for the season board.

**Verified in the sandbox:** `node --check` passes on the edited JS; no `totalSkill` / `avgSkillEl`
references survive outside comments; brace and paren counts balanced in both PHP files with deltas
matching the diff. **`ddev php -l` on both PHP files is still required before commit** — there is no PHP
in the authoring sandbox.

Archives (pre-edit snapshots, taken before the gender change so they cover the whole commit):
`archive/TeamBalancerService_2026-07-29.php`, `archive/TournamentRosterBuilderForm_2026-07-29.php`,
`archive/tournament-roster-builder_2026-07-29.css`, `archive/tournament-roster-builder_2026-07-29.js`.

### Test on LOCAL — items 1 and 2

Run alongside `TOURNAMENT_ROSTER_GENDER_DISPLAY.md` §4; these are the additions.

1. **Skill badges are no longer all 3.** Load the tournament roster builder. Players now show their
   admin skill level if set, else their self-score, else 3 — so a mix, not a wall of 3s. Cross-check two
   players against `/admin/ccsoccer/players`.
2. **The badge still carries the gender colour** — a woman's badge is red whatever number it holds. This
   is the interaction between the two changes, so check it explicitly.
3. **Footer middle stat is now the roster average, 2dp.** Hover it — the tooltip reads **Avg Player
   Skill**. Hand-check one team: sum its players' badge numbers ÷ player count should equal the footer
   value. It will **not** match the Assigned Skill column on the Teams page, and that is the point.
   Remember captains show `C`/`CC` instead of a number but **are** included in the average, so use the
   Teams page or the DB to get their skill when hand-checking.
4. **Format does not shift on drag.** Note a footer showing a trailing zero — e.g. `3.00` or `2.80` —
   then drag a player in or out. It must stay two-decimal, never flip to `3` or `2.8`. This is the
   `number_format` / `toFixed` agreement; it is the single most likely thing to have gone wrong.
5. **Live recalculation.** Drag a player from Unassigned onto a team → that team's count, skill average
   and age average all update; drag them off again → all three revert. Move a player between two teams →
   both columns update. Multi-drag a captain's group.
6. **Empty roster shows `-`.** Drag the last player off a team → skill and age both read `-`, not a stale
   number and not `0.00`. Reload and confirm PHP renders `-` too.
7. **Sidebar "Roster Avg Skill" is drag-invariant.** Note the value (it should now be an average of
   player skills across all 178 players, no longer `2.8` = the average of the ten Assigned Skills). Drag
   players between columns → **it must not change**, because every registration counts whether assigned
   or not. It should only move if a player is added to or removed from the tournament.
8. **Season roster builder unchanged** — footers still recalculate on drag, and Suggest Rosters still
   balances (it calls `getPlayerSkill()`, whose logic did not change — only its visibility).
9. **Schedule builder unaffected** — team skills in the grid still match the **Assigned Skill** column on
   the Teams page, not the roster builder's new averages. Confirm explicitly: this is the boundary the
   whole change depends on.

Item 3 stays separate: it deletes methods and changes what the schedule builder does when a skill is
missing, which deserves its own review and click-through.

**Per repo convention:** archive each file before editing
(`archive/<Name>_<YYYY-MM-DD>.<ext>` — check the filename is not already taken), `ddev php -l`, LOCAL
click-through, TEST, then PROD. Items 1-3 are code-only (`drush cr`); item 4 is not.