Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audiobook time tracking #1288

Merged
merged 27 commits into from
Aug 3, 2023
Merged

Audiobook time tracking #1288

merged 27 commits into from
Aug 3, 2023

Conversation

RishiDiwanTT
Copy link
Contributor

@RishiDiwanTT RishiDiwanTT commented Jul 21, 2023

Description

New tables IdentifierPlaytimeEntries and IdentifierPlaytimes have been added.
The time tracking API has been added as POST /playtimes/<type>/<identifier>/ .
A new environment variable SIMPLIFIED_REPORTING_EMAIL has been added for the temporary requirement of emailing reports via a cron job, this will require a deployment change.
Playtime aggregations run every 12 hours.
Playtime reporting occurs on the 2nd of every month.

Cannot add the api spec for now since HTTP 207 is not supported by flask_pydantic_spec. Bug ticket here.

Motivation and Context

We need to track the total amount of time an audiobook is playing on a user’s device. The apps should send this information to a remote server every 1 minute.
JIRA

How Has This Been Tested?

Manually run the APIs and summation jobs.
Emailed a local smtp server with the CSV report.

Checklist

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

@RishiDiwanTT RishiDiwanTT added DB migration This PR contains a DB migration feature New feature labels Jul 21, 2023
@RishiDiwanTT RishiDiwanTT requested a review from a team July 21, 2023 10:21
@RishiDiwanTT RishiDiwanTT self-assigned this Jul 21, 2023
@RishiDiwanTT RishiDiwanTT changed the title Feature/audiobook time tracking Audiobook time tracking Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Patch coverage: 99.07% and project coverage change: +0.03% 🎉

Comparison is base (d12325e) 89.82% compared to head (ba1fa75) 89.85%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1288      +/-   ##
==========================================
+ Coverage   89.82%   89.85%   +0.03%     
==========================================
  Files         208      210       +2     
  Lines       28549    28655     +106     
  Branches     6545     6556      +11     
==========================================
+ Hits        25644    25749     +105     
  Misses       1893     1893              
- Partials     1012     1013       +1     
Files Changed Coverage Δ
core/jobs/playtime_entries.py 98.03% <98.03%> (ø)
core/model/constants.py 100.00% <100.00%> (ø)
core/query/playtime_entries.py 100.00% <100.00%> (ø)
core/util/datetime_helpers.py 100.00% <100.00%> (ø)
core/util/email.py 83.33% <100.00%> (ø)

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

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a first pass through this one, but have NOT really looked at the tests yet. More detail below, but here are a few high-level thoughts:

  • We need to be able to report at the collection and library level for the CSV report, so we need that information in the database tables.

  • Since the apps will use the presence of a item entry-level link with the time tracking rel to decide whether to perform/report time tracking, we want link to be present for only those books needing tracking to have one. So we need some collection or library/collection-level config to indicate whether time tracking should be used.

  • So that we can more easily tune them over time, it would be useful to be able to set (or override) the following with script options:

    • How old a time entry timestamp should be before it is processed into the summary table.
    • How old a processed==True entry should be (i.e., how long we want to actively avoid duplicates) before it is removed from the entries table.

api/controller.py Show resolved Hide resolved
api/model/time_tracking.py Outdated Show resolved Hide resolved
api/model/time_tracking.py Show resolved Hide resolved
api/opds.py Outdated Show resolved Hide resolved
core/jobs/playtime_entries.py Outdated Show resolved Hide resolved
core/model/time_tracking.py Show resolved Hide resolved
core/jobs/playtime_entries.py Outdated Show resolved Hide resolved
core/jobs/playtime_entries.py Outdated Show resolved Hide resolved
core/model/time_tracking.py Outdated Show resolved Hide resolved
core/model/time_tracking.py Outdated Show resolved Hide resolved
@jonathangreen
Copy link
Member

Just throwing a note on here to remind us that since #1281 went in, and also had a DB migration, that we will need to fix the DB migration here before merging.

@RishiDiwanTT RishiDiwanTT force-pushed the feature/audiobook-time-tracking branch from 7fbb202 to edf3756 Compare July 28, 2023 07:05
core/model/time_tracking.py Outdated Show resolved Hide resolved
core/jobs/playtime_entries.py Outdated Show resolved Hide resolved
core/jobs/playtime_entries.py Show resolved Hide resolved
core/jobs/playtime_entries.py Outdated Show resolved Hide resolved
core/jobs/playtime_entries.py Outdated Show resolved Hide resolved
docker/services/cron/cron.d/circulation Show resolved Hide resolved
api/model/time_tracking.py Show resolved Hide resolved
api/opds.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I resolved some conversations and added a few more comments.

core/jobs/playtime_entries.py Outdated Show resolved Hide resolved
core/util/datetime_helpers.py Outdated Show resolved Hide resolved
core/query/playtime_entries.py Outdated Show resolved Hide resolved
core/query/playtime_entries.py Outdated Show resolved Hide resolved
tests/api/test_controller_playtime_entries.py Outdated Show resolved Hide resolved
tests/api/test_controller_playtime_entries.py Outdated Show resolved Hide resolved
Configuration.REPORTING_EMAIL_ENVIRONMENT_VARIABLE: "reporting@test.email"
},
),
# Horrible unbracketted syntax for python 3.8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So glad this is fixed in more recent Python versions! Absolutely hideous and confusing.

if (
today - entry.during_minute.date()
).days > cls.OLDEST_ACCEPTABLE_ENTRY_DAYS:
# This will count as a success, since we don't want to repeat the entry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reported as a failure, but one that means the client should discard the corresponding entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to count as a failure

api/opds.py Outdated Show resolved Hide resolved
api/routes.py Outdated Show resolved Hide resolved
@RishiDiwanTT
Copy link
Contributor Author

RishiDiwanTT commented Aug 3, 2023 via email

@tdilauro
Copy link
Contributor

tdilauro commented Aug 3, 2023

I did originally use collection id, but found that every other collection based route on the CM, which are a lot, uses collection name. To keep it consistent I also used the name. I'm not privy as to why that's the standard though.

I'd still suggest using the id here, since these URLs might be cached for a long time on the client apps and a name change might cause all of them to break. That said, the client apps should update these links in their registries whenever they fetch the loans feed, so maybe that's good enough.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is looking pretty good. I’m going to do some more testing once I wake up. And, of course, we’ll need to resolve the collection name vs. id issue.

One more fairly minor comment below.

api/routes.py Show resolved Hide resolved
logging.getLogger("TimeTracking").error(
f"An incorrect timezone was received for a playtime ({value.tzname()})."
)
raise ValueError("Timezone MUST be UTC always")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raising ValueError here causes the entire request to fail, even if only one timeEntry of many has an error. Ideally, we'd return this as a 400 response for just this entry.

I think it's unlikely for this to happen to one among many, so I think we can address this later.

Comment on lines 46 to 47
class PlaytimeEntriesPost(CustomBaseModel):
time_entries: List[PlaytimeTimeEntry] = Field(description="A List of time entries")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The book_id and library_id from the spec are missing here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit to fix this.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to go! 🎈🎉

It'll be good to start testing against this with the apps!

@tdilauro
Copy link
Contributor

tdilauro commented Aug 3, 2023

@RishiDiwanTT I'm going to go ahead and merge this, so that we can get it deployed out for testing. Thanks for all your work on this!

@tdilauro tdilauro merged commit b88847e into main Aug 3, 2023
@tdilauro tdilauro deleted the feature/audiobook-time-tracking branch August 3, 2023 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB migration This PR contains a DB migration feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants