# R6 — Group members saved on a different team from their group

**Brief, Oct 12 2026.** Describes the defect and how to fix it. Like every brief here, it does not say
whether the fix has shipped — `OUTSTANDING_ISSUES.md` (item **R6**) is the only place that says that.

## 0. Scope chosen — Oct 12 (Andrew)

**Built:** §4.1 (as `loadGroupMembers()` + `findGroupManager()`), §4.2, and **§4.6** below.
**Not built:** §4.3 (builder shows saved teams), §4.4 (lock check on the invite link), §4.5 (audit
command). Season 49 was repaired by hand: every group box was dragged to the Workbench and back.

With §4.2 + §4.6 there is no ordinary path left that splits a group. What §4.3 would still add is
*visibility*: if a split ever came from somewhere else, the builder would still draw the group
together and only `/teams` would show it.

---

## 1. What the board saw (Late Fall, season 49)

Roster Builder showed 8 players on every team. `/teams?season=49` showed 6 on some teams and 9 on
others. From the Oct 12 screenshots:

| Builder shows | Saved (what `/teams` shows) |
|---|---|
| Giovani Yanez in Renee Torres's group on **Courage** | Giovani on **OL Reign** (9) |
| Caleb Mansure grouped with Luke Crawford on **Red Stars** | Caleb on **Galaxy** (9); Red Stars 7 |
| Andres Bernal + Diego Campos grouped with Jose Lorenzo on **Sounders** | neither on any visible team; Sounders 6 |

The drags were saved. The builder was showing something that was never saved.

## 2. Mechanism

**Read side — the builder rearranges what it shows without saving it.**
`TeamBalancerService::getRosterState()` buckets players by `Registration.team`, then
`consolidateGroups()` moves every group member into their manager's bucket **in memory only**. The
team footers count that rearranged list. `/teams` (`ContentController::teamsPage()`) reads
`Registration.team` directly. Whenever a group's members are saved on different teams, the two pages
disagree. The builder gives no sign of it, and the Workbench reads 0 because unplaced members were
pulled into their manager's box.

**Write side — the player-facing join paths set `group_id` but never `team`.**

| Path | Sets group | Sets team | Lock check |
|---|---|---|---|
| `GroupController::acceptSeasonInvitation()` (My Registrations → Accept) | ✔ | ✘ | ✔ |
| `RegistrationController::acceptSeasonInvitationDirectly()` (invite link, already registered) | ✔ | ✘ | **✘** |
| `OrderCompleteSubscriber::createSeasonRegistration()` (invite link, new registrant) | ✔ | ✘ (NULL) | **✘** |
| `RosterBuilderController::mergeToGroup()` (Shift+drag) | ✔ | first member *found* with a team — not necessarily the manager | n/a (R7) |
| `GroupInvitationsForm` (admin) | ✔ | ✔ via `syncGroupToRoster()` | n/a |

`syncGroupToRoster()` already exists and does the right thing, but only the admin form calls it.

So any player who joins a group **after** they have been placed keeps their old team, and a late
registrant arriving through an invite is saved with **no** team. The builder then draws them in the
manager's box, so it looks finished and nobody drags them.

**Three other things make it worse:**
- *Group members cannot be dragged one at a time* — only the whole box — so the builder offers no
  way to see or fix a single split member.
- *"The group's team" has three different definitions.* `consolidateGroups()` uses the **last**
  manager row it meets, `syncGroupToRoster()` the **first**, and `mergeToGroup()` **any member** with
  a team. These agree only while the data is clean, and split data is exactly when they matter.
- *`groups_locked` does not stop the invite-link paths.* Locking groups before building rosters does
  not prevent this today.

## 3. The rule this fix enforces

> **`Registration.team` is the only truth.** The builder shows saved teams and never rearranges them.
> A group is kept together by **saving** its members to the manager's team when they join, not by
> drawing them there.

"The group's team" = **the manager's saved team** (the live, paid season registration in the group
with no `invited_by`). It is defined in one method, which every caller uses.

## 4. Changes

### 4.1 One definition of the group's team — `TeamBalancerService`

*As built:* `loadGroupMembers($group_id, $season_id)` (season rows with status `paid` or `active`,
the pair `/teams` shows, oldest first) and `findGroupManager($members)` (the oldest row with no
`invited_by`, with a warning logged if there are several). `consolidateGroups()` uses the same
oldest-row rule. The sketch below is the original proposal.

```php
/**
 * The manager's live season registration for a group, and the team it is on.
 *
 * The single definition of "the team a season group belongs on". Used by
 * syncGroupToRoster(), getRosterState() and RosterBuilderController::mergeToGroup().
 *
 * @return array{manager_registration_id: ?int, team_id: ?int}
 */
public function getGroupAnchor(int $season_id, string $group_id): array {
  $ids = $this->entityTypeManager->getStorage('ccsoccer_registration')->getQuery()
    ->accessCheck(FALSE)
    ->condition('season', $season_id)
    ->condition('group_id', $group_id)
    ->condition('registration_type', 'season')
    ->condition('status', 'paid')
    ->notExists('invited_by')
    ->sort('id')
    ->execute();
  if (count($ids) > 1) {
    // Should not happen. Deterministic answer + a log line, never a guess.
    $this->logger->warning('Group @g in season @s has @n manager rows; using the oldest.', [...]);
  }
  ...
}
```

`syncGroupToRoster()` is rewritten on top of it, with an optional `$season_id` argument. The existing
call in `GroupInvitationsForm` keeps working unchanged. **As built, it differs from the old method in
three ways:** (1) it copies the manager's team even when that is *no team*, so a member of a group
still on the Workbench goes to the Workbench too, which is where the builder draws them; (2) every row
carrying the `group_id` counts, not just `accepted` ones, because the builder draws every such row in
the box; (3) `active` rows count as well as `paid`. It still does nothing for a headless group, and
logs every move.

### 4.2 Write side — every season join puts the joiner on the group's team

After the registration save, call `syncGroupToRoster($group_id, $season_id)` in:

1. `GroupController::acceptSeasonInvitation()`
2. `RegistrationController::acceptSeasonInvitationDirectly()`
3. `OrderCompleteSubscriber::createSeasonRegistration()` — only when `$group_id` was set. **Wrap it in
   try/catch and log the error.** A paid checkout must never fail because a roster sync threw.
4. `RosterBuilderController::mergeToGroup()` — replace the "any member's team" loop with
   `getGroupAnchor()['team_id']`.

`createGroup()` is already consistent: it moves the manager to the member's team and both rows end up
equal.

Injection: add `@ccsoccer.team_balancer` to `GroupController::create()`,
`RegistrationController::create()` and the `OrderCompleteSubscriber` service arguments in
`ccsoccer.services.yml`. **The services.yml change makes `drush cr` mandatory on deploy.**

Side effect, intended: the sync moves *every* accepted member who is off the manager's team, not
just the joiner. So any join also repairs older splits in that group.

**Why explicit calls rather than an entity hook:** same reasoning as `archive/D8_OPTIONS_COMPARISON.md`
§8. A `hook_ccsoccer_registration_presave()` would catch every path, including future ones, but this
codebase has already shipped a misnamed registration hook that never fired. Explicit calls are easier
to review. The audit in §4.5 is the safety net that would expose a missed call site.

### 4.3 Read side — the builder stops rearranging players

In `getRosterState()`, replace `consolidateGroups()` with `buildGroupInfo()`. It builds the same
`groups` array the JS already receives (status, counts) but **moves nobody**, and adds:

```php
'split'          => TRUE|FALSE,          // members saved in more than one place
'anchor_team_id' => ?int,                // getGroupAnchor()
'locations'      => [team_id|0 => n],    // 0 = workbench
```

Because `renderPlayersGrouped()` already groups per column, a split group draws as **one partial box
in each column its members are saved in**. `RosterBuilderForm::buildGroupContainer()` adds class
`group-split` and a small badge: *"Split — 1 more on Courage"*. A banner above the grid reads:
*"3 players are saved on a different team from their group. /teams shows where they are saved."*

Team footers then count saved players, so **builder counts equal `/teams` counts.** Fixing a split in
the UI is one drag: drop the partial box on the manager's team. `move()` saves those IDs, and on
reload the box merges.

**JS fix this needs (`roster-builder.js`).** `handleDrop()` finds the dragged group with
`document.querySelector('.player-group[data-group-id="…"]')`. With split boxes there can be two, so
it can move the wrong box on screen while saving the dragged box's players. Keep a reference to the
dragged element in `handleGroupDragStart()` and use it in `handleDrop()`, clearing it on dragend.

CSS: one rule for `.group-split` (dashed red border) in `roster-builder.css`.

### 4.4 Close the lock hole — `acceptSeasonInvitationDirectly()`

Add the same `groups_locked` check, with the same message, that `acceptSeasonInvitation()` has. With
it, "lock groups, then build rosters" actually freezes groups. Checkout under a lock is a policy
question (§6, Q2) and is left alone here.

### 4.5 Audit and repair — `drush ccsoccer:group-teams <season> [--fix]`

- Default (dry run): a table of every split member — player, manager, **saved team**, **manager's
  team** — plus groups with no manager or more than one, plus a count of `status = active` season
  registrations (see §5).
- `--fix`: runs `syncGroupToRoster()` for each split group that has a placed manager. Members whose
  manager is unplaced are reported, not moved.
- Logs to watchdog type `ccsoccer`.

**Repair direction = the manager's team = what the builder showed = what the board approved.**
Running `--fix` on season 49 reproduces the 8-per-team rosters the board signed off. Re-running
should report zero, which is also how a missed call site in §4.2 would show up later.

### 4.6 Drags move the whole saved group — `RosterBuilderController::move()`

A group box carries the player IDs it had when the page loaded. If someone joined the group after
that, or a second board member changed it, dragging the box moved only the players the page knew
about and left the newcomer behind: a split, which the builder then hid.

`move()` now expands every dragged player to all current members of their saved group
(`loadGroupMembers()`) before writing. When that adds anyone the page was not showing, the response
carries `reload: true` and `roster-builder.js` reloads, so the board sees what was saved.

Shift+drag (`mergeToGroup()`) uses `findGroupManager()` for the team, sets the player to it even when
it is *no team*, then runs `syncGroupToRoster()`. Option+drag (`createGroup()`) saves both players on
the same team; before, a member dragged onto from the Workbench stayed there.

## 5. Related, deliberately left alone

- **`active` status.** The builder loads `status = paid`; `/teams` shows `paid` **and** `active`. An
  `active` season registration appears publicly but never in the builder. That is Decision 2 (the
  live-registration allowlist), so it is **not changed here**. §4.5 only reports the count.
- **R7** (builder endpoints ignore `groups_locked`): left as an admin override.
- **R8** (no notification when a player is moved or a roster is published): out of scope. The sync
  already writes a log line per move.

## 6. Open questions for Andrew

1. **Joins after rosters are built.** With §4.2, a placed player who accepts an invite after rosters
   are built moves to the manager's team at once: one team gains a player and another loses one,
   honestly on both pages. *Recommendation:* accept that, and make "lock groups before building
   rosters" the standing process. §4.4 makes the lock real.
2. **Checkout while groups are locked.** A new registrant arriving through an invite link currently
   joins the group regardless. Should a lock register them **without** the group and flag the order,
   as the capacity guard already does?
3. **"Reunite" button on split boxes** (POST → `syncGroupToRoster()`). Small, but dragging the
   partial box already does it. Build only if wanted.

## 7. Deploy and test

**Deploy:** code only, then `cr` — `ccsDeploy && ccsCr`, then `ccsProdDeploy && ccsProdCr`. There is
no update hook and no config change. The `cr` is required for services.yml, the new drush command and
the JS.

**LOCAL test plan:**
1. A manages a group and is on Team 1; B is on Team 2. B accepts from My Registrations → B on Team 1
   in both the builder and `/teams`.
2. The same, through the invite link while already registered.
3. A new registrant checks out through an invite → placed on the manager's team.
4. Groups locked + invite link while registered → refused.
5. Force a split in SQL → builder shows two partial boxes with the badge and banner, and the counts
   match `/teams`. Drag the partial box onto the manager's team → one box after reload.
6. Shift+drag a player into a split group → lands on the **manager's** team.
7. `drush ccsoccer:group-teams <sid>` lists the split; `--fix` clears it; a re-run shows zero.

There are no automated tests on `main` yet (the Phase 0b harness is on `chore/phase0b_harness`).
`getGroupAnchor()` and `buildGroupInfo()` are pure enough to be the first kernel tests when it lands.

**Rough cost:** 4.1–4.2 ≈ 1.5 h · 4.3 incl. JS/CSS ≈ 2 h · 4.4 ≈ 0.25 h · 4.5 ≈ 1 h · LOCAL
testing ≈ 2 h → **about 7 h**.