[books] Fix one-extra-scrobble bug on Koreader imports

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

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

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)."
+            )

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