diff --git a/server/player_service.py b/server/player_service.py index a0f28844a..7a7a568d4 100644 --- a/server/player_service.py +++ b/server/player_service.py @@ -8,7 +8,7 @@ from typing import TYPE_CHECKING, ClassVar, Optional, ValuesView import aiocron -from sqlalchemy import and_, or_, select +from sqlalchemy import and_, func, or_, select import server.metrics as metrics from server.config import config @@ -178,6 +178,28 @@ async def _fetch_player_avatar( player.avatar = None return None + async def _sync_legacy_avatar_selection(self, player_id: int, conn) -> None: + """ + Reconcile the deprecated `avatars.selected` flag with the authoritative + `login.avatar_id`: mark only the granted row matching `avatar_id` as + selected (all of them deselected when `avatar_id` is null). This keeps the + legacy fallback consistent so an explicit clear is not resurrected. + """ + authoritative_avatar_id = ( + select(login.c.avatar_id) + .where(login.c.id == player_id) + .scalar_subquery() + ) + await conn.execute( + avatars.update() + .where(avatars.c.idUser == player_id) + .values( + selected=func.coalesce( + avatars.c.idAvatar == authoritative_avatar_id, False + ) + ) + ) + async def refresh_player_avatar(self, player_id: int) -> bool: """ Re-read avatar for one player and mark them dirty. @@ -189,6 +211,12 @@ async def refresh_player_avatar(self, player_id: int) -> bool: if player is None: return False async with self._db.acquire() as conn: + # An avatar-update event means the API (the authoritative writer of + # `login.avatar_id`) changed this player's selection, so reconcile the + # deprecated `avatars.selected` flag before reading. Otherwise a cleared + # avatar (`avatar_id` set to null) would be undone by the legacy + # fallback in `_avatar_grant_join_onclause`. + await self._sync_legacy_avatar_selection(player_id, conn) avatar_id = await self._fetch_player_avatar(player, conn) avatar_tooltip = player.avatar["tooltip"] if player.avatar else None self._logger.info( diff --git a/tests/unit_tests/test_player_service.py b/tests/unit_tests/test_player_service.py index b46c63950..c2ae42ce0 100644 --- a/tests/unit_tests/test_player_service.py +++ b/tests/unit_tests/test_player_service.py @@ -1,5 +1,8 @@ from unittest import mock +from sqlalchemy import select + +from server.db.models import avatars, login from server.rating import RatingType @@ -67,18 +70,49 @@ async def test_fetch_player_data_non_existent(player_factory, player_service): async def test_refresh_player_avatar_connected( player_factory, player_service ): - player = player_factory(player_id=50) + # Player 51 owns avatars 1 (QAI) and 2 (UEF); make 1 the authoritative + # selection via login.avatar_id while the legacy flag still points at 2. + player = player_factory(player_id=51) player.avatar = None # simulate stale (e.g. just connected) - player_service[50] = player + player_service[51] = player + async with player_service._db.acquire() as conn: + await conn.execute(login.update().where(login.c.id == 51).values(avatar_id=1)) - refreshed = await player_service.refresh_player_avatar(50) + refreshed = await player_service.refresh_player_avatar(51) assert refreshed is True assert player.avatar == { - "url": "https://content.faforever.com/faf/avatars/UEF.png", - "tooltip": "UEF", + "url": "https://content.faforever.com/faf/avatars/qai2.png", + "tooltip": "QAI", } assert player in player_service._dirty_players + # the legacy `selected` flag is reconciled to the authoritative avatar + async with player_service._db.acquire() as conn: + result = await conn.execute( + select(avatars.c.idAvatar, avatars.c.selected).where(avatars.c.idUser == 51) + ) + selected = {row.idAvatar: bool(row.selected) for row in result} + assert selected == {1: True, 2: False} + + +async def test_refresh_player_avatar_clears_legacy_fallback( + player_factory, player_service +): + # Player 50 has a legacy selected avatar but no authoritative login.avatar_id, + # which represents an explicit clear via the API. The refresh must not let the + # legacy fallback resurrect it, and must clean the flag up. + player = player_factory(player_id=50) + player_service[50] = player + + refreshed = await player_service.refresh_player_avatar(50) + + assert refreshed is True + assert player.avatar is None + async with player_service._db.acquire() as conn: + result = await conn.execute( + select(avatars.c.selected).where(avatars.c.idUser == 50) + ) + assert all(not row.selected for row in result) async def test_refresh_player_avatar_not_connected(player_service):