fix: Improve ItemService screenshot management by prioritizing latest timestamped files and ensuring complete deletion.

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()

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 <!-- id: 1 -->
 - [x] Create verification script `verify_screenshot_update.py` <!-- id: 2 -->
 - [x] Run verification and confirm fix <!-- id: 3 -->
+- [x] Modify `ItemService.get_items` to scan filesystem for latest screenshot <!-- id: 4 -->
+- [x] Update `ItemService.delete_item` to clean up all related screenshots <!-- id: 6 -->
+- [x] Create `verify_fix_item_service.py` to test the new logic <!-- id: 5 -->
 
 ## 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.

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()