-
Notifications
You must be signed in to change notification settings - Fork 55
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
Refactor / move entitlement logic to entitlement controller #657
Refactor / move entitlement logic to entitlement controller #657
Conversation
Visit the preview URL for this PR (updated for commit 3784aa3): https://ottwebapp--pr657-refactor-entitlement-xiy81jvu.web.app (expires Sat, 04 Jan 2025 13:27:37 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c198f8a3a199ba8747819f7f1e45cf602b777529 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice step forward by moving most logic from the useContentProtection
and useProtected
hooks to a EntitlementController class, which also potentially can be overridden.
I have no feedback on the code. Looks good!
I was looking at the failing e2e tests and found a typings issue in the module system. For integrations, we use the This currently isn't causing any bugs because we basically never call these methods when no integration is configured, but this is not good practice and did cause an error in this PR which could have been prevented with the correct typings. In my last commit I have made most injected integration services optional which, after doing so, resulted in many TS errors. But these could all be solved by adding the |
859d5f8
to
8150cf4
Compare
8150cf4
to
f026ed2
Compare
Description
This PR is a refactor of the entitlement implementation. This is a bit spontaneous as we didn't plan for this, but a few reasons for this refactor are:
useProtectedMedia
anduseContentProtection
hooks contained too much logiccontentSigningService
config property is not usable with JWP app configsIn short, I have made the following changes:
EntitlementController
class to house most of the entitlement logic from the hooksconfig.custom
paramsAfter rebasing with develop I noticed that these changes conflict with #654. So I fixed that bug in this PR as well, so this PR now supersedes #654 now.