From c75e2781be3d0d3725e5f904d6462108e0b70d84 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Mon, 6 Apr 2026 20:02:58 -0500 Subject: [PATCH] fix: address review feedback (#187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move lazy imports to top level in card_storage.py and players.py (CLAUDE.md violation) - Use os.environ.get() for S3_BUCKET/S3_REGION to allow dev/prod bucket separation - Fix test patch targets from app.db_engine to app.services.card_storage (required after top-level import move) - Fix assert_called_once_with field name: MockBatting.player → MockBatting.player_id Co-Authored-By: Claude Sonnet 4.6 --- app/routers_v2/players.py | 3 +-- app/services/card_storage.py | 13 +++++-------- tests/test_card_storage.py | 16 ++++++++-------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/app/routers_v2/players.py b/app/routers_v2/players.py index 744bad6..6579aae 100644 --- a/app/routers_v2/players.py +++ b/app/routers_v2/players.py @@ -40,6 +40,7 @@ from ..db_engine import ( ) from ..db_helpers import upsert_players from ..dependencies import oauth2_scheme, valid_token +from ..services.card_storage import backfill_variant_image_url from ..services.refractor_boost import compute_variant_hash # --------------------------------------------------------------------------- @@ -925,8 +926,6 @@ async def get_batter_card( (CardModel.player_id == player_id) & (CardModel.variant == variant) ) if card_row.image_url is None: - from app.services.card_storage import backfill_variant_image_url - background_tasks.add_task( backfill_variant_image_url, player_id=player_id, diff --git a/app/services/card_storage.py b/app/services/card_storage.py index 7ad4513..1d68962 100644 --- a/app/services/card_storage.py +++ b/app/services/card_storage.py @@ -23,9 +23,6 @@ backfill_variant_image_url(player_id, variant, card_type, cardset_id, png_path) Design notes ------------ -- backfill_variant_image_url uses a lazy import for BattingCard/PitchingCard - to avoid circular imports at module load time (db_engine imports routers - indirectly via the app module in some paths). - S3 credentials are resolved from the environment by boto3 at call time; no credentials are hard-coded here. - The cache-bust ?d= param matches the card-creation pipeline convention so @@ -33,14 +30,17 @@ Design notes """ import logging +import os from datetime import date import boto3 +from app.db_engine import BattingCard, PitchingCard + logger = logging.getLogger(__name__) -S3_BUCKET = "paper-dynasty" -S3_REGION = "us-east-1" +S3_BUCKET = os.environ.get("S3_BUCKET", "paper-dynasty") +S3_REGION = os.environ.get("S3_REGION", "us-east-1") def get_s3_client(): @@ -150,9 +150,6 @@ def backfill_variant_image_url( Returns: None """ - # Lazy import to avoid circular imports at module load time - from app.db_engine import BattingCard, PitchingCard - try: # 1. Read PNG from disk with open(png_path, "rb") as f: diff --git a/tests/test_card_storage.py b/tests/test_card_storage.py index 6fe528e..4f26d63 100644 --- a/tests/test_card_storage.py +++ b/tests/test_card_storage.py @@ -207,7 +207,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard") as MockBatting, + patch("app.services.card_storage.BattingCard") as MockBatting, ): MockBatting.get.return_value = mock_card @@ -220,7 +220,7 @@ class TestBackfillVariantImageUrl: ) MockBatting.get.assert_called_once_with( - MockBatting.player == 42, MockBatting.variant == 1 + MockBatting.player_id == 42, MockBatting.variant == 1 ) assert mock_card.image_url is not None mock_card.save.assert_called_once() @@ -235,7 +235,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.PitchingCard") as MockPitching, + patch("app.services.card_storage.PitchingCard") as MockPitching, ): MockPitching.get.return_value = mock_card @@ -248,7 +248,7 @@ class TestBackfillVariantImageUrl: ) MockPitching.get.assert_called_once_with( - MockPitching.player == 99, MockPitching.variant == 2 + MockPitching.player_id == 99, MockPitching.variant == 2 ) assert mock_card.image_url is not None mock_card.save.assert_called_once() @@ -263,7 +263,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard") as MockBatting, + patch("app.services.card_storage.BattingCard") as MockBatting, ): MockBatting.get.return_value = MagicMock() @@ -293,7 +293,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard"), + patch("app.services.card_storage.BattingCard"), ): # Must not raise backfill_variant_image_url( @@ -314,7 +314,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client"), - patch("app.db_engine.BattingCard"), + patch("app.services.card_storage.BattingCard"), ): # Must not raise backfill_variant_image_url( @@ -336,7 +336,7 @@ class TestBackfillVariantImageUrl: with ( patch("app.services.card_storage.get_s3_client", return_value=mock_s3), - patch("app.db_engine.BattingCard") as MockBatting, + patch("app.services.card_storage.BattingCard") as MockBatting, ): MockBatting.get.return_value = mock_card