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

Refactor / move entitlement logic to entitlement controller #657

Merged
merged 8 commits into from
Dec 5, 2024

Conversation

ChristiaanScheermeijer
Copy link
Collaborator

@ChristiaanScheermeijer ChristiaanScheermeijer commented Dec 3, 2024

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:

  • We updated the generic entitlement service backend to be more simple since the JWP delivery API changes
  • The entitlement logic was spread over too many components
  • The useProtectedMedia and useContentProtection hooks contained too much logic
  • The contentSigningService config property is not usable with JWP app configs

In short, I have made the following changes:

  • Add a new EntitlementController class to house most of the entitlement logic from the hooks
  • Move the entitlement implementations to a subfolder and extend it from an abstract Entitlement class
  • Updated the GenericEntitlement API request and activation condition to config.custom params
  • Added unit tests for the EntitlementController logic

After 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.

Copy link

github-actions bot commented Dec 3, 2024

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

Copy link
Collaborator

@langemike langemike left a 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!

@ChristiaanScheermeijer
Copy link
Collaborator Author

I was looking at the failing e2e tests and found a typings issue in the module system.

For integrations, we use the getNamedModule function to dynamically inject the requested integration service. However, when no integration is configured, this call results in getNamedModule(AccountService, null);, but the return type remains concrete. This means we wrongfully assumed that the integration services were always set.

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 assertModuleMethod util like we already did for some occasions.

@ChristiaanScheermeijer ChristiaanScheermeijer force-pushed the refactor/entitlement-controller branch from 8150cf4 to f026ed2 Compare December 4, 2024 15:13
@ChristiaanScheermeijer ChristiaanScheermeijer merged commit 7ee979e into develop Dec 5, 2024
10 checks passed
@ChristiaanScheermeijer ChristiaanScheermeijer deleted the refactor/entitlement-controller branch December 5, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants