From 8ee35e564cbf6d45dec697c9803c48c5953a96e5 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Sun, 8 Mar 2026 11:37:39 -0500 Subject: [PATCH] fix: prefix trade validation errors with team abbreviation Roster-level errors, warnings, and suggestions now display as "[BSG] Too many ML players" so users know which team each issue applies to. Co-Authored-By: Claude Opus 4.6 --- services/trade_builder.py | 20 +++++-- tests/test_services_trade_builder.py | 85 ++++++++++++++-------------- 2 files changed, 56 insertions(+), 49 deletions(-) diff --git a/services/trade_builder.py b/services/trade_builder.py index 879959f..36f6a1a 100644 --- a/services/trade_builder.py +++ b/services/trade_builder.py @@ -24,6 +24,7 @@ class TradeValidationResult: def __init__(self): self.is_legal: bool = True self.participant_validations: Dict[int, RosterValidationResult] = {} + self.team_abbrevs: Dict[int, str] = {} # team_id -> abbreviation self.trade_errors: List[str] = [] self.trade_warnings: List[str] = [] self.trade_suggestions: List[str] = [] @@ -32,24 +33,30 @@ class TradeValidationResult: def all_errors(self) -> List[str]: """Get all errors including trade-level and roster-level errors.""" errors = self.trade_errors.copy() - for validation in self.participant_validations.values(): - errors.extend(validation.errors) + for team_id, validation in self.participant_validations.items(): + abbrev = self.team_abbrevs.get(team_id, "???") + for error in validation.errors: + errors.append(f"[{abbrev}] {error}") return errors @property def all_warnings(self) -> List[str]: """Get all warnings across trade and roster levels.""" warnings = self.trade_warnings.copy() - for validation in self.participant_validations.values(): - warnings.extend(validation.warnings) + for team_id, validation in self.participant_validations.items(): + abbrev = self.team_abbrevs.get(team_id, "???") + for warning in validation.warnings: + warnings.append(f"[{abbrev}] {warning}") return warnings @property def all_suggestions(self) -> List[str]: """Get all suggestions across trade and roster levels.""" suggestions = self.trade_suggestions.copy() - for validation in self.participant_validations.values(): - suggestions.extend(validation.suggestions) + for team_id, validation in self.participant_validations.items(): + abbrev = self.team_abbrevs.get(team_id, "???") + for suggestion in validation.suggestions: + suggestions.append(f"[{abbrev}] {suggestion}") return suggestions def get_participant_validation(self, team_id: int) -> Optional[RosterValidationResult]: @@ -456,6 +463,7 @@ class TradeBuilder: # Validate each team's roster after the trade for participant in self.trade.participants: team_id = participant.team.id + result.team_abbrevs[team_id] = participant.team.abbrev if team_id in self._team_builders: builder = self._team_builders[team_id] roster_validation = await builder.validate_transaction(next_week) diff --git a/tests/test_services_trade_builder.py b/tests/test_services_trade_builder.py index dc2f238..4b9ac99 100644 --- a/tests/test_services_trade_builder.py +++ b/tests/test_services_trade_builder.py @@ -4,6 +4,7 @@ Tests for trade builder service. Tests the TradeBuilder service functionality including multi-team management, move validation, and trade validation logic. """ + import pytest from unittest.mock import AsyncMock, MagicMock, patch @@ -109,7 +110,7 @@ class TestTradeBuilder: self.player1.team_id = self.team1.id # Mock team_service to return team1 for this player - with patch('services.trade_builder.team_service') as mock_team_service: + with patch("services.trade_builder.team_service") as mock_team_service: mock_team_service.get_team = AsyncMock(return_value=self.team1) # Don't mock is_same_organization - let the real method work @@ -119,7 +120,7 @@ class TestTradeBuilder: from_team=self.team1, to_team=self.team2, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert success @@ -141,7 +142,7 @@ class TestTradeBuilder: from_team=self.team2, to_team=self.team1, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert not success @@ -156,9 +157,7 @@ class TestTradeBuilder: # Create a player on Free Agency fa_player = PlayerFactory.create( - id=100, - name="FA Player", - team_id=get_config().free_agent_team_id + id=100, name="FA Player", team_id=get_config().free_agent_team_id ) # Try to add player from FA (should fail) @@ -167,7 +166,7 @@ class TestTradeBuilder: from_team=self.team1, to_team=self.team2, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert not success @@ -182,9 +181,7 @@ class TestTradeBuilder: # Create a player without a team no_team_player = PlayerFactory.create( - id=101, - name="No Team Player", - team_id=None + id=101, name="No Team Player", team_id=None ) # Try to add player without team (should fail) @@ -193,7 +190,7 @@ class TestTradeBuilder: from_team=self.team1, to_team=self.team2, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert not success @@ -208,24 +205,22 @@ class TestTradeBuilder: # Create a player on team3 (not in trade) player_on_team3 = PlayerFactory.create( - id=102, - name="Team3 Player", - team_id=self.team3.id + id=102, name="Team3 Player", team_id=self.team3.id ) # Mock team_service to return team3 for this player - with patch('services.trade_builder.team_service') as mock_team_service: + with patch("services.trade_builder.team_service") as mock_team_service: mock_team_service.get_team = AsyncMock(return_value=self.team3) # Mock is_same_organization to return False (different organization, sync method) - with patch('models.team.Team.is_same_organization', return_value=False): + with patch("models.team.Team.is_same_organization", return_value=False): # Try to add player from team3 claiming it's from team1 (should fail) success, error = await builder.add_player_move( player=player_on_team3, from_team=self.team1, to_team=self.team2, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert not success @@ -241,26 +236,24 @@ class TestTradeBuilder: # Create a player on team1's minor league affiliate player_on_team1_mil = PlayerFactory.create( - id=103, - name="Team1 MiL Player", - team_id=999 # Some MiL team ID + id=103, name="Team1 MiL Player", team_id=999 # Some MiL team ID ) # Mock team_service to return the MiL team mil_team = TeamFactory.create(id=999, abbrev="WVMiL", sname="West Virginia MiL") - with patch('services.trade_builder.team_service') as mock_team_service: + with patch("services.trade_builder.team_service") as mock_team_service: mock_team_service.get_team = AsyncMock(return_value=mil_team) # Mock is_same_organization to return True (same organization, sync method) - with patch('models.team.Team.is_same_organization', return_value=True): + with patch("models.team.Team.is_same_organization", return_value=True): # Add player from WVMiL (should succeed because it's same organization as WV) success, error = await builder.add_player_move( player=player_on_team1_mil, from_team=self.team1, to_team=self.team2, from_roster=RosterType.MINOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert success @@ -278,7 +271,7 @@ class TestTradeBuilder: team=self.team1, player=self.player1, from_roster=RosterType.MINOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert success @@ -293,7 +286,7 @@ class TestTradeBuilder: team=self.team3, player=self.player2, from_roster=RosterType.MINOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert not success @@ -309,7 +302,7 @@ class TestTradeBuilder: self.player1.team_id = self.team1.id # Mock team_service for adding the player - with patch('services.trade_builder.team_service') as mock_team_service: + with patch("services.trade_builder.team_service") as mock_team_service: mock_team_service.get_team = AsyncMock(return_value=self.team1) # Add a player move @@ -318,7 +311,7 @@ class TestTradeBuilder: from_team=self.team1, to_team=self.team2, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) assert not builder.is_empty @@ -347,7 +340,7 @@ class TestTradeBuilder: await builder.add_team(self.team2) # Mock the transaction builders - with patch.object(builder, '_get_or_create_builder') as mock_get_builder: + with patch.object(builder, "_get_or_create_builder") as mock_get_builder: mock_builder1 = MagicMock() mock_builder2 = MagicMock() @@ -360,7 +353,7 @@ class TestTradeBuilder: minor_league_count=5, warnings=[], errors=[], - suggestions=[] + suggestions=[], ) mock_builder1.validate_transaction = AsyncMock(return_value=valid_result) @@ -391,7 +384,7 @@ class TestTradeBuilder: await builder.add_team(self.team2) # Mock the transaction builders - with patch.object(builder, '_get_or_create_builder') as mock_get_builder: + with patch.object(builder, "_get_or_create_builder") as mock_get_builder: mock_builder1 = MagicMock() mock_builder2 = MagicMock() @@ -404,7 +397,7 @@ class TestTradeBuilder: minor_league_count=5, warnings=[], errors=[], - suggestions=[] + suggestions=[], ) mock_builder1.validate_transaction = AsyncMock(return_value=valid_result) @@ -440,7 +433,7 @@ class TestTradeBuilder: return self.team2 return None - with patch('services.trade_builder.team_service') as mock_team_service: + with patch("services.trade_builder.team_service") as mock_team_service: mock_team_service.get_team = AsyncMock(side_effect=get_team_side_effect) # Add balanced moves - no need to mock is_same_organization @@ -449,7 +442,7 @@ class TestTradeBuilder: from_team=self.team1, to_team=self.team2, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) await builder.add_player_move( @@ -457,7 +450,7 @@ class TestTradeBuilder: from_team=self.team2, to_team=self.team1, from_roster=RosterType.MAJOR_LEAGUE, - to_roster=RosterType.MAJOR_LEAGUE + to_roster=RosterType.MAJOR_LEAGUE, ) # Validate balanced trade @@ -829,7 +822,9 @@ class TestTradeBuilderCache: """ user_id = 12345 team1 = TeamFactory.west_virginia() - team3 = TeamFactory.create(id=999, abbrev="POR", name="Portland") # Non-participant + team3 = TeamFactory.create( + id=999, abbrev="POR", name="Portland" + ) # Non-participant # Create builder with team1 get_trade_builder(user_id, team1) @@ -955,7 +950,9 @@ class TestTradeBuilderCache: This ensures proper error handling when a GM not in the trade tries to clear it. """ - team3 = TeamFactory.create(id=999, abbrev="POR", name="Portland") # Non-participant + team3 = TeamFactory.create( + id=999, abbrev="POR", name="Portland" + ) # Non-participant result = clear_trade_builder_by_team(team3.id) assert result is False @@ -982,7 +979,7 @@ class TestTradeValidationResult: minor_league_count=5, warnings=["Team1 warning"], errors=["Team1 error"], - suggestions=["Team1 suggestion"] + suggestions=["Team1 suggestion"], ) team2_validation = RosterValidationResult( @@ -991,28 +988,30 @@ class TestTradeValidationResult: minor_league_count=4, warnings=[], errors=[], - suggestions=[] + suggestions=[], ) result.participant_validations[1] = team1_validation result.participant_validations[2] = team2_validation + result.team_abbrevs[1] = "TM1" + result.team_abbrevs[2] = "TM2" result.is_legal = False # One team has errors - # Test aggregated results + # Test aggregated results - roster errors are prefixed with team abbrev all_errors = result.all_errors assert len(all_errors) == 3 # 2 trade + 1 team assert "Trade error 1" in all_errors - assert "Team1 error" in all_errors + assert "[TM1] Team1 error" in all_errors all_warnings = result.all_warnings assert len(all_warnings) == 2 # 1 trade + 1 team assert "Trade warning 1" in all_warnings - assert "Team1 warning" in all_warnings + assert "[TM1] Team1 warning" in all_warnings all_suggestions = result.all_suggestions assert len(all_suggestions) == 2 # 1 trade + 1 team assert "Trade suggestion 1" in all_suggestions - assert "Team1 suggestion" in all_suggestions + assert "[TM1] Team1 suggestion" in all_suggestions # Test participant validation lookup team1_val = result.get_participant_validation(1) @@ -1029,4 +1028,4 @@ class TestTradeValidationResult: assert len(result.all_errors) == 0 assert len(result.all_warnings) == 0 assert len(result.all_suggestions) == 0 - assert len(result.participant_validations) == 0 \ No newline at end of file + assert len(result.participant_validations) == 0