Add pending transaction validation for /dropadd command
Prevents players who are already claimed in another team's pending transaction (frozen=false, cancelled=false) from being added to a new transaction for the same week. Changes: - Add is_player_in_pending_transaction() to TransactionService - Make TransactionBuilder.add_move() async with validation - Add check_pending_transactions flag (default True for /dropadd) - Skip validation for /ilmove and trades (check_pending_transactions=False) - Add tests/conftest.py for proper test isolation - Add 4 new tests for pending transaction validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
466c8985cb
commit
3cbc904478
@ -234,7 +234,7 @@ class DropAddCommands(commands.Cog):
|
|||||||
to_team=None if to_roster == RosterType.FREE_AGENCY else builder.team
|
to_team=None if to_roster == RosterType.FREE_AGENCY else builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success, error_message = builder.add_move(move)
|
success, error_message = await builder.add_move(move)
|
||||||
if not success:
|
if not success:
|
||||||
self.logger.warning(f"Failed to add quick move: {error_message}")
|
self.logger.warning(f"Failed to add quick move: {error_message}")
|
||||||
return False, error_message
|
return False, error_message
|
||||||
|
|||||||
@ -29,6 +29,7 @@ from services.transaction_builder import (
|
|||||||
)
|
)
|
||||||
from services.player_service import player_service
|
from services.player_service import player_service
|
||||||
from services.team_service import team_service
|
from services.team_service import team_service
|
||||||
|
from services.league_service import league_service
|
||||||
from views.transaction_embed import TransactionEmbedView, create_transaction_embed
|
from views.transaction_embed import TransactionEmbedView, create_transaction_embed
|
||||||
|
|
||||||
|
|
||||||
@ -255,7 +256,8 @@ class ILMoveCommands(commands.Cog):
|
|||||||
to_team=to_team
|
to_team=to_team
|
||||||
)
|
)
|
||||||
|
|
||||||
success, error_message = builder.add_move(move)
|
# For /ilmove, skip pending transaction check (immediate moves don't need it)
|
||||||
|
success, error_message = await builder.add_move(move, check_pending_transactions=False)
|
||||||
if not success:
|
if not success:
|
||||||
self.logger.warning(f"Failed to add quick move: {error_message}")
|
self.logger.warning(f"Failed to add quick move: {error_message}")
|
||||||
return False, error_message
|
return False, error_message
|
||||||
|
|||||||
@ -318,14 +318,15 @@ class TradeBuilder:
|
|||||||
)
|
)
|
||||||
|
|
||||||
# Add moves to respective builders
|
# Add moves to respective builders
|
||||||
from_success, from_error = from_builder.add_move(from_move)
|
# 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)
|
||||||
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 = to_builder.add_move(to_move)
|
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)
|
||||||
@ -383,7 +384,8 @@ class TradeBuilder:
|
|||||||
to_team=team
|
to_team=team
|
||||||
)
|
)
|
||||||
|
|
||||||
success, error = builder.add_move(trans_move)
|
# Skip pending transaction check for trade supplementary moves
|
||||||
|
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
|
||||||
|
|||||||
@ -194,17 +194,25 @@ class TransactionBuilder:
|
|||||||
self._existing_transactions = []
|
self._existing_transactions = []
|
||||||
self._existing_transactions_loaded = True
|
self._existing_transactions_loaded = True
|
||||||
|
|
||||||
def add_move(self, move: TransactionMove) -> tuple[bool, str]:
|
async def add_move(
|
||||||
|
self,
|
||||||
|
move: TransactionMove,
|
||||||
|
next_week: Optional[int] = None,
|
||||||
|
check_pending_transactions: bool = True
|
||||||
|
) -> tuple[bool, str]:
|
||||||
"""
|
"""
|
||||||
Add a move to the transaction.
|
Add a move to the transaction.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
move: TransactionMove to add
|
move: TransactionMove to add
|
||||||
|
next_week: Week number for pending transaction check (optional, will be fetched if not provided)
|
||||||
|
check_pending_transactions: Whether to check if player is in another team's pending
|
||||||
|
transaction. Set to True for /dropadd (scheduled moves), False for /ilmove (immediate).
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
Tuple of (success: bool, error_message: str). If success is True, error_message is empty.
|
Tuple of (success: bool, error_message: str). If success is True, error_message is empty.
|
||||||
"""
|
"""
|
||||||
# Check if player is already in a move
|
# Check if player is already in a move in this transaction builder
|
||||||
existing_move = self.get_move_for_player(move.player.id)
|
existing_move = self.get_move_for_player(move.player.id)
|
||||||
if existing_move:
|
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"
|
||||||
@ -219,6 +227,29 @@ class TransactionBuilder:
|
|||||||
logger.warning(error_msg)
|
logger.warning(error_msg)
|
||||||
return False, error_msg
|
return False, error_msg
|
||||||
|
|
||||||
|
# Check if player is already in another team's pending transaction for next week
|
||||||
|
# This prevents duplicate claims that would need to be resolved at freeze time
|
||||||
|
# Only applies to /dropadd (scheduled moves), not /ilmove (immediate moves)
|
||||||
|
if check_pending_transactions:
|
||||||
|
if next_week is None:
|
||||||
|
try:
|
||||||
|
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}")
|
||||||
|
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
|
||||||
|
)
|
||||||
|
|
||||||
|
if is_pending:
|
||||||
|
error_msg = f"{move.player.name} is already in a pending transaction for week {next_week} (claimed by {claiming_team})"
|
||||||
|
logger.warning(error_msg)
|
||||||
|
return False, error_msg
|
||||||
|
|
||||||
self.moves.append(move)
|
self.moves.append(move)
|
||||||
logger.info(f"Added move: {move.description}")
|
logger.info(f"Added move: {move.description}")
|
||||||
return True, ""
|
return True, ""
|
||||||
|
|||||||
@ -434,11 +434,61 @@ class TransactionService(BaseService[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
|
return result
|
||||||
|
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.error(f"Error getting contested transactions: {e}")
|
logger.error(f"Error getting contested transactions: {e}")
|
||||||
return []
|
return []
|
||||||
|
|
||||||
|
async def is_player_in_pending_transaction(
|
||||||
|
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.
|
||||||
|
|
||||||
|
This checks ALL teams' pending transactions (frozen=false, cancelled=false)
|
||||||
|
to prevent duplicate claims on the same player.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
player_id: Player ID to check
|
||||||
|
week: Week number to check
|
||||||
|
season: Season number
|
||||||
|
|
||||||
|
Returns:
|
||||||
|
Tuple of (is_in_pending_transaction, claiming_team_abbrev or None)
|
||||||
|
"""
|
||||||
|
try:
|
||||||
|
# Get all pending transactions for the week (all teams)
|
||||||
|
params = [
|
||||||
|
('season', str(season)),
|
||||||
|
('week', str(week)),
|
||||||
|
('cancelled', 'false'),
|
||||||
|
('frozen', 'false')
|
||||||
|
]
|
||||||
|
|
||||||
|
transactions = await self.get_all_items(params=params)
|
||||||
|
|
||||||
|
# Check if the player is in any of these transactions
|
||||||
|
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"
|
||||||
|
logger.info(
|
||||||
|
f"Player {player_id} already in pending transaction for week {week} "
|
||||||
|
f"(claimed by {claiming_team})"
|
||||||
|
)
|
||||||
|
return True, claiming_team
|
||||||
|
|
||||||
|
return False, None
|
||||||
|
|
||||||
|
except Exception as 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
|
# Global service instance
|
||||||
transaction_service = TransactionService()
|
transaction_service = TransactionService()
|
||||||
53
tests/conftest.py
Normal file
53
tests/conftest.py
Normal file
@ -0,0 +1,53 @@
|
|||||||
|
"""
|
||||||
|
Pytest configuration and fixtures for Discord Bot v2.0 tests.
|
||||||
|
|
||||||
|
This file provides test isolation and shared fixtures.
|
||||||
|
"""
|
||||||
|
import asyncio
|
||||||
|
import os
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
# Ensure environment is set up before any imports happen
|
||||||
|
# This is critical for tests that check GUILD_ID
|
||||||
|
os.environ.setdefault("GUILD_ID", "669356687294988350")
|
||||||
|
os.environ.setdefault("TESTING", "true")
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(autouse=True)
|
||||||
|
def reset_singleton_state():
|
||||||
|
"""
|
||||||
|
Reset any singleton/global state between tests.
|
||||||
|
|
||||||
|
This prevents test pollution from global state in services.
|
||||||
|
"""
|
||||||
|
# Import after test function starts to ensure clean state
|
||||||
|
yield # Run test
|
||||||
|
|
||||||
|
# Cleanup after test
|
||||||
|
# Reset transaction builder caches
|
||||||
|
try:
|
||||||
|
from services.transaction_builder import _transaction_builders
|
||||||
|
_transaction_builders.clear()
|
||||||
|
except ImportError:
|
||||||
|
pass
|
||||||
|
|
||||||
|
try:
|
||||||
|
from services.trade_builder import _trade_builders, _team_to_trade_key
|
||||||
|
_trade_builders.clear()
|
||||||
|
_team_to_trade_key.clear()
|
||||||
|
except ImportError:
|
||||||
|
pass
|
||||||
|
|
||||||
|
# Reset config singleton to ensure clean state
|
||||||
|
try:
|
||||||
|
from config import _config
|
||||||
|
import config as cfg
|
||||||
|
cfg._config = None
|
||||||
|
except (ImportError, AttributeError):
|
||||||
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(scope="session")
|
||||||
|
def event_loop_policy():
|
||||||
|
"""Use default event loop policy."""
|
||||||
|
return asyncio.DefaultEventLoopPolicy()
|
||||||
@ -178,7 +178,7 @@ class TestDropAddCommands:
|
|||||||
"""Test successful quick move addition."""
|
"""Test successful quick move addition."""
|
||||||
mock_builder = MagicMock()
|
mock_builder = MagicMock()
|
||||||
mock_builder.team = mock_team
|
mock_builder.team = mock_team
|
||||||
mock_builder.add_move.return_value = (True, "")
|
mock_builder.add_move = AsyncMock(return_value=(True, "")) # Now async
|
||||||
mock_builder.load_roster_data = AsyncMock()
|
mock_builder.load_roster_data = AsyncMock()
|
||||||
mock_builder._current_roster = MagicMock()
|
mock_builder._current_roster = MagicMock()
|
||||||
mock_builder._current_roster.active_players = []
|
mock_builder._current_roster.active_players = []
|
||||||
@ -262,7 +262,7 @@ class TestDropAddCommands:
|
|||||||
|
|
||||||
mock_builder = MagicMock()
|
mock_builder = MagicMock()
|
||||||
mock_builder.team = mock_team
|
mock_builder.team = mock_team
|
||||||
mock_builder.add_move.return_value = (True, "")
|
mock_builder.add_move = AsyncMock(return_value=(True, "")) # Now async
|
||||||
mock_builder.load_roster_data = AsyncMock()
|
mock_builder.load_roster_data = AsyncMock()
|
||||||
mock_builder._current_roster = MagicMock()
|
mock_builder._current_roster = MagicMock()
|
||||||
mock_builder._current_roster.active_players = []
|
mock_builder._current_roster.active_players = []
|
||||||
|
|||||||
@ -182,6 +182,8 @@ class TestDropAddIntegration:
|
|||||||
mock_player_service.search_players = AsyncMock(return_value=[mock_players[0]]) # Mike Trout
|
mock_player_service.search_players = AsyncMock(return_value=[mock_players[0]]) # Mike Trout
|
||||||
mock_roster_service.get_current_roster = AsyncMock(return_value=mock_roster)
|
mock_roster_service.get_current_roster = AsyncMock(return_value=mock_roster)
|
||||||
mock_tx_service.get_team_transactions = AsyncMock(return_value=[])
|
mock_tx_service.get_team_transactions = AsyncMock(return_value=[])
|
||||||
|
# Mock the new pending transaction check
|
||||||
|
mock_tx_service.is_player_in_pending_transaction = AsyncMock(return_value=(False, None))
|
||||||
|
|
||||||
# Execute /dropadd command with quick move
|
# Execute /dropadd command with quick move
|
||||||
await commands_cog.dropadd.callback(commands_cog,
|
await commands_cog.dropadd.callback(commands_cog,
|
||||||
@ -246,8 +248,8 @@ class TestDropAddIntegration:
|
|||||||
from_team=mock_team
|
from_team=mock_team
|
||||||
)
|
)
|
||||||
|
|
||||||
builder.add_move(add_move)
|
await builder.add_move(add_move, check_pending_transactions=False)
|
||||||
builder.add_move(drop_move)
|
await builder.add_move(drop_move, check_pending_transactions=False)
|
||||||
|
|
||||||
# Verify multi-move transaction
|
# Verify multi-move transaction
|
||||||
assert builder.move_count == 2
|
assert builder.move_count == 2
|
||||||
@ -278,7 +280,7 @@ class TestDropAddIntegration:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=mock_team
|
to_team=mock_team
|
||||||
)
|
)
|
||||||
builder.add_move(move)
|
await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
# Test submission
|
# Test submission
|
||||||
transactions = await builder.submit_transaction(week=11)
|
transactions = await builder.submit_transaction(week=11)
|
||||||
@ -329,7 +331,7 @@ class TestDropAddIntegration:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=mock_team
|
to_team=mock_team
|
||||||
)
|
)
|
||||||
builder.add_move(move)
|
await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
# Submit transactions first to get move IDs
|
# Submit transactions first to get move IDs
|
||||||
transactions = await builder.submit_transaction(week=mock_current_state.week + 1)
|
transactions = await builder.submit_transaction(week=mock_current_state.week + 1)
|
||||||
@ -338,7 +340,7 @@ class TestDropAddIntegration:
|
|||||||
# Reset the builder and add move again for modal test
|
# Reset the builder and add move again for modal test
|
||||||
clear_transaction_builder(mock_interaction.user.id)
|
clear_transaction_builder(mock_interaction.user.id)
|
||||||
builder = get_transaction_builder(mock_interaction.user.id, mock_team)
|
builder = get_transaction_builder(mock_interaction.user.id, mock_team)
|
||||||
builder.add_move(move)
|
await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
# Create the modal
|
# Create the modal
|
||||||
modal = SubmitConfirmationModal(builder)
|
modal = SubmitConfirmationModal(builder)
|
||||||
@ -445,7 +447,7 @@ class TestDropAddIntegration:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=mock_team
|
to_team=mock_team
|
||||||
)
|
)
|
||||||
builder.add_move(move)
|
await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
# Test validation
|
# Test validation
|
||||||
validation = await builder.validate_transaction()
|
validation = await builder.validate_transaction()
|
||||||
@ -483,7 +485,7 @@ class TestDropAddIntegration:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=mock_team
|
to_team=mock_team
|
||||||
)
|
)
|
||||||
builder1.add_move(move)
|
await builder1.add_move(move, check_pending_transactions=False)
|
||||||
assert builder1.move_count == 1
|
assert builder1.move_count == 1
|
||||||
|
|
||||||
# Second command call should get same builder
|
# Second command call should get same builder
|
||||||
|
|||||||
@ -411,8 +411,9 @@ class TestTradeBuilder:
|
|||||||
mock_builder2.validate_transaction = AsyncMock(return_value=valid_result)
|
mock_builder2.validate_transaction = AsyncMock(return_value=valid_result)
|
||||||
|
|
||||||
# Configure add_move methods to return expected tuple (success, error_message)
|
# Configure add_move methods to return expected tuple (success, error_message)
|
||||||
mock_builder1.add_move.return_value = (True, "")
|
# add_move is now async, so use AsyncMock
|
||||||
mock_builder2.add_move.return_value = (True, "")
|
mock_builder1.add_move = AsyncMock(return_value=(True, ""))
|
||||||
|
mock_builder2.add_move = AsyncMock(return_value=(True, ""))
|
||||||
|
|
||||||
def get_builder_side_effect(team):
|
def get_builder_side_effect(team):
|
||||||
if team.id == self.team1.id:
|
if team.id == self.team1.id:
|
||||||
|
|||||||
@ -124,7 +124,8 @@ class TestTransactionBuilder:
|
|||||||
assert builder.move_count == 0
|
assert builder.move_count == 0
|
||||||
assert len(builder.moves) == 0
|
assert len(builder.moves) == 0
|
||||||
|
|
||||||
def test_add_move_success(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_success(self, builder, mock_player):
|
||||||
"""Test successfully adding a move."""
|
"""Test successfully adding a move."""
|
||||||
move = TransactionMove(
|
move = TransactionMove(
|
||||||
player=mock_player,
|
player=mock_player,
|
||||||
@ -133,15 +134,17 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success, error_message = builder.add_move(move)
|
# Skip pending transaction check for unit tests
|
||||||
|
success, error_message = await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
assert success is True
|
assert success is True
|
||||||
assert error_message == ""
|
assert error_message == ""
|
||||||
assert builder.move_count == 1
|
assert builder.move_count == 1
|
||||||
assert builder.is_empty is False
|
assert builder.is_empty is False
|
||||||
assert move in builder.moves
|
assert move in builder.moves
|
||||||
|
|
||||||
def test_add_duplicate_move_fails(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_duplicate_move_fails(self, builder, mock_player):
|
||||||
"""Test that adding duplicate moves for same player fails."""
|
"""Test that adding duplicate moves for same player fails."""
|
||||||
move1 = TransactionMove(
|
move1 = TransactionMove(
|
||||||
player=mock_player,
|
player=mock_player,
|
||||||
@ -157,8 +160,8 @@ class TestTransactionBuilder:
|
|||||||
from_team=builder.team
|
from_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success1, error_message1 = builder.add_move(move1)
|
success1, error_message1 = await builder.add_move(move1, check_pending_transactions=False)
|
||||||
success2, error_message2 = builder.add_move(move2)
|
success2, error_message2 = await builder.add_move(move2, check_pending_transactions=False)
|
||||||
|
|
||||||
assert success1 is True
|
assert success1 is True
|
||||||
assert error_message1 == ""
|
assert error_message1 == ""
|
||||||
@ -166,7 +169,8 @@ class TestTransactionBuilder:
|
|||||||
assert "already has a move" in error_message2
|
assert "already has a move" in error_message2
|
||||||
assert builder.move_count == 1
|
assert builder.move_count == 1
|
||||||
|
|
||||||
def test_add_move_same_team_same_roster_fails(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_same_team_same_roster_fails(self, builder, mock_player):
|
||||||
"""Test that adding a move where from_team, to_team, from_roster, and to_roster are all the same fails."""
|
"""Test that adding a move where from_team, to_team, from_roster, and to_roster are all the same fails."""
|
||||||
move = TransactionMove(
|
move = TransactionMove(
|
||||||
player=mock_player,
|
player=mock_player,
|
||||||
@ -176,14 +180,15 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team # Same team - should fail when roster is also same
|
to_team=builder.team # Same team - should fail when roster is also same
|
||||||
)
|
)
|
||||||
|
|
||||||
success, error_message = builder.add_move(move)
|
success, error_message = await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
assert success is False
|
assert success is False
|
||||||
assert "already in that location" in error_message
|
assert "already in that location" in error_message
|
||||||
assert builder.move_count == 0
|
assert builder.move_count == 0
|
||||||
assert builder.is_empty is True
|
assert builder.is_empty is True
|
||||||
|
|
||||||
def test_add_move_same_team_different_roster_succeeds(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_same_team_different_roster_succeeds(self, builder, mock_player):
|
||||||
"""Test that adding a move where teams are same but rosters are different succeeds."""
|
"""Test that adding a move where teams are same but rosters are different succeeds."""
|
||||||
move = TransactionMove(
|
move = TransactionMove(
|
||||||
player=mock_player,
|
player=mock_player,
|
||||||
@ -193,14 +198,15 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team # Same team - should succeed when rosters differ
|
to_team=builder.team # Same team - should succeed when rosters differ
|
||||||
)
|
)
|
||||||
|
|
||||||
success, error_message = builder.add_move(move)
|
success, error_message = await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
assert success is True
|
assert success is True
|
||||||
assert error_message == ""
|
assert error_message == ""
|
||||||
assert builder.move_count == 1
|
assert builder.move_count == 1
|
||||||
assert builder.is_empty is False
|
assert builder.is_empty is False
|
||||||
|
|
||||||
def test_add_move_different_teams_succeeds(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_different_teams_succeeds(self, builder, mock_player):
|
||||||
"""Test that adding a move where from_team and to_team are different succeeds."""
|
"""Test that adding a move where from_team and to_team are different succeeds."""
|
||||||
other_team = Team(
|
other_team = Team(
|
||||||
id=500,
|
id=500,
|
||||||
@ -218,14 +224,15 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success, error_message = builder.add_move(move)
|
success, error_message = await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
assert success is True
|
assert success is True
|
||||||
assert error_message == ""
|
assert error_message == ""
|
||||||
assert builder.move_count == 1
|
assert builder.move_count == 1
|
||||||
assert builder.is_empty is False
|
assert builder.is_empty is False
|
||||||
|
|
||||||
def test_add_move_none_teams_succeeds(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_none_teams_succeeds(self, builder, mock_player):
|
||||||
"""Test that adding a move where one or both teams are None succeeds."""
|
"""Test that adding a move where one or both teams are None succeeds."""
|
||||||
# From FA to team (from_team=None)
|
# From FA to team (from_team=None)
|
||||||
move1 = TransactionMove(
|
move1 = TransactionMove(
|
||||||
@ -236,7 +243,7 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success1, error_message1 = builder.add_move(move1)
|
success1, error_message1 = await builder.add_move(move1, check_pending_transactions=False)
|
||||||
assert success1 is True
|
assert success1 is True
|
||||||
assert error_message1 == ""
|
assert error_message1 == ""
|
||||||
|
|
||||||
@ -260,11 +267,12 @@ class TestTransactionBuilder:
|
|||||||
to_team=None
|
to_team=None
|
||||||
)
|
)
|
||||||
|
|
||||||
success2, error_message2 = builder.add_move(move2)
|
success2, error_message2 = await builder.add_move(move2, check_pending_transactions=False)
|
||||||
assert success2 is True
|
assert success2 is True
|
||||||
assert error_message2 == ""
|
assert error_message2 == ""
|
||||||
|
|
||||||
def test_remove_move_success(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_remove_move_success(self, builder, mock_player):
|
||||||
"""Test successfully removing a move."""
|
"""Test successfully removing a move."""
|
||||||
move = TransactionMove(
|
move = TransactionMove(
|
||||||
player=mock_player,
|
player=mock_player,
|
||||||
@ -273,7 +281,7 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success, _ = builder.add_move(move)
|
success, _ = await builder.add_move(move, check_pending_transactions=False)
|
||||||
assert success
|
assert success
|
||||||
assert builder.move_count == 1
|
assert builder.move_count == 1
|
||||||
|
|
||||||
@ -290,7 +298,8 @@ class TestTransactionBuilder:
|
|||||||
assert removed is False
|
assert removed is False
|
||||||
assert builder.move_count == 0
|
assert builder.move_count == 0
|
||||||
|
|
||||||
def test_get_move_for_player(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_get_move_for_player(self, builder, mock_player):
|
||||||
"""Test getting move for a specific player."""
|
"""Test getting move for a specific player."""
|
||||||
move = TransactionMove(
|
move = TransactionMove(
|
||||||
player=mock_player,
|
player=mock_player,
|
||||||
@ -299,15 +308,16 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
builder.add_move(move)
|
await builder.add_move(move, check_pending_transactions=False)
|
||||||
|
|
||||||
found_move = builder.get_move_for_player(mock_player.id)
|
found_move = builder.get_move_for_player(mock_player.id)
|
||||||
not_found = builder.get_move_for_player(99999)
|
not_found = builder.get_move_for_player(99999)
|
||||||
|
|
||||||
assert found_move == move
|
assert found_move == move
|
||||||
assert not_found is None
|
assert not_found is None
|
||||||
|
|
||||||
def test_clear_moves(self, builder, mock_player):
|
@pytest.mark.asyncio
|
||||||
|
async def test_clear_moves(self, builder, mock_player):
|
||||||
"""Test clearing all moves."""
|
"""Test clearing all moves."""
|
||||||
move = TransactionMove(
|
move = TransactionMove(
|
||||||
player=mock_player,
|
player=mock_player,
|
||||||
@ -316,7 +326,7 @@ class TestTransactionBuilder:
|
|||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success, _ = builder.add_move(move)
|
success, _ = await builder.add_move(move, check_pending_transactions=False)
|
||||||
assert success
|
assert success
|
||||||
assert builder.move_count == 1
|
assert builder.move_count == 1
|
||||||
|
|
||||||
@ -324,18 +334,18 @@ class TestTransactionBuilder:
|
|||||||
|
|
||||||
assert builder.move_count == 0
|
assert builder.move_count == 0
|
||||||
assert builder.is_empty is True
|
assert builder.is_empty is True
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_validate_transaction_no_roster(self, builder):
|
async def test_validate_transaction_no_roster(self, builder):
|
||||||
"""Test validation when roster data cannot be loaded."""
|
"""Test validation when roster data cannot be loaded."""
|
||||||
with patch.object(builder, '_current_roster', None):
|
with patch.object(builder, '_current_roster', None):
|
||||||
with patch.object(builder, '_roster_loaded', True):
|
with patch.object(builder, '_roster_loaded', True):
|
||||||
validation = await builder.validate_transaction()
|
validation = await builder.validate_transaction()
|
||||||
|
|
||||||
assert validation.is_legal is False
|
assert validation.is_legal is False
|
||||||
assert len(validation.errors) == 1
|
assert len(validation.errors) == 1
|
||||||
assert "Could not load current roster data" in validation.errors[0]
|
assert "Could not load current roster data" in validation.errors[0]
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_validate_transaction_legal(self, builder, mock_roster, mock_player):
|
async def test_validate_transaction_legal(self, builder, mock_roster, mock_player):
|
||||||
"""Test validation of a legal transaction."""
|
"""Test validation of a legal transaction."""
|
||||||
@ -348,15 +358,15 @@ class TestTransactionBuilder:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
success, _ = builder.add_move(move)
|
success, _ = await builder.add_move(move, check_pending_transactions=False)
|
||||||
assert success
|
assert success
|
||||||
|
|
||||||
validation = await builder.validate_transaction()
|
validation = await builder.validate_transaction()
|
||||||
|
|
||||||
assert validation.is_legal is True
|
assert validation.is_legal is True
|
||||||
assert validation.major_league_count == 25 # 24 + 1
|
assert validation.major_league_count == 25 # 24 + 1
|
||||||
assert len(validation.errors) == 0
|
assert len(validation.errors) == 0
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_validate_transaction_over_limit(self, builder, mock_roster):
|
async def test_validate_transaction_over_limit(self, builder, mock_roster):
|
||||||
"""Test validation when transaction would exceed roster limit."""
|
"""Test validation when transaction would exceed roster limit."""
|
||||||
@ -377,9 +387,9 @@ class TestTransactionBuilder:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
success, _ = builder.add_move(move)
|
success, _ = await builder.add_move(move, check_pending_transactions=False)
|
||||||
assert success
|
assert success
|
||||||
|
|
||||||
validation = await builder.validate_transaction()
|
validation = await builder.validate_transaction()
|
||||||
|
|
||||||
assert validation.is_legal is False
|
assert validation.is_legal is False
|
||||||
@ -427,12 +437,12 @@ class TestTransactionBuilder:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
success, _ = builder.add_move(move)
|
success, _ = await builder.add_move(move, check_pending_transactions=False)
|
||||||
assert success
|
assert success
|
||||||
|
|
||||||
with pytest.raises(ValueError, match="Cannot submit illegal transaction"):
|
with pytest.raises(ValueError, match="Cannot submit illegal transaction"):
|
||||||
await builder.submit_transaction(week=11)
|
await builder.submit_transaction(week=11)
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_submit_transaction_success(self, builder, mock_roster, mock_player):
|
async def test_submit_transaction_success(self, builder, mock_roster, mock_player):
|
||||||
"""Test successful transaction submission."""
|
"""Test successful transaction submission."""
|
||||||
@ -445,11 +455,11 @@ class TestTransactionBuilder:
|
|||||||
to_roster=RosterType.MAJOR_LEAGUE,
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
to_team=builder.team
|
to_team=builder.team
|
||||||
)
|
)
|
||||||
success, _ = builder.add_move(move)
|
success, _ = await builder.add_move(move, check_pending_transactions=False)
|
||||||
assert success
|
assert success
|
||||||
|
|
||||||
transactions = await builder.submit_transaction(week=11)
|
transactions = await builder.submit_transaction(week=11)
|
||||||
|
|
||||||
assert len(transactions) == 1
|
assert len(transactions) == 1
|
||||||
transaction = transactions[0]
|
transaction = transactions[0]
|
||||||
assert isinstance(transaction, Transaction)
|
assert isinstance(transaction, Transaction)
|
||||||
@ -458,7 +468,7 @@ class TestTransactionBuilder:
|
|||||||
assert transaction.player == mock_player
|
assert transaction.player == mock_player
|
||||||
assert transaction.newteam == builder.team
|
assert transaction.newteam == builder.team
|
||||||
assert "Season-012-Week-11-" in transaction.moveid
|
assert "Season-012-Week-11-" in transaction.moveid
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_submit_complex_transaction(self, builder, mock_roster):
|
async def test_submit_complex_transaction(self, builder, mock_roster):
|
||||||
"""Test submitting transaction with multiple moves."""
|
"""Test submitting transaction with multiple moves."""
|
||||||
@ -467,7 +477,7 @@ class TestTransactionBuilder:
|
|||||||
# Add one player and drop one player (net zero)
|
# Add one player and drop one player (net zero)
|
||||||
add_player = Player(id=5001, name='Add Player', wara=2.0, season=12, pos_1='OF')
|
add_player = Player(id=5001, name='Add Player', wara=2.0, season=12, pos_1='OF')
|
||||||
drop_player = Player(id=5002, name='Drop Player', wara=1.0, season=12, pos_1='OF')
|
drop_player = Player(id=5002, name='Drop Player', wara=1.0, season=12, pos_1='OF')
|
||||||
|
|
||||||
add_move = TransactionMove(
|
add_move = TransactionMove(
|
||||||
player=add_player,
|
player=add_player,
|
||||||
from_roster=RosterType.FREE_AGENCY,
|
from_roster=RosterType.FREE_AGENCY,
|
||||||
@ -481,13 +491,13 @@ class TestTransactionBuilder:
|
|||||||
to_roster=RosterType.FREE_AGENCY,
|
to_roster=RosterType.FREE_AGENCY,
|
||||||
from_team=builder.team
|
from_team=builder.team
|
||||||
)
|
)
|
||||||
|
|
||||||
success1, _ = builder.add_move(add_move)
|
success1, _ = await builder.add_move(add_move, check_pending_transactions=False)
|
||||||
success2, _ = builder.add_move(drop_move)
|
success2, _ = await builder.add_move(drop_move, check_pending_transactions=False)
|
||||||
assert success1 and success2
|
assert success1 and success2
|
||||||
|
|
||||||
transactions = await builder.submit_transaction(week=11)
|
transactions = await builder.submit_transaction(week=11)
|
||||||
|
|
||||||
assert len(transactions) == 2
|
assert len(transactions) == 2
|
||||||
# Both transactions should have the same move_id
|
# Both transactions should have the same move_id
|
||||||
assert transactions[0].moveid == transactions[1].moveid
|
assert transactions[0].moveid == transactions[1].moveid
|
||||||
@ -655,4 +665,166 @@ class TestTransactionBuilderGlobalFunctions:
|
|||||||
def test_clear_nonexistent_builder(self):
|
def test_clear_nonexistent_builder(self):
|
||||||
"""Test clearing non-existent builder doesn't error."""
|
"""Test clearing non-existent builder doesn't error."""
|
||||||
# Should not raise any exception
|
# Should not raise any exception
|
||||||
clear_transaction_builder(user_id=99999)
|
clear_transaction_builder(user_id=99999)
|
||||||
|
|
||||||
|
|
||||||
|
class TestPendingTransactionValidation:
|
||||||
|
"""
|
||||||
|
Test pending transaction validation in add_move.
|
||||||
|
|
||||||
|
This validates that players who are already in a pending transaction
|
||||||
|
for the next week cannot be added to another transaction.
|
||||||
|
"""
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def mock_team(self):
|
||||||
|
"""Create a mock team for testing."""
|
||||||
|
return Team(
|
||||||
|
id=499,
|
||||||
|
abbrev='WV',
|
||||||
|
sname='Black Bears',
|
||||||
|
lname='West Virginia Black Bears',
|
||||||
|
season=12
|
||||||
|
)
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def mock_player(self):
|
||||||
|
"""Create a mock player for testing."""
|
||||||
|
return Player(
|
||||||
|
id=12472,
|
||||||
|
name='Test Player',
|
||||||
|
wara=2.5,
|
||||||
|
season=12,
|
||||||
|
pos_1='OF'
|
||||||
|
)
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def builder(self, mock_team):
|
||||||
|
"""Create a TransactionBuilder for testing."""
|
||||||
|
return TransactionBuilder(mock_team, user_id=123, season=12)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_player_in_pending_transaction_fails(self, builder, mock_player):
|
||||||
|
"""
|
||||||
|
Test that adding a player who is already in a pending transaction fails.
|
||||||
|
|
||||||
|
When a player is claimed by another team in a pending (unfrozen) transaction
|
||||||
|
for the next week, they should not be able to be added to another transaction.
|
||||||
|
"""
|
||||||
|
move = TransactionMove(
|
||||||
|
player=mock_player,
|
||||||
|
from_roster=RosterType.FREE_AGENCY,
|
||||||
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
|
to_team=builder.team
|
||||||
|
)
|
||||||
|
|
||||||
|
with patch('services.transaction_builder.transaction_service') as mock_tx_service:
|
||||||
|
with patch('services.transaction_builder.league_service') as mock_league_service:
|
||||||
|
# Mock that player IS in a pending transaction (claimed by LAA)
|
||||||
|
mock_tx_service.is_player_in_pending_transaction = AsyncMock(
|
||||||
|
return_value=(True, "LAA")
|
||||||
|
)
|
||||||
|
# Mock current state to provide next week
|
||||||
|
mock_league_service.get_current_state = AsyncMock(
|
||||||
|
return_value=MagicMock(week=10)
|
||||||
|
)
|
||||||
|
|
||||||
|
success, error_message = await builder.add_move(move)
|
||||||
|
|
||||||
|
assert success is False
|
||||||
|
assert "already in a pending transaction" in error_message
|
||||||
|
assert "week 11" in error_message
|
||||||
|
assert "LAA" in error_message
|
||||||
|
assert builder.move_count == 0
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_player_not_in_pending_transaction_succeeds(self, builder, mock_player):
|
||||||
|
"""
|
||||||
|
Test that adding a player who is NOT in a pending transaction succeeds.
|
||||||
|
|
||||||
|
When a player is available (not claimed in any pending transaction),
|
||||||
|
they should be able to be added to the transaction builder.
|
||||||
|
"""
|
||||||
|
move = TransactionMove(
|
||||||
|
player=mock_player,
|
||||||
|
from_roster=RosterType.FREE_AGENCY,
|
||||||
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
|
to_team=builder.team
|
||||||
|
)
|
||||||
|
|
||||||
|
with patch('services.transaction_builder.transaction_service') as mock_tx_service:
|
||||||
|
with patch('services.transaction_builder.league_service') as mock_league_service:
|
||||||
|
# Mock that player is NOT in a pending transaction
|
||||||
|
mock_tx_service.is_player_in_pending_transaction = AsyncMock(
|
||||||
|
return_value=(False, None)
|
||||||
|
)
|
||||||
|
# Mock current state to provide next week
|
||||||
|
mock_league_service.get_current_state = AsyncMock(
|
||||||
|
return_value=MagicMock(week=10)
|
||||||
|
)
|
||||||
|
|
||||||
|
success, error_message = await builder.add_move(move)
|
||||||
|
|
||||||
|
assert success is True
|
||||||
|
assert error_message == ""
|
||||||
|
assert builder.move_count == 1
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_skip_pending_check_with_flag(self, builder, mock_player):
|
||||||
|
"""
|
||||||
|
Test that check_pending_transactions=False skips the validation.
|
||||||
|
|
||||||
|
This is used for IL moves and trade moves where the pending transaction
|
||||||
|
check should not apply.
|
||||||
|
"""
|
||||||
|
move = TransactionMove(
|
||||||
|
player=mock_player,
|
||||||
|
from_roster=RosterType.FREE_AGENCY,
|
||||||
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
|
to_team=builder.team
|
||||||
|
)
|
||||||
|
|
||||||
|
# Even if the service would return True, the check should be skipped
|
||||||
|
with patch('services.transaction_builder.transaction_service') as mock_tx_service:
|
||||||
|
# This mock should NOT be called when check_pending_transactions=False
|
||||||
|
mock_tx_service.is_player_in_pending_transaction = AsyncMock(
|
||||||
|
return_value=(True, "LAA")
|
||||||
|
)
|
||||||
|
|
||||||
|
success, error_message = await builder.add_move(
|
||||||
|
move, check_pending_transactions=False
|
||||||
|
)
|
||||||
|
|
||||||
|
assert success is True
|
||||||
|
assert error_message == ""
|
||||||
|
assert builder.move_count == 1
|
||||||
|
# Verify the service method was NOT called
|
||||||
|
mock_tx_service.is_player_in_pending_transaction.assert_not_called()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_add_move_with_explicit_next_week(self, builder, mock_player):
|
||||||
|
"""
|
||||||
|
Test that providing next_week parameter uses that value.
|
||||||
|
|
||||||
|
The next_week parameter allows the caller to specify which week
|
||||||
|
to check for pending transactions.
|
||||||
|
"""
|
||||||
|
move = TransactionMove(
|
||||||
|
player=mock_player,
|
||||||
|
from_roster=RosterType.FREE_AGENCY,
|
||||||
|
to_roster=RosterType.MAJOR_LEAGUE,
|
||||||
|
to_team=builder.team
|
||||||
|
)
|
||||||
|
|
||||||
|
with patch('services.transaction_builder.transaction_service') as mock_tx_service:
|
||||||
|
mock_tx_service.is_player_in_pending_transaction = AsyncMock(
|
||||||
|
return_value=(False, None)
|
||||||
|
)
|
||||||
|
|
||||||
|
success, error_message = await builder.add_move(move, next_week=15)
|
||||||
|
|
||||||
|
assert success is True
|
||||||
|
# Verify the check was called with the explicit week
|
||||||
|
mock_tx_service.is_player_in_pending_transaction.assert_called_once()
|
||||||
|
call_args = mock_tx_service.is_player_in_pending_transaction.call_args
|
||||||
|
assert call_args.kwargs['week'] == 15
|
||||||
Loading…
Reference in New Issue
Block a user