From c71b51fdb8b86586e89c0b7b9600496e16462dbe Mon Sep 17 00:00:00 2001 From: Colin Powell Date: Fri, 12 Jun 2026 09:52:33 -0400 Subject: [PATCH] [books] Fix one-extra-scrobble bug on Koreader imports --- PROJECT.org | 13 +- vrobbler/apps/books/koreader.py | 114 ++++----------- .../commands/merge_koreader_scrobbles.py | 130 ++++++++++++++++++ vrobbler/apps/books/tests/test_koreader.py | 10 +- 4 files changed, 170 insertions(+), 97 deletions(-) create mode 100644 vrobbler/apps/books/management/commands/merge_koreader_scrobbles.py diff --git a/PROJECT.org b/PROJECT.org index 6272bb3..787eba1 100644 --- a/PROJECT.org +++ b/PROJECT.org @@ -88,7 +88,7 @@ fetching and simple saving. *** Metadata sources **** Scraper -* Backlog [1/15] :vrobbler:project:personal: +* Backlog [2/16] :vrobbler:project:personal: ** TODO [#C] Create small utility to clean up tracks scrobbled with wonky playback times :vrobbler:personal:bug:music:scrobbles: :PROPERTIES: :ID: 702462cf-d54b-48c6-8a7c-78b8de751deb @@ -518,6 +518,17 @@ favorited media objects. :PROPERTIES: :ID: 79f867c3-1288-4143-b6bf-2a452983ee9f :END: +** DONE [#A] Fix bug where last page of book gets separate scrobble :bug:books:importers:koreader: +:PROPERTIES: +:ID: e13e0b4c-461e-e5a9-c685-b972f4e262e5 +:END: + +*** Description + +The new KoReader code is working great to import books with the correct timezone +and what-not. But it has a weird artifact of creating one extra scrobble for the +last page read. Need to button that up. + ** DONE [#B] Fix metadata scraping for books :books:metadata: :PROPERTIES: :ID: ea416a69-a8a8-4d05-b7d4-0a3470820e34 diff --git a/vrobbler/apps/books/koreader.py b/vrobbler/apps/books/koreader.py index 7d92390..c955105 100644 --- a/vrobbler/apps/books/koreader.py +++ b/vrobbler/apps/books/koreader.py @@ -217,7 +217,6 @@ def build_scrobbles_from_book_map(book_map: dict, user: "User") -> list["Scrobbl pages_not_found.append(book_id) continue - should_create_scrobble = False scrobble_page_data = {} playback_position_seconds = 0 prev_page_stats = {} @@ -247,32 +246,39 @@ def build_scrobbles_from_book_map(book_map: dict, user: "User") -> list["Scrobbl end_of_reading = pages_processed == total_pages_read big_jump_to_this_page = (page_number - last_page_number) > 10 is_session_gap = seconds_from_last_page > SESSION_GAP_SECONDS - if (is_session_gap and not big_jump_to_this_page) or end_of_reading: - should_create_scrobble = True + should_create_scrobble = ( + is_session_gap and not big_jump_to_this_page + ) or end_of_reading + + # Always accumulate the current page first + scrobble_page_data[page_number] = stats if should_create_scrobble: - if not scrobble_page_data: - scrobble_page_data[page_number] = stats + # For end-of-reading, the current page is already accumulated + # and belongs in this scrobble. For a session gap, remove the + # current page from this scrobble — it starts a new session. + if is_session_gap and not end_of_reading: + del scrobble_page_data[page_number] + scrobble_page_data = dict( sorted( scrobble_page_data.items(), key=lambda x: x[1]["start_ts"], ) ) - try: - first_page = scrobble_page_data.get( - list(scrobble_page_data.keys())[0] - ) - last_page = scrobble_page_data.get( - list(scrobble_page_data.keys())[-1] - ) - except IndexError: + + if not scrobble_page_data: logger.error( "Could not process book, no page data found", extra={"scrobble_page_data": scrobble_page_data}, ) continue + first_page = next(iter(scrobble_page_data.values())) + last_page = scrobble_page_data[ + list(scrobble_page_data.keys())[-1] + ] + timestamp = user.profile.get_timestamp_with_tz( datetime.fromtimestamp( int(first_page.get("start_ts")) @@ -319,90 +325,18 @@ def build_scrobbles_from_book_map(book_map: dict, user: "User") -> list["Scrobbl timezone=tz, ) ) - # Then start over - should_create_scrobble = False + # Then start over for the next session playback_position_seconds = 0 scrobble_page_data = {} - # We accumulate pages for the scrobble until we should create a new one - scrobble_page_data[page_number] = stats + # For session gaps, re-add the current page as the + # beginning of the next session's accumulation + if is_session_gap and not end_of_reading: + scrobble_page_data[page_number] = stats last_page_number = page_number prev_page_stats = stats - # Handle leftover pages that never triggered a session gap - if scrobble_page_data: - scrobble_page_data = dict( - sorted( - scrobble_page_data.items(), - key=lambda x: x[1]["start_ts"], - ) - ) - try: - first_page = scrobble_page_data.get( - list(scrobble_page_data.keys())[0] - ) - last_page = scrobble_page_data.get( - list(scrobble_page_data.keys())[-1] - ) - except IndexError: - logger.error( - "Could not process book, no page data found", - extra={"scrobble_page_data": scrobble_page_data}, - ) - continue - - playback_position_seconds = sum( - p["duration"] for p in scrobble_page_data.values() - ) - - timestamp = user.profile.get_timestamp_with_tz( - datetime.fromtimestamp( - int(first_page.get("start_ts")) - ) - ) - stop_timestamp = user.profile.get_timestamp_with_tz( - datetime.fromtimestamp( - int(last_page.get("end_ts")) - ) - ) - - scrobble = Scrobble.objects.filter( - timestamp=timestamp, - book_id=book_id, - user_id=user.id, - ).first() - - if not scrobble: - logger.info( - f"Queueing scrobble for {book_id}, page {list(scrobble_page_data.keys())[0]}" - ) - log_data = { - "koreader_hash": book_dict.get("hash"), - "page_data": scrobble_page_data, - "pages_read": len(scrobble_page_data.keys()), - } - if hasattr(timestamp.tzinfo, "tzname"): - tz = timestamp.tzinfo.tzname - if hasattr(timestamp.tzinfo, "name"): - tz = timestamp.tzinfo.name - scrobbles_to_create.append( - Scrobble( - book_id=book_id, - user_id=user.id, - source="KOReader", - media_type=Scrobble.MediaType.BOOK, - timestamp=timestamp, - log=log_data, - stop_timestamp=stop_timestamp, - playback_position_seconds=playback_position_seconds, - in_progress=False, - played_to_completion=True, - long_play_complete=False, - timezone=tz, - ) - ) - if pages_not_found: logger.info(f"Pages not found for books: {set(pages_not_found)}") return scrobbles_to_create diff --git a/vrobbler/apps/books/management/commands/merge_koreader_scrobbles.py b/vrobbler/apps/books/management/commands/merge_koreader_scrobbles.py new file mode 100644 index 0000000..3e4f1ac --- /dev/null +++ b/vrobbler/apps/books/management/commands/merge_koreader_scrobbles.py @@ -0,0 +1,130 @@ +import logging + +from datetime import timedelta + +from django.core.management.base import BaseCommand +from django.db import transaction + +from books.koreader import SESSION_GAP_SECONDS, fix_long_play_stats_for_scrobbles + +logger = logging.getLogger(__name__) +SESSION_GAP = timedelta(seconds=SESSION_GAP_SECONDS) + + +def _page_data_keys(pages): + return sorted(int(k) for k in (pages or {})) + + +class Command(BaseCommand): + help = "Merge orphaned 1-page KOReader scrobbles into the preceding scrobble" + + def add_arguments(self, parser): + parser.add_argument( + "--commit", + action="store_true", + help="Commit changes to the database", + ) + + def handle(self, *args, **options): + from scrobbles.models import Scrobble + + commit = options["commit"] + + qs = Scrobble.objects.filter( + media_type="Book", source="KOReader" + ).order_by("book_id", "timestamp") + + if not qs.exists(): + self.stdout.write("No KOReader book scrobbles found.") + return + + merged = 0 + affected_books = set() + + # Group by book_id manually since we're iterating in order + book_scrobbles = {} + for s in qs: + book_scrobbles.setdefault(s.book_id, []).append(s) + + if not commit: + self.stdout.write("Dry run — no changes will be saved. Use --commit to apply.") + + for book_id, scrobbles in book_scrobbles.items(): + batch_merged = 0 + i = 0 + while i < len(scrobbles) - 1: + current = scrobbles[i] + orphan = scrobbles[i + 1] + + orphan_pages = orphan.logdata.page_data if orphan.logdata else {} + orphan_keys = _page_data_keys(orphan_pages) + if len(orphan_keys) != 1: + i += 1 + continue + + current_pages = current.logdata.page_data if current.logdata else {} + current_keys = _page_data_keys(current_pages) + if not current_keys: + i += 1 + continue + + orphan_page_num = orphan_keys[0] + current_last_page = current_keys[-1] + + if orphan_page_num != current_last_page + 1: + i += 1 + continue + + # Check that the orphan is close enough in time + gap = orphan.timestamp - current.stop_timestamp + if gap > SESSION_GAP: + i += 1 + continue + + # Merge orphan into current + current_pages[str(orphan_page_num)] = orphan_pages[str(orphan_page_num)] + current.log["page_data"] = current_pages + current.log["pages_read"] = len(current_pages) + current.stop_timestamp = orphan.stop_timestamp + current.playback_position_seconds += orphan.playback_position_seconds + + affected_books.add(book_id) + + if commit: + with transaction.atomic(): + current.save( + update_fields=[ + "log", + "stop_timestamp", + "playback_position_seconds", + ] + ) + orphan.delete() + + merged += 1 + batch_merged += 1 + scrobbles.pop(i + 1) + + if batch_merged: + self.stdout.write( + f" Book {book_id}: merged {batch_merged} orphan scrobble(s)" + ) + + self.stdout.write(f"\nTotal orphans merged: {merged}") + + if commit and affected_books: + self.stdout.write("Recalculating long_play_stats for affected books...") + for book_id in affected_books: + scrobbles_to_fix = ( + Scrobble.objects.filter(book_id=book_id, source="KOReader") + .order_by("timestamp") + ) + fix_long_play_stats_for_scrobbles(list(scrobbles_to_fix)) + + self.stdout.write(f"Fixed stats for {len(affected_books)} books.") + + if not commit: + self.stdout.write( + f"\nWould merge {merged} orphan scrobble(s) across " + f"{len(affected_books)} book(s)." + ) diff --git a/vrobbler/apps/books/tests/test_koreader.py b/vrobbler/apps/books/tests/test_koreader.py index b9435ca..db101ee 100644 --- a/vrobbler/apps/books/tests/test_koreader.py +++ b/vrobbler/apps/books/tests/test_koreader.py @@ -46,18 +46,16 @@ def test_build_scrobbles_from_pages(get_mock, koreader_rows, demo_user, valid_re scrobbles = build_scrobbles_from_book_map(book_map, demo_user) # The test data generator adds the session-gap 3600s AFTER the trigger page # (not before), so the first session includes 21 pages (1-21), and each - # subsequent session has 20 until the last. The last trigger page (120) was - # previously orphaned by the loop structure; the post-loop fix now creates a - # scrobble for it. - expected_scrobbles = 7 * len(book_map.keys()) + # subsequent session has 20 until the last. The last page is now included + # in the final scrobble instead of being orphaned. + expected_scrobbles = 6 * len(book_map.keys()) assert len(scrobbles) == expected_scrobbles assert len(scrobbles[0].logdata.page_data.keys()) == 21 assert len(scrobbles[1].logdata.page_data.keys()) == 20 assert len(scrobbles[2].logdata.page_data.keys()) == 20 assert len(scrobbles[3].logdata.page_data.keys()) == 20 assert len(scrobbles[4].logdata.page_data.keys()) == 20 - assert len(scrobbles[5].logdata.page_data.keys()) == 18 - assert len(scrobbles[6].logdata.page_data.keys()) == 1 + assert len(scrobbles[5].logdata.page_data.keys()) == 19 def test_get_author_str_from_row():