diff --git a/app/services/watchlist_service.py b/app/services/watchlist_service.py index a8fa4a8..1a52ecc 100644 --- a/app/services/watchlist_service.py +++ b/app/services/watchlist_service.py @@ -26,6 +26,11 @@ logger = logging.getLogger(__name__) DEFAULT_AUTO_SIZE = 10 MAX_MANUAL = 10 +# entry_type values. "dismissed" is a tombstone: the user removed the ticker, so +# auto-population must not silently re-add it on the next read. Dismissed rows are +# hidden from the watchlist and revived (→ manual) if the user adds them again. +DISMISSED = "dismissed" + async def _get_ticker(db: AsyncSession, symbol: str) -> Ticker: normalised = symbol.strip().upper() @@ -64,19 +69,20 @@ async def auto_populate( ) ) - # Get manual ticker_ids so we don't duplicate - manual_result = await db.execute( + # Get manual + dismissed ticker_ids so we don't duplicate or resurrect a + # ticker the user has explicitly removed. + kept_result = await db.execute( select(WatchlistEntry.ticker_id).where( WatchlistEntry.user_id == user_id, - WatchlistEntry.entry_type == "manual", + WatchlistEntry.entry_type.in_(["manual", DISMISSED]), ) ) - manual_ticker_ids = {row[0] for row in manual_result.all()} + excluded_ticker_ids = {row[0] for row in kept_result.all()} now = datetime.now(timezone.utc) for ticker_id in top_ticker_ids: - if ticker_id in manual_ticker_ids: - continue # Already on watchlist as manual + if ticker_id in excluded_ticker_ids: + continue # Already manual, or dismissed by the user entry = WatchlistEntry( user_id=user_id, ticker_id=ticker_id, @@ -100,17 +106,19 @@ async def add_manual_entry( """ ticker = await _get_ticker(db, symbol) - # Check if already on watchlist + # Check if already on watchlist. A dismissed row is revived as manual rather + # than rejected; an active (auto/manual) row is a genuine duplicate. existing = await db.execute( select(WatchlistEntry).where( WatchlistEntry.user_id == user_id, WatchlistEntry.ticker_id == ticker.id, ) ) - if existing.scalar_one_or_none() is not None: + existing_entry = existing.scalar_one_or_none() + if existing_entry is not None and existing_entry.entry_type != DISMISSED: raise DuplicateError(f"Ticker already on watchlist: {ticker.symbol}") - # Count current manual entries + # Count current manual entries (dismissed tombstones don't count) count_result = await db.execute( select(func.count()).select_from(WatchlistEntry).where( WatchlistEntry.user_id == user_id, @@ -125,10 +133,11 @@ async def add_manual_entry( "Remove an entry before adding a new one." ) - # Check total cap + # Check total cap (exclude dismissed tombstones) total_result = await db.execute( select(func.count()).select_from(WatchlistEntry).where( WatchlistEntry.user_id == user_id, + WatchlistEntry.entry_type != DISMISSED, ) ) total_count = total_result.scalar() or 0 @@ -140,6 +149,14 @@ async def add_manual_entry( "Remove an entry before adding a new one." ) + if existing_entry is not None: + # Revive a dismissed entry as manual. + existing_entry.entry_type = "manual" + existing_entry.added_at = datetime.now(timezone.utc) + await db.commit() + await db.refresh(existing_entry) + return existing_entry + entry = WatchlistEntry( user_id=user_id, ticker_id=ticker.id, @@ -157,20 +174,26 @@ async def remove_entry( user_id: int, symbol: str, ) -> None: - """Remove a watchlist entry (manual or auto).""" + """Remove a watchlist entry (manual or auto). + + Marks the entry as dismissed (a tombstone) rather than deleting it, so the + next auto-population pass won't immediately re-add a top-ranked ticker the + user just removed. A subsequent manual add revives the row. + """ ticker = await _get_ticker(db, symbol) result = await db.execute( select(WatchlistEntry).where( WatchlistEntry.user_id == user_id, WatchlistEntry.ticker_id == ticker.id, + WatchlistEntry.entry_type != DISMISSED, ) ) entry = result.scalar_one_or_none() if entry is None: raise NotFoundError(f"Ticker not on watchlist: {ticker.symbol}") - await db.delete(entry) + entry.entry_type = DISMISSED await db.commit() @@ -254,7 +277,10 @@ async def get_watchlist( stmt = ( select(WatchlistEntry, Ticker.symbol) .join(Ticker, WatchlistEntry.ticker_id == Ticker.id) - .where(WatchlistEntry.user_id == user_id) + .where( + WatchlistEntry.user_id == user_id, + WatchlistEntry.entry_type != DISMISSED, + ) ) result = await db.execute(stmt) rows = result.all() diff --git a/frontend/src/pages/TickerDetailPage.tsx b/frontend/src/pages/TickerDetailPage.tsx index 7d7c903..6a35806 100644 --- a/frontend/src/pages/TickerDetailPage.tsx +++ b/frontend/src/pages/TickerDetailPage.tsx @@ -2,6 +2,7 @@ import { useMemo, useEffect, useState } from 'react'; import { useParams } from 'react-router-dom'; import { useTickerDetail } from '../hooks/useTickerDetail'; import { useFetchSymbolData } from '../hooks/useFetchSymbolData'; +import { useWatchlist, useAddToWatchlist, useRemoveFromWatchlist } from '../hooks/useWatchlist'; import type { FetchSelector } from '../api/ingestion'; import { CandlestickChart } from '../components/charts/CandlestickChart'; import { ScoreCard } from '../components/ui/ScoreCard'; @@ -98,6 +99,14 @@ export default function TickerDetailPage() { const { symbol = '' } = useParams<{ symbol: string }>(); const { ohlcv, scores, srLevels, sentiment, fundamentals, trades } = useTickerDetail(symbol); const ingestion = useFetchSymbolData(); + const watchlist = useWatchlist(); + const addToWatchlist = useAddToWatchlist(); + const removeFromWatchlist = useRemoveFromWatchlist(); + const onWatchlist = useMemo( + () => (watchlist.data ?? []).some((e) => e.symbol.toUpperCase() === symbol.toUpperCase()), + [watchlist.data, symbol], + ); + const watchlistBusy = addToWatchlist.isPending || removeFromWatchlist.isPending; const [activeTab, setActiveTab] = useState('Analysis'); const [refreshingLabel, setRefreshingLabel] = useState(null); @@ -216,12 +225,28 @@ export default function TickerDetailPage() { )} - +
+ + +
{/* Data freshness bar */} diff --git a/tests/unit/test_watchlist_service.py b/tests/unit/test_watchlist_service.py new file mode 100644 index 0000000..1289bda --- /dev/null +++ b/tests/unit/test_watchlist_service.py @@ -0,0 +1,104 @@ +"""Tests for watchlist remove → dismiss → revive behaviour. + +Regression cover for the bug where removing a top-ranked ticker did nothing, +because get_watchlist re-runs auto_populate on every read and would instantly +re-add it. Removals are now tombstoned as "dismissed" so auto-population skips +them; a later manual add revives the row. +""" + +from __future__ import annotations + +from datetime import datetime, timezone + +import pytest +from sqlalchemy import select + +from app.models.score import CompositeScore +from app.models.ticker import Ticker +from app.models.user import User +from app.models.watchlist import WatchlistEntry +from app.services.watchlist_service import ( + DISMISSED, + add_manual_entry, + auto_populate, + get_watchlist, + remove_entry, +) + +# Plain (committing) session — the watchlist service commits, so the rolling-back +# db_session fixture would fight it. Reuse the shared in-memory engine. +from tests.conftest import _test_session_factory # type: ignore + + +@pytest.fixture +async def session(): + async with _test_session_factory() as s: + yield s + + +async def _seed(session) -> tuple[int, list[int]]: + user = User(username="u", password_hash="x", role="user", has_access=True) + session.add(user) + await session.flush() + + now = datetime.now(timezone.utc) + ticker_ids: list[int] = [] + for i, sym in enumerate(["AAA", "BBB", "CCC"]): + t = Ticker(symbol=sym) + session.add(t) + await session.flush() + ticker_ids.append(t.id) + session.add( + CompositeScore( + ticker_id=t.id, + score=90.0 - i, # AAA highest + is_stale=False, + weights_json="{}", + computed_at=now, + ) + ) + await session.commit() + return user.id, ticker_ids + + +async def test_remove_dismisses_and_survives_auto_populate(session): + user_id, _ = await _seed(session) + + await auto_populate(session, user_id) + await session.commit() + + rows = await get_watchlist(session, user_id) + assert {r["symbol"] for r in rows} == {"AAA", "BBB", "CCC"} + + # Remove a top-ranked ticker, then force another auto-population pass. + await remove_entry(session, user_id, "AAA") + await auto_populate(session, user_id) + await session.commit() + + rows = await get_watchlist(session, user_id) + symbols = {r["symbol"] for r in rows} + assert "AAA" not in symbols + assert symbols == {"BBB", "CCC"} + + # The row is tombstoned, not deleted. + res = await session.execute( + select(WatchlistEntry).join(Ticker).where(Ticker.symbol == "AAA") + ) + entry = res.scalar_one() + assert entry.entry_type == DISMISSED + + +async def test_manual_add_revives_dismissed(session): + user_id, _ = await _seed(session) + await auto_populate(session, user_id) + await session.commit() + + await remove_entry(session, user_id, "AAA") + + # Re-adding the dismissed ticker should succeed (not DuplicateError) and + # reappear as a manual entry. + revived = await add_manual_entry(session, user_id, "AAA") + assert revived.entry_type == "manual" + + rows = await get_watchlist(session, user_id) + assert "AAA" in {r["symbol"] for r in rows}