fix: trade validation now checks against next week's projected roster
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m5s
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m5s
validate_trade() was passing next_week=None to each team's validate_transaction(), which skipped load_existing_transactions() entirely. Trades were validated against the current roster only, ignoring pending /dropadd transactions for next week. Now auto-fetches current week from league_service and passes next_week=current_week+1, matching /dropadd validation behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
e98a658fde
commit
58fe9f22de
@ -3,6 +3,7 @@ Trade Builder Service
|
|||||||
|
|
||||||
Extends the TransactionBuilder to support multi-team trades and player exchanges.
|
Extends the TransactionBuilder to support multi-team trades and player exchanges.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import logging
|
import logging
|
||||||
from typing import Dict, List, Optional, Set
|
from typing import Dict, List, Optional, Set
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
@ -12,10 +13,14 @@ from config import get_config
|
|||||||
from models.trade import Trade, TradeMove, TradeStatus
|
from models.trade import Trade, TradeMove, TradeStatus
|
||||||
from models.team import Team, RosterType
|
from models.team import Team, RosterType
|
||||||
from models.player import Player
|
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
|
from services.team_service import team_service
|
||||||
|
|
||||||
logger = logging.getLogger(f'{__name__}.TradeBuilder')
|
logger = logging.getLogger(f"{__name__}.TradeBuilder")
|
||||||
|
|
||||||
|
|
||||||
class TradeValidationResult:
|
class TradeValidationResult:
|
||||||
@ -52,7 +57,9 @@ class TradeValidationResult:
|
|||||||
suggestions.extend(validation.suggestions)
|
suggestions.extend(validation.suggestions)
|
||||||
return suggestions
|
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."""
|
"""Get validation result for a specific team."""
|
||||||
return self.participant_validations.get(team_id)
|
return self.participant_validations.get(team_id)
|
||||||
|
|
||||||
@ -64,7 +71,12 @@ class TradeBuilder:
|
|||||||
Extends the functionality of TransactionBuilder to support trades between teams.
|
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.
|
Initialize trade builder.
|
||||||
|
|
||||||
@ -79,7 +91,7 @@ class TradeBuilder:
|
|||||||
status=TradeStatus.DRAFT,
|
status=TradeStatus.DRAFT,
|
||||||
initiated_by=initiated_by,
|
initiated_by=initiated_by,
|
||||||
created_at=datetime.now(timezone.utc).isoformat(),
|
created_at=datetime.now(timezone.utc).isoformat(),
|
||||||
season=season
|
season=season,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Add the initiating team as first participant
|
# Add the initiating team as first participant
|
||||||
@ -91,7 +103,9 @@ class TradeBuilder:
|
|||||||
# Track which teams have accepted the trade (team_id -> True)
|
# Track which teams have accepted the trade (team_id -> True)
|
||||||
self.accepted_teams: Set[int] = set()
|
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
|
@property
|
||||||
def trade_id(self) -> str:
|
def trade_id(self) -> str:
|
||||||
@ -127,7 +141,11 @@ class TradeBuilder:
|
|||||||
@property
|
@property
|
||||||
def pending_teams(self) -> List[Team]:
|
def pending_teams(self) -> List[Team]:
|
||||||
"""Get list of teams that haven't accepted yet."""
|
"""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:
|
def accept_trade(self, team_id: int) -> bool:
|
||||||
"""
|
"""
|
||||||
@ -140,7 +158,9 @@ class TradeBuilder:
|
|||||||
True if all teams have now accepted, False otherwise
|
True if all teams have now accepted, False otherwise
|
||||||
"""
|
"""
|
||||||
self.accepted_teams.add(team_id)
|
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
|
return self.all_teams_accepted
|
||||||
|
|
||||||
def reject_trade(self) -> None:
|
def reject_trade(self) -> None:
|
||||||
@ -160,7 +180,9 @@ class TradeBuilder:
|
|||||||
Returns:
|
Returns:
|
||||||
Dict mapping team_id to acceptance status (True/False)
|
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:
|
def has_team_accepted(self, team_id: int) -> bool:
|
||||||
"""Check if a specific team has accepted."""
|
"""Check if a specific team has accepted."""
|
||||||
@ -184,7 +206,9 @@ class TradeBuilder:
|
|||||||
participant = self.trade.add_participant(team)
|
participant = self.trade.add_participant(team)
|
||||||
|
|
||||||
# Create transaction builder for this 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
|
# Register team in secondary index for multi-GM access
|
||||||
trade_key = f"{self.trade.initiated_by}:trade"
|
trade_key = f"{self.trade.initiated_by}:trade"
|
||||||
@ -209,7 +233,10 @@ class TradeBuilder:
|
|||||||
|
|
||||||
# Check if team has moves - prevent removal if they do
|
# Check if team has moves - prevent removal if they do
|
||||||
if participant.all_moves:
|
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
|
# Remove team
|
||||||
removed = self.trade.remove_participant(team_id)
|
removed = self.trade.remove_participant(team_id)
|
||||||
@ -229,7 +256,7 @@ class TradeBuilder:
|
|||||||
from_team: Team,
|
from_team: Team,
|
||||||
to_team: Team,
|
to_team: Team,
|
||||||
from_roster: RosterType,
|
from_roster: RosterType,
|
||||||
to_roster: RosterType
|
to_roster: RosterType,
|
||||||
) -> tuple[bool, str]:
|
) -> tuple[bool, str]:
|
||||||
"""
|
"""
|
||||||
Add a player move to the trade.
|
Add a player move to the trade.
|
||||||
@ -246,7 +273,10 @@ class TradeBuilder:
|
|||||||
"""
|
"""
|
||||||
# Validate player is not from Free Agency
|
# Validate player is not from Free Agency
|
||||||
if player.team_id == get_config().free_agent_team_id:
|
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
|
# Validate player has a valid team assignment
|
||||||
if not player.team_id:
|
if not player.team_id:
|
||||||
@ -259,7 +289,10 @@ class TradeBuilder:
|
|||||||
|
|
||||||
# Check if player's team is in the same organization as from_team
|
# Check if player's team is in the same organization as from_team
|
||||||
if not player_team.is_same_organization(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)
|
# Ensure both teams are participating (check by organization for ML authority)
|
||||||
from_participant = self.trade.get_participant_by_organization(from_team)
|
from_participant = self.trade.get_participant_by_organization(from_team)
|
||||||
@ -274,7 +307,10 @@ class TradeBuilder:
|
|||||||
for participant in self.trade.participants:
|
for participant in self.trade.participants:
|
||||||
for existing_move in participant.all_moves:
|
for existing_move in participant.all_moves:
|
||||||
if existing_move.player.id == player.id:
|
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
|
# Create trade move
|
||||||
trade_move = TradeMove(
|
trade_move = TradeMove(
|
||||||
@ -284,7 +320,7 @@ class TradeBuilder:
|
|||||||
from_team=from_team,
|
from_team=from_team,
|
||||||
to_team=to_team,
|
to_team=to_team,
|
||||||
source_team=from_team,
|
source_team=from_team,
|
||||||
destination_team=to_team
|
destination_team=to_team,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Add to giving team's moves
|
# Add to giving team's moves
|
||||||
@ -303,7 +339,7 @@ class TradeBuilder:
|
|||||||
from_roster=from_roster,
|
from_roster=from_roster,
|
||||||
to_roster=RosterType.FREE_AGENCY, # Conceptually leaving the org
|
to_roster=RosterType.FREE_AGENCY, # Conceptually leaving the org
|
||||||
from_team=from_team,
|
from_team=from_team,
|
||||||
to_team=None
|
to_team=None,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Move for receiving team (player joining)
|
# Move for receiving team (player joining)
|
||||||
@ -312,19 +348,23 @@ class TradeBuilder:
|
|||||||
from_roster=RosterType.FREE_AGENCY, # Conceptually joining from outside
|
from_roster=RosterType.FREE_AGENCY, # Conceptually joining from outside
|
||||||
to_roster=to_roster,
|
to_roster=to_roster,
|
||||||
from_team=None,
|
from_team=None,
|
||||||
to_team=to_team
|
to_team=to_team,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Add moves to respective builders
|
# Add moves to respective builders
|
||||||
# Skip pending transaction check for trades - they have their own validation workflow
|
# 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:
|
if not from_success:
|
||||||
# Remove from trade if builder failed
|
# Remove from trade if builder failed
|
||||||
from_participant.moves_giving.remove(trade_move)
|
from_participant.moves_giving.remove(trade_move)
|
||||||
to_participant.moves_receiving.remove(trade_move)
|
to_participant.moves_receiving.remove(trade_move)
|
||||||
return False, f"Error adding move to {from_team.abbrev}: {from_error}"
|
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:
|
if not to_success:
|
||||||
# Rollback both if second failed
|
# Rollback both if second failed
|
||||||
from_builder.remove_move(player.id)
|
from_builder.remove_move(player.id)
|
||||||
@ -332,15 +372,13 @@ class TradeBuilder:
|
|||||||
to_participant.moves_receiving.remove(trade_move)
|
to_participant.moves_receiving.remove(trade_move)
|
||||||
return False, f"Error adding move to {to_team.abbrev}: {to_error}"
|
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, ""
|
return True, ""
|
||||||
|
|
||||||
async def add_supplementary_move(
|
async def add_supplementary_move(
|
||||||
self,
|
self, team: Team, player: Player, from_roster: RosterType, to_roster: RosterType
|
||||||
team: Team,
|
|
||||||
player: Player,
|
|
||||||
from_roster: RosterType,
|
|
||||||
to_roster: RosterType
|
|
||||||
) -> tuple[bool, str]:
|
) -> tuple[bool, str]:
|
||||||
"""
|
"""
|
||||||
Add a supplementary move (internal organizational move) for roster legality.
|
Add a supplementary move (internal organizational move) for roster legality.
|
||||||
@ -366,7 +404,7 @@ class TradeBuilder:
|
|||||||
from_team=team,
|
from_team=team,
|
||||||
to_team=team,
|
to_team=team,
|
||||||
source_team=team,
|
source_team=team,
|
||||||
destination_team=team
|
destination_team=team,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Add to participant's supplementary moves
|
# Add to participant's supplementary moves
|
||||||
@ -379,16 +417,20 @@ class TradeBuilder:
|
|||||||
from_roster=from_roster,
|
from_roster=from_roster,
|
||||||
to_roster=to_roster,
|
to_roster=to_roster,
|
||||||
from_team=team,
|
from_team=team,
|
||||||
to_team=team
|
to_team=team,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Skip pending transaction check for trade supplementary moves
|
# 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:
|
if not success:
|
||||||
participant.supplementary_moves.remove(supp_move)
|
participant.supplementary_moves.remove(supp_move)
|
||||||
return False, error
|
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, ""
|
return True, ""
|
||||||
|
|
||||||
async def remove_move(self, player_id: int) -> tuple[bool, str]:
|
async def remove_move(self, player_id: int) -> tuple[bool, str]:
|
||||||
@ -432,21 +474,41 @@ class TradeBuilder:
|
|||||||
for builder in self._team_builders.values():
|
for builder in self._team_builders.values():
|
||||||
builder.remove_move(player_id)
|
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, ""
|
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.
|
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:
|
Args:
|
||||||
next_week: Week to validate for (optional)
|
next_week: Week to validate for (auto-fetched if not provided)
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
TradeValidationResult with comprehensive validation
|
TradeValidationResult with comprehensive validation
|
||||||
"""
|
"""
|
||||||
result = TradeValidationResult()
|
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
|
# Validate trade structure
|
||||||
is_balanced, balance_errors = self.trade.validate_trade_balance()
|
is_balanced, balance_errors = self.trade.validate_trade_balance()
|
||||||
if not is_balanced:
|
if not is_balanced:
|
||||||
@ -472,13 +534,17 @@ class TradeBuilder:
|
|||||||
if self.team_count < 2:
|
if self.team_count < 2:
|
||||||
result.trade_suggestions.append("Add another team to create a trade")
|
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
|
return result
|
||||||
|
|
||||||
def _get_or_create_builder(self, team: Team) -> TransactionBuilder:
|
def _get_or_create_builder(self, team: Team) -> TransactionBuilder:
|
||||||
"""Get or create a transaction builder for a team."""
|
"""Get or create a transaction builder for a team."""
|
||||||
if team.id not in self._team_builders:
|
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]
|
return self._team_builders[team.id]
|
||||||
|
|
||||||
def clear_trade(self) -> None:
|
def clear_trade(self) -> None:
|
||||||
@ -592,4 +658,4 @@ def clear_trade_builder_by_team(team_id: int) -> bool:
|
|||||||
|
|
||||||
def get_active_trades() -> Dict[str, TradeBuilder]:
|
def get_active_trades() -> Dict[str, TradeBuilder]:
|
||||||
"""Get all active trade builders (for debugging/admin purposes)."""
|
"""Get all active trade builders (for debugging/admin purposes)."""
|
||||||
return _active_trade_builders.copy()
|
return _active_trade_builders.copy()
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user