[PP-1456] Add loan count column to audio book playback time reports. #2002

dbernstein · 2024-08-21T22:39:55Z

Description

As the title indicates, this PR adds a loan count column to the audio book playback time reports.

When playback time data comes in through the API, the code attempts to resolve the loan associated with the audio book and patron. If the loan cannot be found (ie because it was purged) we use the user's patron id. Then we create a sha1 of the loan or patron ID in order to uniquely identify the loan.

Then on the reporting side, we perform a count of the unique loans within the time range and join that count to the seconds summation query.

The default value for the loan_identifier is an empty string. Since we don't have a way of associating loans or patrons with existing playtime records, I don't see a good way of counting old playtime entries and summaries. Once the start date of the reporting window passes the date of the release of this update to production the loan counting will be correct.
Before that, it will be one loan per unique isbn, collection, library.

Motivation and Context

https://ebce-lyrasis.atlassian.net/browse/PP-1456

How Has This Been Tested?

Updated the existing tests and added addition assertions.

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2024-08-22T01:38:23Z

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.68%. Comparing base (de62c4e) to head (f56fcd5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../palace/manager/api/controller/playtime_entries.py	92.30%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2002   +/-   ##
=======================================
  Coverage   90.68%   90.68%           
=======================================
  Files         342      342           
  Lines       40517    40538   +21     
  Branches     8782     8784    +2     
=======================================
+ Hits        36742    36763   +21     
  Misses       2510     2510           
  Partials     1265     1265

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tdilauro

I have some concerns here about how isbn/title could affect loan counts. A few other concerns, as well. I didn't really get into the tests yet, since they might be affected by how we decide to deal with these other concerns.

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

tdilauro · 2024-08-22T20:23:48Z

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

+            .where(
+                LicensePool.identifier == identifier,
+                Loan.patron == flask.request.patron,
+            )
+            .order_by(Loan.start.desc())


There should probably be a further constraint here: the loan should have been active during the timestamp extracted from the posted data.

src/palace/manager/scripts/playtime_entries.py

tdilauro · 2024-08-22T21:12:54Z

alembic/versions/20240821_7a2fcaac8b63_add_loan_identifier_column_to_playtime_tables.py

+        "playtime_entries",
+        [
+            "tracking_id",
+            "timestamp",


The appearance of timestamp in this downgrade is unexpected, since the previous index did not include it...

__table_args__ = ( UniqueConstraint( "tracking_id", "identifier_str", "collection_name", "library_name", name="unique_playtime_entry", ), )```

Very good catch.

tdilauro · 2024-08-22T21:17:50Z

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

    __table_args__ = (
        UniqueConstraint(
            "tracking_id",
            "identifier_str",
            "collection_name",
            "library_name",
+            "loan_identifier",


I'm not entirely sure that we need to update the constraint for playtime_entries, since a given patron shouldn't be "reading" the same book both with and without a loan_identifier within the already-existing constraints (tracking_id/timestamp, identifier_str, etc.).

That said, there's probably no harm in it (other than increasing the size of the index, of course).

Maybe we should leave it in there as a sanity check? I don't know.

I defer to your judgement on this one, not only because I respect your opinion but also because you are the Playtime Tracking Man!!! :)

Regardless of what we do for playtime_entries, I think we DO want loan_identifier included in the constraint on playtime_summaries.

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

src/palace/manager/scripts/playtime_entries.py

tdilauro

👏🏽 Nice solution to the isbn and/or title mismatch issue. Additional comments below.

tdilauro · 2024-08-28T18:17:17Z

src/palace/manager/scripts/playtime_entries.py

+                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,
-                sum(PlaytimeSummary.total_seconds_played),
+                PlaytimeSummary.identifier_id,


This is a clever solution to a tricky problem!

tdilauro · 2024-08-28T18:22:20Z

src/palace/manager/scripts/playtime_entries.py

+            None if isbn == "" else isbn,
            collection_name,
            library_name,
-            title,
+            None if title == "" else title,


Minor: I assume that an empty string would be fine for both isbn and title.

It should be.

tdilauro · 2024-08-28T18:32:03Z

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

+                LicensePool.identifier == identifier,
+                Loan.patron == flask.request.patron,
+                Loan.start >= min_time_entry,
+                Loan.start + default_loan_period < max_time_entry,


Can we not just use Loan.end here?

Good point - I didn't see the end field.

tdilauro · 2024-08-28T18:39:24Z

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

    __table_args__ = (
        UniqueConstraint(
            "tracking_id",
            "identifier_str",
            "collection_name",
            "library_name",
+            "loan_identifier",


Regardless of what we do for playtime_entries, I think we DO want loan_identifier included in the constraint on playtime_summaries.

tdilauro · 2024-08-28T18:42:42Z

tests/manager/api/controller/test_playtime_entries.py

        # 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)


Should we verify here that the loan_identifier is what we expect it to be?

Sure why not.

tdilauro · 2024-08-28T18:57:35Z

tests/manager/scripts/test_playtime_entries.py

+            loan_identifier6,
+        )
+
+        # log a summary where a title that was previously unavailable is now available for the same loan


It's more likely that book (or the entire collection) will be gone, though this probably covers the case for now.

tdilauro · 2024-08-28T19:07:23Z

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

+                Loan.start >= min_time_entry,
+                Loan.start + default_loan_period < max_time_entry,


It doesn't look like we create any loans in our tests. If not, I'm not sure how the loan lookup gets tested.

I'll fix that.

dbernstein · 2024-09-03T16:42:03Z

@tdilauro : I've made updates to the PR to address the issues you raised.

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.

tdilauro

I think this one is ready to go!! 🚀

dbernstein marked this pull request as draft August 21, 2024 22:40

dbernstein force-pushed the track-loan-in-playtime-entries branch from ba0e30d to 7938839 Compare August 22, 2024 01:14

dbernstein marked this pull request as ready for review August 22, 2024 04:11

dbernstein force-pushed the track-loan-in-playtime-entries branch 2 times, most recently from d3ce2cb to e34ca0c Compare August 22, 2024 04:12

dbernstein added the feature New feature label Aug 22, 2024

dbernstein requested a review from a team August 22, 2024 15:44

tdilauro reviewed Aug 22, 2024

View reviewed changes

dbernstein force-pushed the track-loan-in-playtime-entries branch from 9cb8bca to e5c2823 Compare August 27, 2024 15:33

dbernstein requested a review from tdilauro August 27, 2024 16:52

dbernstein force-pushed the track-loan-in-playtime-entries branch from e5c2823 to f938efc Compare August 28, 2024 17:59

tdilauro reviewed Aug 28, 2024

View reviewed changes

dbernstein force-pushed the track-loan-in-playtime-entries branch 3 times, most recently from e82dec9 to 11f85db Compare September 3, 2024 16:31

dbernstein requested a review from tdilauro September 3, 2024 20:50

dbernstein force-pushed the track-loan-in-playtime-entries branch from 11f85db to aa5fe45 Compare September 4, 2024 16:09

dbernstein added 11 commits September 4, 2024 11:00

[PP-1456] Add loan count column to audio book playback time reports.

ec6adc8

Add migration to add loan_identifier columns.

a2939c3

Fix test.

db083a0

Fix other tests.

c0cd699

Fix migration.

f5950be

Fix migration.

3ffcd9f

Fix broken time_tracking model test.

d9cf751

Re-add the loan identifier to the unique index on playtime_summaries

e71ac61

Add test for loan_identifer resolver function.

211d9dd

Use Loan.end in query.

87031bd

dbernstein added 3 commits September 4, 2024 11:00

Ensure that the playtime_entries controller tests for an existing loan.

a5b37e6

Fix mypy and broken test.

22985c8

Fix alembic tests.

f56fcd5

dbernstein force-pushed the track-loan-in-playtime-entries branch from aa5fe45 to f56fcd5 Compare September 4, 2024 18:00

tdilauro approved these changes Sep 4, 2024

View reviewed changes

dbernstein merged commit 3f5d5ae into main Sep 4, 2024
20 checks passed

dbernstein deleted the track-loan-in-playtime-entries branch September 4, 2024 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PP-1456] Add loan count column to audio book playback time reports. #2002

[PP-1456] Add loan count column to audio book playback time reports. #2002

dbernstein commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

tdilauro left a comment

tdilauro Aug 22, 2024

tdilauro Aug 22, 2024

dbernstein Aug 23, 2024

tdilauro Aug 22, 2024

dbernstein Aug 23, 2024

tdilauro Aug 28, 2024

tdilauro left a comment •

edited

Loading

tdilauro Aug 28, 2024

dbernstein Aug 29, 2024

tdilauro Aug 28, 2024

dbernstein Aug 29, 2024

tdilauro Aug 28, 2024

dbernstein Aug 29, 2024

tdilauro Aug 28, 2024

tdilauro Aug 28, 2024

dbernstein Aug 29, 2024

tdilauro Aug 28, 2024

tdilauro Aug 28, 2024

dbernstein Aug 29, 2024

dbernstein commented Sep 3, 2024

tdilauro left a comment

		Loan.start >= min_time_entry,
		Loan.start + default_loan_period < max_time_entry,

[PP-1456] Add loan count column to audio book playback time reports. #2002

[PP-1456] Add loan count column to audio book playback time reports. #2002

Conversation

dbernstein commented Aug 21, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

tdilauro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdilauro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbernstein commented Sep 3, 2024

tdilauro left a comment

Choose a reason for hiding this comment

dbernstein commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

tdilauro left a comment •

edited

Loading