search

jwplayer / ott-web-app

Reference implementation for JWP-powered apps
Apache License 2.0
69 stars 53 forks source link

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

Open mirovladimitrovski opened 3 weeks ago

mirovladimitrovski commented 3 weeks ago

Description

This PR solves #PS-226.

Steps completed:

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

github-actions[bot] commented 3 weeks ago

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

ChristiaanScheermeijer commented 1 week ago

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 😄

mirovladimitrovski commented 1 week ago

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 😄

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.

ChristiaanScheermeijer commented 1 week ago

@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 commented 1 week ago

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