From aa27769ed63559895e2329cf8f9afc2a8552eed3 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Sun, 1 Mar 2026 10:45:21 -0600 Subject: [PATCH] fix: roster validation now includes org affiliate transactions (closes #49) load_existing_transactions only queried for the base team abbreviation (e.g. "POR"), missing trades involving MiL/IL affiliates ("PORMIL", "PORIL"). This caused false "too many players" errors when a pending trade would have cleared a roster spot. - get_team_transactions now accepts Union[str, List[str]] for team_abbrev - load_existing_transactions queries all org affiliates [BASE, BASEMiL, BASEIL] - Added 5 tests covering the fix and backwards compatibility Co-Authored-By: Claude Opus 4.6 --- services/transaction_builder.py | 296 ++++++++++++------ services/transaction_service.py | 283 +++++++++-------- .../test_roster_validation_org_affiliates.py | 277 ++++++++++++++++ 3 files changed, 627 insertions(+), 229 deletions(-) create mode 100644 tests/test_roster_validation_org_affiliates.py diff --git a/services/transaction_builder.py b/services/transaction_builder.py index ffa4927..0ee9e2d 100644 --- a/services/transaction_builder.py +++ b/services/transaction_builder.py @@ -3,6 +3,7 @@ Transaction Builder Service Handles the complex logic for building multi-move transactions interactively. """ + import logging from typing import Dict, List, Optional from dataclasses import dataclass @@ -18,7 +19,7 @@ from services.transaction_service import transaction_service from services.league_service import league_service from models.team import RosterType -logger = logging.getLogger(f'{__name__}.TransactionBuilder') +logger = logging.getLogger(f"{__name__}.TransactionBuilder") # Removed MoveAction enum - using simple from/to roster locations instead @@ -27,53 +28,87 @@ logger = logging.getLogger(f'{__name__}.TransactionBuilder') @dataclass class TransactionMove: """Individual move within a transaction.""" + player: Player from_roster: RosterType to_roster: RosterType from_team: Optional[Team] = None to_team: Optional[Team] = None - + @property def description(self) -> str: """Human readable move description.""" # Determine emoji and format based on from/to locations - if self.from_roster == RosterType.FREE_AGENCY and self.to_roster != RosterType.FREE_AGENCY: + if ( + self.from_roster == RosterType.FREE_AGENCY + and self.to_roster != RosterType.FREE_AGENCY + ): # Add from Free Agency emoji = "➕" return f"{emoji} {self.player.name}: FA → {self.to_team.abbrev} ({self.to_roster.value.upper()})" - elif self.from_roster != RosterType.FREE_AGENCY and self.to_roster == RosterType.FREE_AGENCY: + elif ( + self.from_roster != RosterType.FREE_AGENCY + and self.to_roster == RosterType.FREE_AGENCY + ): # Drop to Free Agency emoji = "➖" return f"{emoji} {self.player.name}: {self.from_team.abbrev} ({self.from_roster.value.upper()}) → FA" - elif self.from_roster == RosterType.MINOR_LEAGUE and self.to_roster == RosterType.MAJOR_LEAGUE: + elif ( + self.from_roster == RosterType.MINOR_LEAGUE + and self.to_roster == RosterType.MAJOR_LEAGUE + ): # Recall from MiL to ML emoji = "⬆️" return f"{emoji} {self.player.name}: {self.from_team.abbrev} (MiL) → {self.to_team.abbrev} (ML)" - elif self.from_roster == RosterType.MAJOR_LEAGUE and self.to_roster == RosterType.MINOR_LEAGUE: + elif ( + self.from_roster == RosterType.MAJOR_LEAGUE + and self.to_roster == RosterType.MINOR_LEAGUE + ): # Demote from ML to MiL emoji = "⬇️" return f"{emoji} {self.player.name}: {self.from_team.abbrev} (ML) → {self.to_team.abbrev} (MiL)" elif self.to_roster == RosterType.INJURED_LIST: # Move to Injured List emoji = "🏥" - from_desc = "FA" if self.from_roster == RosterType.FREE_AGENCY else f"{self.from_team.abbrev} ({self.from_roster.value.upper()})" - return f"{emoji} {self.player.name}: {from_desc} → {self.to_team.abbrev} (IL)" + from_desc = ( + "FA" + if self.from_roster == RosterType.FREE_AGENCY + else f"{self.from_team.abbrev} ({self.from_roster.value.upper()})" + ) + return ( + f"{emoji} {self.player.name}: {from_desc} → {self.to_team.abbrev} (IL)" + ) elif self.from_roster == RosterType.INJURED_LIST: # Return from Injured List emoji = "💊" - to_desc = "FA" if self.to_roster == RosterType.FREE_AGENCY else f"{self.to_team.abbrev} ({self.to_roster.value.upper()})" - return f"{emoji} {self.player.name}: {self.from_team.abbrev} (IL) → {to_desc}" + to_desc = ( + "FA" + if self.to_roster == RosterType.FREE_AGENCY + else f"{self.to_team.abbrev} ({self.to_roster.value.upper()})" + ) + return ( + f"{emoji} {self.player.name}: {self.from_team.abbrev} (IL) → {to_desc}" + ) else: # Generic move emoji = "🔄" - from_desc = "FA" if self.from_roster == RosterType.FREE_AGENCY else f"{self.from_team.abbrev} ({self.from_roster.value.upper()})" - to_desc = "FA" if self.to_roster == RosterType.FREE_AGENCY else f"{self.to_team.abbrev} ({self.to_roster.value.upper()})" + from_desc = ( + "FA" + if self.from_roster == RosterType.FREE_AGENCY + else f"{self.from_team.abbrev} ({self.from_roster.value.upper()})" + ) + to_desc = ( + "FA" + if self.to_roster == RosterType.FREE_AGENCY + else f"{self.to_team.abbrev} ({self.to_roster.value.upper()})" + ) return f"{emoji} {self.player.name}: {from_desc} → {to_desc}" @dataclass class RosterValidationResult: """Results of roster validation.""" + is_legal: bool major_league_count: int minor_league_count: int @@ -88,7 +123,7 @@ class RosterValidationResult: pre_existing_ml_swar_change: float = 0.0 pre_existing_mil_swar_change: float = 0.0 pre_existing_transaction_count: int = 0 - + @property def major_league_status(self) -> str: """Status string for major league roster.""" @@ -96,7 +131,7 @@ class RosterValidationResult: return f"❌ Major League: {self.major_league_count}/{self.major_league_limit} (Over limit!)" else: return f"✅ Major League: {self.major_league_count}/{self.major_league_limit} (Legal)" - + @property def minor_league_status(self) -> str: """Status string for minor league roster.""" @@ -124,7 +159,9 @@ class RosterValidationResult: if self.pre_existing_transaction_count == 0: return "" - total_swar_change = self.pre_existing_ml_swar_change + self.pre_existing_mil_swar_change + total_swar_change = ( + self.pre_existing_ml_swar_change + self.pre_existing_mil_swar_change + ) if total_swar_change == 0: return f"ℹ️ **Pre-existing Moves**: {self.pre_existing_transaction_count} scheduled moves (no sWAR impact)" @@ -136,11 +173,11 @@ class RosterValidationResult: class TransactionBuilder: """Interactive transaction builder for complex multi-move transactions.""" - + def __init__(self, team: Team, user_id: int, season: int = get_config().sba_season): """ Initialize transaction builder. - + Args: team: Team making the transaction user_id: Discord user ID of the GM @@ -151,7 +188,7 @@ class TransactionBuilder: self.season = season self.moves: List[TransactionMove] = [] self.created_at = datetime.now(timezone.utc) - + # Cache for roster data self._current_roster: Optional[TeamRoster] = None self._roster_loaded = False @@ -160,8 +197,10 @@ class TransactionBuilder: self._existing_transactions: Optional[List[Transaction]] = None self._existing_transactions_loaded = False - logger.info(f"TransactionBuilder initialized for {team.abbrev} by user {user_id}") - + logger.info( + f"TransactionBuilder initialized for {team.abbrev} by user {user_id}" + ) + async def load_roster_data(self) -> None: """Load current roster data for the team.""" if self._roster_loaded: @@ -177,29 +216,42 @@ class TransactionBuilder: self._roster_loaded = True async def load_existing_transactions(self, next_week: int) -> None: - """Load pre-existing transactions for next week.""" + """ + Load pre-existing transactions for next week. + + Queries for all organizational affiliates (ML, MiL, IL) to ensure + trades involving affiliate teams are included in roster projections. + """ if self._existing_transactions_loaded: return try: - self._existing_transactions = await transaction_service.get_team_transactions( - team_abbrev=self.team.abbrev, - season=self.season, - cancelled=False, - week_start=next_week + # Include all org affiliates so trades involving MiL/IL teams are captured + base_abbrev = self.team._get_base_abbrev() + org_abbrevs = [base_abbrev, f"{base_abbrev}MIL", f"{base_abbrev}IL"] + + self._existing_transactions = ( + await transaction_service.get_team_transactions( + team_abbrev=org_abbrevs, + season=self.season, + cancelled=False, + week_start=next_week, + ) ) self._existing_transactions_loaded = True - logger.debug(f"Loaded {len(self._existing_transactions or [])} existing transactions for {self.team.abbrev} week {next_week}") + logger.debug( + f"Loaded {len(self._existing_transactions or [])} existing transactions for {self.team.abbrev} org ({org_abbrevs}) week {next_week}" + ) except Exception as e: logger.error(f"Failed to load existing transactions: {e}") self._existing_transactions = [] self._existing_transactions_loaded = True - + async def add_move( self, move: TransactionMove, next_week: Optional[int] = None, - check_pending_transactions: bool = True + check_pending_transactions: bool = True, ) -> tuple[bool, str]: """ Add a move to the transaction. @@ -216,14 +268,20 @@ class TransactionBuilder: # Check if player is already in a move in this transaction builder existing_move = self.get_move_for_player(move.player.id) if existing_move: - error_msg = f"Player {move.player.name} already has a move in this transaction" + error_msg = ( + f"Player {move.player.name} already has a move in this transaction" + ) logger.warning(error_msg) return False, error_msg # Check if from_team and to_team are the same AND from_roster and to_roster are the same # (when both teams are not None) - this would be a meaningless move - if (move.from_team is not None and move.to_team is not None and - move.from_team.id == move.to_team.id and move.from_roster == move.to_roster): + if ( + move.from_team is not None + and move.to_team is not None + and move.from_team.id == move.to_team.id + and move.from_roster == move.to_roster + ): error_msg = f"Cannot move {move.player.name} from {move.from_team.abbrev} ({move.from_roster.value.upper()}) to {move.to_team.abbrev} ({move.to_roster.value.upper()}) - player is already in that location" logger.warning(error_msg) return False, error_msg @@ -237,13 +295,15 @@ class TransactionBuilder: 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 get current week for pending transaction check: {e}") + logger.warning( + f"Could not get current week for pending transaction check: {e}" + ) next_week = 1 - is_pending, claiming_team = await transaction_service.is_player_in_pending_transaction( - player_id=move.player.id, - week=next_week, - season=self.season + is_pending, claiming_team = ( + await transaction_service.is_player_in_pending_transaction( + player_id=move.player.id, week=next_week, season=self.season + ) ) if is_pending: @@ -254,34 +314,36 @@ class TransactionBuilder: self.moves.append(move) logger.info(f"Added move: {move.description}") return True, "" - + def remove_move(self, player_id: int) -> bool: """ Remove a move for a specific player. - + Args: player_id: ID of player to remove move for - + Returns: True if move was removed """ original_count = len(self.moves) self.moves = [move for move in self.moves if move.player.id != player_id] - + removed = len(self.moves) < original_count if removed: logger.info(f"Removed move for player {player_id}") - + return removed - + def get_move_for_player(self, player_id: int) -> Optional[TransactionMove]: """Get the move for a specific player if it exists.""" for move in self.moves: if move.player.id == player_id: return move return None - - async def validate_transaction(self, next_week: Optional[int] = None) -> RosterValidationResult: + + async def validate_transaction( + self, next_week: Optional[int] = None + ) -> RosterValidationResult: """ Validate the current transaction and return detailed results. @@ -296,17 +358,17 @@ class TransactionBuilder: # Load existing transactions if next_week is provided if next_week is not None: await self.load_existing_transactions(next_week) - + if not self._current_roster: return RosterValidationResult( is_legal=False, major_league_count=0, - minor_league_count=0, + minor_league_count=0, warnings=[], errors=["Could not load current roster data"], - suggestions=[] + suggestions=[], ) - + # Calculate roster changes from moves ml_changes = 0 mil_changes = 0 @@ -315,8 +377,12 @@ class TransactionBuilder: suggestions = [] # Calculate current sWAR for each roster - current_ml_swar = sum(player.wara for player in self._current_roster.active_players) - current_mil_swar = sum(player.wara for player in self._current_roster.minor_league_players) + current_ml_swar = sum( + player.wara for player in self._current_roster.active_players + ) + current_mil_swar = sum( + player.wara for player in self._current_roster.minor_league_players + ) # Track sWAR changes from moves ml_swar_changes = 0.0 @@ -371,46 +437,66 @@ class TransactionBuilder: for move in self.moves: # Log move being processed for diagnostics - logger.debug(f"🔍 VALIDATION: Processing move - {move.player.name} (ID={move.player.id})") - logger.debug(f"🔍 VALIDATION: from_roster={move.from_roster.value}, to_roster={move.to_roster.value}") + logger.debug( + f"🔍 VALIDATION: Processing move - {move.player.name} (ID={move.player.id})" + ) + logger.debug( + f"🔍 VALIDATION: from_roster={move.from_roster.value}, to_roster={move.to_roster.value}" + ) # Calculate roster changes based on from/to locations if move.from_roster == RosterType.MAJOR_LEAGUE: ml_changes -= 1 ml_swar_changes -= move.player.wara - logger.debug(f"🔍 VALIDATION: ML decrement - ml_changes now {ml_changes}") + logger.debug( + f"🔍 VALIDATION: ML decrement - ml_changes now {ml_changes}" + ) elif move.from_roster == RosterType.MINOR_LEAGUE: mil_changes -= 1 mil_swar_changes -= move.player.wara - logger.debug(f"🔍 VALIDATION: MiL decrement - mil_changes now {mil_changes}") + logger.debug( + f"🔍 VALIDATION: MiL decrement - mil_changes now {mil_changes}" + ) # Note: INJURED_LIST and FREE_AGENCY don't count toward ML roster limit if move.to_roster == RosterType.MAJOR_LEAGUE: ml_changes += 1 ml_swar_changes += move.player.wara - logger.debug(f"🔍 VALIDATION: ML increment - ml_changes now {ml_changes}") + logger.debug( + f"🔍 VALIDATION: ML increment - ml_changes now {ml_changes}" + ) elif move.to_roster == RosterType.MINOR_LEAGUE: mil_changes += 1 mil_swar_changes += move.player.wara - logger.debug(f"🔍 VALIDATION: MiL increment - mil_changes now {mil_changes}") + logger.debug( + f"🔍 VALIDATION: MiL increment - mil_changes now {mil_changes}" + ) # Note: INJURED_LIST and FREE_AGENCY don't count toward ML roster limit - + # Calculate projected roster sizes and sWAR # Only Major League players count toward ML roster limit (IL and MiL are separate) current_ml_size = len(self._current_roster.active_players) current_mil_size = len(self._current_roster.minor_league_players) - logger.debug(f"🔍 VALIDATION: Current roster - ML:{current_ml_size}, MiL:{current_mil_size}") - logger.debug(f"🔍 VALIDATION: Changes calculated - ml_changes:{ml_changes}, mil_changes:{mil_changes}") + logger.debug( + f"🔍 VALIDATION: Current roster - ML:{current_ml_size}, MiL:{current_mil_size}" + ) + logger.debug( + f"🔍 VALIDATION: Changes calculated - ml_changes:{ml_changes}, mil_changes:{mil_changes}" + ) projected_ml_size = current_ml_size + ml_changes projected_mil_size = current_mil_size + mil_changes projected_ml_swar = current_ml_swar + ml_swar_changes projected_mil_swar = current_mil_swar + mil_swar_changes - logger.debug(f"🔍 VALIDATION: Projected roster - ML:{projected_ml_size}, MiL:{projected_mil_size}") - logger.debug(f"🔍 VALIDATION: Projected sWAR - ML:{projected_ml_swar:.2f}, MiL:{projected_mil_swar:.2f}") - + logger.debug( + f"🔍 VALIDATION: Projected roster - ML:{projected_ml_size}, MiL:{projected_mil_size}" + ) + logger.debug( + f"🔍 VALIDATION: Projected sWAR - ML:{projected_ml_swar:.2f}, MiL:{projected_mil_swar:.2f}" + ) + # Get current week and config to determine roster limits config = get_config() try: @@ -432,38 +518,54 @@ class TransactionBuilder: else: ml_limit = config.ml_roster_limit_late mil_limit = config.mil_roster_limit_late - + # Validate roster limits is_legal = True if projected_ml_size > ml_limit: is_legal = False - errors.append(f"Major League roster would have {projected_ml_size} players (limit: {ml_limit})") - suggestions.append(f"Drop {projected_ml_size - ml_limit} ML player(s) to make roster legal") + errors.append( + f"Major League roster would have {projected_ml_size} players (limit: {ml_limit})" + ) + suggestions.append( + f"Drop {projected_ml_size - ml_limit} ML player(s) to make roster legal" + ) elif projected_ml_size < 0: is_legal = False errors.append("Cannot have negative players on Major League roster") - + if projected_mil_size > mil_limit: is_legal = False - errors.append(f"Minor League roster would have {projected_mil_size} players (limit: {mil_limit})") - suggestions.append(f"Drop {projected_mil_size - mil_limit} MiL player(s) to make roster legal") + errors.append( + f"Minor League roster would have {projected_mil_size} players (limit: {mil_limit})" + ) + suggestions.append( + f"Drop {projected_mil_size - mil_limit} MiL player(s) to make roster legal" + ) elif projected_mil_size < 0: is_legal = False errors.append("Cannot have negative players on Minor League roster") # Validate sWAR cap # Use team-specific cap if set, otherwise fall back to config default - team_swar_cap = self.team.salary_cap if self.team.salary_cap is not None else config.swar_cap_limit + team_swar_cap = ( + self.team.salary_cap + if self.team.salary_cap is not None + else config.swar_cap_limit + ) if projected_ml_swar > team_swar_cap: is_legal = False - errors.append(f"Major League sWAR would be {projected_ml_swar:.2f} (cap: {team_swar_cap:.1f})") + errors.append( + f"Major League sWAR would be {projected_ml_swar:.2f} (cap: {team_swar_cap:.1f})" + ) over_cap = projected_ml_swar - team_swar_cap - suggestions.append(f"Remove {over_cap:.2f} sWAR from ML roster to be under cap") + suggestions.append( + f"Remove {over_cap:.2f} sWAR from ML roster to be under cap" + ) # Add suggestions for empty transaction if not self.moves: suggestions.append("Add player moves to build your transaction") - + return RosterValidationResult( is_legal=is_legal, major_league_count=projected_ml_size, @@ -478,10 +580,12 @@ class TransactionBuilder: major_league_swar_cap=team_swar_cap, pre_existing_ml_swar_change=pre_existing_ml_swar_change, pre_existing_mil_swar_change=pre_existing_mil_swar_change, - pre_existing_transaction_count=pre_existing_count + pre_existing_transaction_count=pre_existing_count, ) - - async def submit_transaction(self, week: int, check_existing_transactions: bool = True) -> List[Transaction]: + + async def submit_transaction( + self, week: int, check_existing_transactions: bool = True + ) -> List[Transaction]: """ Submit the transaction by creating individual Transaction models. @@ -504,11 +608,13 @@ class TransactionBuilder: validation = await self.validate_transaction() if not validation.is_legal: - raise ValueError(f"Cannot submit illegal transaction: {', '.join(validation.errors)}") - + raise ValueError( + f"Cannot submit illegal transaction: {', '.join(validation.errors)}" + ) + transactions = [] move_id = f"Season-{self.season:03d}-Week-{week:02d}-{int(self.created_at.timestamp())}" - + # Create FA team for drops using config value config = get_config() fa_team = Team( @@ -516,9 +622,9 @@ class TransactionBuilder: abbrev="FA", sname="Free Agents", lname="Free Agency", - season=self.season - ) # type: ignore - + season=self.season, + ) # type: ignore + for move in self.moves: # Determine old and new teams based on roster locations # We need to map RosterType to the actual team (ML, MiL, or IL affiliate) @@ -553,7 +659,7 @@ class TransactionBuilder: # For cases where we don't have specific teams, fall back to defaults if not old_team: continue - + # Create transaction transaction = Transaction( id=0, # Will be set by API @@ -564,24 +670,26 @@ class TransactionBuilder: oldteam=old_team, newteam=new_team, cancelled=False, - frozen=False + frozen=False, ) - + transactions.append(transaction) - - logger.info(f"Created {len(transactions)} transactions for submission with move_id {move_id}") + + logger.info( + f"Created {len(transactions)} transactions for submission with move_id {move_id}" + ) return transactions - + def clear_moves(self) -> None: """Clear all moves from the transaction builder.""" self.moves.clear() logger.info("Cleared all moves from transaction builder") - + @property def is_empty(self) -> bool: """Check if transaction builder has no moves.""" return len(self.moves) == 0 - + @property def move_count(self) -> int: """Get total number of moves in transaction.""" @@ -595,17 +703,17 @@ _active_builders: Dict[int, TransactionBuilder] = {} def get_transaction_builder(user_id: int, team: Team) -> TransactionBuilder: """ Get or create a transaction builder for a user. - + Args: user_id: Discord user ID team: Team object - + Returns: TransactionBuilder instance """ if user_id not in _active_builders: _active_builders[user_id] = TransactionBuilder(team, user_id) - + return _active_builders[user_id] @@ -613,4 +721,4 @@ def clear_transaction_builder(user_id: int) -> None: """Clear transaction builder for a user.""" if user_id in _active_builders: del _active_builders[user_id] - logger.info(f"Cleared transaction builder for user {user_id}") \ No newline at end of file + logger.info(f"Cleared transaction builder for user {user_id}") diff --git a/services/transaction_service.py b/services/transaction_service.py index 44e2a01..80ce0e5 100644 --- a/services/transaction_service.py +++ b/services/transaction_service.py @@ -3,85 +3,91 @@ Transaction service for Discord Bot v2.0 Handles transaction CRUD operations and business logic. """ + import logging -from typing import Optional, List +from typing import Optional, List, Union from datetime import datetime, UTC from services.base_service import BaseService from models.transaction import Transaction, RosterValidation from exceptions import APIException -logger = logging.getLogger(f'{__name__}.TransactionService') +logger = logging.getLogger(f"{__name__}.TransactionService") class TransactionService(BaseService[Transaction]): """Service for transaction operations.""" - + def __init__(self): """Initialize transaction service.""" - super().__init__( - model_class=Transaction, - endpoint='transactions' - ) + super().__init__(model_class=Transaction, endpoint="transactions") logger.debug("TransactionService initialized") - + async def get_team_transactions( - self, - team_abbrev: str, + self, + team_abbrev: Union[str, List[str]], season: int, cancelled: Optional[bool] = None, frozen: Optional[bool] = None, week_start: Optional[int] = None, - week_end: Optional[int] = None + week_end: Optional[int] = None, ) -> List[Transaction]: """ - Get transactions for a specific team. - + Get transactions for a specific team or list of teams. + Args: - team_abbrev: Team abbreviation + team_abbrev: Team abbreviation or list of abbreviations (e.g., ["POR", "PORMIL", "PORIL"]) season: Season number cancelled: Filter by cancelled status frozen: Filter by frozen status week_start: Start week for filtering week_end: End week for filtering - + Returns: List of matching transactions """ try: - params = [ - ('season', str(season)), - ('team_abbrev', team_abbrev) - ] - + params = [("season", str(season))] + + # Support single string or list of abbreviations + if isinstance(team_abbrev, list): + for abbrev in team_abbrev: + params.append(("team_abbrev", abbrev)) + else: + params.append(("team_abbrev", team_abbrev)) + if cancelled is not None: - params.append(('cancelled', str(cancelled).lower())) + params.append(("cancelled", str(cancelled).lower())) if frozen is not None: - params.append(('frozen', str(frozen).lower())) + params.append(("frozen", str(frozen).lower())) if week_start is not None: - params.append(('week_start', str(week_start))) + params.append(("week_start", str(week_start))) if week_end is not None: - params.append(('week_end', str(week_end))) - + params.append(("week_end", str(week_end))) + transactions = await self.get_all_items(params=params) - + # Sort by week, then by moveid transactions.sort(key=lambda t: (t.week, t.moveid)) - - logger.debug(f"Retrieved {len(transactions)} transactions for {team_abbrev}") + + logger.debug( + f"Retrieved {len(transactions)} transactions for {team_abbrev}" + ) return transactions - + except Exception as e: logger.error(f"Error getting transactions for team {team_abbrev}: {e}") raise APIException(f"Failed to retrieve transactions: {e}") - - async def get_pending_transactions(self, team_abbrev: str, season: int) -> List[Transaction]: + + async def get_pending_transactions( + self, team_abbrev: str, season: int + ) -> List[Transaction]: """Get pending (future) transactions for a team.""" try: # Get current week to filter future transactions current_data = await self.get_client() - current_response = await current_data.get('current') - current_week = current_response.get('week', 0) if current_response else 0 + current_response = await current_data.get("current") + current_week = current_response.get("week", 0) if current_response else 0 # Get transactions from current week onward return await self.get_team_transactions( @@ -89,105 +95,105 @@ class TransactionService(BaseService[Transaction]): season, cancelled=False, frozen=False, - week_start=current_week + week_start=current_week, ) except Exception as e: - logger.warning(f"Could not get current week, returning all non-cancelled/non-frozen transactions: {e}") + logger.warning( + f"Could not get current week, returning all non-cancelled/non-frozen transactions: {e}" + ) # Fallback to all non-cancelled/non-frozen if we can't get current week return await self.get_team_transactions( - team_abbrev, - season, - cancelled=False, - frozen=False + team_abbrev, season, cancelled=False, frozen=False ) - - async def get_frozen_transactions(self, team_abbrev: str, season: int) -> List[Transaction]: + + async def get_frozen_transactions( + self, team_abbrev: str, season: int + ) -> List[Transaction]: """Get frozen (scheduled for processing) transactions for a team.""" - return await self.get_team_transactions( - team_abbrev, - season, - frozen=True - ) - + return await self.get_team_transactions(team_abbrev, season, frozen=True) + async def get_processed_transactions( - self, - team_abbrev: str, - season: int, - recent_weeks: int = 4 + self, team_abbrev: str, season: int, recent_weeks: int = 4 ) -> List[Transaction]: """Get recently processed transactions for a team.""" # Get current week to limit search try: current_data = await self.get_client() - current_response = await current_data.get('current') - current_week = current_response.get('week', 0) if current_response else 0 - + current_response = await current_data.get("current") + current_week = current_response.get("week", 0) if current_response else 0 + week_start = max(1, current_week - recent_weeks) - + # For processed transactions, we need to filter by completed/processed status # Since the API structure doesn't have a processed status, we'll get all non-pending/non-frozen all_transactions = await self.get_team_transactions( - team_abbrev, - season, - week_start=week_start + team_abbrev, season, week_start=week_start ) # Filter for transactions that are neither pending nor frozen (i.e., processed) - processed = [t for t in all_transactions if not t.is_pending and not t.is_frozen and not t.cancelled] + processed = [ + t + for t in all_transactions + if not t.is_pending and not t.is_frozen and not t.cancelled + ] return processed except Exception as e: logger.warning(f"Could not get current week, using basic query: {e}") - all_transactions = await self.get_team_transactions( - team_abbrev, - season - ) + all_transactions = await self.get_team_transactions(team_abbrev, season) # Filter for processed transactions - processed = [t for t in all_transactions if not t.is_pending and not t.is_frozen and not t.cancelled] + processed = [ + t + for t in all_transactions + if not t.is_pending and not t.is_frozen and not t.cancelled + ] return processed - + async def validate_transaction(self, transaction: Transaction) -> RosterValidation: """ Validate a transaction for legality. - + Args: transaction: Transaction to validate - + Returns: Validation results with any errors or warnings """ try: validation = RosterValidation(is_legal=True) - + # Basic validation rules for single-move transactions if not transaction.player: validation.is_legal = False validation.errors.append("Transaction has no player") - + if not transaction.oldteam or not transaction.newteam: validation.is_legal = False validation.errors.append("Transaction missing team information") - + # Validate player eligibility (basic checks) if transaction.player and transaction.player.wara < 0: validation.warnings.append("Player has negative WARA") - + # Add more validation logic as needed # - Roster size limits # - Position requirements # - Contract constraints # - etc. - - logger.debug(f"Validated transaction {transaction.id}: legal={validation.is_legal}") + + logger.debug( + f"Validated transaction {transaction.id}: legal={validation.is_legal}" + ) return validation - + except Exception as e: logger.error(f"Error validating transaction {transaction.id}: {e}") # Return failed validation on error return RosterValidation( - is_legal=False, - errors=[f"Validation error: {str(e)}"] + is_legal=False, errors=[f"Validation error: {str(e)}"] ) - async def create_transaction_batch(self, transactions: List[Transaction]) -> List[Transaction]: + async def create_transaction_batch( + self, transactions: List[Transaction] + ) -> List[Transaction]: """ Create multiple transactions via API POST (for immediate execution). @@ -233,15 +239,12 @@ class TransactionService(BaseService[Transaction]): "season": transaction.season, "moveid": transaction.moveid, "cancelled": transaction.cancelled or False, - "frozen": transaction.frozen or False + "frozen": transaction.frozen or False, } moves.append(move) # Create batch request payload - batch_data = { - "count": len(moves), - "moves": moves - } + batch_data = {"count": len(moves), "moves": moves} # POST batch to API client = await self.get_client() @@ -249,7 +252,11 @@ class TransactionService(BaseService[Transaction]): # API returns a string like "2 transactions have been added" # We need to return the original Transaction objects (they won't have IDs assigned by API) - if response and isinstance(response, str) and "transactions have been added" in response: + if ( + response + and isinstance(response, str) + and "transactions have been added" in response + ): logger.info(f"Successfully created batch: {response}") return transactions else: @@ -276,8 +283,8 @@ class TransactionService(BaseService[Transaction]): try: # Update transaction status using direct API call to handle bulk updates update_data = { - 'cancelled': True, - 'cancelled_at': datetime.now(UTC).isoformat() + "cancelled": True, + "cancelled_at": datetime.now(UTC).isoformat(), } # Call API directly since bulk update returns a message string, not a Transaction object @@ -286,12 +293,12 @@ class TransactionService(BaseService[Transaction]): self.endpoint, update_data, object_id=transaction_id, - use_query_params=True + use_query_params=True, ) # Check if response indicates success # Response will be a string like "Updated 4 transactions" for bulk updates - if response and (isinstance(response, str) and 'Updated' in response): + if response and (isinstance(response, str) and "Updated" in response): logger.info(f"Cancelled transaction(s) {transaction_id}: {response}") return True elif response: @@ -324,14 +331,14 @@ class TransactionService(BaseService[Transaction]): client = await self.get_client() response = await client.patch( self.endpoint, - {'frozen': False}, + {"frozen": False}, object_id=transaction_id, - use_query_params=True + use_query_params=True, ) # Check if response indicates success # Response will be a string like "Updated 4 transactions" for bulk updates - if response and (isinstance(response, str) and 'Updated' in response): + if response and (isinstance(response, str) and "Updated" in response): logger.info(f"Unfroze transaction(s) {transaction_id}: {response}") return True elif response: @@ -347,10 +354,7 @@ class TransactionService(BaseService[Transaction]): return False async def get_frozen_transactions_by_week( - self, - season: int, - week_start: int, - week_end: int + self, season: int, week_start: int, week_end: int ) -> List[Transaction]: """ Get all frozen transactions for a week range (all teams). @@ -368,25 +372,27 @@ class TransactionService(BaseService[Transaction]): """ try: params = [ - ('season', str(season)), - ('week_start', str(week_start)), - ('week_end', str(week_end)), - ('frozen', 'true') + ("season", str(season)), + ("week_start", str(week_start)), + ("week_end", str(week_end)), + ("frozen", "true"), ] transactions = await self.get_all_items(params=params) - logger.debug(f"Retrieved {len(transactions)} frozen transactions for weeks {week_start}-{week_end}") + logger.debug( + f"Retrieved {len(transactions)} frozen transactions for weeks {week_start}-{week_end}" + ) return transactions except Exception as e: - logger.error(f"Error getting frozen transactions for weeks {week_start}-{week_end}: {e}") + logger.error( + f"Error getting frozen transactions for weeks {week_start}-{week_end}: {e}" + ) return [] async def get_regular_transactions_by_week( - self, - season: int, - week: int + self, season: int, week: int ) -> List[Transaction]: """ Get non-frozen, non-cancelled transactions for a specific week. @@ -404,62 +410,66 @@ class TransactionService(BaseService[Transaction]): """ try: params = [ - ('season', str(season)), - ('week_start', str(week)), - ('week_end', str(week)), - ('frozen', 'false'), - ('cancelled', 'false') + ("season", str(season)), + ("week_start", str(week)), + ("week_end", str(week)), + ("frozen", "false"), + ("cancelled", "false"), ] transactions = await self.get_all_items(params=params) - logger.debug(f"Retrieved {len(transactions)} regular transactions for week {week}") + logger.debug( + f"Retrieved {len(transactions)} regular transactions for week {week}" + ) return transactions except Exception as e: logger.error(f"Error getting regular transactions for week {week}: {e}") return [] - async def get_contested_transactions(self, season: int, week: int) -> List[Transaction]: + async def get_contested_transactions( + self, season: int, week: int + ) -> List[Transaction]: """ Get transactions that may be contested (multiple teams want same player). - + Args: season: Season number week: Week number - + Returns: List of potentially contested transactions """ try: # Get all pending transactions for the week params = [ - ('season', str(season)), - ('week', str(week)), - ('cancelled', 'false'), - ('frozen', 'false') + ("season", str(season)), + ("week", str(week)), + ("cancelled", "false"), + ("frozen", "false"), ] - + transactions = await self.get_all_items(params=params) - + # Group by players being targeted (simplified contest detection) player_target_map = {} contested = [] - + for transaction in transactions: # In the new model, each transaction is a single player move # Contest occurs when multiple teams try to acquire the same player - if transaction.newteam.abbrev != 'FA': # Not dropping to free agency + if transaction.newteam.abbrev != "FA": # Not dropping to free agency player_name = transaction.player.name.lower() if player_name not in player_target_map: player_target_map[player_name] = [] player_target_map[player_name].append(transaction) - + # Find contested players (wanted by multiple teams) for player_name, player_transactions in player_target_map.items(): if len(player_transactions) > 1: contested.extend(player_transactions) - + # Remove duplicates while preserving order seen = set() result = [] @@ -467,8 +477,10 @@ class TransactionService(BaseService[Transaction]): if transaction.id not in seen: seen.add(transaction.id) result.append(transaction) - - logger.debug(f"Found {len(result)} potentially contested transactions for week {week}") + + logger.debug( + f"Found {len(result)} potentially contested transactions for week {week}" + ) return result except Exception as e: @@ -476,10 +488,7 @@ class TransactionService(BaseService[Transaction]): return [] async def is_player_in_pending_transaction( - self, - player_id: int, - week: int, - season: int + self, player_id: int, week: int, season: int ) -> tuple[bool, Optional[str]]: """ Check if a player is already in a pending transaction for a specific week. @@ -499,10 +508,10 @@ class TransactionService(BaseService[Transaction]): # Get all pending transactions for the week (all teams) # Use week_start to filter out keepers (week=0) and earlier transactions params = [ - ('season', str(season)), - ('week_start', str(week)), - ('cancelled', 'false'), - ('frozen', 'false') + ("season", str(season)), + ("week_start", str(week)), + ("cancelled", "false"), + ("frozen", "false"), ] transactions = await self.get_all_items(params=params) @@ -511,7 +520,9 @@ class TransactionService(BaseService[Transaction]): for transaction in transactions: if transaction.player and transaction.player.id == player_id: # Found the player in a pending transaction - claiming_team = transaction.newteam.abbrev if transaction.newteam else "Unknown" + claiming_team = ( + transaction.newteam.abbrev if transaction.newteam else "Unknown" + ) logger.info( f"Player {player_id} already in pending transaction for week {week} " f"(claimed by {claiming_team})" @@ -521,11 +532,13 @@ class TransactionService(BaseService[Transaction]): return False, None except Exception as e: - logger.error(f"Error checking pending transactions for player {player_id}: {e}") + logger.error( + f"Error checking pending transactions for player {player_id}: {e}" + ) # On error, allow the transaction (fail open) but log the issue # The freeze task will still catch duplicates if they occur return False, None # Global service instance -transaction_service = TransactionService() \ No newline at end of file +transaction_service = TransactionService() diff --git a/tests/test_roster_validation_org_affiliates.py b/tests/test_roster_validation_org_affiliates.py new file mode 100644 index 0000000..83af678 --- /dev/null +++ b/tests/test_roster_validation_org_affiliates.py @@ -0,0 +1,277 @@ +""" +Tests for roster validation with organizational affiliate transactions. + +Verifies that pending trades involving MiL/IL affiliate teams are correctly +included in roster projections. This was a bug where load_existing_transactions +only queried for the base team abbreviation (e.g., "POR") and missed +transactions involving affiliates ("PORMIL", "PORIL"). + +Fixes: https://git.manticorum.com/cal/major-domo-v2/issues/49 +""" + +import pytest +from unittest.mock import AsyncMock, patch + +from services.transaction_builder import ( + TransactionBuilder, + TransactionMove, + clear_transaction_builder, +) +from models.team import Team, RosterType +from models.player import Player +from models.roster import TeamRoster +from models.transaction import Transaction +from models.current import Current +from tests.factories import PlayerFactory, TeamFactory + + +def _make_player(id: int, name: str, wara: float = 1.0) -> Player: + """Create a minimal Player for testing.""" + return PlayerFactory.create(id=id, name=name, wara=wara) + + +def _make_team(id: int, abbrev: str, sname: str, lname: str) -> Team: + """Create a minimal Team for testing.""" + return TeamFactory.create(id=id, abbrev=abbrev, sname=sname, lname=lname) + + +class TestLoadExistingTransactionsOrgAffiliates: + """Tests that load_existing_transactions queries all org affiliates.""" + + @pytest.fixture + def por_team(self): + """POR major league team.""" + return _make_team(100, "POR", "Porters", "Portland Porters") + + @pytest.fixture + def por_mil_team(self): + """PORMIL minor league team.""" + return _make_team(101, "PORMIL", "MiL Porters", "Portland Porters MiL") + + @pytest.fixture + def por_il_team(self): + """PORIL injured list team.""" + return _make_team(102, "PORIL", "Porter IL", "Portland Porters IL") + + @pytest.fixture + def other_team(self): + """Another team for trades.""" + return _make_team(200, "NY", "Yankees", "New York Yankees") + + @pytest.fixture + def other_mil_team(self): + """Another team's MiL.""" + return _make_team(201, "NYMIL", "RailRiders", "New York RailRiders") + + @pytest.fixture + def mock_roster_at_mil_limit(self, por_team): + """POR roster with MiL at the 6-player limit. + + When MiL is already at limit, adding a player from IL to MiL + should be illegal UNLESS a pending trade clears a spot. + """ + ml_players = [_make_player(1000 + i, f"ML Player {i}") for i in range(24)] + mil_players = [_make_player(2000 + i, f"MiL Player {i}") for i in range(6)] + return TeamRoster( + team_id=por_team.id, + team_abbrev=por_team.abbrev, + week=10, + season=12, + active_players=ml_players, + minor_league_players=mil_players, + ) + + @pytest.mark.asyncio + async def test_load_existing_transactions_queries_all_org_abbrevs(self, por_team): + """Verify load_existing_transactions passes ML, MiL, and IL abbreviations. + + This is the core fix: the API's team_abbrev filter does exact matching, + so we must query for all organizational affiliates. + """ + with patch( + "services.transaction_builder.transaction_service" + ) as mock_tx_service: + mock_tx_service.get_team_transactions = AsyncMock(return_value=[]) + + builder = TransactionBuilder(por_team, user_id=1, season=12) + await builder.load_existing_transactions(next_week=11) + + # Verify the call included all org affiliates + mock_tx_service.get_team_transactions.assert_called_once_with( + team_abbrev=["POR", "PORMIL", "PORIL"], + season=12, + cancelled=False, + week_start=11, + ) + + @pytest.mark.asyncio + async def test_trade_clearing_mil_spot_allows_il_to_mil_move( + self, por_team, por_mil_team, por_il_team, other_team, mock_roster_at_mil_limit + ): + """Test the exact POR scenario: trade clears MiL spot, allowing IL→MiL move. + + Scenario: + - POR has 6 MiL players (at limit) + - POR traded a MiL player to NY for next week (transaction: PORMIL → NY) + - POR wants to move Jeffrey Springs from IL to MiL next week + - Without the fix: validation doesn't see the trade, says "7 players (limit: 6)" + - With the fix: validation sees the trade (-1 MiL), allows the move (6 - 1 + 1 = 6) + """ + # The trade: a PORMIL player going to NY next week + traded_player = _make_player(3000, "Traded Away", wara=1.5) + trade_transaction = Transaction.from_api_data( + { + "id": 999, + "week": 11, + "season": 12, + "moveid": "trade-001", + "player": { + "id": traded_player.id, + "name": traded_player.name, + "wara": traded_player.wara, + "season": 12, + "pos_1": "SP", + }, + "oldteam": { + "id": por_mil_team.id, + "abbrev": por_mil_team.abbrev, + "sname": por_mil_team.sname, + "lname": por_mil_team.lname, + "season": 12, + }, + "newteam": { + "id": other_team.id, + "abbrev": other_team.abbrev, + "sname": other_team.sname, + "lname": other_team.lname, + "season": 12, + }, + "cancelled": False, + "frozen": False, + } + ) + + # Jeffrey Springs: moving from IL to MiL + springs = _make_player(4000, "Jeffrey Springs", wara=0.8) + + with patch( + "services.transaction_builder.roster_service" + ) as mock_roster_service: + with patch( + "services.transaction_builder.transaction_service" + ) as mock_tx_service: + with patch( + "services.transaction_builder.league_service" + ) as mock_league_service: + mock_roster_service.get_current_roster = AsyncMock( + return_value=mock_roster_at_mil_limit + ) + # Return the trade transaction when queried with org abbrevs + mock_tx_service.get_team_transactions = AsyncMock( + return_value=[trade_transaction] + ) + mock_league_service.get_current_state = AsyncMock( + return_value=Current(week=10, season=12, freeze=False) + ) + + builder = TransactionBuilder(por_team, user_id=1, season=12) + + # Add the IL → MiL move + move = TransactionMove( + player=springs, + from_roster=RosterType.INJURED_LIST, + to_roster=RosterType.MINOR_LEAGUE, + from_team=por_team, + to_team=por_team, + ) + await builder.add_move(move, check_pending_transactions=False) + + # Validate with next_week to trigger pre-existing transaction loading + validation = await builder.validate_transaction(next_week=11) + + # With the trade factored in: 6 (current) - 1 (trade out) + 1 (Springs) = 6 + assert validation.is_legal is True + assert validation.minor_league_count == 6 + assert validation.pre_existing_transaction_count == 1 + + @pytest.mark.asyncio + async def test_without_trade_il_to_mil_is_illegal( + self, por_team, mock_roster_at_mil_limit + ): + """Without any pending trade, IL→MiL move should be illegal at MiL limit. + + This confirms the baseline behavior: if there's no trade clearing a spot, + validation correctly rejects the move. + """ + springs = _make_player(4000, "Jeffrey Springs", wara=0.8) + + with patch( + "services.transaction_builder.roster_service" + ) as mock_roster_service: + with patch( + "services.transaction_builder.transaction_service" + ) as mock_tx_service: + with patch( + "services.transaction_builder.league_service" + ) as mock_league_service: + mock_roster_service.get_current_roster = AsyncMock( + return_value=mock_roster_at_mil_limit + ) + mock_tx_service.get_team_transactions = AsyncMock(return_value=[]) + mock_league_service.get_current_state = AsyncMock( + return_value=Current(week=10, season=12, freeze=False) + ) + + builder = TransactionBuilder(por_team, user_id=1, season=12) + move = TransactionMove( + player=springs, + from_roster=RosterType.INJURED_LIST, + to_roster=RosterType.MINOR_LEAGUE, + from_team=por_team, + to_team=por_team, + ) + await builder.add_move(move, check_pending_transactions=False) + + validation = await builder.validate_transaction(next_week=11) + + # Without trade: 6 (current) + 1 (Springs) = 7 > 6 limit + assert validation.is_legal is False + assert validation.minor_league_count == 7 + assert "7 players (limit: 6)" in validation.errors[0] + + +class TestGetTeamTransactionsListSupport: + """Tests that get_team_transactions accepts a list of abbreviations.""" + + @pytest.mark.asyncio + async def test_single_string_backwards_compatible(self): + """Passing a single string still works as before.""" + from services.transaction_service import TransactionService + + service = TransactionService() + with patch.object(service, "get_all_items", new_callable=AsyncMock) as mock_get: + mock_get.return_value = [] + await service.get_team_transactions("POR", 12) + + # Should have a single team_abbrev param + call_params = mock_get.call_args[1]["params"] + team_abbrev_params = [p for p in call_params if p[0] == "team_abbrev"] + assert len(team_abbrev_params) == 1 + assert team_abbrev_params[0][1] == "POR" + + @pytest.mark.asyncio + async def test_list_of_abbreviations(self): + """Passing a list sends multiple team_abbrev params.""" + from services.transaction_service import TransactionService + + service = TransactionService() + with patch.object(service, "get_all_items", new_callable=AsyncMock) as mock_get: + mock_get.return_value = [] + await service.get_team_transactions(["POR", "PORMIL", "PORIL"], 12) + + call_params = mock_get.call_args[1]["params"] + team_abbrev_params = [p for p in call_params if p[0] == "team_abbrev"] + assert len(team_abbrev_params) == 3 + assert team_abbrev_params[0][1] == "POR" + assert team_abbrev_params[1][1] == "PORMIL" + assert team_abbrev_params[2][1] == "PORIL"