Merge pull request 'fix(dev_tools): use unified cards/{id} endpoint for /dev refractor-test' (#168) from fix/refractor-test-unified-cards-lookup into main

cogs/dev_tools.py

@@ -11,9 +11,10 @@ import discord
 from discord import app_commands
 from discord.ext import commands
 
-from api_calls import db_delete, db_get, db_post
+import aiohttp
+
+from api_calls import AUTH_TOKEN, db_delete, db_get, db_post, get_req_url
 from helpers.constants import PD_SEASON
-from helpers.main import get_team_by_owner
 from helpers.refractor_constants import TIER_NAMES
 from helpers.refractor_test_data import (
     build_batter_plays,
@@ -25,7 +26,9 @@ from helpers.refractor_test_data import (
 
 CURRENT_SEASON = PD_SEASON
 
-logger = logging.getLogger(__name__)
+SENTINEL_PITCHER_ID = 3
+
+logger = logging.getLogger("discord_app")
 
 
 class CleanupView(discord.ui.View):
@@ -105,36 +108,45 @@ class DevToolsCog(commands.Cog):
     @group_dev.command(
         name="refractor-test", description="Run refractor integration test on a card"
     )
-    @app_commands.describe(card_id="The batting or pitching card ID to test")
+    @app_commands.describe(
+        card_id="Card-instance ID (from the unified cards table; discoverable via /refractor status)"
+    )
     async def refractor_test(self, interaction: discord.Interaction, card_id: int):
         await interaction.response.defer()
 
         # --- Phase 1: Setup ---
-        # Look up card (try batting first, then pitching)
-        card = await db_get("battingcards", object_id=card_id)
-        card_type_key = "batting"
-        if card is None:
-            card = await db_get("pitchingcards", object_id=card_id)
-            card_type_key = "pitching"
-
+        # Look up card via the unified cards endpoint. cards.id is
+        # globally unique across all card instances, so there's no
+        # batting/pitching template-ID collision (spec
+        # 2026-04-11-refractor-test-unified-cards-lookup-design.md).
+        card = await db_get("cards", object_id=card_id)
         if card is None:
             await interaction.edit_original_response(
-                content=f"❌ Card #{card_id} not found (checked batting and pitching)."
+                content=f"❌ Card #{card_id} not found."
             )
             return
 
-        player_id = card["player"]["id"]
+        player_id = card["player"]["player_id"]
         player_name = card["player"]["p_name"]
-        team_id = card.get("team_id") or card["player"].get("team_id")
 
+        # The card instance's owning team is the authoritative context
+        # for this test — its refractor state will be read and synthetic
+        # plays will be credited against it.
+        team_id = (card.get("team") or {}).get("id")
         if team_id is None:
-            team = await get_team_by_owner(interaction.user.id)
-            if team is None:
-                await interaction.edit_original_response(
-                    content="❌ Could not determine team ID. You must own a team."
-                )
-                return
-            team_id = team["id"]
+            await interaction.edit_original_response(
+                content=f"❌ Card #{card_id} has no owning team."
+            )
+            return
+
+        # Derive card type from the player's primary position.
+        pos_1 = (card["player"].get("pos_1") or "").upper()
+        if pos_1 == "SP":
+            card_type_key, card_type_default = "pitching", "sp"
+        elif pos_1 in ("RP", "CP"):
+            card_type_key, card_type_default = "pitching", "rp"
+        else:
+            card_type_key, card_type_default = "batting", "batter"
 
         # Fetch refractor state
         refractor_data = await db_get(
@@ -142,6 +154,17 @@ class DevToolsCog(commands.Cog):
             params=[("team_id", team_id), ("limit", 100)],
         )
 
+        logger.info(
+            "dev_tools state-lookup: team_id=%s player_id=%s resp_keys=%s item_count=%s item_pids=%s",
+            team_id,
+            player_id,
+            list(refractor_data.keys()) if refractor_data else None,
+            len(refractor_data.get("items", [])) if refractor_data else 0,
+            [i.get("player_id") for i in refractor_data.get("items", [])]
+            if refractor_data
+            else [],
+        )
+
         # Find this player's entry
         card_state = None
         if refractor_data and refractor_data.get("items"):
@@ -150,6 +173,12 @@ class DevToolsCog(commands.Cog):
                     card_state = item
                     break
 
+        logger.info(
+            "dev_tools state-lookup matched: player_id=%s card_state=%s",
+            player_id,
+            card_state,
+        )
+
         # Determine current state and thresholds
         if card_state:
             current_tier = card_state["current_tier"]
@@ -159,7 +188,7 @@ class DevToolsCog(commands.Cog):
         else:
             current_tier = 0
             current_value = 0
-            card_type = "batter" if card_type_key == "batting" else "sp"
+            card_type = card_type_default
             next_threshold = (
                 37 if card_type == "batter" else (10 if card_type == "sp" else 3)
             )
@@ -174,25 +203,7 @@ class DevToolsCog(commands.Cog):
         gap = max(0, next_threshold - current_value)
         plan = calculate_plays_needed(gap, card_type)
 
-        # Find an opposing player
-        if card_type == "batter":
-            opposing_cards = await db_get(
-                "pitchingcards",
-                params=[("team_id", team_id), ("variant", 0)],
-            )
-        else:
-            opposing_cards = await db_get(
-                "battingcards",
-                params=[("team_id", team_id), ("variant", 0)],
-            )
-
-        if not opposing_cards or not opposing_cards.get("cards"):
-            await interaction.edit_original_response(
-                content=f"❌ No opposing {'pitcher' if card_type == 'batter' else 'batter'} cards found on team {team_id}."
-            )
-            return
-
-        opposing_player_id = opposing_cards["cards"][0]["player"]["id"]
+        opposing_player_id = SENTINEL_PITCHER_ID
 
         # Build and send initial embed
         tier_name = TIER_NAMES.get(current_tier, f"T{current_tier}")
@@ -357,27 +368,19 @@ class DevToolsCog(commands.Cog):
                     continue
                 try:
                     today = date.today().isoformat()
-                    render_resp = await db_get(
+                    render_url = get_req_url(
                         f"players/{tu['player_id']}/{card_type_key}card/{today}/{variant}",
-                        none_okay=True,
+                        api_ver=2,
                     )
-                    if render_resp:
-                        results.append("✅ Card rendered + S3 upload triggered")
-                        img_url = (
-                            render_resp
-                            if isinstance(render_resp, str)
-                            else render_resp.get("image_url")
-                        )
-                        if (
-                            img_url
-                            and isinstance(img_url, str)
-                            and img_url.startswith("http")
-                        ):
-                            embed.set_image(url=img_url)
-                    else:
-                        results.append(
-                            "⚠️ Card render returned no data (may still be processing)"
-                        )
+                    async with aiohttp.ClientSession(headers=AUTH_TOKEN) as sess:
+                        async with sess.get(render_url) as r:
+                            if r.status == 200:
+                                results.append("✅ Card rendered + S3 upload triggered")
+                            else:
+                                body = await r.text()
+                                results.append(
+                                    f"⚠️ Card render non-200 ({r.status}): {body[:80]}"
+                                )
                 except Exception as e:
                     results.append(f"⚠️ Card render failed (non-fatal): {e}")
 

tests/test_dev_tools.py

@@ -100,13 +100,34 @@ class TestRefractorTestSetup:
         return MagicMock(spec=commands.Bot)
 
     @pytest.fixture
-    def batting_card_response(self):
-        return {
-            "id": 1234,
-            "player": {"id": 100, "p_name": "Mike Trout"},
-            "variant": 0,
-            "image_url": None,
-        }
+    def unified_card_response(self):
+        """Factory for a response from GET /v2/cards/{id}.
+
+        The unified cards endpoint returns a card-instance record with
+        top-level player, team, pack, value, and variant fields. This
+        replaces the separate battingcards/pitchingcards template endpoints
+        that previously caused ID collisions (see spec
+        2026-04-11-refractor-test-unified-cards-lookup-design.md).
+
+        The factory lets each test customize pos_1 and the IDs without
+        duplicating the full response shape.
+        """
+
+        def _make(pos_1="CF", player_id=100, team_id=31, card_id=1234):
+            return {
+                "id": card_id,
+                "player": {
+                    "player_id": player_id,
+                    "p_name": "Mike Trout",
+                    "pos_1": pos_1,
+                },
+                "team": {"id": team_id},
+                "variant": 0,
+                "pack": None,
+                "value": None,
+            }
+
+        return _make
 
     @pytest.fixture
     def refractor_cards_response(self):
@@ -134,112 +155,212 @@ class TestRefractorTestSetup:
             ],
         }
 
-    @pytest.fixture
-    def opposing_cards_response(self):
-        """A valid pitching cards response with the 'cards' key."""
-        return {
-            "cards": [
-                {
-                    "id": 9000,
-                    "player": {"id": 200, "p_name": "Clayton Kershaw"},
-                    "variant": 0,
-                }
-            ]
-        }
-
-    async def test_batting_card_lookup(
+    async def test_unified_card_lookup(
         self,
         mock_interaction,
         mock_bot,
-        batting_card_response,
+        unified_card_response,
         refractor_cards_response,
-        opposing_cards_response,
     ):
-        """Command should try the batting card endpoint first.
+        """The setup phase should make a single db_get call targeting
+        the unified 'cards' endpoint.
 
-        Verifies that the first db_get call targets 'battingcards', not
-        'pitchingcards', when looking up a card ID.
+        Regression guard for the previous two-step battingcards/pitchingcards
+        fallback that caused ID collisions (e.g. card 494 resolving to
+        Cameron Maybin instead of the intended pitcher Grayson Rodriguez).
         """
         from cogs.dev_tools import DevToolsCog
 
         cog = DevToolsCog(mock_bot)
-        with (
-            patch("cogs.dev_tools.db_get", new_callable=AsyncMock) as mock_get,
-            patch("cogs.dev_tools.db_post", new_callable=AsyncMock),
-        ):
-            mock_get.side_effect = [
-                batting_card_response,  # GET battingcards/{id}
-                refractor_cards_response,  # GET refractor/cards
-                opposing_cards_response,  # GET pitchingcards (for opposing player)
-            ]
-            with patch.object(cog, "_execute_refractor_test", new_callable=AsyncMock):
-                await cog.refractor_test.callback(cog, mock_interaction, card_id=1234)
-            first_call = mock_get.call_args_list[0]
-            assert "battingcards" in str(first_call)
-
-    async def test_pitching_card_fallback(
-        self,
-        mock_interaction,
-        mock_bot,
-        refractor_cards_response,
-    ):
-        """If batting card returns None, command should fall back to pitching card.
-
-        Ensures the two-step lookup: batting first, then pitching if batting
-        returns None. The second db_get call must target 'pitchingcards'.
-        """
-        from cogs.dev_tools import DevToolsCog
-
-        cog = DevToolsCog(mock_bot)
-        pitching_card = {
-            "id": 5678,
-            "player": {"id": 200, "p_name": "Clayton Kershaw"},
-            "variant": 0,
-            "image_url": None,
-        }
-        refractor_cards_response["items"][0]["player_id"] = 200
-        refractor_cards_response["items"][0]["track"]["card_type"] = "sp"
-        refractor_cards_response["items"][0]["next_threshold"] = 10
-
-        opposing_batters = {
-            "cards": [
-                {"id": 7000, "player": {"id": 300, "p_name": "Babe Ruth"}, "variant": 0}
-            ]
-        }
+        card = unified_card_response(pos_1="CF", player_id=100, card_id=1234)
 
         with (
             patch("cogs.dev_tools.db_get", new_callable=AsyncMock) as mock_get,
             patch("cogs.dev_tools.db_post", new_callable=AsyncMock),
         ):
-            mock_get.side_effect = [
-                None,  # batting card not found
-                pitching_card,  # pitching card found
-                refractor_cards_response,  # refractor/cards
-                opposing_batters,  # battingcards for opposing player
-            ]
+            mock_get.side_effect = [card, refractor_cards_response]
             with patch.object(cog, "_execute_refractor_test", new_callable=AsyncMock):
-                await cog.refractor_test.callback(cog, mock_interaction, card_id=5678)
-            second_call = mock_get.call_args_list[1]
-            assert "pitchingcards" in str(second_call)
+                await cog.refractor_test.callback(cog, mock_interaction, 1234)
+
+        # First call: the unified cards endpoint
+        first_call = mock_get.call_args_list[0]
+        assert first_call.args[0] == "cards"
+        assert first_call.kwargs.get("object_id") == 1234
+
+        # No secondary template-table fallback
+        endpoints_called = [c.args[0] for c in mock_get.call_args_list]
+        assert "battingcards" not in endpoints_called
+        assert "pitchingcards" not in endpoints_called
 
     async def test_card_not_found_reports_error(self, mock_interaction, mock_bot):
-        """If neither batting nor pitching card exists, report an error and return.
-
-        The command should call edit_original_response with a message containing
-        'not found' and must NOT call _execute_refractor_test.
+        """If cards/{id} returns None, report 'not found' and never call
+        _execute_refractor_test. The single unified endpoint means only
+        one db_get is made before the error path.
         """
         from cogs.dev_tools import DevToolsCog
 
         cog = DevToolsCog(mock_bot)
-        with patch("cogs.dev_tools.db_get", new_callable=AsyncMock, return_value=None):
+        with patch(
+            "cogs.dev_tools.db_get", new_callable=AsyncMock, return_value=None
+        ) as mock_get:
             with patch.object(
                 cog, "_execute_refractor_test", new_callable=AsyncMock
             ) as mock_exec:
-                await cog.refractor_test.callback(cog, mock_interaction, card_id=9999)
+                await cog.refractor_test.callback(cog, mock_interaction, 9999)
                 mock_exec.assert_not_called()
+
+        # Exactly one db_get call — the unified lookup, no template fallback
+        assert mock_get.call_count == 1
         call_kwargs = mock_interaction.edit_original_response.call_args[1]
         assert "not found" in call_kwargs["content"].lower()
 
+    async def test_pos_sp_derives_sp_type(
+        self,
+        mock_interaction,
+        mock_bot,
+        unified_card_response,
+        refractor_cards_response,
+    ):
+        """pos_1='SP' should derive card_type='sp', card_type_key='pitching'
+        and pass those into _execute_refractor_test.
+        """
+        from cogs.dev_tools import DevToolsCog
+
+        cog = DevToolsCog(mock_bot)
+        card = unified_card_response(pos_1="SP", player_id=200)
+        # Make sure the refractor/cards lookup finds no matching entry,
+        # so the command falls through to the pos_1-derived defaults.
+        refractor_cards_response["items"] = []
+
+        with (
+            patch("cogs.dev_tools.db_get", new_callable=AsyncMock) as mock_get,
+            patch("cogs.dev_tools.db_post", new_callable=AsyncMock),
+            patch.object(
+                cog, "_execute_refractor_test", new_callable=AsyncMock
+            ) as mock_exec,
+        ):
+            mock_get.side_effect = [card, refractor_cards_response]
+            await cog.refractor_test.callback(cog, mock_interaction, 1234)
+
+        kwargs = mock_exec.call_args.kwargs
+        assert kwargs["card_type"] == "sp"
+        assert kwargs["card_type_key"] == "pitching"
+
+    async def test_pos_rp_derives_rp_type(
+        self,
+        mock_interaction,
+        mock_bot,
+        unified_card_response,
+        refractor_cards_response,
+    ):
+        """pos_1='RP' should derive card_type='rp', card_type_key='pitching'."""
+        from cogs.dev_tools import DevToolsCog
+
+        cog = DevToolsCog(mock_bot)
+        card = unified_card_response(pos_1="RP", player_id=201)
+        refractor_cards_response["items"] = []
+
+        with (
+            patch("cogs.dev_tools.db_get", new_callable=AsyncMock) as mock_get,
+            patch("cogs.dev_tools.db_post", new_callable=AsyncMock),
+            patch.object(
+                cog, "_execute_refractor_test", new_callable=AsyncMock
+            ) as mock_exec,
+        ):
+            mock_get.side_effect = [card, refractor_cards_response]
+            await cog.refractor_test.callback(cog, mock_interaction, 1234)
+
+        kwargs = mock_exec.call_args.kwargs
+        assert kwargs["card_type"] == "rp"
+        assert kwargs["card_type_key"] == "pitching"
+
+    async def test_pos_cp_derives_rp_type(
+        self,
+        mock_interaction,
+        mock_bot,
+        unified_card_response,
+        refractor_cards_response,
+    ):
+        """pos_1='CP' (closer) should also map to card_type='rp'."""
+        from cogs.dev_tools import DevToolsCog
+
+        cog = DevToolsCog(mock_bot)
+        card = unified_card_response(pos_1="CP", player_id=202)
+        refractor_cards_response["items"] = []
+
+        with (
+            patch("cogs.dev_tools.db_get", new_callable=AsyncMock) as mock_get,
+            patch("cogs.dev_tools.db_post", new_callable=AsyncMock),
+            patch.object(
+                cog, "_execute_refractor_test", new_callable=AsyncMock
+            ) as mock_exec,
+        ):
+            mock_get.side_effect = [card, refractor_cards_response]
+            await cog.refractor_test.callback(cog, mock_interaction, 1234)
+
+        kwargs = mock_exec.call_args.kwargs
+        assert kwargs["card_type"] == "rp"
+        assert kwargs["card_type_key"] == "pitching"
+
+    async def test_pos_batter_derives_batter_type(
+        self,
+        mock_interaction,
+        mock_bot,
+        unified_card_response,
+        refractor_cards_response,
+    ):
+        """pos_1='CF' (or any non-pitcher position) should derive
+        card_type='batter', card_type_key='batting'.
+        """
+        from cogs.dev_tools import DevToolsCog
+
+        cog = DevToolsCog(mock_bot)
+        card = unified_card_response(pos_1="CF", player_id=203)
+        refractor_cards_response["items"] = []
+
+        with (
+            patch("cogs.dev_tools.db_get", new_callable=AsyncMock) as mock_get,
+            patch("cogs.dev_tools.db_post", new_callable=AsyncMock),
+            patch.object(
+                cog, "_execute_refractor_test", new_callable=AsyncMock
+            ) as mock_exec,
+        ):
+            mock_get.side_effect = [card, refractor_cards_response]
+            await cog.refractor_test.callback(cog, mock_interaction, 1234)
+
+        kwargs = mock_exec.call_args.kwargs
+        assert kwargs["card_type"] == "batter"
+        assert kwargs["card_type_key"] == "batting"
+
+    async def test_card_without_team_reports_error(
+        self,
+        mock_interaction,
+        mock_bot,
+        unified_card_response,
+    ):
+        """If the unified card response has team=None, the command should
+        report an error and not proceed to execute the refractor chain.
+
+        The card instance's owning team is now the authoritative team
+        context for the test (see spec — option A: card's team is
+        authoritative, no get_team_by_owner fallback).
+        """
+        from cogs.dev_tools import DevToolsCog
+
+        cog = DevToolsCog(mock_bot)
+        card = unified_card_response(pos_1="CF", player_id=100)
+        card["team"] = None
+
+        with patch("cogs.dev_tools.db_get", new_callable=AsyncMock, return_value=card):
+            with patch.object(
+                cog, "_execute_refractor_test", new_callable=AsyncMock
+            ) as mock_exec:
+                await cog.refractor_test.callback(cog, mock_interaction, 1234)
+                mock_exec.assert_not_called()
+
+        call_kwargs = mock_interaction.edit_original_response.call_args[1]
+        assert "no owning team" in call_kwargs["content"].lower()
+
 
 class TestRefractorTestExecute:
     """Test the execution phase: API calls, step-by-step reporting,