Read & write login.avatar_id alongside legacy avatars.selected

[Brutus5000]

Summary

• Adds the new login.avatar_id FK column (introduced in faf/db#331) to the login Table.
• Player fetch (PlayerService.fetch_player_data) now joins avatars_list on COALESCE(login.avatar_id, avatars.idAvatar) — preferring the new authoritative column with a fallback to the legacy avatars.selected = 1 row so legacy data still works.
• command_avatar select action now also writes login.avatar_id whenever it touches avatars.selected, so the lobby never produces drift between the two columns going forward.

Why

The DB schema is migrating away from "exactly one of N selected rows" toward a single FK on login. Until all writers are on the new column, both columns must stay aligned. This is the lobby's half — the API will follow once login.avatar_id becomes its authoritative write path.

Test plan

• Player with login.avatar_id set → fetch returns that avatar's url/tooltip.
• Player with login.avatar_id NULL but a legacy avatars.selected = 1 row → fetch returns the legacy avatar (backwards compatibility).
• Player with neither → fetch returns no avatar.
• Send {"command": "avatar", "action": "select", "avatar": <url>} → both avatars.selected = 1 AND login.avatar_id are updated to point at that avatar.
• Send {"command": "avatar", "action": "select", "avatar": null} → avatars.selected cleared AND login.avatar_id set to NULL.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved avatar selection reliability by persisting the chosen avatar and clearing it when removed, keeping it consistent across sessions.
  • Updated player avatar retrieval to prefer the newly stored avatar when available, while falling back to the legacy selection behavior for older accounts.
• Chores
  • Updated the database migration baseline used in CI and compose to ensure migrations run against the expected version.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bda0fe0c-a1be-4123-9641-abc911c536be

📥 Commits

Reviewing files that changed from the base of the PR and between c506bea and ae53aba.

📒 Files selected for processing (2)

• server/player_service.py
• tests/unit_tests/test_lobbyconnection.py

---

📝 Walkthrough

Walkthrough

A new avatar_id foreign-key column is added to the login table referencing avatars_list.id. The player data fetch query is updated to join avatars_list using conditional or_ logic that prefers login.avatar_id when set and falls back to the legacy selected == 1 flag when null. On avatar selection, command_avatar now also writes the chosen (or cleared) avatar_id back to t_login. Tests verify both selection and clearing behavior. Database migration tooling is updated to version v143 to support the schema change.

Changes

Authoritative avatar_id on login

Layer / File(s)	Summary
Schema column and player-fetch read query server/db/models.py, server/player_service.py	login table gains avatar_id FK to avatars_list.id. PlayerService imports or_, adds a comment documenting lookup precedence, and rewrites the avatars_list join to use or_ logic: prefer avatars.idAvatar == login.avatar_id when login.avatar_id is set; otherwise fall back to avatars.selected == 1 when null.
Avatar selection write to login.avatar_id server/lobbyconnection.py	command_avatar derives new_avatar_id from the selected avatar row (or None on deselect) and persists it to t_login.avatar_id for the current player.
Tests for avatar selection and clearing tests/unit_tests/test_lobbyconnection.py	test_command_avatar_select verifies that selecting an avatar stores the matching avatar_id in the login table. test_command_avatar_select_clear verifies that deselecting (avatar=None) clears login.avatar_id to None and removes all selected=1 flags from avatars.

Database migration version update

Layer / File(s)	Summary
DB migration version v138 → v143 .github/workflows/test.yml, compose.yaml	CI workflow and Docker Compose service now reference FAF database migrations version v143 to deploy the schema and migration changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A column was born in the login with care,
avatar_id gleaming, the new choice so fair.
When it's set, prefer it with logical flair,
when it's null, pick the flag that was there.
Or logic shines bright—hop hop everywhere! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: introducing the new login.avatar_id column and maintaining synchronization with the legacy avatars.selected during migration.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/login-avatar-id-sync

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

server/lobbyconnection.py (1)

976-984: ⚡ Quick win

Extend avatar selection tests to assert login.avatar_id synchronization.

At Line 978, the new authoritative column is written, but existing coverage (per tests/unit_tests/test_lobbyconnection.py snippet) only checks avatars.selected. Add assertions for login.avatar_id on both select and clear paths to lock the migration contract.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/lobbyconnection.py` around lines 976 - 984, The avatar selection logic
in the lobbyconnection module now synchronizes the authoritative login.avatar_id
column when avatars are selected. Update the tests in
tests/unit_tests/test_lobbyconnection.py to add assertions that verify
login.avatar_id is correctly synchronized on both the select avatar and clear
avatar (if applicable) paths. For each test case that exercises avatar
selection, assert that the login record's avatar_id matches the selected
avatar_id to ensure the migration contract between the legacy avatars.selected
flag and the new authoritative login.avatar_id column is properly maintained.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/player_service.py`:
- Around line 111-116: The outerjoin using coalesce with login.avatar_id does
not validate that the avatar is owned by the player. Add an EXISTS or join guard
condition to the outerjoin onclause in the coalesce block to ensure that
non-null login.avatar_id values are only matched to avatars_list rows when there
is a corresponding row where both avatars.idUser equals login.id and
avatars.idAvatar equals login.avatar_id, enforcing ownership validation
consistent with the legacy path validated in the join above it.

---

Nitpick comments:
In `@server/lobbyconnection.py`:
- Around line 976-984: The avatar selection logic in the lobbyconnection module
now synchronizes the authoritative login.avatar_id column when avatars are
selected. Update the tests in tests/unit_tests/test_lobbyconnection.py to add
assertions that verify login.avatar_id is correctly synchronized on both the
select avatar and clear avatar (if applicable) paths. For each test case that
exercises avatar selection, assert that the login record's avatar_id matches the
selected avatar_id to ensure the migration contract between the legacy
avatars.selected flag and the new authoritative login.avatar_id column is
properly maintained.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d92c8323-d992-4909-81a0-59f76f19d2a2

📥 Commits

Reviewing files that changed from the base of the PR and between 1b98cda and 7603cb3.

📒 Files selected for processing (3)

• server/db/models.py
• server/lobbyconnection.py
• server/player_service.py