-
Notifications
You must be signed in to change notification settings - Fork 54
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
Handle multiple plans from the AppConfig in the Web App #550
base: develop
Are you sure you want to change the base?
Conversation
* fix: remove profiles * fix: remove code * fix: remove profile user menu code * fix: remove user menu profle code * fix: remove route paths for profiles
Visit the preview URL for this PR (updated for commit ebe78be): https://ottwebapp--pr550-ps-226-multiple-plan-nq8wlvhp.web.app (expires Thu, 25 Jul 2024 10:55:43 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c198f8a3a199ba8747819f7f1e45cf602b777529 |
packages/ui-react/src/containers/AccountModal/forms/Checkout.tsx
Outdated
Show resolved
Hide resolved
packages/ui-react/src/containers/AccountModal/forms/Checkout.tsx
Outdated
Show resolved
Hide resolved
I'm a bit hesitant of some parts of this PR because it doesn't leverage the existing IoC pattern but adds another layer of complexity on top. I think our goal should be to make the services/integrations more simple and flexible by generalising and abstracting implementation logic out of the hooks and views. By skipping this now, it will make it even more difficult to simplify the integrations later. Simplified flow of the current situation graph TD
subgraph ChooseOffers
A[View]
B[useOffers Hook]
C[CheckoutController]
D[IoC Container]
E[Cleeng Service]
F[JWP Service]
end
A --> B
B --> C
C --> D
D --> E
D --> F
C == Offers ==> B
B == Offers ==> A
style E fill:#ff9,stroke:#333,stroke-width:4px,color:black
style F fill:#9ff,stroke:#333,stroke-width:4px,color:black
After the plans addition graph TD
subgraph ChooseOffers
A[View]
AA[Choose Offer Component]
AB[List Plans Component]
B[useOffers Hook]
BB[usePlan Hook]
C[CheckoutController]
D[IoC Container]
E[Cleeng Service]
F[JWP Service]
end
A --> AA
A --> AB
AB --> BB
BB --> F
AA --> B
B --> C
C --> D
D --> E
D --> F
C == Offers ==> B
B == Offers ==> A
style E fill:#ff9,stroke:#333,stroke-width:4px,color:black
style F fill:#9ff,stroke:#333,stroke-width:4px,color:black
I don't think it needs a lot more work to improve this. Let me know what you think 😄 |
packages/ui-react/src/containers/AccountModal/forms/CancelSubscription.tsx
Show resolved
Hide resolved
With the introduction of support for multiple plans which this PR is implementing, we have a revised layout for |
const hasSubscriptionPlansOrOffers = !!subscriptionOffers.length; | ||
|
||
const hasOffers = accessModel === ACCESS_MODEL.SVOD || (accessModel === ACCESS_MODEL.AUTHVOD && hasMediaOffers) || hasSubscriptionPlansOrOffers; |
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.
Are you sure this change is needed? When the access model is SVOD, we can already assume that there are subscription offers that will be fetched in the next step. Or are we not configuring global SVOD plans in the new setup?
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.
There might be no subscription offers yet, in which case it makes sense to skip the choose-offer
modal and go straight to welcome
.
@mirovladimitrovski what is your timeline for this feature? Do you have a config ID that we can use to test the new plans feature? 😄 PS. Don't take my feedback as resistance to this change, I mainly worry about features being removed or breaking changes which has a direct impact for users of this repo (including us). I'm hoping that we can do these changes in a "next" or "v7" branch because we're basically removing the existing integration and forcing users to migrate. When working in a different branch it will also become easier to refactor services and integrations to shape them around the plans model. In theory both Cleeng and InPlayer can be used with plans, right? |
As soon as possible. This feature got to be a lot more complex on the OTT app and took some more dev time than expected, so we're a even a little behind schedule.
I'm mostly using
I don't, no worries. :) You're giving a valuable feedback and I appreciate it.
We're not removing any existing integration at all. If you're referring to the previous implementation of JWP plans, we're not removing that either. :) We're only expanding it from supporting a single plan to supporting multiple plans per app. In any case, the
In theory, yes. |
Description
This PR solves #PS-226.
Steps completed:
According to our definition of done, I have completed the following steps: