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

Handle multiple plans from the AppConfig in the Web App #550

Open
wants to merge 49 commits into
base: develop
Choose a base branch
from

Conversation

mirovladimitrovski
Copy link
Collaborator

@mirovladimitrovski mirovladimitrovski commented Jun 12, 2024

Description

This PR solves #PS-226.

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

@mirovladimitrovski mirovladimitrovski marked this pull request as draft June 12, 2024 08:36
Copy link

github-actions bot commented Jun 12, 2024

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

@mirovladimitrovski mirovladimitrovski marked this pull request as ready for review June 19, 2024 18:44
@ChristiaanScheermeijer
Copy link
Collaborator

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
Loading

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
Loading

I don't think it needs a lot more work to improve this. Let me know what you think 😄

@mirovladimitrovski
Copy link
Collaborator Author

mirovladimitrovski commented Jun 24, 2024

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
Loading

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
Loading

I don't think it needs a lot more work to improve this. Let me know what you think 😄

With the introduction of support for multiple plans which this PR is implementing, we have a revised layout for plans, but we're keeping the current layout for offers (meaning we don't want to change the UI for Cleeng users). This meant we had to look at plans and offers as two separate concepts, as a cleaner alternative to blending them together in a single common layout (such as the ChooseOfferForm component). But in order to avoid the trap of accessing JW or Cleeng services directly, we're introducing a generic variable called accessMethod which can either be plan or offer, although they implicitly mean JWP and Cleeng respectively at the moment. I agree I've made an exception to the rule in my new hook that I'm introducing - usePlansForMedia, which I'm going to fix right away. But I think that's as far as we can go in terms of being generic, because the said distinction between plans and offers is generic enough and from that point on plans and offers are inevitably going to have their own exclusive hooks and, as it turns out, their own exclusive components as well. Anyway, I'm open to suggestions.

packages/common/src/modules/register.ts Outdated Show resolved Hide resolved
packages/hooks-react/src/usePlansForMedia.tsx Outdated Show resolved Hide resolved
packages/ui-react/src/components/Button/Button.tsx Outdated Show resolved Hide resolved
packages/hooks-react/src/usePlansForMedia.tsx Outdated Show resolved Hide resolved
Comment on lines +43 to +45
const hasSubscriptionPlansOrOffers = !!subscriptionOffers.length;

const hasOffers = accessModel === ACCESS_MODEL.SVOD || (accessModel === ACCESS_MODEL.AUTHVOD && hasMediaOffers) || hasSubscriptionPlansOrOffers;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@ChristiaanScheermeijer
Copy link
Collaborator

@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?

@mirovladimitrovski
Copy link
Collaborator Author

mirovladimitrovski commented Jun 25, 2024

@mirovladimitrovski what is your timeline for this feature?

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.

Do you have a config ID that we can use to test the new plans feature? 😄

I'm mostly using aecedypm (with access model free) and wxpmaqis (with access model svod), but the environment in this PR is temporarily switched to dev, so you can conveniently test with any config from https://dev-dashboard.jwplayer.com/. The appropriate changes are already done in the dashboard, so you can create apps and play with plans and prices you want to test as you see fit. Let me know if I can help with anything.

PS. Don't take my feedback as resistance to this change,

I don't, no worries. :) You're giving a valuable feedback and I appreciate it.

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.

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 offers integration is supposed to remain intact.

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?

In theory, yes.

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.

3 participants