Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

Plans & Intro-Offers: Stop serving introductory offers through plans endpoints. Refactor to make use of `wpcom/v2/introductory-offers` #93009

Open chriskmnds opened 3 months ago

chriskmnds commented 3 months ago

I think we got off the wrong starting point with introductory offers: how we serve through the plans' endpoints and compile on the frontend. Unless I am wrong, we've introduced overhead and duplication of state by opting to serve introductory offers through plans' endpoints. It can manifest itself into all sorts of issues if intro offers are accessible through multiple state/endpoint handlers serving potentially different prices each - explain below.

Since building the Woo $1 promo pricing (peapX7-2Dm-p2), we opted to serve the intro offers through /sites/:id/plans endpoint. We later also explored (not sure if the diff was shipped in the end) to serve these similarly through the /plans endpoint for signup flows during the WPCOM intro offers work (pau2Xa-5mX-p2).

The problem with this setup is we already have a dedicated endpoint for these through v2 API: /wpcom/v2/introductory-offers (which applies at the Product level). We briefly chatted about this in Slack and the main reasoning behind serving through plans was (1) not aware of the existence of /introductory-offers state/endpoint, and (2) enabling prorated pricing, which is likely a site&plan-related concern. On the second point, I do not think we prorate the intro offer prices, irrespective. The /introductory-offers endpoint also accepts a site parameter for tailoring the prices in the context of a given site (so proration can happen there - and assuming it can, then there's no need to duplicate the source?).

We intend to un-dupe and centralize plan pricing behind data stores (e.g. https://github.com/Automattic/wp-calypso/issues/86638). There is already complexity introduced from the above setup. A case I've spotted in the code is JP App plans, which query for introductory offers on both plans and other JP products - they do this through /introductory-offers.

Suggestion

My suggestion is to stick with one source - to remove from plans' endpoints altogether. As far as plans are concerned, they can still be compiled through Plans.usePricingMetaForGridPlans, which is mostly a convenience hook/selector for combining other eligibility conditions.

How

  1. Create an intro-offers slice in @automattic/data-stores with the types and a query hook useIntroductoryOffers
  2. Migrate existing uses to query the data-store
  3. Remove the intro-offers Calypso state altegether
  4. Update Plans.usePlans & Plans.useSitePlans to not compile these at all in the returned objects
  5. Query intro-offers in Plans.usePricingMetaForGridPlans and attach to the pricing objects
  6. Update /plans and /sites/:id/plans endpoints to stop sending introductory offer properties through
chriskmnds commented 3 months ago

cc @daledupreez @Automattic/martech @Automattic/shilling for awareness and in case I am missing something e.g. on the whole "proration" part. Would you agree with the Suggestion above?

dzver commented 3 months ago

I don't have an opinion on how the data is assembled on the frontend. However, here are my 2 cents about the endpoints.

The /plans endpoint returning the introductory offer represents the backend reality closer than the alternative. The /plans endpoint returns a price. That price comes from the associated billing plan. That billing plan can have introductory pricing. We can't charge a customer two different things, it's only one price, coming from one billing plan. Given that the /plans endpoint returns exactly one pricing scheme per WordPress.com plan, there's no point in having another endpoint to be assembled with the output from /plans.

chriskmnds commented 3 months ago

Hey @dzver. Thanks for the reply. I don't have a strong opinion - and I haven't actually inspected beyond the frontend to have a concrete understanding.

Given that the /plans endpoint returns exactly one pricing scheme per WordPress.com plan, there's no point in having another endpoint to be assembled with the output from /plans.

Just to make sure we are on the same page - the plan price returned from either /sites/:id/plans or /plans endpoint is different to the introductory offer. These are a separate set of props attached to the JSON object, these in particular:

    introductory_offer_formatted_price
    introductory_offer_raw_price
    introductory_offer_interval_unit
    introductory_offer_interval_count
    introductory_offer_end_date

The price of the billing plan is unaffected (unless changed intentionally due to the presence of an intro offer) - which may be relevant to what you are saying if I understand. but they are two separate things (prices).

So any consumer dealing with both "plans" and "products" can query and retrieve introductory offers for both from this endpoint (like JP App does). I feel that's where the problem starts to surface - if you start sending different intro offers for the same product through /introductory-offers and /plans endpoints.

The alternative to stop sending through "plans" endpoints, to my mind, would be to deprecate /introductory-offers and attach these similarly to /products and /sites/:id/products (to be consistent with plans). Otherwise, the situation with JP App won't seize to exist/threaten to surface.

Ok - these are quite rough thoughts. I may be wrong, and I haven't inspected enough.

jjchrisdiehl commented 2 months ago

The alternative to stop sending through "plans" endpoints, to my mind, would be to deprecate /introductory-offers and attach these similarly to /products and /sites/:id/products (to be consistent with plans). Otherwise, the situation with JP App won't seize to exist/threaten to surface.

I think this is a better way to go about it. Generally, calculating the introductory offer figures in one place on the backend and passing them through to the plans endpoint would be safest and would help us avoid displaying incorrect information.

So any consumer dealing with both "plans" and "products" can query and retrieve introductory offers for both from this endpoint (like JP App does)

Is there a specific reason to move this data into a separate introductory offer endpoint in the first place? Are there contexts where we wouldn't want to call the /plans endpoint from a security or performance standpoint?

chriskmnds commented 2 months ago

Hey @jjchrisdiehl !

Is there a specific reason to move this data into a separate introductory offer endpoint in the first place? Are there contexts where we wouldn't want to call the /plans endpoint from a security or performance standpoint?

I don't think there's a security concern. I think it's just more work on the client - ultimately 2 endpoints serving data that need to be coordinated (per entity - so 4 for both products & plans), vs backend serving them all through 1.

p.s. my initial intention was also to remove the introductory-offers state slice, but then found JP querying it and thought "hmm this is easy".

‐-------------

I also question now why "plans" endpoints are serving pricing when it's all products at the end of it. Is there any data that we serve differently through "plans" endpoint(s)? Or is my reading/understanding of these concepts incorrect?

We talk about "a single pricing scheme" above, but I think that's also inaccurate. The current plan, for instance, gets its price through "purchases" endpoint. So the only "real single pricing scheme" is technically usePricingMetaForGridPlans.