cal/major-domo-v2
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Merge pull request 'fix: prefix trade validation errors with team abbreviation' (#71) from fix/trade-errors-show-team-abbrev into next-release
All checks were successful
Build Docker Image / build (push) Successful in 3m5s
Details

Browse Source 
Reviewed-on: #71
This commit is contained in:
cal 2026-03-10 15:46:42 +00:00
commit bebe25b9dd
2 changed files with 158 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

158
services/trade_builder.py
View File

@ -3,6 +3,7 @@ Trade Builder Service
Extends the TransactionBuilder to support multi-team trades and player exchanges.
"""
import logging
from typing import Dict, List, Optional, Set
from datetime import datetime, timezone
@ -12,10 +13,14 @@ from config import get_config
from models.trade import Trade, TradeMove, TradeStatus
from models.team import Team, RosterType
from models.player import Player
from services.transaction_builder import TransactionBuilder, RosterValidationResult, TransactionMove
from services.transaction_builder import (
TransactionBuilder,
RosterValidationResult,
TransactionMove,
)
from services.team_service import team_service
logger = logging.getLogger(f'{__name__}.TradeBuilder')
logger = logging.getLogger(f"{__name__}.TradeBuilder")
class TradeValidationResult:
@ -24,6 +29,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,27 +38,35 @@ 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]:
def get_participant_validation(
self, team_id: int
) -> Optional[RosterValidationResult]:
"""Get validation result for a specific team."""
return self.participant_validations.get(team_id)
@ -64,7 +78,12 @@ class TradeBuilder:
Extends the functionality of TransactionBuilder to support trades between teams.
"""
def __init__(self, initiated_by: int, initiating_team: Team, season: int = get_config().sba_season):
def __init__(
self,
initiated_by: int,
initiating_team: Team,
season: int = get_config().sba_season,
):
"""
Initialize trade builder.
@ -79,7 +98,7 @@ class TradeBuilder:
status=TradeStatus.DRAFT,
initiated_by=initiated_by,
created_at=datetime.now(timezone.utc).isoformat(),
season=season
season=season,
)
# Add the initiating team as first participant
@ -91,7 +110,9 @@ class TradeBuilder:
# Track which teams have accepted the trade (team_id -> True)
self.accepted_teams: Set[int] = set()
logger.info(f"TradeBuilder initialized: {self.trade.trade_id} by user {initiated_by} for {initiating_team.abbrev}")
logger.info(
f"TradeBuilder initialized: {self.trade.trade_id} by user {initiated_by} for {initiating_team.abbrev}"
)
@property
def trade_id(self) -> str:
@ -127,7 +148,11 @@ class TradeBuilder:
@property
def pending_teams(self) -> List[Team]:
"""Get list of teams that haven't accepted yet."""
return [team for team in self.participating_teams if team.id not in self.accepted_teams]
return [
team
for team in self.participating_teams
if team.id not in self.accepted_teams
]
def accept_trade(self, team_id: int) -> bool:
"""
@ -140,7 +165,9 @@ class TradeBuilder:
True if all teams have now accepted, False otherwise
"""
self.accepted_teams.add(team_id)
logger.info(f"Team {team_id} accepted trade {self.trade_id}. Accepted: {len(self.accepted_teams)}/{self.team_count}")
logger.info(
f"Team {team_id} accepted trade {self.trade_id}. Accepted: {len(self.accepted_teams)}/{self.team_count}"
)
return self.all_teams_accepted
def reject_trade(self) -> None:
@ -160,7 +187,9 @@ class TradeBuilder:
Returns:
Dict mapping team_id to acceptance status (True/False)
"""
return {team.id: team.id in self.accepted_teams for team in self.participating_teams}
return {
team.id: team.id in self.accepted_teams for team in self.participating_teams
}
def has_team_accepted(self, team_id: int) -> bool:
"""Check if a specific team has accepted."""
@ -184,7 +213,9 @@ class TradeBuilder:
participant = self.trade.add_participant(team)
# Create transaction builder for this team
self._team_builders[team.id] = TransactionBuilder(team, self.trade.initiated_by, self.trade.season)
self._team_builders[team.id] = TransactionBuilder(
team, self.trade.initiated_by, self.trade.season
)
# Register team in secondary index for multi-GM access
trade_key = f"{self.trade.initiated_by}:trade"
@ -209,7 +240,10 @@ class TradeBuilder:
# Check if team has moves - prevent removal if they do
if participant.all_moves:
return False, f"{participant.team.abbrev} has moves in this trade and cannot be removed"
return (
False,
f"{participant.team.abbrev} has moves in this trade and cannot be removed",
)
# Remove team
removed = self.trade.remove_participant(team_id)
@ -229,7 +263,7 @@ class TradeBuilder:
from_team: Team,
to_team: Team,
from_roster: RosterType,
to_roster: RosterType
to_roster: RosterType,
) -> tuple[bool, str]:
"""
Add a player move to the trade.
@ -246,7 +280,10 @@ class TradeBuilder:
"""
# Validate player is not from Free Agency
if player.team_id == get_config().free_agent_team_id:
return False, f"Cannot add {player.name} from Free Agency. Players must be traded from teams within the organizations involved in the trade."
return (
False,
f"Cannot add {player.name} from Free Agency. Players must be traded from teams within the organizations involved in the trade.",
)
# Validate player has a valid team assignment
if not player.team_id:
@ -259,7 +296,10 @@ class TradeBuilder:
# Check if player's team is in the same organization as from_team
if not player_team.is_same_organization(from_team):
return False, f"{player.name} is on {player_team.abbrev}, they are not eligible to be added to the trade."
return (
False,
f"{player.name} is on {player_team.abbrev}, they are not eligible to be added to the trade.",
)
# Ensure both teams are participating (check by organization for ML authority)
from_participant = self.trade.get_participant_by_organization(from_team)
@ -274,7 +314,10 @@ class TradeBuilder:
for participant in self.trade.participants:
for existing_move in participant.all_moves:
if existing_move.player.id == player.id:
return False, f"{player.name} is already involved in a move in this trade"
return (
False,
f"{player.name} is already involved in a move in this trade",
)
# Create trade move
trade_move = TradeMove(
@ -284,7 +327,7 @@ class TradeBuilder:
from_team=from_team,
to_team=to_team,
source_team=from_team,
destination_team=to_team
destination_team=to_team,
)
# Add to giving team's moves
@ -303,7 +346,7 @@ class TradeBuilder:
from_roster=from_roster,
to_roster=RosterType.FREE_AGENCY, # Conceptually leaving the org
from_team=from_team,
to_team=None
to_team=None,
)
# Move for receiving team (player joining)
@ -312,19 +355,23 @@ class TradeBuilder:
from_roster=RosterType.FREE_AGENCY, # Conceptually joining from outside
to_roster=to_roster,
from_team=None,
to_team=to_team
to_team=to_team,
)
# Add moves to respective builders
# Skip pending transaction check for trades - they have their own validation workflow
from_success, from_error = await from_builder.add_move(from_move, check_pending_transactions=False)
from_success, from_error = await from_builder.add_move(
from_move, check_pending_transactions=False
)
if not from_success:
# Remove from trade if builder failed
from_participant.moves_giving.remove(trade_move)
to_participant.moves_receiving.remove(trade_move)
return False, f"Error adding move to {from_team.abbrev}: {from_error}"
to_success, to_error = await to_builder.add_move(to_move, check_pending_transactions=False)
to_success, to_error = await to_builder.add_move(
to_move, check_pending_transactions=False
)
if not to_success:
# Rollback both if second failed
from_builder.remove_move(player.id)
@ -332,15 +379,13 @@ class TradeBuilder:
to_participant.moves_receiving.remove(trade_move)
return False, f"Error adding move to {to_team.abbrev}: {to_error}"
logger.info(f"Added player move to trade {self.trade_id}: {trade_move.description}")
logger.info(
f"Added player move to trade {self.trade_id}: {trade_move.description}"
)
return True, ""
async def add_supplementary_move(
self,
team: Team,
player: Player,
from_roster: RosterType,
to_roster: RosterType
self, team: Team, player: Player, from_roster: RosterType, to_roster: RosterType
) -> tuple[bool, str]:
"""
Add a supplementary move (internal organizational move) for roster legality.
@ -366,7 +411,7 @@ class TradeBuilder:
from_team=team,
to_team=team,
source_team=team,
destination_team=team
destination_team=team,
)
# Add to participant's supplementary moves
@ -379,16 +424,20 @@ class TradeBuilder:
from_roster=from_roster,
to_roster=to_roster,
from_team=team,
to_team=team
to_team=team,
)
# Skip pending transaction check for trade supplementary moves
success, error = await builder.add_move(trans_move, check_pending_transactions=False)
success, error = await builder.add_move(
trans_move, check_pending_transactions=False
)
if not success:
participant.supplementary_moves.remove(supp_move)
return False, error
logger.info(f"Added supplementary move for {team.abbrev}: {supp_move.description}")
logger.info(
f"Added supplementary move for {team.abbrev}: {supp_move.description}"
)
return True, ""
async def remove_move(self, player_id: int) -> tuple[bool, str]:
@ -432,21 +481,41 @@ class TradeBuilder:
for builder in self._team_builders.values():
builder.remove_move(player_id)
logger.info(f"Removed move from trade {self.trade_id}: {removed_move.description}")
logger.info(
f"Removed move from trade {self.trade_id}: {removed_move.description}"
)
return True, ""
async def validate_trade(self, next_week: Optional[int] = None) -> TradeValidationResult:
async def validate_trade(
self, next_week: Optional[int] = None
) -> TradeValidationResult:
"""
Validate the entire trade including all teams' roster legality.
Validates against next week's projected roster (current roster + pending
transactions), matching the behavior of /dropadd validation.
Args:
next_week: Week to validate for (optional)
next_week: Week to validate for (auto-fetched if not provided)
Returns:
TradeValidationResult with comprehensive validation
"""
result = TradeValidationResult()
# Auto-fetch next week so validation includes pending transactions
if next_week is None:
try:
from services.league_service import league_service
current_state = await league_service.get_current_state()
next_week = (current_state.week + 1) if current_state else 1
except Exception as e:
logger.warning(
f"Could not determine next week for trade validation: {e}"
)
next_week = None
# Validate trade structure
is_balanced, balance_errors = self.trade.validate_trade_balance()
if not is_balanced:
@ -456,6 +525,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)
@ -472,13 +542,17 @@ class TradeBuilder:
if self.team_count < 2:
result.trade_suggestions.append("Add another team to create a trade")
logger.debug(f"Trade validation for {self.trade_id}: Legal={result.is_legal}, Errors={len(result.all_errors)}")
logger.debug(
f"Trade validation for {self.trade_id}: Legal={result.is_legal}, Errors={len(result.all_errors)}"
)
return result
def _get_or_create_builder(self, team: Team) -> TransactionBuilder:
"""Get or create a transaction builder for a team."""
if team.id not in self._team_builders:
self._team_builders[team.id] = TransactionBuilder(team, self.trade.initiated_by, self.trade.season)
self._team_builders[team.id] = TransactionBuilder(
team, self.trade.initiated_by, self.trade.season
)
return self._team_builders[team.id]
def clear_trade(self) -> None:
@ -592,4 +666,4 @@ def clear_trade_builder_by_team(team_id: int) -> bool:
def get_active_trades() -> Dict[str, TradeBuilder]:
"""Get all active trade builders (for debugging/admin purposes)."""
return _active_trade_builders.copy()
return _active_trade_builders.copy()

85
tests/test_services_trade_builder.py
View File

@ -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
assert len(result.participant_validations) == 0