watchlist history: record automatic scans too (#933)
save_watchlist_scan_run had a single caller — the manual scan endpoint. the automatic/ scheduled path (process_watchlist_scan_automatically) ran the full scan but never wrote a history row, so nightly scans never showed up in the History modal — only manual ones. - new shared helper persist_scan_run(database, state, ...) extracts the run from the finished watchlist_scan_state and writes one history row - the automatic path now stamps scan_run_id/scan_track_events and calls it - the manual path is refactored onto the same helper so the two can't drift apart again - history is global (no profile filter), so the all-profiles nightly scan records one aggregate row (profile_id None → 1, never NULL) tests: 4 new persist_scan_run seam tests (real DB) + 2 new auto-scan integration tests proving the auto path actually records (completed + cancelled, exactly once). 420 watchlist/automation tests green.
This commit is contained in:
parent
b1f061a2a8
commit
eddaea2f93
5 changed files with 165 additions and 17 deletions
|
|
@ -185,7 +185,11 @@ def process_watchlist_scan_automatically(automation_id=None, profile_id=None, de
|
|||
'results': [],
|
||||
'summary': {},
|
||||
'error': None,
|
||||
'cancel_requested': False
|
||||
'cancel_requested': False,
|
||||
# #933: stamp these so this scan lands in the History modal too —
|
||||
# the scanner fills scan_track_events; persist_scan_run reads both.
|
||||
'scan_run_id': datetime.now().strftime('%Y%m%d-%H%M%S'),
|
||||
'scan_track_events': [],
|
||||
}
|
||||
|
||||
scan_results = []
|
||||
|
|
@ -294,6 +298,17 @@ def process_watchlist_scan_automatically(automation_id=None, profile_id=None, de
|
|||
total_added_to_wishlist = deps.watchlist_scan_state.get('summary', {}).get('tracks_added_to_wishlist', 0)
|
||||
logger.warning("Automatic watchlist scan cancelled — skipping post-scan steps")
|
||||
|
||||
# #933: record this run in the History ledger — same helper the manual
|
||||
# scan uses, so scheduled scans show up alongside manual ones.
|
||||
try:
|
||||
from core.watchlist.scan_history import persist_scan_run
|
||||
persist_scan_run(
|
||||
database, deps.watchlist_scan_state,
|
||||
profile_id=profile_id, was_cancelled=was_cancelled,
|
||||
)
|
||||
except Exception as _hist_err:
|
||||
logger.error(f"Failed to persist watchlist scan run: {_hist_err}")
|
||||
|
||||
# Post-scan steps — skip if cancelled
|
||||
if not was_cancelled:
|
||||
# Populate discovery pool from similar artists (per-profile)
|
||||
|
|
|
|||
51
core/watchlist/scan_history.py
Normal file
51
core/watchlist/scan_history.py
Normal file
|
|
@ -0,0 +1,51 @@
|
|||
"""Persist a finished watchlist scan to the History ledger (#831 / #933).
|
||||
|
||||
Both the manual scan (``web_server.start_watchlist_scan``) and the automatic
|
||||
scan (``core.watchlist.auto_scan.process_watchlist_scan_automatically``) finish
|
||||
with the same ``watchlist_scan_state`` shape, but only the manual path used to
|
||||
record a history row — so scheduled/nightly scans never showed up in the
|
||||
History modal (#933). This single helper is the shared seam: both paths call it,
|
||||
so they can't drift apart again.
|
||||
|
||||
Pure except for the one ``database.save_watchlist_scan_run`` call — the field
|
||||
extraction is unit-testable with a fake database.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import datetime
|
||||
from typing import Any, Dict, Optional
|
||||
|
||||
|
||||
def _iso(value: Any) -> Optional[str]:
|
||||
"""ISO-format a datetime; pass through an already-stringified timestamp."""
|
||||
if value is None:
|
||||
return None
|
||||
return value.isoformat() if hasattr(value, 'isoformat') else str(value)
|
||||
|
||||
|
||||
def persist_scan_run(database: Any, state: Dict[str, Any], *,
|
||||
profile_id: Any, was_cancelled: bool) -> bool:
|
||||
"""Record one watchlist scan run + its track ledger from ``state``.
|
||||
|
||||
Reads the counts/timestamps/ledger off the live ``watchlist_scan_state`` the
|
||||
scanner just finished writing, and writes a single history row. ``run_id``
|
||||
comes from ``state['scan_run_id']`` (both paths stamp it); a timestamp
|
||||
fallback keeps it from ever colliding if that's somehow missing. Returns the
|
||||
DB call's truthiness; callers wrap in their own try/except so a history-write
|
||||
failure never breaks the scan.
|
||||
"""
|
||||
summary = state.get('summary') or {}
|
||||
run_id = state.get('scan_run_id') or datetime.now().strftime('%Y%m%d-%H%M%S')
|
||||
return database.save_watchlist_scan_run(
|
||||
run_id=run_id,
|
||||
profile_id=profile_id if profile_id else 1,
|
||||
status='cancelled' if was_cancelled else 'completed',
|
||||
started_at=_iso(state.get('started_at')),
|
||||
completed_at=_iso(state.get('completed_at')) or datetime.now().isoformat(),
|
||||
total_artists=summary.get('total_artists', state.get('total_artists', 0)),
|
||||
artists_scanned=summary.get('successful_scans', 0),
|
||||
tracks_found=state.get('tracks_found_this_scan', 0),
|
||||
tracks_added=state.get('tracks_added_this_scan', 0),
|
||||
track_events=state.get('scan_track_events') or [],
|
||||
)
|
||||
|
|
@ -47,6 +47,62 @@ def test_resave_is_idempotent_on_run_id(db):
|
|||
assert len(runs) == 1 and runs[0]['tracks_added'] == 11
|
||||
|
||||
|
||||
# ── persist_scan_run: the shared seam both scan paths use (#933) ──
|
||||
|
||||
from datetime import datetime
|
||||
|
||||
from core.watchlist.scan_history import persist_scan_run
|
||||
|
||||
|
||||
def _state(**over):
|
||||
"""A watchlist_scan_state as the scanner leaves it when a scan finishes."""
|
||||
s = {
|
||||
'scan_run_id': 'auto-run-1',
|
||||
'started_at': datetime(2026, 6, 26, 2, 0, 0),
|
||||
'completed_at': datetime(2026, 6, 26, 2, 4, 0),
|
||||
'total_artists': 40,
|
||||
'tracks_found_this_scan': 7,
|
||||
'tracks_added_this_scan': 3,
|
||||
'scan_track_events': _events(n_added=3, n_skipped=1),
|
||||
'summary': {'total_artists': 40, 'successful_scans': 40,
|
||||
'new_tracks_found': 7, 'tracks_added_to_wishlist': 3},
|
||||
}
|
||||
s.update(over)
|
||||
return s
|
||||
|
||||
|
||||
def test_persist_scan_run_records_a_history_row(db):
|
||||
# the #933 fix: an automatic (all-profiles → profile_id=None) scan must land in History.
|
||||
assert persist_scan_run(db, _state(), profile_id=None, was_cancelled=False) is True
|
||||
runs = db.get_watchlist_scan_runs()
|
||||
assert len(runs) == 1
|
||||
r = runs[0]
|
||||
assert (r['run_id'], r['status'], r['tracks_found'], r['tracks_added']) == \
|
||||
('auto-run-1', 'completed', 7, 3)
|
||||
assert r['profile_id'] == 1 # None coerced to a concrete profile, never NULL
|
||||
# the per-run ledger came through too
|
||||
assert [e['status'] for e in db.get_watchlist_scan_run_events('auto-run-1')] == \
|
||||
['added', 'added', 'added', 'skipped']
|
||||
|
||||
|
||||
def test_persist_scan_run_cancelled_status(db):
|
||||
persist_scan_run(db, _state(scan_run_id='c1'), profile_id=2, was_cancelled=True)
|
||||
assert db.get_watchlist_scan_runs()[0]['status'] == 'cancelled'
|
||||
|
||||
|
||||
def test_persist_scan_run_accepts_datetime_or_iso_string(db):
|
||||
# state timestamps may be datetime (auto path) or already-iso strings — both must persist.
|
||||
persist_scan_run(db, _state(scan_run_id='dt', completed_at='2026-06-26T02:09:00'),
|
||||
profile_id=1, was_cancelled=False)
|
||||
assert db.get_watchlist_scan_runs()[0]['run_id'] == 'dt'
|
||||
|
||||
|
||||
def test_persist_scan_run_tolerates_sparse_state(db):
|
||||
# a bare/early-finished state must not raise — history-write must never break a scan.
|
||||
assert persist_scan_run(db, {'scan_run_id': 'sparse'}, profile_id=None, was_cancelled=False)
|
||||
assert db.get_watchlist_scan_runs()[0]['tracks_added'] == 0
|
||||
|
||||
|
||||
def test_prune_keeps_most_recent(db):
|
||||
for i in range(1, 8):
|
||||
db.save_watchlist_scan_run(
|
||||
|
|
|
|||
|
|
@ -81,6 +81,11 @@ class _FakeDB:
|
|||
self._watchlist_artists = watchlist_artists or []
|
||||
self._lb_profiles = lb_profiles or []
|
||||
self.database_path = '/tmp/test.db'
|
||||
self.scan_runs_saved = []
|
||||
|
||||
def save_watchlist_scan_run(self, **kwargs):
|
||||
self.scan_runs_saved.append(kwargs)
|
||||
return True
|
||||
|
||||
def get_all_profiles(self):
|
||||
return self._profiles
|
||||
|
|
@ -237,6 +242,39 @@ def test_successful_scan_runs_post_steps(patched_modules):
|
|||
assert deps._state_ref[0]['summary']['new_tracks_found'] == 2
|
||||
|
||||
|
||||
def test_successful_scan_records_history_run(patched_modules):
|
||||
"""#933: the automatic scan must persist a History row (it used to skip this,
|
||||
so only manual scans appeared in History)."""
|
||||
scanner, db = patched_modules
|
||||
deps = _build_deps()
|
||||
|
||||
autosc.process_watchlist_scan_automatically(automation_id='a1', deps=deps)
|
||||
|
||||
assert len(db.scan_runs_saved) == 1 # exactly one row, no double-record
|
||||
saved = db.scan_runs_saved[0]
|
||||
assert saved['status'] == 'completed'
|
||||
assert saved['artists_scanned'] == 1 # one successful _ScanResult
|
||||
assert saved['run_id'] # a run id was stamped
|
||||
|
||||
|
||||
def test_cancelled_scan_still_records_history_run(patched_modules, monkeypatch):
|
||||
"""A cancelled scan is recorded too, with status='cancelled'."""
|
||||
scanner, db = patched_modules
|
||||
deps = _build_deps()
|
||||
# Make the scan observe a cancel request mid-run.
|
||||
orig = scanner.scan_watchlist_artists
|
||||
def _cancel_during(artists, *, scan_state, progress_callback, cancel_check):
|
||||
scan_state['cancel_requested'] = True
|
||||
return orig(artists, scan_state=scan_state, progress_callback=progress_callback,
|
||||
cancel_check=cancel_check)
|
||||
monkeypatch.setattr(scanner, 'scan_watchlist_artists', _cancel_during)
|
||||
|
||||
autosc.process_watchlist_scan_automatically(automation_id='a1', deps=deps)
|
||||
|
||||
assert len(db.scan_runs_saved) == 1
|
||||
assert db.scan_runs_saved[0]['status'] == 'cancelled'
|
||||
|
||||
|
||||
def test_completion_emits_automation_event(patched_modules):
|
||||
"""Successful scan emits 'watchlist_scan_completed' on automation_engine."""
|
||||
scanner, _ = patched_modules
|
||||
|
|
|
|||
|
|
@ -28223,22 +28223,10 @@ def start_watchlist_scan():
|
|||
# #831 round 2: persist this run + its track ledger so the
|
||||
# Watchlist History modal can show what every past scan did.
|
||||
try:
|
||||
_state = watchlist_scan_state
|
||||
get_database().save_watchlist_scan_run(
|
||||
run_id=_state.get('scan_run_id') or datetime.now().strftime('%Y%m%d-%H%M%S'),
|
||||
profile_id=scan_profile_id,
|
||||
status='cancelled' if was_cancelled else 'completed',
|
||||
started_at=(_state.get('started_at').isoformat()
|
||||
if _state.get('started_at') else None),
|
||||
completed_at=(_state.get('completed_at') or datetime.now()).isoformat()
|
||||
if not isinstance(_state.get('completed_at'), str)
|
||||
else _state.get('completed_at'),
|
||||
total_artists=(_state.get('summary') or {}).get('total_artists',
|
||||
_state.get('total_artists', 0)),
|
||||
artists_scanned=(_state.get('summary') or {}).get('successful_scans', 0),
|
||||
tracks_found=_state.get('tracks_found_this_scan', 0),
|
||||
tracks_added=_state.get('tracks_added_this_scan', 0),
|
||||
track_events=_state.get('scan_track_events') or [],
|
||||
from core.watchlist.scan_history import persist_scan_run
|
||||
persist_scan_run(
|
||||
get_database(), watchlist_scan_state,
|
||||
profile_id=scan_profile_id, was_cancelled=was_cancelled,
|
||||
)
|
||||
except Exception as _hist_err:
|
||||
logger.error(f"Failed to persist watchlist scan run: {_hist_err}")
|
||||
|
|
|
|||
Loading…
Reference in a new issue