Addresses PR feedback:

The main fix here is to ensure that loans are not over counted when associated title and/or isbn metadata changes between playtime entry updates for a given loan.
Other fixes include:
Removal of loan identifier from index
Change loan_identifier field length from 50 to 40
Remove incorrect column "timestamp" in alembic downgrade
Changed comment
Added constraint to loan query.

alembic/versions/20240821_7a2fcaac8b63_add_loan_identifier_column_to_playtime_tables.py

@@ -22,82 +22,15 @@ def upgrade() -> None:
 
     op.add_column(
         "playtime_entries",
-        sa.Column("loan_identifier", sa.String(length=50), nullable=False, default=""),
-    )
-
-    op.drop_constraint(
-        "unique_playtime_entry",
-        "playtime_entries",
-        type_="unique",
-    )
-
-    op.create_unique_constraint(
-        "unique_playtime_entry",
-        "playtime_entries",
-        [
-            "tracking_id",
-            "identifier_str",
-            "collection_name",
-            "library_name",
-            "loan_identifier",
-        ],
+        sa.Column("loan_identifier", sa.String(length=40), nullable=False, default=""),
     )
 
     op.add_column(
         "playtime_summaries",
-        sa.Column("loan_identifier", sa.String(length=50), nullable=False, default=""),
-    )
-
-    op.drop_constraint(
-        "unique_playtime_summary",
-        "playtime_summaries",
-        type_="unique",
-    )
-
-    op.create_unique_constraint(
-        "unique_playtime_summary",
-        "playtime_summaries",
-        [
-            "timestamp",
-            "identifier_str",
-            "collection_name",
-            "library_name",
-            "loan_identifier",
-        ],
+        sa.Column("loan_identifier", sa.String(length=40), nullable=False, default=""),
     )
 
 
 def downgrade() -> None:
-    op.drop_constraint(
-        "unique_playtime_entry",
-        "playtime_entries",
-        type_="unique",
-    )
-
     op.drop_column("playtime_entries", "loan_identifier")
-
-    op.create_unique_constraint(
-        "unique_playtime_entry",
-        "playtime_entries",
-        [
-            "tracking_id",
-            "timestamp",
-            "identifier_str",
-            "collection_name",
-            "library_name",
-        ],
-    )
-
-    op.drop_constraint(
-        "unique_playtime_summary",
-        "playtime_summaries",
-        type_="unique",
-    )
-
     op.drop_column("playtime_summaries", "loan_identifier")
-
-    op.create_unique_constraint(
-        "unique_playtime_summary",
-        "playtime_summaries",
-        ["timestamp", "identifier_str", "collection_name", "library_name"],
-    )

src/palace/manager/api/controller/playtime_entries.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-import hashlib
+from datetime import timedelta
 
 import flask
 from pydantic import ValidationError
@@ -16,19 +16,20 @@
 from palace.manager.api.problem_details import NOT_FOUND_ON_REMOTE
 from palace.manager.core.problem_details import INVALID_INPUT
 from palace.manager.core.query.playtime_entries import PlaytimeEntries
+from palace.manager.sqlalchemy.constants import EditionConstants
 from palace.manager.sqlalchemy.model.collection import Collection
 from palace.manager.sqlalchemy.model.identifier import Identifier
 from palace.manager.sqlalchemy.model.library import Library
 from palace.manager.sqlalchemy.model.licensing import LicensePool
-from palace.manager.sqlalchemy.model.patron import Loan, Patron
+from palace.manager.sqlalchemy.model.patron import Loan
 from palace.manager.sqlalchemy.util import get_one
 
 
-def resolve_loan_identifier(patron: Patron, loan: Loan | None) -> str:
+def resolve_loan_identifier(loan: Loan | None) -> str:
     def sha1(msg):
-        return hashlib.sha1(msg.encode()).hexdigest()
+        return
 
-    return sha1(f"loan: {loan.id}") if loan else sha1(f"patron:{patron.id}")
+    return sha1(f"loan: {loan.id}") if loan else "no-loan-found"
 
 
 class PlaytimeEntriesController(CirculationManagerController):
@@ -61,20 +62,29 @@ def track_playtimes(self, collection_id, identifier_type, identifier_idn):
         except ValidationError as ex:
             return INVALID_INPUT.detailed(ex.json())
 
+        min_time_entry = min([x.during_minute for x in data.time_entries])
+        max_time_entry = max([x.during_minute for x in data.time_entries])
+
+        default_loan_period = timedelta(
+            collection.default_loan_period(
+                library=library, medium=EditionConstants.AUDIO_MEDIUM
+            )
+        )
+
         loan = self._db.execute(
             select(Loan)
             .select_from(Loan)
             .join(LicensePool)
             .where(
                 LicensePool.identifier == identifier,
                 Loan.patron == flask.request.patron,
+                Loan.start >= min_time_entry,
+                Loan.start + default_loan_period < max_time_entry,
             )
             .order_by(Loan.start.desc())
         ).first()
 
-        loan_identifier = resolve_loan_identifier(
-            loan=loan, patron=flask.request.patron
-        )
+        loan_identifier = resolve_loan_identifier(loan=loan)
 
         responses, summary = PlaytimeEntries.insert_playtime_entries(
             self._db,

src/palace/manager/scripts/playtime_entries.py

@@ -12,7 +12,9 @@
 import dateutil.parser
 import pytz
 from sqlalchemy.sql.expression import and_, distinct, false, select, true
-from sqlalchemy.sql.functions import coalesce, count, sum
+from sqlalchemy.sql.functions import coalesce, count
+from sqlalchemy.sql.functions import max as sql_max
+from sqlalchemy.sql.functions import sum
 
 from palace.manager.core.config import Configuration
 from palace.manager.scripts.base import Script
@@ -57,7 +59,7 @@ def do_run(self):
             PlaytimeEntry.timestamp <= cut_off,
         )
 
-        # Aggregate entries per identifier-timestamp-collection-library grouping.
+        # Aggregate entries per identifier-timestamp-collection-library-loan_identifier grouping.
         # The label forms of the identifier, collection, and library are also
         # factored in, in case any of the foreign keys are missing.
         # Since timestamps should be on minute-boundaries the aggregation
@@ -218,22 +220,30 @@ def do_run(self):
                 self.log.warning(temp.read())
 
     def _fetch_report_records(self, start: datetime, until: datetime) -> Query:
+        # The loan count query returns only non-empty string isbns and titles if there is more
+        # than one row returned with the grouping.  This way we ensure that we do not
+        # count the same loan twice in the case we have when a
+        # 1. a single loan with identifier A
+        # 2. and one or more playtime summaries with title A or no title or isbn A or no isbn
+        # 3. and one more playtime summaries with title B, isbn B
+        # This situation can occur when the title and isbn  metadata associated with an ID changes due to a feed
+        # update that occurs between playlist entry posts.
+        # in this case we just associate the loan identifier with one unique combination of the list of titles and isbn
+        # values.
         loan_count_query = (
             select(
                 PlaytimeSummary.identifier_str.label("identifier_str2"),
                 PlaytimeSummary.collection_name.label("collection_name2"),
                 PlaytimeSummary.library_name.label("library_name2"),
-                coalesce(PlaytimeSummary.isbn, "").label("isbn2"),
-                coalesce(PlaytimeSummary.title, "").label("title2"),
+                sql_max(coalesce(PlaytimeSummary.isbn, "")).label("isbn2"),
+                sql_max(coalesce(PlaytimeSummary.title, "")).label("title2"),
                 count(distinct(PlaytimeSummary.loan_identifier)).label("loan_count"),
             )
             .where(PlaytimeSummary.timestamp.between(start, until))
             .group_by(
                 PlaytimeSummary.identifier_str,
                 PlaytimeSummary.collection_name,
                 PlaytimeSummary.library_name,
-                PlaytimeSummary.isbn,
-                PlaytimeSummary.title,
                 PlaytimeSummary.identifier_id,
             )
             .subquery()
@@ -260,14 +270,14 @@ def _fetch_report_records(self, start: datetime, until: datetime) -> Query:
             .subquery()
         )
 
-        combined = self._db.query(seconds_query, loan_count_query).join(
+        combined = self._db.query(seconds_query, loan_count_query).outerjoin(
             loan_count_query,
             and_(
-                loan_count_query.c.identifier_str2 == seconds_query.c.identifier_str,
-                loan_count_query.c.collection_name2 == seconds_query.c.collection_name,
-                loan_count_query.c.library_name2 == seconds_query.c.library_name,
-                loan_count_query.c.isbn2 == seconds_query.c.isbn,
-                loan_count_query.c.title2 == seconds_query.c.title,
+                seconds_query.c.identifier_str == loan_count_query.c.identifier_str2,
+                seconds_query.c.collection_name == loan_count_query.c.collection_name2,
+                seconds_query.c.library_name == loan_count_query.c.library_name2,
+                seconds_query.c.isbn == loan_count_query.c.isbn2,
+                seconds_query.c.title == loan_count_query.c.title2,
             ),
         )
         combined_sq = combined.subquery()
@@ -279,7 +289,7 @@ def _fetch_report_records(self, start: datetime, until: datetime) -> Query:
             combined_sq.c.isbn,
             combined_sq.c.title,
             combined_sq.c.total_seconds_played,
-            combined_sq.c.loan_count,
+            coalesce(combined_sq.c.loan_count, 0),
         ).order_by(
             combined_sq.c.collection_name,
             combined_sq.c.library_name,

src/palace/manager/sqlalchemy/model/time_tracking.py

@@ -74,15 +74,14 @@ class PlaytimeEntry(Base):
     collection: Mapped[Collection] = relationship("Collection", uselist=False)
     library: Mapped[Library] = relationship("Library", uselist=False)
 
-    loan_identifier = Column(String(50), nullable=False)
+    loan_identifier = Column(String(40), nullable=False)
 
     __table_args__ = (
         UniqueConstraint(
             "tracking_id",
             "identifier_str",
             "collection_name",
             "library_name",
-            "loan_identifier",
             name="unique_playtime_entry",
         ),
     )
@@ -131,7 +130,7 @@ class PlaytimeSummary(Base):
 
     title = Column(String)
     isbn = Column(String)
-    loan_identifier = Column(String(50), nullable=False)
+    loan_identifier = Column(String(40), nullable=False)
 
     identifier: Mapped[Identifier] = relationship("Identifier", uselist=False)
     collection: Mapped[Collection] = relationship("Collection", uselist=False)
@@ -143,7 +142,6 @@ class PlaytimeSummary(Base):
             "identifier_str",
             "collection_name",
             "library_name",
-            "loan_identifier",
             name="unique_playtime_summary",
         ),
     )

tests/manager/api/controller/test_playtime_entries.py

@@ -106,17 +106,16 @@ def test_track_playtime_duplicate_id_ok(
         identifier = db.identifier()
         collection = db.default_collection()
         library = db.default_library()
-
+        patron = db.patron()
         # Attach the identifier to the collection
         pool = db.licensepool(
             db.edition(
                 identifier_type=identifier.type, identifier_id=identifier.identifier
             ),
             collection=collection,
         )
-        patron = db.patron()
 
-        loan_identifier = resolve_loan_identifier(loan=None, patron=patron)
+        loan_identifier = resolve_loan_identifier(loan=None)
 
         db.session.add(
             PlaytimeEntry(