Reconcile legacy avatars.selected on avatar update events

[Brutus5000]

Problem

Clearing one's avatar via the API doesn't stick. The API sets login.avatar_id = NULL and publishes success.player_avatar.update (verified: Elide fires the update lifecycle hook on the relationship DELETE too). The lobby consumes the event and refreshes — but _avatar_grant_join_onclause falls back to the legacy avatars.selected = 1 row when login.avatar_id IS NULL, so the just-cleared avatar gets resurrected from the stale legacy flag.

Observed:

api:    Avatar 'null' has been selected on player '76365'
lobby:  Player 76365 avatar refreshed ... FAF_Developer.png   # legacy fallback

Root cause: login.avatar_id = NULL is ambiguous — it means both "never migrated" and "explicitly cleared" — and the fallback always resolves it toward the legacy flag.

Fix

An avatar-update event means the API (the authoritative writer of login.avatar_id) just changed this player's selection. So refresh_player_avatar now reconciles avatars.selected with login.avatar_id before re-reading: the granted row matching avatar_id is marked selected, all others deselected (all deselected when avatar_id is null).

• Clear → avatar_id null → legacy flags cleared → no avatar (sticks).
• Set → matching row selected, others cleared → lazily migrates the legacy flag for that player.
• Login (fetch_player_data) is unchanged, so players who haven't touched the new system still get their legacy-selected avatar via the fallback until their first update event.

Tests

• test_refresh_player_avatar_connected — set case: authoritative avatar_id wins and the legacy flag is reconciled to it.
• test_refresh_player_avatar_clears_legacy_fallback — clear case: no avatar, legacy flags cleaned.
• Full test_player_service.py + test_avatar_change_queue_service.py pass (24 tests) against a v144 schema.

Summary by CodeRabbit

• Bug Fixes

  • Refreshing a player’s avatar now reliably matches the account’s saved (authoritative) avatar, preventing mismatches after refresh.
  • When an account has no avatar set, any previously stored “selected” flags are fully cleared so the wrong avatar no longer reappears.
  • Improved handling of legacy selection data to ensure consistent avatar display.

• Tests

  • Expanded avatar refresh coverage to verify both authoritative syncing and legacy cleanup behavior.

[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: 3b491586-2a59-4017-8a62-746b05907c67

📥 Commits

Reviewing files that changed from the base of the PR and between 32be0a4 and ea22b2b.

📒 Files selected for processing (1)

• tests/unit_tests/test_player_service.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/unit_tests/test_player_service.py

---

📝 Walkthrough

Walkthrough

PlayerService now reconciles legacy avatars.selected state against login.avatar_id during avatar refresh. The refresh path updates the database before reloading the player avatar, and tests now verify both the authoritative-avatar and null-avatar cases with direct database assertions.

Changes

Legacy Avatar Selection Reconciliation

Layer / File(s)	Summary
_sync_legacy_avatar_selection helper and refresh integration server/player_service.py	Adds func import, implements the helper to update avatars.selected from login.avatar_id, and calls it in refresh_player_avatar before reading avatar data.
Tests for sync and legacy fallback clearing tests/unit_tests/test_player_service.py	Adds select, avatars, and login imports; updates avatar refresh tests to assert the refreshed avatar matches login.avatar_id, reconcile avatars.selected, and clear all legacy flags when login.avatar_id is null.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• FAForever/server#1095: Modifies the same refresh_player_avatar and avatars.selected reconciliation path with related fallback behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.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 matches the main change: reconciling legacy avatars.selected during avatar update events.
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.

✨ 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 fix/reconcile-legacy-avatar-flag

---

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.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@tests/unit_tests/test_player_service.py`:
- Around line 111-115: The legacy-avatar clear-path assertion in the player
service test is vacuous because all(...) will pass on an empty result. Update
the test around the _db.acquire() query for avatars.c.selected and
avatars.c.idUser == 50 to first assert that result contains at least one row,
then verify all returned rows are not selected so the test fails if no legacy
avatar rows exist.

🪄 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: e6d2f9b5-0ebf-4370-82ae-954fe05d3ebd

📥 Commits

Reviewing files that changed from the base of the PR and between ac6d11d and 32be0a4.

📒 Files selected for processing (2)

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