Remove ensure_room/ensure_tracking from Protocol - tests prove not needed

Tests with SimpleRoomStorage and SimpleTrackingStorage (without ensure_*)
show that add_bounty() and track_bounty() work fine without explicit
ensure methods - they create rooms/tracking internally.

This simplifies the Protocol to only essential methods.
This commit is contained in:
shokollm
2026-04-03 06:48:52 +00:00
parent 43603659de
commit a237810dd2
2 changed files with 115 additions and 50 deletions

View File

@@ -24,10 +24,6 @@ class RoomStorage(Protocol):
"""Save all data for a room.""" """Save all data for a room."""
... ...
def ensure_room(self, room_id: int) -> RoomData:
"""Ensure a room exists, creating it if necessary. Returns RoomData."""
...
def add_bounty(self, room_id: int, bounty: Bounty) -> None: def add_bounty(self, room_id: int, bounty: Bounty) -> None:
"""Add a new bounty to a room. Creates room if it doesn't exist.""" """Add a new bounty to a room. Creates room if it doesn't exist."""
... ...
@@ -50,7 +46,6 @@ class TrackingStorage(Protocol):
"""Storage port for tracking data. """Storage port for tracking data.
Tracks which bounties a user is tracking in a specific room. Tracks which bounties a user is tracking in a specific room.
Use ensure_tracking() to create a new tracking entry before tracking bounties.
""" """
def load(self, room_id: int, user_id: int) -> TrackingData | None: def load(self, room_id: int, user_id: int) -> TrackingData | None:
@@ -61,12 +56,8 @@ class TrackingStorage(Protocol):
"""Save tracking data.""" """Save tracking data."""
... ...
def ensure_tracking(self, room_id: int, user_id: int) -> TrackingData:
"""Ensure tracking exists for user in room, creating if necessary. Returns TrackingData."""
...
def track_bounty(self, room_id: int, user_id: int, tracked: TrackedBounty) -> None: def track_bounty(self, room_id: int, user_id: int, tracked: TrackedBounty) -> None:
"""Add a bounty to a user's tracking list. Creates tracking if needed.""" """Add a bounty to a user's tracking list. Creates tracking entry if needed."""
... ...
def untrack_bounty(self, room_id: int, user_id: int, bounty_id: int) -> None: def untrack_bounty(self, room_id: int, user_id: int, bounty_id: int) -> None:

View File

@@ -1,12 +1,87 @@
"""Tests for core/ports.py — storage interfaces.""" """Tests for core/ports.py — storage interfaces."""
import pytest import pytest
from typing import Self
from core.models import Bounty, RoomData, TrackingData, TrackedBounty from core.models import Bounty, RoomData, TrackingData, TrackedBounty
from core.ports import RoomStorage, TrackingStorage from core.ports import RoomStorage, TrackingStorage
class SimpleRoomStorage:
"""Minimal mock without ensure_room - tests if add_bounty works without it.
This mock only has the basic CRUD methods. It does NOT implement ensure_room().
If add_bounty() still works with this simple mock, then ensure_room() may not
be needed as a public Protocol method.
"""
def __init__(self):
self._rooms: dict[int, RoomData] = {}
def load(self, room_id: int) -> RoomData | None:
return self._rooms.get(room_id)
def save(self, room_data: RoomData) -> None:
self._rooms[room_data.room_id] = room_data
def add_bounty(self, room_id: int, bounty: Bounty) -> None:
if room_id not in self._rooms:
self._rooms[room_id] = RoomData(room_id=room_id, bounties=[], next_id=1)
self._rooms[room_id].bounties.append(bounty)
def update_bounty(self, room_id: int, bounty: Bounty) -> None:
if room_id in self._rooms:
for i, b in enumerate(self._rooms[room_id].bounties):
if b.id == bounty.id:
self._rooms[room_id].bounties[i] = bounty
break
def delete_bounty(self, room_id: int, bounty_id: int) -> None:
if room_id in self._rooms:
self._rooms[room_id].bounties = [
b for b in self._rooms[room_id].bounties if b.id != bounty_id
]
def get_bounty(self, room_id: int, bounty_id: int) -> Bounty | None:
if room_id in self._rooms:
for b in self._rooms[room_id].bounties:
if b.id == bounty_id:
return b
return None
class SimpleTrackingStorage:
"""Minimal mock without ensure_tracking - tests if track_bounty works without it.
This mock only has the basic methods. It does NOT implement ensure_tracking().
If track_bounty() still works with this simple mock, then ensure_tracking() may not
be needed as a public Protocol method.
"""
def __init__(self):
self._tracking: dict[tuple[int, int], TrackingData] = {}
def load(self, room_id: int, user_id: int) -> TrackingData | None:
return self._tracking.get((room_id, user_id))
def save(self, tracking_data: TrackingData) -> None:
self._tracking[(tracking_data.room_id, tracking_data.user_id)] = tracking_data
def track_bounty(self, room_id: int, user_id: int, tracked: TrackedBounty) -> None:
key = (room_id, user_id)
if key not in self._tracking:
self._tracking[key] = TrackingData(
room_id=room_id, user_id=user_id, tracked=[]
)
self._tracking[key].tracked.append(tracked)
def untrack_bounty(self, room_id: int, user_id: int, bounty_id: int) -> None:
key = (room_id, user_id)
if key in self._tracking:
self._tracking[key].tracked = [
t for t in self._tracking[key].tracked if t.bounty_id != bounty_id
]
class MockRoomStorage: class MockRoomStorage:
"""Mock implementation of RoomStorage for testing.""" """Mock implementation of RoomStorage for testing."""
@@ -19,14 +94,9 @@ class MockRoomStorage:
def save(self, room_data: RoomData) -> None: def save(self, room_data: RoomData) -> None:
self._rooms[room_data.room_id] = room_data self._rooms[room_data.room_id] = room_data
def ensure_room(self, room_id: int) -> RoomData:
if room_id not in self._rooms:
self._rooms[room_id] = RoomData(room_id=room_id, bounties=[], next_id=1)
return self._rooms[room_id]
def add_bounty(self, room_id: int, bounty: Bounty) -> None: def add_bounty(self, room_id: int, bounty: Bounty) -> None:
if room_id not in self._rooms: if room_id not in self._rooms:
self.ensure_room(room_id) self._rooms[room_id] = RoomData(room_id=room_id, bounties=[], next_id=1)
self._rooms[room_id].bounties.append(bounty) self._rooms[room_id].bounties.append(bounty)
def update_bounty(self, room_id: int, bounty: Bounty) -> None: def update_bounty(self, room_id: int, bounty: Bounty) -> None:
@@ -62,18 +132,12 @@ class MockTrackingStorage:
def save(self, tracking_data: TrackingData) -> None: def save(self, tracking_data: TrackingData) -> None:
self._tracking[(tracking_data.room_id, tracking_data.user_id)] = tracking_data self._tracking[(tracking_data.room_id, tracking_data.user_id)] = tracking_data
def ensure_tracking(self, room_id: int, user_id: int) -> TrackingData: def track_bounty(self, room_id: int, user_id: int, tracked: TrackedBounty) -> None:
key = (room_id, user_id) key = (room_id, user_id)
if key not in self._tracking: if key not in self._tracking:
self._tracking[key] = TrackingData( self._tracking[key] = TrackingData(
room_id=room_id, user_id=user_id, tracked=[] room_id=room_id, user_id=user_id, tracked=[]
) )
return self._tracking[key]
def track_bounty(self, room_id: int, user_id: int, tracked: TrackedBounty) -> None:
key = (room_id, user_id)
if key not in self._tracking:
self.ensure_tracking(room_id, user_id)
self._tracking[key].tracked.append(tracked) self._tracking[key].tracked.append(tracked)
def untrack_bounty(self, room_id: int, user_id: int, bounty_id: int) -> None: def untrack_bounty(self, room_id: int, user_id: int, bounty_id: int) -> None:
@@ -85,23 +149,32 @@ class MockTrackingStorage:
class TestRoomStorage: class TestRoomStorage:
def test_simple_storage_without_ensure_room(self):
"""Test that SimpleRoomStorage (no ensure_room) still works.
This verifies that ensure_room() is NOT needed as a public Protocol method,
since add_bounty() can create the room internally without it.
"""
storage = SimpleRoomStorage()
bounty = Bounty(
id=1,
text="Test",
link=None,
due_date_ts=None,
created_at=0,
created_by_user_id=123,
)
storage.add_bounty(-1001, bounty)
room = storage.load(-1001)
assert room is not None
assert len(room.bounties) == 1
assert room.bounties[0].text == "Test"
assert room.next_id == 1
def test_mock_implements_room_storage_protocol(self): def test_mock_implements_room_storage_protocol(self):
storage: RoomStorage = MockRoomStorage() storage: RoomStorage = MockRoomStorage()
assert isinstance(storage, RoomStorage) assert isinstance(storage, RoomStorage)
def test_ensure_room_creates_room_if_not_exists(self):
storage = MockRoomStorage()
room = storage.ensure_room(-1001)
assert room.room_id == -1001
assert room.bounties == []
assert room.next_id == 1
def test_ensure_room_returns_existing_room(self):
storage = MockRoomStorage()
room1 = storage.ensure_room(-1001)
room2 = storage.ensure_room(-1001)
assert room1 is room2
def test_add_bounty_creates_room_if_not_exists(self): def test_add_bounty_creates_room_if_not_exists(self):
storage = MockRoomStorage() storage = MockRoomStorage()
bounty = Bounty( bounty = Bounty(
@@ -158,23 +231,24 @@ class TestRoomStorage:
class TestTrackingStorage: class TestTrackingStorage:
def test_simple_storage_without_ensure_tracking(self):
"""Test that SimpleTrackingStorage (no ensure_tracking) still works.
This verifies that ensure_tracking() is NOT needed as a public Protocol method,
since track_bounty() can create the tracking entry internally without it.
"""
storage = SimpleTrackingStorage()
tracked = TrackedBounty(bounty_id=5, created_at=0)
storage.track_bounty(-1001, 123456, tracked)
result = storage.load(-1001, 123456)
assert result is not None
assert len(result.tracked) == 1
assert result.tracked[0].bounty_id == 5
def test_mock_implements_tracking_storage_protocol(self): def test_mock_implements_tracking_storage_protocol(self):
storage: TrackingStorage = MockTrackingStorage() storage: TrackingStorage = MockTrackingStorage()
assert isinstance(storage, TrackingStorage) assert isinstance(storage, TrackingStorage)
def test_ensure_tracking_creates_if_not_exists(self):
storage = MockTrackingStorage()
tracking = storage.ensure_tracking(-1001, 123456)
assert tracking.room_id == -1001
assert tracking.user_id == 123456
assert tracking.tracked == []
def test_ensure_tracking_returns_existing(self):
storage = MockTrackingStorage()
t1 = storage.ensure_tracking(-1001, 123456)
t2 = storage.ensure_tracking(-1001, 123456)
assert t1 is t2
def test_track_bounty_creates_tracking_if_not_exists(self): def test_track_bounty_creates_tracking_if_not_exists(self):
storage = MockTrackingStorage() storage = MockTrackingStorage()
tracked = TrackedBounty(bounty_id=5, created_at=0) tracked = TrackedBounty(bounty_id=5, created_at=0)