Focused Launch: Fix billing period tracking in summary #49930

alshakero · 2021-02-10T10:55:05Z

Changes proposed in this Pull Request

I added getLastPlanBillingPeriod selector to the plan store to get the billing period of the current or last selected paid plan's period.
I added a private (i.e unexported) action creator __setBillingPeriod to the launch store. And I changed the setPlanProductId action creator into a generator that queries the plans store for the plans billing period using its productId, then it dispatches the billing period using the newly-added __setBillingPeriod and the reducer records it. The makes the plan readonly and can only be changed by changing the plan.
I updated the usePlans hook to return the plan products, too.
I refactored summary view logic around plans and plan products.

Testing instructions

Create or reuse a site that is created with /start flow.
Sandbox this site.
In apps/editing-toolkit run yarn dev --sync.
Go to https://[yoursite].wordpress.com/wp-admin/post-new.php
Click launch.
Go to detailed plan page, pick a monthly plan.
In summary, now pick a free plan, the prices should remain monthly.
Pick a non-default paid plan, it should be rendered in summary, re-pick the default paid plan (Premium), the non-default plan should remain listed.

Fixes #49713

matticbot · 2021-02-10T10:55:07Z

Test live: https://calypso.live/?branch=fix/keep-track-of-paid-plan

matticbot · 2021-02-10T11:02:25Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~2 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding        +34 B  (+0.0%)       +2 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

ciampo

The launch store had a selector that returns the current paid plan. While it is meant to return the current or once-picked paid plan. I fixed that selector.

I wasn't really aware of how that selector was meant to work until I read your explanation.

As a user of the Launch data-store, when seeing a selector called getPaidPlanProductId(), I would expect the result of that selector to be undefined if there isn't a selected paid plan.

The fact that the result is assigned to a variable named selectedPaidPlanProductId would make me even more confused. If the user selects the free plan, why would a variable called selectedPaidPlanProductId still hold the value of a paid plan that is not currently selected?

To bring clarity, I think we should rename a few variables (sorry for the added verbosity, but it would make the code much easier to understand):

the reducer could be called lastSelectedPaidPlanProductId
the selector could be called getLastSelectedPaidPlanProductId()
this variable could be called lastSelectedPaidPlanProductId
this variable could be called lastSelectedPaidPlan

Finally, I believe that we should refactor this logic too, because now selectedPaidPlan can be defined even when the user hasn't currently selected a paid plan (we could use selectedPlan instead, with an additional check to see it's a free plan or a paid plan?)

alshakero · 2021-02-11T21:14:33Z

@ciampo I agree it was pretty confusing. I removed the reducer, the selector, and moved the logic to the focused launch.

StefanNieuwenhuis

The behavior looks good to me (see https://www.loom.com/share/f637c03af41d4b14abf78f1375afcfa6) and can you address my comment, please?

StefanNieuwenhuis · 2021-02-12T10:18:03Z

packages/launch/src/hooks/use-plans.tsx

+		const defaultFreePlan = select( PLANS_STORE ).getDefaultFreePlan( locale );
+		const defaultPaidPlan = select( PLANS_STORE ).getDefaultPaidPlan( locale );
+		const defaultPaidPlanProduct = select( PLANS_STORE ).getPlanProduct(
+			defaultPaidPlan?.periodAgnosticSlug,
+			billingPeriod
+		);
+		const defaultFreePlanProduct = select( PLANS_STORE ).getPlanProduct(
+			defaultFreePlan?.periodAgnosticSlug,
+			billingPeriod
+		);


I think this is a bit cleaner and easier to maintain. What do you think?

Suggested change

const defaultFreePlan = select( PLANS_STORE ).getDefaultFreePlan( locale );

const defaultPaidPlan = select( PLANS_STORE ).getDefaultPaidPlan( locale );

const defaultPaidPlanProduct = select( PLANS_STORE ).getPlanProduct(

defaultPaidPlan?.periodAgnosticSlug,

billingPeriod

);

const defaultFreePlanProduct = select( PLANS_STORE ).getPlanProduct(

defaultFreePlan?.periodAgnosticSlug,

billingPeriod

);

const plansStore = select(PLANS_STORE);

const defaultFreePlan = plansStore.getDefaultFreePlan( locale );

const defaultPaidPlan = plansStore.getDefaultPaidPlan( locale );

const defaultPaidPlanProduct = plansStore.getPlanProduct(

defaultPaidPlan?.periodAgnosticSlug,

billingPeriod

);

const defaultFreePlanProduct = plansStore.getPlanProduct(

defaultFreePlan?.periodAgnosticSlug,

billingPeriod

);

Also, don't forget to add the second argument to useSelect function which in this case should be [locale, billingPeriod]. See p7H4VZ-2Q6-p2#comment-9214

razvanpapadopol · 2021-02-12T20:39:35Z

packages/launch/src/focused-launch/summary/index.tsx

@@ -32,13 +32,12 @@ import FocusedLaunchSummaryItem, {
 	LeadingContentSide,
 	TrailingContentSide,
 } from './focused-launch-summary-item';
-import { LAUNCH_STORE, SITE_STORE, Plan } from '../../stores';
+import { LAUNCH_STORE, SITE_STORE, Plan, PlanProduct, PLANS_STORE } from '../../stores';


I think it's also fine to add on another line: import type { Plan, PlanProduct } from '../../stores';

razvanpapadopol · 2021-02-12T20:49:48Z

packages/data-stores/src/launch/selectors.ts

-
-	return productId && ! isFree ? state.planProductId : undefined;
-};
-


I agree we don't need this selector as it is but if you look at changes from #48452 we are using twice this selector but just to check if the selected plan is a paid plan so isSelectedPlanPaid might be a good candidate for a selector on Launch store.

alshakero · 2021-02-15T11:55:19Z

Thanks everyone! I addressed all feedback.

StefanNieuwenhuis

LGTM! 🚢

ciampo

I left a few comments, although the changes seem to work well!

I also wanted to raise one more edge case that I came across while testing:

Open focused launch modal
In summary view, select the free subdomain
Go to Plan details view, and pick a monthly billed plan
Go back to Summary view — everything looks as expected
Select the free plan in the Summary view — the Premium plan stays unchanged (monthly billed), even if it's not selected anymore
Go to Plan details and click the "Back" button, back to Summary view
The Free plan is selected, but now the plan above it is the annually billed premium plan (and not the monthly)

focused-launch-summar-plans-billing-preiod.mp4

To me this is behaviour is acceptable (plus, it's very rare edge case), but I just wanted to document it in my review notes.

packages/data-stores/src/launch/selectors.ts

packages/launch/src/focused-launch/summary/index.tsx

ciampo · 2021-02-15T16:34:11Z

packages/launch/src/focused-launch/summary/index.tsx

+	const allAvailablePlansProducts: ( PlanProduct | undefined )[] = nonDefaultPaidPlanProduct
+		? [ defaultPaidPlanProduct, nonDefaultPaidPlanProduct, defaultFreePlanProduct ]
+		: [ defaultPaidPlanProduct, defaultFreePlanProduct ];


With these changes, the allAvaiablePlans and allAvailablePlanProducts arrays can technically go "out of sync".

We should think about a way of making sure that this doesn't happen — and so I though that one potential solution is to add one more check to https://github.com/Automattic/wp-calypso/blob/77f73e53b72bd1d5e074a0c61f1d8b7953807e21/packages/launch/src/focused-launch/summary/index.tsx#L422-L423

and change it to:

typeof plan === 'undefined' || typeof allAvailablePlansProducts?.[ index ] === 'undefined' || plan.periodAgnosticSlug !== allAvailablePlansProducts?.[ index ]?.periodAgnosticSlug

Basically, the third check makes sure that the plan and the planProduct are in sync and refer to the same plan.

What do you think?

I think they will never be out of sync. Two factors determine their order:

These two synchronous lines. Because they control the ternary.

These lines that use the same resolver that won't resolve until all is fetched.

Nonetheless, I cleaned up that code a bit.

ciampo · 2021-02-16T14:03:45Z

Adding the "Hold" label because, as discussed in p1613476301450800/1613380064.426100-slack-C0KDTA48Y, we should wait for #48452 to be merged first, rebase, and then act on the points suggested by this and this comment

alshakero · 2021-02-17T20:33:12Z

packages/data-stores/src/launch/actions.ts

+type SetPlanActionParams = () =>
+	| {
+			readonly type: 'SET_PLAN_BILLING_PERIOD';
+			readonly billingPeriod: Plans.PlanBillingPeriod;
+	  }
+	| {
+			readonly type: 'SET_PLAN_PRODUCT_ID';
+			readonly planProductId: number | undefined;
+	  };
+


@sirreal Hi Jon! I couldn't figure out a way to type a generator action creator. Do you have a more elegant solution? This one isn't quite maintainable.

How about swapping ReturnType below for a new mapped type helper:

type ReturnOrGeneratorYieldUnion<T extends (...args: any) => any> = T extends ( ...args: any ) => infer Return ? (Return extends Generator<infer T, infer U, any> ? T | U : Return) : never; export type LuanchAction = ReturnOrGeneratorYieldUnion< | typeof setPlanProductId | typeof setSiteTitle /* … */ >

I think you'll get the result you're looking for. Here's a playground link.

Thank you so much! We could add this type to wp.data pacakge. Since it will be needed whenever someone needs a generator action.

I'm undecided between that above and this simpler solution:

export type LaunchAction = | ReturnType< | typeof setSiteTitle . . | typeof hideModalTitle > | ReturnType< ReturnType< typeof setPlanProductId >[ 'next' ] >[ 'value' ]; // <--- this

That's simpler here for a one-off case. I'd like to see a library-level solution for this that we could life to Gutenberg when it's ready like you suggest.

sirreal · 2021-02-18T11:49:27Z

packages/data-stores/src/launch/actions.ts

+/**
+ * Usually we use ReturnType of all the action creators to deduce all the actions.
+ * This works until one of the action creators is a generator and doesn't actually "Return" an action.
+ * This type helper allows for actions to be both functions and generators
+ */
+type ReturnOrGeneratorYieldUnion< T extends ( ...args: any ) => any > = T extends (
+	...args: any
+) => infer Return
+	? Return extends Generator< infer T, infer U, any >
+		? T | U
+		: Return
+	: never;


If this better describes action creators, we should move it with other helpers and use it in other store action creators as well.

wp-calypso/packages/data-stores/src/mapped-types.ts

Lines 31 to 61 in 61acc95

/**

* Maps a "raw" actionCreators object to the actions available when registered on the @wordpress/data store.

*

* @template A Selector map, usually from `import * as actions from './my-store/actions';`

*/

export type DispatchFromMap< A extends Record< string, ( ...args: any[] ) => any > > = {

[ actionCreator in keyof A ]: (

...args: Parameters< A[ actionCreator ] >

) => A[ actionCreator ] extends ( ...args: any[] ) => Generator

? Promise< GeneratorReturnType< A[ actionCreator ] > >

: void;

};

/**

* Parameters type of a function, excluding the first parameter.

*

* This is useful for typing some @wordpres/data functions that make a leading

* `state` argument implicit.

*/

export type TailParameters< F extends Function > = F extends ( head: any, ...tail: infer T ) => any

? T

: never;

/**

* Obtain the type finally returned by the generator when it's done iterating.

*/

export type GeneratorReturnType< T extends ( ...args: any[] ) => Generator > = T extends (

...args: any

) => Generator< any, infer R, any >

? R

: never;

This is the type of thing wp-data should handle in its type system and I think a lot of the utilities we've developed here can move there when the library is typed. This is the kind of thing the system should take care of if we get it right and we shouldn't have to deal with them to build a soundly typed store 🙂

ciampo · 2021-02-18T12:38:29Z

packages/data-stores/src/launch/actions.ts

@@ -52,11 +55,27 @@ export const setDomainSearch = ( domainSearch: string ) =>
 		domainSearch,
 	} as const );

-export const setPlanProductId = ( planProductId: number | undefined ) =>
+const __setBillingPeriod = ( billingPeriod: Plans.PlanBillingPeriod ) =>


No need of prefixing with __, as this function is not exported anyway (plus, we may actually need it when working on #49253 and/or #50186)

ciampo · 2021-02-18T12:44:52Z

packages/data-stores/src/launch/actions.ts

+		billingPeriod,
+	} as const );
+
+export const setPlanProductId = function* setPlanProductId( planProductId: number | undefined ) {


No need to repeat setPlanProductId twice. It can be rewritten to

Suggested change

export const setPlanProductId = function* setPlanProductId( planProductId: number | undefined ) {

export function* setPlanProductId( planProductId: number | undefined ) {

or, to keep consistency with the rest of the exported functions, to

Suggested change

export const setPlanProductId = function* setPlanProductId( planProductId: number | undefined ) {

export const setPlanProductId = function* ( planProductId: number | undefined ) {

ciampo · 2021-02-18T12:53:21Z

packages/data-stores/src/plans/index.ts

@@ -38,6 +38,7 @@ export {
 	TIMELESS_PLAN_PREMIUM,
 	TIMELESS_PLAN_BUSINESS,
 	TIMELESS_PLAN_ECOMMERCE,
+	FREE_PLAN_PRODUCT_ID,


~~Looks like this export does not exist in the file~~

Never mind this. Looks like my TypeScript server had a hiccup — all good :)

ciampo

LGTM 🚢

(mobile e2e tests permitting)

alshakero added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Focused Launch Issues and PRs related to Focused Launch labels Feb 10, 2021

alshakero self-assigned this Feb 10, 2021

razvanpapadopol self-requested a review February 11, 2021 09:49

ciampo self-requested a review February 11, 2021 10:37

ciampo requested changes Feb 11, 2021

View reviewed changes

alshakero changed the title ~~Launch store: fix launch store paid plan selector~~ Focused Launch: Fix billing period tracking in summary Feb 11, 2021

StefanNieuwenhuis reviewed Feb 12, 2021

View reviewed changes

razvanpapadopol reviewed Feb 12, 2021

View reviewed changes

ciampo self-requested a review February 15, 2021 16:01

StefanNieuwenhuis self-requested a review February 15, 2021 16:02

StefanNieuwenhuis approved these changes Feb 15, 2021

View reviewed changes

ciampo mentioned this pull request Feb 15, 2021

Focused Launch: Only launch site if checkout is complete #48452

Merged

11 tasks

ciampo reviewed Feb 15, 2021

View reviewed changes

ciampo added [Status] Blocked / Hold [Status] Needs Rebase labels Feb 16, 2021

razvanpapadopol mentioned this pull request Feb 17, 2021

Focused Launch: show billing period in Summary view #50186

Open

alshakero added 6 commits February 17, 2021 21:00

Fix launch store paid plan selector

5caa4c0

Create free plan product ID constant

fd76223

Remove getPaidPlanProductId selector and refactor summary view

92c7613

Track billing period seperately

f53fc81

Commit stores.ts

b6d54a1

Address feedback

970d6d6

alshakero added 3 commits February 17, 2021 21:02

Clean up isPlanProductFree

c85398d

Lift the billing period to the launch store

ebb6ca9

Adapt to the new store API

bc18130

alshakero force-pushed the fix/keep-track-of-paid-plan branch from 969db9c to bc18130 Compare February 17, 2021 20:04

alshakero added 2 commits February 17, 2021 21:21

Address feedback

d37dbbc

Persist planBillingPeriod to ensure consistency

a49f1b8

alshakero commented Feb 17, 2021

View reviewed changes

alshakero added 3 commits February 17, 2021 21:50

Check-in success file

16c5a8d

Fix isSelectedPlanPaid

5c4360c

Improve setPlanProductId type

d42b217

sirreal reviewed Feb 18, 2021

View reviewed changes

ciampo reviewed Feb 18, 2021

View reviewed changes

Rename __setBillingPeriod to __internalSetBillingPeriod

9624b35

ciampo self-requested a review February 18, 2021 14:08

ciampo approved these changes Feb 18, 2021

View reviewed changes

alshakero merged commit 8e78abe into trunk Feb 18, 2021

alshakero deleted the fix/keep-track-of-paid-plan branch February 18, 2021 14:20

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 18, 2021

ciampo mentioned this pull request Feb 18, 2021

Editing Toolkit: Update to v2.20 #50204

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Focused Launch: Fix billing period tracking in summary #49930

Focused Launch: Fix billing period tracking in summary #49930

alshakero commented Feb 10, 2021 •

edited

Loading

matticbot commented Feb 10, 2021

matticbot commented Feb 10, 2021 •

edited

Loading

ciampo left a comment •

edited

Loading

alshakero commented Feb 11, 2021

StefanNieuwenhuis left a comment

StefanNieuwenhuis Feb 12, 2021

razvanpapadopol Feb 12, 2021

razvanpapadopol Feb 12, 2021

razvanpapadopol Feb 12, 2021

alshakero commented Feb 15, 2021

StefanNieuwenhuis left a comment

ciampo left a comment •

edited

Loading

ciampo Feb 15, 2021 •

edited

Loading

alshakero Feb 17, 2021 •

edited

Loading

ciampo commented Feb 16, 2021 •

edited

Loading

alshakero Feb 17, 2021

sirreal Feb 17, 2021 •

edited

Loading

alshakero Feb 18, 2021 •

edited

Loading

alshakero Feb 18, 2021

sirreal Feb 18, 2021

sirreal Feb 18, 2021

ciampo Feb 18, 2021 •

edited

Loading

ciampo Feb 18, 2021

ciampo Feb 18, 2021 •

edited

Loading

ciampo left a comment


		return productId && ! isFree ? state.planProductId : undefined;
		};

	/**
	* Maps a "raw" actionCreators object to the actions available when registered on the @wordpress/data store.
	*
	* @template A Selector map, usually from `import * as actions from './my-store/actions';`
	*/
	export type DispatchFromMap< A extends Record< string, ( ...args: any[] ) => any > > = {
	[ actionCreator in keyof A ]: (
	...args: Parameters< A[ actionCreator ] >
	) => A[ actionCreator ] extends ( ...args: any[] ) => Generator
	? Promise< GeneratorReturnType< A[ actionCreator ] > >
	: void;
	};

	/**
	* Parameters type of a function, excluding the first parameter.
	*
	* This is useful for typing some @wordpres/data functions that make a leading
	* `state` argument implicit.
	*/
	export type TailParameters< F extends Function > = F extends ( head: any, ...tail: infer T ) => any
	? T
	: never;

	/**
	* Obtain the type finally returned by the generator when it's done iterating.
	*/
	export type GeneratorReturnType< T extends ( ...args: any[] ) => Generator > = T extends (
	...args: any
	) => Generator< any, infer R, any >
	? R
	: never;

	export const setPlanProductId = function* setPlanProductId( planProductId: number \| undefined ) {
	export function* setPlanProductId( planProductId: number \| undefined ) {

Focused Launch: Fix billing period tracking in summary #49930

Focused Launch: Fix billing period tracking in summary #49930

Conversation

alshakero commented Feb 10, 2021 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Feb 10, 2021

matticbot commented Feb 10, 2021 • edited Loading

ciampo left a comment • edited Loading

Choose a reason for hiding this comment

alshakero commented Feb 11, 2021

StefanNieuwenhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero commented Feb 15, 2021

StefanNieuwenhuis left a comment

Choose a reason for hiding this comment

ciampo left a comment • edited Loading

Choose a reason for hiding this comment

ciampo Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

alshakero Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

ciampo commented Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

sirreal Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

alshakero Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

alshakero commented Feb 10, 2021 •

edited

Loading

matticbot commented Feb 10, 2021 •

edited

Loading

ciampo left a comment •

edited

Loading

ciampo left a comment •

edited

Loading

ciampo Feb 15, 2021 •

edited

Loading

alshakero Feb 17, 2021 •

edited

Loading

ciampo commented Feb 16, 2021 •

edited

Loading

sirreal Feb 17, 2021 •

edited

Loading

alshakero Feb 18, 2021 •

edited

Loading

ciampo Feb 18, 2021 •

edited

Loading

ciampo Feb 18, 2021 •

edited

Loading