Plans: Make storage selection calculations in plan prices optional #96520

jeyip · 2024-11-19T01:40:07Z

Related to https://github.com/Automattic/martech/issues/3403 and #96357 (review)

Proposed Changes

Disable storage add-on selection updates in header price for the calypso_plans_page_emphasize_longer_plan_savings experiment with a new reflectStorageSelectionInPlanPrices flag
Pass reflectStorageSelectionInPlanPrices to context to reference throughout plans-grid-next. If false, prevent storage add-on calculations in features grid + comparison grid plan pricing.

Why are these changes being made?

See p1726619377025979/1724962428.022989-slack-C07ABDFQEMS

Testing Instructions

Go to /start/plans?flags=plans/term-savings-price-display
Ensure that when storage add-ons are selected for business or ecommerce annual plans, the header price doesn't change

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-11-19T01:40:20Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-127619

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-127619&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-127619&env=a8c-for-agencies

jeyip

This is the first approach that came to mind. Totally possible that there might be a more neat / tidy solution. I'll give it more thought tomorrow.

matticbot · 2024-11-19T01:45:22Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications
odyssey-stats
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/term-savings-pricing-to-ignore-storage-addon-selections on your sandbox.

matticbot · 2024-11-19T01:47:35Z

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

Sections (~867 bytes added 📈 [gzipped])

name                     parsed_size           gzip_size
site-overview                +1485 B  (+0.1%)     +370 B  (+0.1%)
hosting                      +1485 B  (+0.1%)     +370 B  (+0.1%)
update-design-flow           +1434 B  (+0.1%)     +292 B  (+0.1%)
plugins                      +1434 B  (+0.0%)     +289 B  (+0.0%)
plans                        +1434 B  (+0.1%)     +292 B  (+0.1%)
link-in-bio-tld-flow         +1434 B  (+0.1%)     +285 B  (+0.1%)
jetpack-app                  +1434 B  (+0.4%)     +291 B  (+0.2%)
theme                          +44 B  (+0.0%)      +25 B  (+0.0%)
stats                          +44 B  (+0.0%)      +22 B  (+0.0%)
site-purchases                 +44 B  (+0.0%)      +27 B  (+0.0%)
purchases                      +44 B  (+0.0%)      +27 B  (+0.0%)
posts-custom                   +44 B  (+0.0%)      +26 B  (+0.0%)
posts                          +44 B  (+0.0%)      +26 B  (+0.0%)
migrate                        +44 B  (+0.0%)      +28 B  (+0.0%)
import-hosted-site-flow        +44 B  (+0.0%)      +28 B  (+0.0%)
domains                        +44 B  (+0.0%)      +26 B  (+0.0%)
checkout                       +44 B  (+0.0%)      +27 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~317 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected      +1434 B  (+0.4%)     +291 B  (+0.2%)
async-load-signup-steps-plans                        +1434 B  (+0.4%)     +291 B  (+0.2%)
async-load-design-blocks                               +44 B  (+0.0%)      +26 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

packages/data-stores/src/plans/hooks/use-pricing-meta-for-grid-plans.ts

chriskmnds

Hey @jeyip. Do we also need to pass this along from usePlanBillingDescription? It makes a call to Plans.usePricingMetaForGridPlans and passes storageAddOns through (so they are definitely accounted for).

I think we'd just need to add the prop to plans-grid-context and grab from usePlanBillingDescription.

client/my-sites/plans-features-main/index.tsx

jeyip · 2024-11-21T02:20:13Z

Hey @chriskmnds just leaving a message here to say I didn't miss your comments. Will get to them tomorrow 😎

jeyip · 2024-11-22T02:17:05Z

It's getting near the end of my day. I started reviewing the comment, but will continue tomorrow. Thanks for the patience!

jeyip · 2024-11-25T23:16:17Z

Addressing merge conflicts and CR comments now :D

jeyip · 2024-12-06T03:00:15Z

@chriskmnds would love your input on 57e2f8a. There's one change that's definitely worth discussing, and it's around the useEligibilityForTermSavingsPriceDisplay hook.

Context

Although we disable storage add-on calculations in this PR, I believe it makes sense to re-enable storage add-on calculations when term savings are not shown. With #96631, this includes re-enabling storage add-on calculations when there are discounts or special offers in the plans grid.

To do so, I leverage the enableTermSavingsPriceDisplay and the useEligibilityForTermSavingsPriceDisplay hook here.

The tricky part is that we handle reflectStorageSelectionInPlanPrices in the usePricingMetaForGridPlans hook, which is called by useGridPlans. We want the results of useEligibilityForTermSavingsPriceDisplay to determine if we reflect storage selection, but useEligibilityForTermSavingsPriceDisplay currently depends on the results of useGridPlans.

Reiterated more linearly:

We handle reflectStorageSelectionInPlanPrices in the pricing meta for grid plans
We join pricing meta to plans metadata in useGridPlans
We use the generated gridPlans to determine useEligibilityForTermSavingsPriceDisplay
We, however, need the results of useEligibilityForTermSavingsPriceDisplay to properly determine reflectStorageSelectionInPlanPrices, which is handled in useGridPlans, which useEligibilityForTermSavingsPriceDisplay depends on

Hoping that makes sense, but happy to clarify further if needed.

Possible Solutions

I've pushed one possible solution in 57e2f8a, where, instead of useEligibilityForTermSavingsPriceDisplay depending on generated grid plans, we generate the required data ourselves and calculate allAvailablePlanSlugs within the hook instead. I would clean this up further if this seems reasonable

Another alternative might be to prop drill the useEligibilityForTermSavingsPriceDisplay through useGridPlansForComparisonGrid and useGridPlansForFeaturesGrid into useGridPlans. I don't have a strong opinion of if this is more or less ideal yet. Will sleep on this and think of other ways to handle this as well.

Lemme know your thoughts 🙂

chriskmnds · 2024-12-06T09:39:29Z

client/my-sites/plans-features-main/hooks/use-eligibility-for-term-savings-price-display.ts

+	const availablePlanSlugs = usePlansFromTypes( {
+		planTypes: usePlanTypesWithIntent( {
+			intent: 'default',
+			selectedPlan,
+			siteId,
+			hiddenPlans,
+			isSubdomainNotGenerated,
+		} ),
+		term,
+		intent,
+	} );
+	const planSlugsForAllDisplayedIntervals = availablePlanSlugs.flatMap(


This does make the hook a lot more complicated, also indirectly linked to how we generate plan slugs in useGridPlans, which might be hard to keep aligned going forward.

I wonder if we relax the criteria a bit (of reflecting or not reflecting storage selection) might help? e.g. let's show it
by default, and not show it if the experiment.isEligibleForTermSavings is set. Simple. So we disconnect it from needing access to useEligibilityForTermSavingsPriceDisplay, which does a lot more than that. WDYT?

Although, not sure if that might help after we conclude the experiment and we run into this problem again 🤔

Hmm so yeah, I think what you have here might be the way forward. Happy to give it some more thought later and get back to you.

p.s. we'd need to use the actual intent passed through here (without a default value), and also export usePlansFromTypes to import normally (not from the src folder). if we stick with this.

I wonder if we relax the criteria a bit (of reflecting or not reflecting storage selection) might help? e.g. let's show it
by default, and not show it if the experiment.isEligibleForTermSavings is set. Simple. So we disconnect it from needing access to useEligibilityForTermSavingsPriceDisplay, which does a lot more than that. WDYT?

Although, not sure if that might help after we conclude the experiment and we run into this problem again 🤔

The more I look at the code and weigh the complexity we introduce with the functionality we gain, the more I'm considering this 👍 But yes, like you say, if the experiment concludes successfully we'd have to revisit this discussion

p.s. we'd need to use the actual intent passed through here (without a default value), and also export usePlansFromTypes to import normally (not from the src folder). if we stick with this.

On it

Happy to give it some more thought later and get back to you.

Sounds great -- would love your thoughts as we move forward 🙂

chriskmnds · 2024-12-06T09:40:36Z

client/my-sites/plans-features-main/hooks/use-eligibility-for-term-savings-price-display.ts

 	displayedIntervals: UrlFriendlyTermType[];
 	storageAddOns: ( AddOns.AddOnMeta | null )[] | null;
 	coupon?: string;
 	siteId?: number | null;
 	isInSignup?: boolean;
 } ) => {
 	const longerPlanTermDefaultExperiment = useLongerPlanTermDefaultExperiment();
-	const planSlugs = gridPlans.map( ( { planSlug } ) => planSlug );


p.s. NIT - if we bring back the original (prior to these changes), would it make sense to pass planSlugs instead? So do the mapping at consuming end perhaps.

Oh good question. I'll think this over 👍

If we go back to the original, I prefer planSlugs. It more concisely describes what's needed for the hook, and it generalizes the interface more.

chriskmnds · 2024-12-06T09:41:20Z

Good thinking @jeyip . I replied in a couple of comments, so we can have discussion there instead quoting each other here :-)

jeyip · 2024-12-13T20:36:54Z

Starting to revisit this PR. Planning to merge later today for two reasons:

to close the loop on remaining work for https://github.com/Automattic/martech/issues/3403
this work isn't user facing at the moment, so our changes are easily reversible

This reverts commit 29abd6f.

…bled

…lanPrices`

…omparison Grid props

…ation logic

jeyip · 2024-12-14T06:41:45Z

Testing

Requirements

For each of the flows listed below:

Verified header price and billing timeframe price with production
Verified pricing for monthly and yearly pricing
Ensured that a plan could be purchased
Verified that storage add-ons do not affect price when plan term savings are shown
Verified that storage add-ons do affect price when plan term savings are not shown
In /plans, verified that long term plan savings are never shown

Flows:

/start
/setup/link-in-bio-tld
/plans

Browsers

Chrome

jeyip added the Multi-year Plans by Default Experiment label Nov 19, 2024

jeyip self-assigned this Nov 19, 2024

jeyip requested a review from a team as a code owner November 19, 2024 01:40

jeyip requested a review from southp November 19, 2024 01:40

matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 19, 2024

jeyip mentioned this pull request Nov 19, 2024

Plans: Enable term savings price display - as a grid feature #96357

Merged

9 tasks

jeyip commented Nov 19, 2024

View reviewed changes

jeyip requested a review from chriskmnds November 19, 2024 01:48

chriskmnds reviewed Nov 19, 2024

View reviewed changes

packages/data-stores/src/plans/hooks/use-pricing-meta-for-grid-plans.ts Outdated Show resolved Hide resolved

chriskmnds reviewed Nov 19, 2024

View reviewed changes

packages/data-stores/src/plans/hooks/use-pricing-meta-for-grid-plans.ts Outdated Show resolved Hide resolved

chriskmnds requested changes Nov 20, 2024

View reviewed changes

chriskmnds reviewed Nov 20, 2024

View reviewed changes

client/my-sites/plans-features-main/index.tsx Outdated Show resolved Hide resolved

chriskmnds force-pushed the update/plans-grid-header-price-cleanup branch from 9ece3ab to 14945ae Compare November 21, 2024 07:33

chriskmnds force-pushed the update/plans-grid-header-price-cleanup branch 2 times, most recently from c8e0442 to d196100 Compare November 25, 2024 09:55

Base automatically changed from update/plans-grid-header-price-cleanup to trunk November 25, 2024 10:13

jeyip force-pushed the update/term-savings-pricing-to-ignore-storage-addon-selections branch 2 times, most recently from 86db0e8 to a182e23 Compare November 26, 2024 23:02

jeyip mentioned this pull request Nov 27, 2024

Plans: Move useStorageAddOns logic selection in pricing meta hook #96800

Open

8 tasks

jeyip force-pushed the update/term-savings-pricing-to-ignore-storage-addon-selections branch 3 times, most recently from 4e4bd2f to 9598f54 Compare December 2, 2024 18:59

jeyip changed the title ~~Plans: Make storage selection reflection in header price optional~~ Plans: Make storage selection reflection in plan prices optional Dec 6, 2024

jeyip changed the title ~~Plans: Make storage selection reflection in plan prices optional~~ Plans: Make storage selection calculations in plan prices optional Dec 6, 2024

chriskmnds reviewed Dec 6, 2024

View reviewed changes

chriskmnds and others added 17 commits December 13, 2024 12:39

use wp-data store

2b5f3ac

Revert "use wp-data store"

beca743

This reverts commit 29abd6f.

wrap header discounting logic in a hook

47b63e7

Disable storage add-on updates to header price if term savings is ena…

6a9f37f

…bled

Rename enableTermSavingsPriceDisplay to `reflectStorageSelectionInP…

b4f8e31

…lanPrices`

DRY enableTermSavingsPriceDisplay in plans features main

b68d798

Default enable storage add-on calculations in plan prices

d736f71

Add docs for reflectStorageSelectionInPlanPrices variable name

b41cfeb

Fix disabling storage selection logic

099912f

Set reflectStorageSelectionInPlanPrices default param to false

aa30f47

Fix failing usePricingMetaForGridPlans test

bc3cbd9

Set reflectStorageSelectionInPlanPrices flag in Features Grid and C…

8fec7d2

…omparison Grid props

Reference plans grid context where appropriate

fbdc1bb

Remove reflectStorageSelectionInPlanPrices param from gridPlans gener…

fe1058d

…ation logic

Ensure that storage add-on is calculated if term savings are hidden

ebf0e5e

Pass intent through to usePlansFromTypes

34d6839

Import usePlansFromTypes from @automattic/plans-grid-next

acd3c2d

jeyip force-pushed the update/term-savings-pricing-to-ignore-storage-addon-selections branch from 82f95b3 to acd3c2d Compare December 13, 2024 20:39

Remove inline comments

8a57fbd

jeyip merged commit b8226dd into trunk Dec 14, 2024
11 checks passed

jeyip deleted the update/term-savings-pricing-to-ignore-storage-addon-selections branch December 14, 2024 07:14

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plans: Make storage selection calculations in plan prices optional #96520

Plans: Make storage selection calculations in plan prices optional #96520

jeyip commented Nov 19, 2024 •

edited

Loading

github-actions bot commented Nov 19, 2024 •

edited by matticbot

Loading

jeyip left a comment •

edited

Loading

matticbot commented Nov 19, 2024 •

edited

Loading

matticbot commented Nov 19, 2024 •

edited

Loading

chriskmnds left a comment

jeyip commented Nov 21, 2024

jeyip commented Nov 22, 2024 •

edited

Loading

jeyip commented Nov 25, 2024

jeyip commented Dec 6, 2024 •

edited

Loading

chriskmnds Dec 6, 2024

chriskmnds Dec 6, 2024

chriskmnds Dec 6, 2024

jeyip Dec 6, 2024 •

edited

Loading

chriskmnds Dec 6, 2024

jeyip Dec 6, 2024

jeyip Dec 14, 2024

chriskmnds commented Dec 6, 2024

jeyip commented Dec 13, 2024 •

edited

Loading

jeyip commented Dec 14, 2024 •

edited

Loading

Plans: Make storage selection calculations in plan prices optional #96520

Plans: Make storage selection calculations in plan prices optional #96520

Conversation

jeyip commented Nov 19, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Nov 19, 2024 • edited by matticbot Loading

jeyip left a comment • edited Loading

Choose a reason for hiding this comment

matticbot commented Nov 19, 2024 • edited Loading

matticbot commented Nov 19, 2024 • edited Loading

chriskmnds left a comment

Choose a reason for hiding this comment

jeyip commented Nov 21, 2024

jeyip commented Nov 22, 2024 • edited Loading

jeyip commented Nov 25, 2024

jeyip commented Dec 6, 2024 • edited Loading

Context

Possible Solutions

chriskmnds Dec 6, 2024

Choose a reason for hiding this comment

chriskmnds Dec 6, 2024

Choose a reason for hiding this comment

chriskmnds Dec 6, 2024

Choose a reason for hiding this comment

jeyip Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

chriskmnds Dec 6, 2024

Choose a reason for hiding this comment

jeyip Dec 6, 2024

Choose a reason for hiding this comment

jeyip Dec 14, 2024

Choose a reason for hiding this comment

chriskmnds commented Dec 6, 2024

jeyip commented Dec 13, 2024 • edited Loading

jeyip commented Dec 14, 2024 • edited Loading

Testing

Requirements

Browsers

jeyip commented Nov 19, 2024 •

edited

Loading

github-actions bot commented Nov 19, 2024 •

edited by matticbot

Loading

jeyip left a comment •

edited

Loading

matticbot commented Nov 19, 2024 •

edited

Loading

matticbot commented Nov 19, 2024 •

edited

Loading

jeyip commented Nov 22, 2024 •

edited

Loading

jeyip commented Dec 6, 2024 •

edited

Loading

jeyip Dec 6, 2024 •

edited

Loading

jeyip commented Dec 13, 2024 •

edited

Loading

jeyip commented Dec 14, 2024 •

edited

Loading