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

Update Enki non-DRM to use RedirectFulfillment #2036

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Sep 6, 2024

Description

In #2035 I noticed that we appear to be streaming full Enki ebooks though the CM, which isn't ideal. This updates to use a redirect fulfillment.

🚨 From what I see this should work, but I need to find a way to do a real world test on this. Going to leave this in draft until I figure that bit out.

Edit: I did some testing with this, it does look like we are currently proxying these titles though the CM. Its not a huge number of titles, but we are better off using a redirect for them. All the local testing I can do says this should work, but can't fully test until its deployed somewhere and we can pull in a title with the mobile app.

I think this one can get merged, and we'll do some testing on minotaur then roll back if we need to.

I was testing with the title: "All I Really Need to Know I Learned from KISS" by Chris Epting.

Motivation and Context

Looks like this code came in way back as part of this PR: NYPL-Simplified/circulation#717. Looks like it was a band-aid fix that lasted ~7 years, so pretty good bandaid actually 😆.

How Has This Been Tested?

  • Added unit test
  • Tested that previously the CM acted as a proxy
  • Tested that after this change, the CM serves a redirect link
    • Tested that I can access that redirect link
    • This should be fine for the apps, this is how other content works

Checklist

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

@jonathangreen jonathangreen added the bug Something isn't working label Sep 6, 2024
@jonathangreen jonathangreen requested a review from a team September 6, 2024 16:22
@jonathangreen jonathangreen force-pushed the chore/refactor-fulfillment branch from 1fdb345 to d166625 Compare September 6, 2024 16:23
@jonathangreen jonathangreen force-pushed the bugfix/open-access-enki-streaming branch from 53f36d4 to f79db96 Compare September 6, 2024 16:25
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.76%. Comparing base (69f6e3e) to head (001a0ea).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2036   +/-   ##
=======================================
  Coverage   90.76%   90.76%           
=======================================
  Files         342      342           
  Lines       40512    40511    -1     
  Branches     8770     8770           
=======================================
+ Hits        36769    36770    +1     
+ Misses       2486     2485    -1     
+ Partials     1257     1256    -1     

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

Base automatically changed from chore/refactor-fulfillment to main September 6, 2024 17:50
@jonathangreen jonathangreen force-pushed the bugfix/open-access-enki-streaming branch from f79db96 to 0abfbc4 Compare September 6, 2024 17:55
@jonathangreen jonathangreen marked this pull request as ready for review September 6, 2024 18:45
@jonathangreen jonathangreen force-pushed the bugfix/open-access-enki-streaming branch from 0abfbc4 to 001a0ea Compare September 6, 2024 18:45
@jonathangreen jonathangreen changed the title Update Enki non-drm to use RedirectFulfillment Update Enki non-DRM to use RedirectFulfillment Sep 6, 2024
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.

Looks good! 🚀

@jonathangreen jonathangreen merged commit b136123 into main Sep 10, 2024
20 checks passed
@jonathangreen jonathangreen deleted the bugfix/open-access-enki-streaming branch September 10, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants