diff --git a/common/COMPONENT_IMPROVEMENTS.md b/common/COMPONENT_IMPROVEMENTS.md index 1e53c67..b3037f6 100644 --- a/common/COMPONENT_IMPROVEMENTS.md +++ b/common/COMPONENT_IMPROVEMENTS.md @@ -32,7 +32,10 @@ All functions with mutable defaults (`attributes` and `children`) changed from ` What was fixed: `attributes: list[HTMLAttribute] = []` and `children: list[HTMLTag] | HTMLTag = []` are a classic Python gotcha — the default is shared across all callers and could silently corrupt state if ever mutated in place. Changed 8 functions (`Component`, `Popover`, `A`, `Button`, `Div`, `Input`, `Form`, `Icon`) to use the `None` sentinel pattern, preventing future bugs and eliminating linter warnings. -## Incomplete +### NameWithIcon dead code and untestable design +The `NameWithIcon()` function had a `platform` parameter that was immediately overwritten by `platform = None` and never used (dead code). The function mixed data lookup (database queries via IDs) with rendering, making it untestable. + +**Fix**: Refactored `NameWithIcon()` to follow the `LinkedPurchase` pattern — accepts model objects (`Game`, `Session`) instead of IDs. Extracted `_resolve_name_with_icon()` helper for testable computation logic (name resolution, platform extraction, link creation). Fixed bug where `platform` was not extracted when `session` parameter was passed. Removed dead `platform` parameter from the public API. Updated all 3 production call sites (already using model objects). Added 10 unit tests for `_resolve_name_with_icon()` covering session override, custom names, linkify behavior, platform resolution, and edge cases. Updated 6 integration tests to use model-based parameters. ### No tests Zero test coverage for the entire component system. diff --git a/common/components.py b/common/components.py index 148f0f7..514fd2c 100644 --- a/common/components.py +++ b/common/components.py @@ -271,30 +271,15 @@ def LinkedPurchase(purchase: Purchase) -> SafeText: def NameWithIcon( name: str = "", - platform: str = "", - game_id: int = 0, - session_id: int = 0, - purchase_id: int = 0, + game: Game | None = None, + session: Session | None = None, linkify: bool = True, emulated: bool = False, ) -> SafeText: - create_link = False - link = "" - platform = None - if (game_id != 0 or session_id != 0 or purchase_id != 0) and linkify: - create_link = True - if session_id: - session = Session.objects.get(pk=session_id) - emulated = session.emulated - game_id = session.game.pk - if purchase_id: - purchase = Purchase.objects.get(pk=purchase_id) - game_id = purchase.games.first().pk - if game_id: - game = Game.objects.get(pk=game_id) - name = name or game.name - platform = game.platform - link = reverse("view_game", args=[int(game_id)]) + _name, platform, final_emulated, create_link, link = _resolve_name_with_icon( + name, game, session, linkify + ) + content = Div( [("class", "inline-flex gap-2 items-center")], [ @@ -304,8 +289,8 @@ def NameWithIcon( ) if platform else "", - Icon("emulated", [("title", "Emulated")]) if emulated else "", - PopoverTruncated(name), + Icon("emulated", [("title", "Emulated")]) if final_emulated else "", + PopoverTruncated(_name), ], ) @@ -319,6 +304,35 @@ def NameWithIcon( ) +def _resolve_name_with_icon( + name: str, + game: Game | None, + session: Session | None, + linkify: bool, +) -> tuple[str, Any, bool, bool, str]: + create_link = False + link = "" + platform = None + final_emulated = False + + if session is not None: + game = session.game + platform = game.platform + final_emulated = session.emulated + if linkify: + create_link = True + link = reverse("view_game", args=[int(game.pk)]) + elif game is not None: + platform = game.platform + if linkify: + create_link = True + link = reverse("view_game", args=[int(game.pk)]) + + _name = name or (game.name if game else "") + + return _name, platform, final_emulated, create_link, link + + def PurchasePrice(purchase) -> SafeText: return Popover( popover_content=f"{floatformat(purchase.price)} {purchase.price_currency}", diff --git a/games/views/game.py b/games/views/game.py index 3cb8fb5..873106b 100644 --- a/games/views/game.py +++ b/games/views/game.py @@ -104,7 +104,7 @@ def list_games(request: HttpRequest, search_string: str = "") -> HttpResponse: ], "rows": [ [ - NameWithIcon(game_id=game.pk), + NameWithIcon(game=game), PopoverTruncated( game.sort_name if game.sort_name is not None and game.name != game.sort_name @@ -308,9 +308,7 @@ def view_game(request: HttpRequest, game_id: int) -> HttpResponse: "columns": ["Game", "Date", "Duration", "Actions"], "rows": [ [ - NameWithIcon( - session_id=session.pk, - ), + NameWithIcon(session=session), f"{local_strftime(session.timestamp_start)}{f' — {local_strftime(session.timestamp_end, timeformat)}' if session.timestamp_end else ''}", session.duration_formatted_with_mark, render_to_string( diff --git a/games/views/session.py b/games/views/session.py index 1aba2b0..a340c31 100644 --- a/games/views/session.py +++ b/games/views/session.py @@ -131,7 +131,7 @@ def list_sessions(request: HttpRequest, search_string: str = "") -> HttpResponse "hx_select": f"#session-row-{session.pk}", "hx_swap": "outerHTML", "cell_data": [ - NameWithIcon(session_id=session.pk), + NameWithIcon(session=session), f"{local_strftime(session.timestamp_start)}{f' — {local_strftime(session.timestamp_end, timeformat)}' if session.timestamp_end else ''}", session.duration_formatted_with_mark, render_to_string( diff --git a/tests/test_components.py b/tests/test_components.py index b8999e0..b33ee97 100644 --- a/tests/test_components.py +++ b/tests/test_components.py @@ -1,5 +1,6 @@ import unittest from functools import lru_cache +from unittest.mock import MagicMock, patch import django @@ -405,13 +406,13 @@ class ComponentReturnTypeTest(unittest.TestCase): self.assertIn("text-white bg-brand", result) def test_name_with_icon_no_link(self): - result = components.NameWithIcon(name="Game", platform="Steam", linkify=False) + result = components.NameWithIcon(name="Game", linkify=False) self.assertIsInstance(result, SafeText) self.assertIn("Game", result) self.assertNotIn("