From 97664a4a16aad3920077cf852ad7c162eeeed967 Mon Sep 17 00:00:00 2001 From: Michael SCHAL Date: Thu, 18 Dec 2025 08:50:45 +0100 Subject: [PATCH] fix: Improve ItemService screenshot management by prioritizing latest timestamped files and ensuring complete deletion. --- app/services/item_service.py | 74 +++++++++++----- task.md | 6 ++ verify_fix_item_service.py | 164 +++++++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 20 deletions(-) create mode 100644 verify_fix_item_service.py diff --git a/app/services/item_service.py b/app/services/item_service.py index d809cd8..a61d594 100644 --- a/app/services/item_service.py +++ b/app/services/item_service.py @@ -15,26 +15,57 @@ logger = logging.getLogger(__name__) class ItemService: @staticmethod def get_items(db: Session): + import glob items = db.query(models.Item).all() result = [] for item in items: - # Get latest price history with screenshot - latest_history = ( - db.query(models.PriceHistory) - .filter(models.PriceHistory.item_id == item.id) - .filter(models.PriceHistory.screenshot_path.isnot(None)) - .order_by(models.PriceHistory.timestamp.desc()) - .first() - ) - + # Strategy: Look for the latest screenshot file on disk + # Format: item_{id}_{timestamp}.png screenshot_url = None - if latest_history and latest_history.screenshot_path: - # Convert absolute path /app/screenshots/... to URL /screenshots/... - filename = os.path.basename(latest_history.screenshot_path) - screenshot_url = f"/screenshots/{filename}" - elif os.path.exists(f"screenshots/item_{item.id}.png"): - # Fallback to legacy static file - screenshot_url = f"/screenshots/item_{item.id}.png" + + try: + # Find all timestamped screenshots for this item + files = glob.glob(f"screenshots/item_{item.id}_*.png") + + if files: + # Sort by timestamp valid in filename + # We expect format item_ID_TIMESTAMP.png + def get_timestamp(f): + try: + # Extract timestamp part: remove extension, split by _, take last part + ts_str = os.path.splitext(f)[0].split('_')[-1] + return float(ts_str) + except (ValueError, IndexError): + return 0.0 + + latest_file = max(files, key=get_timestamp) + screenshot_url = f"/screenshots/{os.path.basename(latest_file)}" + + # Primary fallback: Legacy static file + elif os.path.exists(f"screenshots/item_{item.id}.png"): + screenshot_url = f"/screenshots/item_{item.id}.png" + + # Secondary fallback: use DB history if for some reason file scan missed but DB has record + # (This is less likely to be useful if we assume files exist, but good for safety) + if not screenshot_url: + latest_history = ( + db.query(models.PriceHistory) + .filter(models.PriceHistory.item_id == item.id) + .filter(models.PriceHistory.screenshot_path.isnot(None)) + .order_by(models.PriceHistory.timestamp.desc()) + .first() + ) + if latest_history and latest_history.screenshot_path: + filename = os.path.basename(latest_history.screenshot_path) + # Verify it exists + if os.path.exists(f"screenshots/{filename}"): + screenshot_url = f"/screenshots/{filename}" + + except Exception as e: + logger.error(f"Error determining screenshot for item {item.id}: {e}") + # Ultimate fallback + if os.path.exists(f"screenshots/item_{item.id}.png"): + screenshot_url = f"/screenshots/item_{item.id}.png" result.append({**item.__dict__, "screenshot_url": screenshot_url}) return result @@ -68,15 +99,18 @@ class ItemService: @staticmethod def delete_item(db: Session, item_id: int): + import glob item = db.query(models.Item).filter(models.Item.id == item_id).first() if not item: raise HTTPException(status_code=404, detail="Item not found") - if os.path.exists(f"screenshots/item_{item_id}.png"): + # Delete all associated screenshots (legacy and timestamped) + for file_path in glob.glob(f"screenshots/item_{item_id}*.png"): try: - os.remove(f"screenshots/item_{item_id}.png") - except OSError: - pass + os.remove(file_path) + logger.info(f"Deleted screenshot: {file_path}") + except OSError as e: + logger.warning(f"Failed to delete {file_path}: {e}") db.delete(item) db.commit() diff --git a/task.md b/task.md index 7af09ed..d10146a 100644 --- a/task.md +++ b/task.md @@ -14,8 +14,14 @@ Implementing logic to generate unique timestamped filenames for screenshots to b - [x] Verify that `item_service.py` correctly picks up the new files - [x] Create verification script `verify_screenshot_update.py` - [x] Run verification and confirm fix +- [x] Modify `ItemService.get_items` to scan filesystem for latest screenshot +- [x] Update `ItemService.delete_item` to clean up all related screenshots +- [x] Create `verify_fix_item_service.py` to test the new logic ## Progress Log - Identified the issue: `ScraperService` overwrites `item_{id}.png`. - Created implementation plan. +- User approved plan. +- Implemented filesystem scanning in `ItemService`. +- Verified fix with `verify_fix_item_service.py` successfully. diff --git a/verify_fix_item_service.py b/verify_fix_item_service.py new file mode 100644 index 0000000..28ce677 --- /dev/null +++ b/verify_fix_item_service.py @@ -0,0 +1,164 @@ +import os +import glob +import sys +from unittest.mock import MagicMock + +# --- MOCKS SETUP --- +# We need to mock these BEFORE importing app.services.item_service +# to avoid ImportErrors due to missing dependencies in the test env. + +# 1. Mock External Libs +sys.modules["fastapi"] = MagicMock() +sys.modules["sqlalchemy"] = MagicMock() +sys.modules["sqlalchemy.orm"] = MagicMock() + +# 2. Mock Internal App Modules that have heavy dependencies +# Mock app.database +mock_database = MagicMock() +sys.modules["app.database"] = mock_database + +# Mock app.models +# We need models.Item and models.PriceHistory to be accessible attributes +mock_models = MagicMock() +sys.modules["app.models"] = mock_models + +# Mock app.schemas +sys.modules["app.schemas"] = MagicMock() + +# Mock app.services.settings_service +sys.modules["app.services.settings_service"] = MagicMock() + +# Mock app.url_validation +sys.modules["app.url_validation"] = MagicMock() + +# --- IMPORT TARGET --- +from app.services.item_service import ItemService + +def verify_item_service_fix(): + print("Starting verification of ItemService fix...") + + # 1. Setup Mock DB and Item + # We must ensure that when ItemService does `item.id`, it works. + mock_db = MagicMock() + + # Create a simple class to act as the Item model instance + class MockItem: + def __init__(self, id, name): + self.id = id + self.name = name + self.url = "http://test.com" + self.current_price = 10.0 + self.in_stock = True + self.screenshot_url = None # This will be set by the service + + # Attributes accessed by the service + self.notification_channel = None + self.target_price = None + self.current_price_confidence = 1.0 + self.in_stock_confidence = 1.0 + self.is_active = True + self.last_checked = None + self.is_refreshing = False + self.last_error = None + self.category = None + self.tags = None + self.description = None + + # __dict__ is used by the service to create the result + self.dict_storage = {k:v for k,v in self.__dict__.items()} + + @property + def __dict__(self): + # Update dict storage with current attributes + return { + "id": self.id, + "name": self.name, + "url": self.url + } + + item_888 = MockItem(888, "Test Item") + + # ItemService.get_items calls db.query(models.Item).all() + # We need to make sure models.Item is used in the query. + # The service does: items = db.query(models.Item).all() + + mock_db.query.return_value.all.return_value = [item_888] + + # It also queries PriceHistory + # db.query(models.PriceHistory).filter(...).first() + # Let's mock that to return None to force filesytem check (or check logic priority) + mock_db.query.return_value.filter.return_value.filter.return_value.order_by.return_value.first.return_value = None + + # 2. Create Dummy Screenshot Files + os.makedirs("screenshots", exist_ok=True) + + # Clean up + for f in glob.glob("screenshots/item_888_*.png"): + os.remove(f) + if os.path.exists("screenshots/item_888.png"): + os.remove("screenshots/item_888.png") + + # Scenario: + # 1. item_888.png exists (legacy) + # 2. item_888_1000.png exists (old timestamp) + # 3. item_888_2000.png exists (new timestamp) + + # Expected: get_items should pick item_888_2000.png + + file_legacy = "screenshots/item_888.png" + file_old = "screenshots/item_888_1000.png" + file_new = "screenshots/item_888_2000.png" + + with open(file_legacy, "w") as f: f.write(".") + with open(file_old, "w") as f: f.write(".") + with open(file_new, "w") as f: f.write(".") + + print(f"Created files: {file_legacy}, {file_old}, {file_new}") + + try: + # 3. Test get_items + print("Testing get_items()...") + items = ItemService.get_items(mock_db) + + if not items: + print("FAILURE: No items returned") + exit(1) + + result = items[0] + screenshot_url = result.get("screenshot_url") + print(f"Returned screenshot_url: {screenshot_url}") + + expected_url = f"/screenshots/{os.path.basename(file_new)}" + + if screenshot_url == expected_url: + print("SUCCESS: Correctly identified the latest screenshot!") + else: + print(f"FAILURE: Expected {expected_url}, got {screenshot_url}") + # If it failed, maybe it picked legacy? + if screenshot_url == f"/screenshots/{os.path.basename(file_legacy)}": + print("Picked legacy file instead of timestamped one.") + exit(1) + + # 4. Test delete_item + print("Testing delete_item()...") + # Ensure the query returns our item + mock_db.query.return_value.filter.return_value.first.return_value = item_888 + + ItemService.delete_item(mock_db, 888) + + # Check files + remaining = glob.glob("screenshots/item_888*.png") + if not remaining: + print("SUCCESS: All screenshots deleted.") + else: + print(f"FAILURE: Files remaining: {remaining}") + exit(1) + + finally: + # Cleanup + for f in [file_legacy, file_old, file_new]: + if os.path.exists(f): + os.remove(f) + +if __name__ == "__main__": + verify_item_service_fix()