Plans (Storage): Enable storage selection for current plan, and other fixes/refactors #91050

chriskmnds · 2024-05-23T12:36:12Z

Fixes #90905 (start of the pit)
Fixes #90947
Fixes #90949
Fixes #91047
Based on #90989

Proposed Changes

Enables showing the storage dropdown selector for the current plan in spotlight
Renders the combined storage volume for the site/plan when a storage add-on has been purchased
Renders the combined storage volume in the dropdown selections (as a fallback in case of similar regressions to Plans (Storage): Wrong values and broken CTA on upgrade plan in admin when storage previously purchased #91047)
Some refactors to clean up the code a bit, start moving things into their own folder (there are enough pieces to storage logic that merit its own little space in the package). We will follow-up with more refactors down the line, consolidate and centralise more of the logic, avoid repeating things across Comparison & Features Grid, etc.

Media

Needs Clarity

The remaining parts (to my mind) that need clarity:

I am going by the assumption that we show the updated/full storage for the site under the current plan after a purchase upgrade. This should align with what we want to do with the new Spotlight designs: p9Jlb4-bSl-p2
Should the price of the plans in the grid when a storage upgrade has been purchased reflect the new overall cost to the user? Before a purchase, we update the yearly prices in the grid to reflect the additional cost of extra storage - should this not be the case after the purchase? e.g.:

On current plan:

with the 100GB storage shown, should the price be 70.96 and not 25?

On upgrade plan (Entrepreneur):

with the 100GB storage shown, should the price be 90.96 and not 45?

Why are these changes being made?

Addresses all of #90905, #90947, #90949, #91047

Testing Instructions

Go to /start/plans and confirm storage selection and rendering works as before
Create a site and go to /plans/[site] and confirm the same
Confirm the current/spotlight plan allows for selecting/purchasing additional storage
Purchase 50GB additional storage and revisit /plans/[site with extra storage]
- Confirm the media above:
  - the total storage reflected in the storage label rendered for plans that allow storage upgrades
  - storage selection no longer enabled
  - (optional - the fallback) remove the storageAddOn.purchased constraints from the code and reload. The selectors for current and Entrepreneur plans should show selection that reflects what the site would have upgraded to, as outlined in Plans (Storage): Wrong values and broken CTA on upgrade plan in admin when storage previously purchased #91047 (so 150GB and 200GB, instead of 100GB and 150GB)

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-05-23T12:36:28Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-05-23T12:36:41Z

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

Sections (~340 bytes added 📈 [gzipped])

name                  parsed_size           gzip_size
update-design-flow        +1377 B  (+0.1%)     +406 B  (+0.1%)
plugins                   +1377 B  (+0.1%)     +406 B  (+0.1%)
plans                     +1377 B  (+0.1%)     +406 B  (+0.1%)
link-in-bio-tld-flow      +1377 B  (+0.1%)     +406 B  (+0.1%)
jetpack-app               +1377 B  (+0.4%)     +406 B  (+0.4%)
add-ons                    +149 B  (+0.0%)      +29 B  (+0.0%)
hosting                    +106 B  (+0.0%)       +7 B  (+0.0%)
theme                       -43 B  (-0.0%)      -36 B  (-0.0%)
stats                       -43 B  (-0.0%)      -30 B  (-0.0%)
import-flow                 -43 B  (-0.0%)      -29 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 (~440 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected      +1377 B  (+0.4%)     +406 B  (+0.3%)
async-load-signup-steps-plans                        +1377 B  (+0.4%)     +406 B  (+0.3%)
async-load-signup-steps-add-ons                       +149 B  (+0.2%)      +34 B  (+0.1%)

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.

chriskmnds · 2024-05-23T12:44:14Z

@jeyip @southp can we please give this a thorough round of testing to see if it's heading in the right direction? Lots more refactors can come on top, but if the functionality is improved, then we may consider merging some version.

One possible venue (depending on what we decide in the Needs Clarity section), would be to show the extended storage description in the label e.g. with the green letters, if the plan price is going to reflect the additional cost after purchase e.g. instead of 100GB (as in the media above), to show 100GB + 45.96/month (like in the dropdown options) - assuming we update the prices to be 90.96, etc.

southp · 2024-05-24T10:14:37Z

Thanks for working on this, @chriskmnds . People say that one stone two birds is good, we've got 4 birds(bugs) here :D

I am going by the assumption that we show the updated/full storage for the site under the current plan after a purchase upgrade. This should align with what we want to do with the new Spotlight designs: p9Jlb4-bSl-p2

Sounds good to me. To close the loop here, I've left similar sentiment here: #90949 (comment).

Should the price of the plans in the grid when a storage upgrade has been purchased reflect the new overall cost to the user? Before a purchase, we update the yearly prices in the grid to reflect the additional cost of extra storage - should this not be the case after the purchase?

There is a related issue here: #86154, just that the behavior seems to have changed at some points. Given the cited new design spotlight design above, showing a combined price makes the most sense to me.

update: @vinimotaa could you help share your insights here with us? I'd be grateful :)

southp · 2024-05-24T10:54:24Z

Hmm, wait. one more case I recalled is to toggle to the longer term when we have purchased a storage add-on:

CleanShot.2024-05-24.at.18.40.03.mp4

In this case, the total storage is incorrectly resetted to 50G. However, what's more problematic is that I don't really know how we can ever combine the price in the reasonable way since the storage add-ons are only available in the annual term :/ @niranjan-uma-shankar also once raised that here: Automattic/martech#2614.

For the time being, maybe showing them as a separate price, e.g. something like "+x / month, billed annually" along side the storage volume label. It's not pretty, but at least it conveys correct info. @vinimotaa @chriskmnds what do you think? 🤔

chriskmnds · 2024-05-24T11:05:27Z

For the time being, maybe showing them as a separate price, e.g. something like "+x / month, billed annually" along side the storage volume label. It's not pretty, but at least it conveys correct info. @vinimotaa @chriskmnds what do you think? 🤔

@southp I'm definitely on board with that, for the time being. It's also a little weird to be showing the updated prices only for the "storage-upgradable" plans (cause that's kinda the reality - you cannot have more storage on the others, so it may even be more confusing to be updating all the plan prices, as the alternative).

such a maze

chriskmnds · 2024-05-24T11:35:03Z

In this case, the total storage is incorrectly resetted to 50G. However, what's more problematic is that I don't really know how we can ever combine the price in the reasonable way since the storage add-ons are only available in the annual term :/

Good point @southp. I feel "not resetting it to 50GB" (without additional fixes) would be equally incorrect. You are looking at 2-year price/features comparison, and the storage is technically a 1-year concern. So it either needs be weirdly smart about it (and equally confusing to the user) or clarified down to detail that the "100GB, plus whatever cost" is only for the first year. So something like 50GB (plus 50GB for the first year at $123/month), etc.

jeyip · 2024-05-27T01:40:15Z

Hi folks! Sorry I missed this on Friday. Going to check out the PR and respond after dinner tonight 🙂

jeyip · 2024-05-27T04:24:27Z

Testing out the branch now 👀

jeyip · 2024-05-27T04:52:50Z

Testing

Requirements

The code generally looks reasonable to me. I can take a closer look when I'm back on Tuesday if not merged by then.

For each of the flows listed below:

Go to /start/plans and confirm storage selection and rendering works as before
Create a site and go to /plans/[site] and confirm the same
Confirm the current/spotlight plan allows for selecting/purchasing additional storage
Purchase 50GB additional storage and revisit /plans/[site with extra storage]

Flows:

/start
/setup/newsletter
/plans

Browsers

Chrome

jeyip · 2024-05-27T06:22:20Z

I agree with what has been laid out so far between Christos and James.

would be to show the extended storage description in the label e.g. with the green letters, if the plan price is going to reflect the additional cost after purchase e.g. instead of 100GB (as in the media above), to show 100GB + 45.96/month (like in the dropdown options) - assuming we update the prices to be 90.96, etc.

Yes making a distinction between the cost of the add-on and the cost of the plan for spotlight plans makes a lot of sense to me, especially if it aligns with future designs.

Should the price of the plans in the grid when a storage upgrade has been purchased reflect the new overall cost to the user? Before a purchase, we update the yearly prices in the grid to reflect the additional cost of extra storage - should this not be the case after the purchase?

The concern to me is the user mistakenly believing that they have to pay $x more for the plan because of their add-on. Not sure if this is the best way to handle things, but I wonder if we could also this update the storage label in this case. Something denoting that the upgraded storage would be carried over to their new plan

jeyip · 2024-05-27T06:31:46Z

packages/plans-grid-next/src/components/storage/hooks/use-storage-string-from-feature.ts

+	FEATURE_13GB_STORAGE,
+	FEATURE_50GB_STORAGE,
+	FEATURE_200GB_STORAGE,
+	FEATURE_50GB_STORAGE_ADD_ON,


NB:

It's unfortunate that my initial technical implementation of add-ons relying on feature slugs is spreading a bit more 😕

In the past, we've talked about FEATURE_50GB_STORAGE_ADD_ON and FEATURE_100GB_STORAGE_ADD_ON being a misnomer. The add-on is not a feature, and at the very least, it should just be 50GB_STORAGE_ADD_ON. I know there's a lot more to refactor beyond that, but since we're starting to taking a closer look at this part of the code again, let me know if you think renaming might help with any confusion. I'm happy to set up a PR if we'd like.

Good thinking @jeyip

Let's come back to this later and figure out the best way forward. I'm wondering if we can skip a "meaningful" slug for this altogether and just go with how storage add-ons work (a combination of PRODUCT_1GB_SPACE & quantity). So in a way finding a solution that won't need to enumerate/define the options via slugs. Then for the state store we could just store the index of the item selected, not the slug (and whether add-on).

It's unfortunate that my initial technical implementation of add-ons relying on feature slugs is spreading a bit more 😕

Not at all! I can only feel sympathetic seeing the complexity around this. The problem is deep, and we also didn't exactly have time to "breathe" at the time. 😎

jeyip · 2024-05-30T01:09:58Z

I finally got some other time sensitive work off my plate p1717030570883619/1715880961.253219-slack-C06QNEKS4NN :D Going to take a cloesr look at this PR tonight after dinner

jeyip · 2024-05-30T07:18:43Z

packages/plans-grid-next/src/components/comparison-grid/index.tsx

-							{ getStorageStringFromFeature( defaultStorageOption?.slug || '' ) }
-						</StorageButton>
-					) }
+					{ storageJSX }


Thanks for making the nested conditionals more readable 👍

doing a lot more of this in #91196 and likely others on top

chriskmnds · 2024-05-30T07:26:22Z

I finally got some other time sensitive work off my plate p1717030570883619/1715880961.253219-slack-C06QNEKS4NN :D Going to take a cloesr look at this PR tonight after dinner

Thanks @jeyip @southp

I pushed some changes that will/should handle showing the added cost next/under the label when the storage is upgraded (but keep the plan price the same). Some images:

The changes don't fiddle with the 2/3-year plans situation. The storage gets reset in that case and is not selectable. We'll need to revisit and decide on that. For an intermediate solution, we could ship the same functionality for those. I think best to address those in a separate PR. Also the new designs for spotlight will need to address that one way or another.

jeyip

After taking a closer look at the PR, everything seems reasonable so far, and I don't have any concerns with the direction the code is going. I also appreciate the general improvements to readability along the way.

I see that recent changes were pushed with regards to storage label pricing. I will test these updates first thing in the morning 🙂

chriskmnds · 2024-05-30T10:09:40Z

@jeyip Rebased and updated. So it should be all ready now (we did some refactors to storage-options component in trunk). worth double-checking the mobile views. I'm done with this for today - will carry on tomorrow 😅

chriskmnds · 2024-05-30T10:16:54Z

packages/plans-grid-next/src/components/storage/hooks/use-storage-string-from-feature.ts

+			/**
+			 * TODO: Don't do this here. Diverge and show the version with the green pricing next
+			 */


Fine now. we may come back to this component later

chriskmnds · 2024-05-30T10:19:11Z

packages/plans-grid-next/src/components/storage/components/plan-storage-label.tsx

+	planSlug: PlanSlug;
+}
+
+const ELIGIBLE_PLANS_FOR_STORAGE_UPGRADE = [ PLAN_BUSINESS, PLAN_ECOMMERCE ];


this needs to be revisited and centralized differently. it might actually just be redundant actually

matticbot · 2024-05-30T10:31:17Z

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

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

odyssey-stats
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/plans-next-storage-dropdown-for-current-plan on your sandbox.

jeyip · 2024-05-30T19:22:01Z

Testing

Requirements

For each of the flows listed below:

Verified header price and billing timeframe price when a storage add-on is selected
Verified large currency formatting with IDR
Verified that storage add-ons are only available for yearly plans
Verified that spotlight plans have the storage add-on dropdown rendered to allow storage add-on purchases
Verified that, once purchased, storage add-on dropdowns are no longer rendered in the plans grid
Verified behavior of the inline storage add-on price label ( rendered once a storage add-on is purchased )
Verified that the dropdown is still keyboard navigable
Ensured that a plan could be purchased
Spot checked desktop and mobile
Spot checked the spotlight plan on the /plans page

Flows:

/start
/setup/free
/start/onboarding-pm
/plans

Browsers

Chrome

Notes

This is already present in production. IDR header pricing overflows borders in comparison grid at certain screen resolutions when storage add-on is selected ( Ex. screen width of 883 px in the /start page )

Not necessary to resolve right now, and something we can easily update in a future PR. There appears to be a lot of extra empty space for the spotlight plan dropdown menu. It seems like we could let menu width be dynamically sized by its content

jeyip · 2024-05-30T19:41:30Z

Rerunning what appear to be flakey E2E specs

jeyip

Tests well and behaves as discussed in the PR description -- we can get these changes into production and continue building on work here in follow-up PRs

chriskmnds · 2024-05-31T09:29:46Z

Thank you again @jeyip ! I will ship this now and move things into follow-ups.

This is already present in production. IDR header pricing overflows borders in comparison grid at certain screen resolutions when storage add-on is selected ( Ex. screen width of 883 px in the /start page )

Interesting. Not sure what to do about this now since the price text is already reduced. We could explore a couple of options (switch the view to tablet or otherwise even smaller text 😬 )

Not necessary to resolve right now, and something we can easily update in a future PR. There appears to be a lot of extra empty space for the spotlight plan dropdown menu. It seems like we could let menu width be dynamically sized by its content

Noticed. I'll port this to a new issue and work it out in a follow up.

I'm just glad it works at some level right now. More to come! I'll take this up as a bit of a side project to keep improving.

…rice

continuing continuing - make the filtering on site storage across all plan storage options. fix price not upgraded when exceeding - e.g. on enterprise plan with current creator plus 50gb - should filter the option out instead continuing. introducing useStorageStringFromFeature to account for current site storage update storage titles to reflect upgrade from the existing/real site storage disable when any storage upgrade purchased. as before show the actual storage size in the badge.. but TODO added show the actual storage size in the badge.. but TODO added cleanup. move things around into hooks, folders, etc. cleanup

chriskmnds · 2024-05-31T10:03:26Z

Not necessary to resolve right now, and something we can easily update in a future PR. There appears to be a lot of extra empty space for the spotlight plan dropdown menu. It seems like we could let menu width be dynamically sized by its content

Noticed. I'll port this to a new issue and work it out in a follow up.

@jeyip #91333

niranjan-uma-shankar · 2024-06-03T03:32:55Z

@chriskmnds @jeyip Since there's ongoing work to improve the Storage dropdown presentation, I'm bumping to notice that the storage dropdown is missing on the comparison grid in the offerings experiment treatment view. Check pau2Xa-5Sh-p2#comment-15574 for details.

chriskmnds added the Storage Addon label May 23, 2024

chriskmnds self-assigned this May 23, 2024

chriskmnds changed the base branch from trunk to update/plans-next-always-reflect-selected-storage-option-on-price May 23, 2024 12:36

chriskmnds marked this pull request as ready for review May 23, 2024 12:37

chriskmnds requested a review from a team as a code owner May 23, 2024 12:37

chriskmnds requested a review from southp May 23, 2024 12:37

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 23, 2024

chriskmnds mentioned this pull request May 23, 2024

Plans (Storage): Always reflect selected storage option in the plan price #90989

Closed

7 tasks

chriskmnds requested a review from jeyip May 23, 2024 12:40

chriskmnds mentioned this pull request May 24, 2024

Plans: Storage volume rendered for spotlight plan not reflecting purchased storage #90949

Closed

jeyip reviewed May 27, 2024

View reviewed changes

chriskmnds mentioned this pull request May 28, 2024

Plans (Storage): More base cleanup of existing hooks/props #91196

Merged

7 tasks

jeyip reviewed May 30, 2024

View reviewed changes

chriskmnds force-pushed the update/plans-next-always-reflect-selected-storage-option-on-price branch from 9196728 to 0796e33 Compare May 30, 2024 10:06

chriskmnds force-pushed the update/plans-next-storage-dropdown-for-current-plan branch from 04c291d to 7a96eea Compare May 30, 2024 10:06

chriskmnds commented May 30, 2024

View reviewed changes

jeyip approved these changes May 30, 2024

View reviewed changes

chriskmnds changed the base branch from update/plans-next-always-reflect-selected-storage-option-on-price to trunk May 31, 2024 09:24

chriskmnds added 5 commits May 31, 2024 12:30

Plans (Storage): Always reflect selected storage option in the plan p…

fe08113

…rice

also remove exceedsSiteStorageLimits condition

ff40507

include added price/cost in upgraded storage label

f0555f3

styling fixes after rebase

c4cc59d

chriskmnds force-pushed the update/plans-next-storage-dropdown-for-current-plan branch from 7a96eea to c4cc59d Compare May 31, 2024 09:31

chriskmnds merged commit 0107d53 into trunk May 31, 2024
11 checks passed

chriskmnds deleted the update/plans-next-storage-dropdown-for-current-plan branch May 31, 2024 09:55

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 May 31, 2024

chriskmnds mentioned this pull request May 31, 2024

Plans (Storage): Misaligned dropdown on spotlight-plan storage selection #91333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plans (Storage): Enable storage selection for current plan, and other fixes/refactors #91050

Plans (Storage): Enable storage selection for current plan, and other fixes/refactors #91050

chriskmnds commented May 23, 2024 •

edited

github-actions bot commented May 23, 2024 •

edited by matticbot

matticbot commented May 23, 2024 •

edited

chriskmnds commented May 23, 2024 •

edited

southp commented May 24, 2024 •

edited

southp commented May 24, 2024

chriskmnds commented May 24, 2024 •

edited

chriskmnds commented May 24, 2024

jeyip commented May 27, 2024

jeyip commented May 27, 2024

jeyip commented May 27, 2024 •

edited

jeyip commented May 27, 2024

jeyip May 27, 2024 •

edited

chriskmnds May 27, 2024

chriskmnds May 27, 2024

jeyip commented May 30, 2024 •

edited

jeyip May 30, 2024

chriskmnds May 30, 2024

chriskmnds commented May 30, 2024

jeyip left a comment

chriskmnds commented May 30, 2024

chriskmnds May 30, 2024

chriskmnds May 30, 2024

matticbot commented May 30, 2024 •

edited

jeyip commented May 30, 2024 •

edited

jeyip commented May 30, 2024

jeyip left a comment

chriskmnds commented May 31, 2024

chriskmnds commented May 31, 2024

niranjan-uma-shankar commented Jun 3, 2024 •

edited by chriskmnds

Plans (Storage): Enable storage selection for current plan, and other fixes/refactors #91050

Plans (Storage): Enable storage selection for current plan, and other fixes/refactors #91050

Conversation

chriskmnds commented May 23, 2024 • edited

Proposed Changes

Media

Needs Clarity

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented May 23, 2024 • edited by matticbot

matticbot commented May 23, 2024 • edited

chriskmnds commented May 23, 2024 • edited

southp commented May 24, 2024 • edited

southp commented May 24, 2024

chriskmnds commented May 24, 2024 • edited

chriskmnds commented May 24, 2024

jeyip commented May 27, 2024

jeyip commented May 27, 2024

jeyip commented May 27, 2024 • edited

Testing

Requirements

Browsers

jeyip commented May 27, 2024

jeyip May 27, 2024 • edited

Choose a reason for hiding this comment

chriskmnds May 27, 2024

Choose a reason for hiding this comment

chriskmnds May 27, 2024

Choose a reason for hiding this comment

jeyip commented May 30, 2024 • edited

jeyip May 30, 2024

Choose a reason for hiding this comment

chriskmnds May 30, 2024

Choose a reason for hiding this comment

chriskmnds commented May 30, 2024

jeyip left a comment

Choose a reason for hiding this comment

chriskmnds commented May 30, 2024

chriskmnds May 30, 2024

Choose a reason for hiding this comment

chriskmnds May 30, 2024

Choose a reason for hiding this comment

matticbot commented May 30, 2024 • edited

jeyip commented May 30, 2024 • edited

Testing

Requirements

Browsers

Notes

jeyip commented May 30, 2024

jeyip left a comment

Choose a reason for hiding this comment

chriskmnds commented May 31, 2024

chriskmnds commented May 31, 2024

niranjan-uma-shankar commented Jun 3, 2024 • edited by chriskmnds

chriskmnds commented May 23, 2024 •

edited

github-actions bot commented May 23, 2024 •

edited by matticbot

matticbot commented May 23, 2024 •

edited

chriskmnds commented May 23, 2024 •

edited

southp commented May 24, 2024 •

edited

chriskmnds commented May 24, 2024 •

edited

jeyip commented May 27, 2024 •

edited

jeyip May 27, 2024 •

edited

jeyip commented May 30, 2024 •

edited

matticbot commented May 30, 2024 •

edited

jeyip commented May 30, 2024 •

edited

niranjan-uma-shankar commented Jun 3, 2024 •

edited by chriskmnds