Add/plans meta data #23314

adamziel · 2018-03-14T20:50:31Z

Related to 2-year plans in general (p9jf6J-eR-p2).

This is a new version of #23258 which had to be reverted.

We are adding a set of plans to make 2-year subscriptions possible. Current codebase assumes that there is just one variant of each plan type (except for jetpack annual/monthly subscriptions). I can see at least the following problems off the top of my head:

Comparisons like planSlug === PLAN_BUSINESS would have to check also for PLAN_BUSINESS_2_YEAR
Code fragments like <Banner plan={PLAN_BUSINESS} /> may need to receive PLAN_BUSINESS_2_YEAR if user is subscribed to a 2-year plan

This PR proposes a solution to above and similar issues. I went through all places where plan constants were used directly and sketched this code and it seems to cover all use cases I saw, namely:

Get all business plans for both Jetpack and wp.com
Get all premium plans of Jetpack
Get personal plans of wp.com
Get business plan for the same site (Jetpack/wp.com) as current user but for 2-year term
Get premium plan for the same site (Jetpack/wp.com) as current user for the same term as current user
Get the same type of plan as current user have, but with a 2-year term
...and more similar

Instead of using/comparing plans constants directly, we introduce some metadata to PLANS_LIST and also a few helper functions to encapsulate that logic.

We can get rid of some checks like planSlug === PLAN_BUSINESS in favour of checking features, like here: #23076 - however this is not possible in all (most) cases.

With this PR, you can check like

planMatches(planSlug,  {type: TYPE_BUSINESS}); // or shortcut: isBusinessPlan( planSlug );

If you need to check just for jetpack or wp.com specific plans, you can do

planMatches(planSlug, {type: TYPE_BUSINESS, group: GROUP_WPCOM})

Or, let's say you need a business plan with the same subscription term as active user and related to the same site (jetpack/wp) as current user's plan:

findSimilarPlan(currentPlanSlug, {type: TYPE_BUSINESS})

Not only this would allow us to remove some boilerplate code from many components, but would also introduce a consistent way to work with plans - currently they are dealt with in a dozens of different ways.

Finally, with this PR we are able to get rid of other constants such as:

export const JETPACK_PLANS = [
export const JETPACK_MONTHLY_PLANS = [

And have PLANS_LIST as a single source of truth allowing us to cross-reference anything in any way without other constants containing additional groupings.

Test plan:

Check if purchase/upgrade page with plans displays correct prices and looks properly.
Check if purchase/upgrade page with plans displays correct details about current plan and upgrade options for monthly users

matticbot · 2018-03-14T20:50:33Z

Test live: https://calypso.live/?branch=add/plans-meta-data

This is a first step in removing code like "if(plan === PLAN_BUSINESS)" which will no longer be handy after we introduce 2-year plans

markryall

looks good! excellent test coverage

taggon · 2018-03-15T13:44:26Z

client/lib/plans/index.js

-export function getPlan( plan ) {
-	return PLANS_LIST[ plan ];
+export function getPlan( planKey ) {
+	if ( Object.prototype.toString.apply( planKey ) === '[object Object]' ) {


How about using a lodash function isPlainObject instead? Though, checking if planKey is string here would be simpler.

Besides, I think the documentation should be added since the function has changed.

I only wanted to make a lookup in PLANS_LIST values for objects and not for e.g. arrays; I like the idea of using isPlainObject though - I'll do it in one of follow-up PRs

Why do we have to allow for object arguments here anyway? This is the kind of permissive interface and implementation that I think has the potential to get us in trouble. I'd go as far as to say it's a bad enough antipattern that it's worth getting rid of in our legacy code, but certainly something we shouldn't introduce when writing new code.

There might be 'canonical' literature on this, but my main line of argument is that this sort of permissiveness/indecisiveness makes code harder to read (we can't ever be sure what kind of data we're dealing with), and consequentially will lead to duplication of logic/more conditionals -- and in the places where we miss to add those additional conditionals because we overlook them, things are more likely to break.

Shouldn't the consumer of this function know whether we're dealing with a plan slug/key (string) or an object and act accordingly?

@ockham we could get rid of it. I introduced it before starting a major calypso-wide refactoring since it looked like it could save a significant amount of time as it looked like a lot of places in the codebase wasn't sure about the argument type. In the end, it turned out that we don't rely on it that much (or at all).

As a side note, I don't think it's that bad though:

we can't ever be sure what kind of data we're dealing with

Either a plan key (string) or a plan (object), the check seems pretty clear.

and in the places where we miss to add those additional conditionals because we overlook them, things are more likely to break.

We don't need to add any conditionals out of this function, just use the plan object returned by this function. The return type is always the same (unless it's null).

Anyway - feel free to start a PR and I'll be happy to review :-)

taggon · 2018-03-15T13:54:52Z

client/lib/plans/index.js

+}
+
+export function isBusinessPlan( planSlug ) {
+	return getPlan( planSlug ).type === TYPE_BUSINESS;


This type of operation can be safer. If the planSlug is invalid, the popular error TypeError: ‘undefined’ Is Not an Object will occur.

I want to make getPlan throw an explicit error if planSlug is invalid - this would mean less time spent on debugging troublesome calls to getPlan.

This reverts commit 8e96b36.

gwwar · 2018-04-06T16:43:09Z

client/lib/plans/index.js

+		throw new Error(
+			`planMatches can only match against ${ acceptedKeys.join( ',' ) }, ` +
+				`but unknown keys ${ unknownKeys.join( ',' ) } were passed.`
+		);


Be careful of throwing errors when the caller has no intention of catching them. If we'd like to make local dev easier, there are alternative methods:

Perhaps a simple debug statement

console.error in dev environments only

static analysis with a custom eslint rule, which runs in githooks before commit

We could consider pulling in an assertion library in a separate PR to help us check for conditions that should never occur. Say maybe https://github.com/unassert-js/unassert or similar

I don't like hiding errors, and that's what debug statement or console.error would do. How would we know something bad occurred if there are no signs of it? Other than browsing console of course - but you have to know to do it and in what circumstances.

Static analysis is better, and it could be just enough when combined with assertions. As a side note I think having assertions would bring calypso-wide benefits.

As a side note I think having assertions would bring calypso-wide benefits.

We have a few other use cases in the repo, so I think we'd probably just need an open PR to look at a few options and add a library. Especially if the assertions are removable in prod code.

Can't say I'm a big fan of assertions. IMO there's a chance that they might cover up underlying problems, such as code being too permissive or badly designed (think sane defaults/fallbacks). If we're talking about less fragile interfaces, I'd be more interested in static typing.

Static typing would be great as well. It is just one side of the coin though - think design by contract. Assertions are not designed for improving app architecture and I don't agree that they may cover up for underlying problems. On the contrary, their only purpose is to help uncover problems. Of course it is possible to write assertions checking if wrong assumptions are fulfilled, the same is possible with unit tests and yet there is a great value in using them.

the deal with static typing is that it removes a huge swath of these problems which we consistently face needlessly every day. sure it wouldn't solve all our problems or make up for poor technical choices, but at least it can block mistakes and code that is knowably wrong.

Just to be clear - I'm all for static typing :-)

gwwar · 2018-04-06T16:48:11Z

client/lib/plans/test/plan-lookups.js

+	} );
+
+	test( 'should return a proper plan - by value', () => {
+		expect( getPlan( PLANS_LIST[ PLAN_PERSONAL ] ) ).to.equal( PLANS_LIST[ PLAN_PERSONAL ] );


I'm curious about this use case, why would the caller ask for a plan object when it already has one?

AFAIR I was worried about places when argument was named plan and it wasn't clear if it's universally string or object - but I think we ended up clearing things up and this usage was never utilized.

Got it, we can leave it for a future janitorial, probably worth doing a usage audit, and simplifying the function here.

ockham · 2018-05-11T12:43:25Z

I have a use case where I'd like to use isJetpackPlan (which internally uses planMatches). My plan slugs come from the REST API and read e.g. jetpack-personal.

Turns out that planMatches uses getPlan (which is essentially just a dict lookup from the PLANS_LIST const). Also turns out that plan slug keys there use underscores (e.g. jetpack_personal), not hyphens.

Do we have some sort of agreed-upon normalization for plan slugs? And can we expose this subtle issue some more?

dmsnell · 2018-05-11T13:12:58Z

client/lib/plans/index.js

+			return planKey;
+		}
+	}
+	return PLANS_LIST[ planKey ];


how about something like this…

/** * Returns plan information given name of plan * * @param {string} planKey name of plan (@see ./constants) * @returns {?object} plan information if available */ export function getPlan( planKey ) { if ( 'development' === process.env.NODE_ENV ) { if ( 'string' !== typeof planKey ) { console.error( 'Received a non-string plan name for getPlan(): please check call-site' ); if ( planKey.slugOrWhatever ) { console.error( 'Did you mean to pass `planKey.slugOrWhatever` instead?' ); } } return PLANS_LIST[ planKey ]; } }

As I mentioned above, we can probably get rid of the part for handling Object argument. I like this here, it's pretty much a static typing :-) I wonder how easy would it be to start using https://flow.org/ for cases like this here.

it's pretty much a static typing

well, if by "pretty much" we mean "the polar opposite" haha. why have the computer automate bug-finding for us at no cost to our customers if we can do runtime type-checking manually and incompletely by only adding development time?

I wonder how easy would it be to start using https://flow.org/ for cases like this here.

super easy technically and complicated holistically. there are a few of us who have been exploring gradually-typed systems for coming up on a year almost. if you are interested then please join the discussion in the framework channel on Slack 🙂

adamziel added [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Status] Needs e2e Testing (BETA) labels Mar 14, 2018

adamziel force-pushed the add/plans-meta-data branch 2 times, most recently from 4bc31af to 7b8aeea Compare March 15, 2018 09:46

Add metadata and lookup function to plans. …

fac8030

This is a first step in removing code like "if(plan === PLAN_BUSINESS)" which will no longer be handy after we introduce 2-year plans

adamziel force-pushed the add/plans-meta-data branch from 7b8aeea to fac8030 Compare March 15, 2018 10:07

adamziel added [Status] Needs e2e Testing (BETA) and removed [Status] Needs e2e Testing (BETA) labels Mar 15, 2018

adamziel requested review from markryall, dzver and taggon March 15, 2018 10:54

adamziel added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 15, 2018

Remove public/test.html and yarn.lock which were added by accident

5430200

markryall approved these changes Mar 15, 2018

View reviewed changes

adamziel merged commit 8e96b36 into master Mar 15, 2018

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

taggon reviewed Mar 15, 2018

View reviewed changes

adamziel deleted the add/plans-meta-data branch March 15, 2018 14:02

lsinger added a commit that referenced this pull request Mar 15, 2018

Revert "Add metadata and lookup function to plans. (#23314)"

a5adcc3

This reverts commit 8e96b36.

seear mentioned this pull request Mar 19, 2018

Jetpack Plans: Monthly plans show 'billed yearly' #23413

Closed

gwwar reviewed Apr 6, 2018

View reviewed changes

dmsnell reviewed May 11, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/plans meta data #23314

Add/plans meta data #23314

adamziel commented Mar 14, 2018 •

edited

Loading

matticbot commented Mar 14, 2018

markryall left a comment

taggon Mar 15, 2018 •

edited

Loading

adamziel Mar 15, 2018

ockham May 11, 2018

adamziel May 15, 2018

taggon Mar 15, 2018

adamziel Mar 15, 2018

gwwar Apr 6, 2018 •

edited

Loading

adamziel Apr 12, 2018

gwwar Apr 12, 2018

ockham May 11, 2018

adamziel May 15, 2018

dmsnell May 15, 2018

adamziel May 15, 2018

gwwar Apr 6, 2018

adamziel Apr 12, 2018

gwwar Apr 12, 2018

ockham commented May 11, 2018

dmsnell May 11, 2018

adamziel May 15, 2018

dmsnell May 15, 2018

Add/plans meta data #23314

Add/plans meta data #23314

Conversation

adamziel commented Mar 14, 2018 • edited Loading

matticbot commented Mar 14, 2018

markryall left a comment

Choose a reason for hiding this comment

taggon Mar 15, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar Apr 6, 2018 • 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

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

Choose a reason for hiding this comment

ockham commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Mar 14, 2018 •

edited

Loading

taggon Mar 15, 2018 •

edited

Loading

gwwar Apr 6, 2018 •

edited

Loading