diff --git a/PROJECT.org b/PROJECT.org index 8589eae..1b9c67e 100644 --- a/PROJECT.org +++ b/PROJECT.org @@ -18,7 +18,7 @@ tasks, Todoist tasks, web pages I've read and trails I've hiked has turned out to be sometimes cathartic and sometimes functional as I try to remember when I did a thing. -* Backlog [2/28] :vrobbler:project:personal: +* Backlog [5/29] :vrobbler:project:personal: ** TODO [#C] After transition to linux add curl_cffi as webpage scrapper again :webpages:metadata: :PROPERTIES: :ID: d3cce1a7-d540-4d66-bf66-e75378e4eac7 @@ -512,7 +512,6 @@ named constants for maintainability. - ~vrobbler/apps/webpages/models.py~ (line 290) -- ="url"= - ~vrobbler/apps/scrobbles/importers/tsv.py~ (line 55) -- ="S"= completion status - ** TODO [#B] Is there way to create unique slugs for media instances :media_types: ** TODO [#A] Update how board game scrobbles work :boardgames: @@ -558,6 +557,40 @@ Added a `workouts` Django app so we can scrobble gym sessions. and `tests/workouts_tests/` (unit conversion, form round-trips, importer, imperial POST) included. +** DONE [#B] Investigate how historice lastfm imports work :importers:music: +:PROPERTIES: +:ID: f91fdd53-9da7-4859-9322-f08b2c587061 +:END: +*** Description + +Some time ago we added an ability for looking at the earliest LastFM import for +a given user and to kick off a series of monthly imports of past lastfm +scrobbles until there are no more. I tried running this a while ago and it +didn't work, and I wonder where we left it off. + +*** Investigation findings + +The historical import mechanism (`dispatch_historical_imports` in +`vrobbler/apps/scrobbles/importers/lastfm.py`) was added in commit `4e1c3ffb` +and ran successfully on 2026-05-24, importing 1,450 scrobbles covering +Feb-Apr 2023 (imports 1, 4, 5) — the user's full Last.fm history back to +registration. + +Two real bugs were found and fixed: + +- Resume cursor: the dispatch loop found the import with the earliest + `processed_finished` instead of the import covering the oldest scrobbles, + so re-running would re-dispatch already-imported months. It now walks back + from the earliest scrobble across all completed imports. +- Dedup: `import_from_lastfm` filtered existing scrobbles by `created` + (row-insert time) against the Last.fm play timestamp ± 20s, which never + matches for historical data and caused duplicate scrobbles on re-import. + It now compares `timestamp`. Also fixed a `tzinfo.name` crash for + `ZoneInfo` timezones. + +Tests added in `tests/scrobbles_tests/test_lastfm.py` (5 tests) covering +dedup-by-timestamp, creation, and dispatch resume/no-resume behavior. + ** DONE [#B] Fix LookupError on scrobble start/long-plays views :bug:scrobbles:videos: :PROPERTIES: :ID: 868e2a91-c974-4656-a8ae-b90a1f94ae74 diff --git a/tests/scrobbles_tests/test_lastfm.py b/tests/scrobbles_tests/test_lastfm.py new file mode 100644 index 0000000..505590f --- /dev/null +++ b/tests/scrobbles_tests/test_lastfm.py @@ -0,0 +1,221 @@ +from datetime import datetime, timedelta +from unittest.mock import MagicMock, patch + +import pytest +import pytz +from django.contrib.auth import get_user_model +from django.utils import timezone +from music.models import Track +from scrobbles.importers.lastfm import LastFM, dispatch_historical_imports +from scrobbles.models import LastFmImport, Scrobble + +User = get_user_model() + +UTC = pytz.utc + + +@pytest.fixture +def lfm_user(): + user = User.objects.create(username="lfmuser") + profile = user.profile + profile.lastfm_username = "lfmuser" + profile.lastfm_password = "secret" + profile.save() + return user + + +@pytest.fixture +def mock_lastfm_network(): + with patch("scrobbles.importers.lastfm.pylast.LastFMNetwork") as network, patch( + "scrobbles.importers.lastfm.pylast.md5", side_effect=lambda x: x + ): + network.return_value.get_user.return_value = MagicMock() + yield network + + +def make_scrobble_dict(timestamp, title="Emotion", artist="Carly Rae Jepsen"): + return { + "artist": artist, + "album": "Emotion", + "title": title, + "mbid": None, + "run_time_seconds": 60, + "timestamp": timestamp, + } + + +@pytest.fixture +def mock_track_find_or_create(): + with patch("scrobbles.importers.lastfm.Track.find_or_create") as mock_find: + + def _create(**kwargs): + title = kwargs.get("title", "Emotion") + track = Track.objects.filter(title=title).first() + if not track: + track = Track.objects.create( + title=title, + base_run_time_seconds=kwargs.get("run_time_seconds"), + ) + return track + + mock_find.side_effect = _create + yield mock_find + + +class TestImportFromLastfm: + @pytest.mark.django_db + def test_import_creates_scrobble( + self, lfm_user, mock_lastfm_network, mock_track_find_or_create + ): + timestamp = datetime(2023, 2, 15, 12, 0, 0, tzinfo=UTC) + lastfm = LastFM(lfm_user) + lastfm.get_last_scrobbles = MagicMock( + return_value=[make_scrobble_dict(timestamp)] + ) + + created = lastfm.import_from_lastfm() + + assert len(created) == 1 + scrobble = created[0] + assert scrobble.user == lfm_user + assert scrobble.timestamp == timestamp + assert scrobble.source == "Last.fm" + assert scrobble.media_type == "Track" + + @pytest.mark.django_db + def test_import_skips_existing_scrobble_by_timestamp( + self, lfm_user, mock_lastfm_network, mock_track_find_or_create + ): + timestamp = datetime(2023, 2, 15, 12, 0, 0, tzinfo=UTC) + track = Track.objects.create(title="Emotion", base_run_time_seconds=60) + existing = Scrobble.objects.create( + user=lfm_user, + timestamp=timestamp, + stop_timestamp=timestamp + timedelta(seconds=60), + source="Mopidy", + track=track, + media_type="Track", + ) + # The existing scrobble was created long ago, not at import time. + Scrobble.objects.filter(pk=existing.pk).update( + created=timestamp - timedelta(days=365) + ) + + lastfm = LastFM(lfm_user) + lastfm.get_last_scrobbles = MagicMock( + return_value=[make_scrobble_dict(timestamp)] + ) + + created = lastfm.import_from_lastfm() + + assert created == [] + assert Scrobble.objects.filter(track=track).count() == 1 + + @pytest.mark.django_db + def test_import_creates_scrobble_when_only_created_close( + self, lfm_user, mock_lastfm_network, mock_track_find_or_create + ): + timestamp = datetime(2023, 2, 15, 12, 0, 0, tzinfo=UTC) + track = Track.objects.create(title="Emotion", base_run_time_seconds=60) + # An existing scrobble for the same track but at a *different* play + # time (its row was created recently) must not block the new one. + Scrobble.objects.create( + user=lfm_user, + timestamp=timestamp - timedelta(hours=5), + stop_timestamp=timestamp - timedelta(hours=4, minutes=59), + source="Mopidy", + track=track, + media_type="Track", + ) + + lastfm = LastFM(lfm_user) + lastfm.get_last_scrobbles = MagicMock( + return_value=[make_scrobble_dict(timestamp)] + ) + + created = lastfm.import_from_lastfm() + + assert len(created) == 1 + assert Scrobble.objects.filter(track=track).count() == 2 + + +class TestDispatchHistoricalImports: + def _seed_import(self, user, scrobble, processed_finished): + lfm_import = LastFmImport.objects.create(user=user) + lfm_import.record_log([scrobble]) + lfm_import.mark_started() + LastFmImport.objects.filter(pk=lfm_import.pk).update( + processed_finished=processed_finished + ) + return lfm_import + + @pytest.mark.django_db + def test_resumes_before_earliest_scrobble_not_earliest_processed( + self, lfm_user, mock_lastfm_network + ): + # Import A finished first but covers a LATER month; import B finished + # second but holds the EARLIEST scrobbles. This is the exact scenario + # from production (import 1 = April finished 19:33, import 5 = February). + feb_scrobble = Scrobble.objects.create( + user=lfm_user, + timestamp=datetime(2023, 2, 15, 12, 0, 0, tzinfo=UTC), + source="Last.fm", + media_type="Track", + ) + apr_scrobble = Scrobble.objects.create( + user=lfm_user, + timestamp=datetime(2023, 4, 15, 12, 0, 0, tzinfo=UTC), + source="Last.fm", + media_type="Track", + ) + now = timezone.now() + self._seed_import(lfm_user, apr_scrobble, now - timedelta(hours=2)) + self._seed_import(lfm_user, feb_scrobble, now - timedelta(hours=1)) + + mock_lastfm = MagicMock() + mock_lastfm.get_earliest_scrobble_timestamp.return_value = datetime( + 2022, 6, 1, tzinfo=UTC + ) + + def fake_get_last_scrobbles(time_from=None, time_to=None, check=False): + if time_from and time_from.month == 1 and time_from.year == 2023: + return True + return False + + mock_lastfm.get_last_scrobbles = fake_get_last_scrobbles + + with patch( + "scrobbles.importers.lastfm.LastFM", return_value=mock_lastfm + ), patch("scrobbles.tasks.process_lastfm_import.delay") as mock_delay: + dispatched = dispatch_historical_imports(lfm_user.id) + + assert dispatched == 1 + mock_delay.assert_called_once() + args, kwargs = mock_delay.call_args + assert kwargs["time_from"] == datetime(2023, 1, 1, tzinfo=UTC) + assert kwargs["time_to"] == datetime( + 2023, 1, 31, 23, 59, 59, 999999, tzinfo=UTC + ) + + # Only one new import (January 2023); February and April are untouched. + new_imports = LastFmImport.objects.filter( + user=lfm_user, processed_finished__isnull=True + ) + assert new_imports.count() == 1 + + @pytest.mark.django_db + def test_no_existing_imports_starts_from_now(self, lfm_user, mock_lastfm_network): + mock_lastfm = MagicMock() + mock_lastfm.get_earliest_scrobble_timestamp.return_value = datetime( + 2022, 6, 1, tzinfo=UTC + ) + mock_lastfm.get_last_scrobbles = MagicMock(return_value=False) + + with patch( + "scrobbles.importers.lastfm.LastFM", return_value=mock_lastfm + ), patch("scrobbles.tasks.process_lastfm_import.delay") as mock_delay: + dispatched = dispatch_historical_imports(lfm_user.id) + + assert dispatched == 0 + mock_delay.assert_not_called() + assert not LastFmImport.objects.filter(user=lfm_user).exists() diff --git a/vrobbler/apps/scrobbles/importers/lastfm.py b/vrobbler/apps/scrobbles/importers/lastfm.py index 4410348..1f5554e 100644 --- a/vrobbler/apps/scrobbles/importers/lastfm.py +++ b/vrobbler/apps/scrobbles/importers/lastfm.py @@ -66,6 +66,8 @@ class LastFM: ) timestamp = lfm_scrobble.get("timestamp") stop_timestamp = timestamp + timedelta(seconds=track.run_time_seconds) + tzinfo = tz_timestamp.tzinfo + timezone = getattr(tzinfo, "key", None) or getattr(tzinfo, "name", None) new_scrobble = Scrobble( user=self.vrobbler_user, timestamp=timestamp, @@ -75,15 +77,15 @@ class LastFM: played_to_completion=True, in_progress=False, media_type=Scrobble.MediaType.TRACK, - timezone=tz_timestamp.tzinfo.name, + timezone=timezone, visibility="private", ) # Vrobbler scrobbles on finish, LastFM scrobbles on start seconds_eariler = timestamp - timedelta(seconds=20) seconds_later = timestamp + timedelta(seconds=20) existing = Scrobble.objects.filter( - created__gte=seconds_eariler, - created__lte=seconds_later, + timestamp__gte=seconds_eariler, + timestamp__lte=seconds_later, track=track, ).first() if existing: @@ -238,21 +240,23 @@ def dispatch_historical_imports(user_id): day=last_day, hour=23, minute=59, second=59, microsecond=999999 ) - earliest = ( - LastFmImport.objects.filter(user_id=user_id, processed_finished__isnull=False) - .order_by("processed_finished") - .first() + earliest_scrobble_dt = None + completed_imports = LastFmImport.objects.filter( + user_id=user_id, processed_finished__isnull=False ) - if earliest: - earliest_log_scrobble = earliest.scrobbles().order_by("timestamp").first() - cursor = ( - _first_of_month(earliest_log_scrobble.timestamp) - if earliest_log_scrobble - else earliest.processed_finished - ) + for lfm_import in completed_imports: + log_scrobble = lfm_import.scrobbles().order_by("timestamp").first() + if log_scrobble and ( + earliest_scrobble_dt is None + or log_scrobble.timestamp < earliest_scrobble_dt + ): + earliest_scrobble_dt = log_scrobble.timestamp + + if earliest_scrobble_dt: + cursor = _first_of_month(earliest_scrobble_dt) logger.info( - "Found existing import; earliest scrobble %s, cursor set to %s", - earliest_log_scrobble.timestamp if earliest_log_scrobble else None, + "Found existing imports; earliest scrobble %s, cursor set to %s", + earliest_scrobble_dt, cursor, ) else: