Refactor logic for checking storage add-on availability #97450

fredrikekelund · 2024-12-13T12:32:49Z

Proposed Changes

The main bug this PR fixes is that free sites with the 100 GB add-on keep displaying the 50 GB add-on on /add-ons/:site_slug—indicating that it's available for purchase even though it's not. I suspect this is also true for Premium sites, but I haven't tested it.

While working on this issue, I found the existing availability logic for storage add-ons to be quite spaghetti-esque, so this PR also takes a stab at simplifying that logic. I've implemented the following refactorings:

Remove the storage-enabled feature flag. It has been enabled in all environments since Jun, 2023 (Add-Ons: Enable Storage Add On Feature Flag on All Environments #78775)
Reduce props drilling between AddOnsMain, AddOnsGrid, and AddOnCard
Remove add-on availability checks from useAddOns. Why? Because we previously combined other hooks like useAddOnPurchaseStatus with the availability checks in useAddOns. With my refactoring, we clarify the purpose of useAddOns solely as a data provider.
Added a useStorageAddOnAvailability hook for checking if a single storage add-on is available for purchase when compared to the maximum site storage offered on WordPress.com.

Testing Instructions

I've tagged a few different people for review. @taipeicoder, if you have a moment to review this, I'd appreciate it, since you worked on this logic previously. @rcrdortiz, if you have some groundskeeping time left this afternoon, I'd also appreciate a review.

Apply 168696-ghe-Automattic/wpcom to your sandbox
Sandbox public-api.wordpress.com
Do NOT test with the store sandbox enabled. This alters your results.
I've included a table with four different site configurations to test on three different pages. The slugs for those pages are:
3.1. Add-ons page: /add-ons/:site_slug
3.2. Site overview page: /sites/overview/:site_slug

	Add-ons page: 50 GB add-on	Add-ons page: 100 GB add-on	Storage meter on site overview page
Paid site with 100 GB storage add-on	Hidden	Visible with “Manage add-on” button	No “Need more storage?” link
Paid site with 50 GB storage add-on	Visible with “Manage add-on” button	Purchasable	“Need more storage?” link
Free site without storage add-ons	Purchasable	Purchasable	“Need more storage?” link
Free site with 100 GB storage add-on	Hidden	Visible with “Manage add-on” button	No “Need more storage?” link

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 UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
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-12-13T12:33:03Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-12-13T12:49:49Z

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

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

name                   parsed_size           gzip_size
entry-stepper              +1657 B  (+0.1%)     +292 B  (+0.1%)
entry-main                  -950 B  (-0.0%)    +1376 B  (+0.2%)
entry-login                 +714 B  (+0.0%)    +1478 B  (+0.2%)
entry-subscriptions         +685 B  (+0.0%)     +157 B  (+0.0%)
entry-domains-landing       +685 B  (+0.1%)     +157 B  (+0.1%)
entry-browsehappy           +685 B  (+0.3%)     +157 B  (+0.2%)

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

Sections (~49677 bytes added 📈 [gzipped])

name                                parsed_size           gzip_size
add-ons                                 +1112 B  (+0.3%)     +194 B  (+0.2%)
site-purchases                          -1100 B  (-0.1%)     -384 B  (-0.1%)
purchases                               -1100 B  (-0.1%)     -384 B  (-0.1%)
posts-custom                            -1100 B  (-0.2%)     -487 B  (-0.2%)
posts                                   -1100 B  (-0.2%)     -487 B  (-0.2%)
migrate                                 -1100 B  (-0.3%)     -703 B  (-0.6%)
import-hosted-site-flow                 -1100 B  (-0.1%)    -1000 B  (-0.3%)
domains                                 -1100 B  (-0.1%)     -673 B  (-0.1%)
checkout                                -1100 B  (-0.1%)     -281 B  (-0.1%)
theme                                    -948 B  (-0.1%)     +269 B  (+0.1%)
stats                                    -866 B  (-0.1%)     -598 B  (-0.2%)
signup                                   -824 B  (-0.3%)      -63 B  (-0.1%)
design-first-flow                        +815 B  (+2.1%)      +33 B  (+0.4%)
hundred-year-plan                        +740 B  (+1.5%)      +42 B  (+0.3%)
link-in-bio-tld-flow                     -734 B  (-0.0%)    +1447 B  (+0.2%)
start-writing-flow                       +615 B  (+1.6%)      +26 B  (+0.3%)
with-theme-assembler-flow                +563 B  (+0.3%)     +103 B  (+0.2%)
update-options-flow                      +563 B  (+0.7%)      +97 B  (+1.4%)
trial-wooexpress-flow                    +563 B  (+0.7%)      +93 B  (+1.2%)
tailored-ecommerce-flow                  +563 B  (+0.7%)     +100 B  (+1.3%)
site-setup-wg                            +563 B  (+0.4%)     +109 B  (+0.3%)
site-setup-flow                          +563 B  (+0.4%)     +109 B  (+0.4%)
site-migration-flow                      +563 B  (+0.5%)      +96 B  (+0.4%)
readymade-template-flow                  +563 B  (+0.2%)     +102 B  (+0.2%)
migration-signup                         +563 B  (+0.6%)      +97 B  (+0.9%)
migration-flow                           +563 B  (+0.5%)      +96 B  (+0.5%)
import-flow                              +563 B  (+0.6%)     +100 B  (+0.7%)
hosted-site-migration-flow               +563 B  (+0.4%)      +96 B  (+0.4%)
free-post-setup-flow                     +563 B  (+0.7%)      +93 B  (+1.4%)
entrepreneur-flow                        +563 B  (+0.2%)      +94 B  (+0.1%)
assembler-first-flow                     +563 B  (+0.2%)     +101 B  (+0.2%)
ai-assembler-flow                        +563 B  (+0.5%)      +98 B  (+0.4%)
site-overview                            -551 B  (-0.0%)     -324 B  (-0.1%)
hosting                                  -551 B  (-0.0%)     -399 B  (-0.1%)
onboarding-flow                          +547 B  (+0.6%)     +108 B  (+0.9%)
newsletter-flow                          +400 B  (+1.2%)      +55 B  (+0.6%)
plans                                    -357 B  (-0.0%)    -1303 B  (-0.3%)
newsletter-post-setup-flow               -337 B  (-0.1%)      -62 B  (-0.1%)
link-in-bio-post-setup-flow              -337 B  (-0.1%)      -62 B  (-0.1%)
write-flow                               +289 B  (+0.0%)    +1562 B  (+0.5%)
build-flow                               +289 B  (+0.0%)    +1566 B  (+0.5%)
update-design-flow                       -226 B  (-0.0%)     -194 B  (-0.0%)
stepper-user-step                        +196 B  (+0.1%)     -445 B  (-0.5%)
copy-site-flow                           +196 B  (+0.0%)     +691 B  (+0.2%)
plugins                                  -161 B  (-0.0%)     -827 B  (-0.1%)
jetpack-app                              -161 B  (-0.0%)     -827 B  (-0.6%)
hundred-year-domain                      +108 B  (+0.2%)      +29 B  (+0.2%)
reblogging-flow                           +92 B  (+1.3%)       +0 B
new-hosted-site-flow                      +92 B  (+0.8%)       +1 B  (+0.0%)
staging-site                              +67 B  (+0.0%)      +23 B  (+0.0%)
site-tools                                +67 B  (+0.0%)       -5 B  (-0.0%)
site-settings                             +67 B  (+0.0%)      +22 B  (+0.0%)
settings-performance                      +67 B  (+0.0%)      +21 B  (+0.0%)
scan                                      +67 B  (+0.0%)      +19 B  (+0.0%)
media                                     +67 B  (+0.0%)      +12 B  (+0.0%)
marketplace                               +67 B  (+0.0%)      +29 B  (+0.0%)
jetpack-cloud-plugin-management           +67 B  (+0.0%)      +25 B  (+0.0%)
hosting-features                          +67 B  (+0.0%)      +21 B  (+0.0%)
export                                    +67 B  (+0.0%)      +24 B  (+0.0%)
backup                                    +67 B  (+0.0%)      +67 B  (+0.0%)
a8c-for-agencies-sites                    +67 B  (+0.0%)      +67 B  (+0.0%)
new-hosted-site-flow-user-included        -16 B  (-0.2%)      -29 B  (-1.3%)
connect-domain                            -16 B  (-0.1%)      -28 B  (-0.4%)
site-monitoring                           -13 B  (-0.0%)      -48 B  (-0.0%)
site-logs                                 -13 B  (-0.0%)      -13 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 (~6909 bytes added 📈 [gzipped])

name                                               parsed_size           gzip_size
async-load-design-blocks                                -934 B  (-0.0%)     +461 B  (+0.1%)
async-load-calypso-my-sites-checkout-modal              +196 B  (+0.0%)     +689 B  (+0.2%)
async-load-signup-steps-plans-theme-preselected         -161 B  (-0.0%)     -827 B  (-0.6%)
async-load-signup-steps-plans                           -161 B  (-0.0%)     -827 B  (-0.6%)
async-load-signup-steps-design-picker                   +108 B  (+0.1%)      +45 B  (+0.2%)
async-load-calypso-post-editor-media-modal               +67 B  (+0.0%)      +12 B  (+0.0%)
async-load-calypso-post-editor-editor-media-modal        +67 B  (+0.0%)      +12 B  (+0.0%)
async-load-store-app-store-stats-listview                -39 B  (-0.0%)      -78 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.

fredrikekelund · 2024-12-13T15:27:52Z

@southp, I worked several rounds on this before successfully replicating the issue you described in #97225

I'd appreciate a review when you have a moment 🙏

southp · 2024-12-16T06:29:03Z

Thanks for working on this, @fredrikekelund . I will prioritize reviewing this. Sorry that I deployed overlapped clean-up before noticing your work though:

Hopefully I won't cause you too much code conflict troubles.

taipeicoder

Tested the scenarios listed in the table from the Testing Instructions section, and they work as expected 👍

southp · 2024-12-16T08:19:44Z

packages/data-stores/src/add-ons/hooks/use-add-on-purchase-status.ts

 }

+type AddOnPurchaseStatus = {
+	available: boolean;
+	hidden?: boolean;


I don't see this flag used anywhere. I suppose we can remove it :)

Good eyes! This is a leftover from a solution I pursued previously.

southp · 2024-12-16T08:24:20Z

packages/data-stores/src/add-ons/hooks/use-available-storage-add-ons.ts

+	storage?: SiteMediaStorage
+): boolean {
+	if ( ! storage ) {
+		return true;


Could you help me better understand the reason of returning true here? Without context, my interpretation is that this hook would return true when the app hasn't or can't provide it with the correct media storage data, which sounds suspicious.

Good question. storage is undefined while the /media-storage API endpoint is loading. I agree it makes more sense to handle the loading state elsewhere 👍

southp · 2024-12-16T08:31:08Z

packages/data-stores/src/add-ons/hooks/use-available-storage-add-ons.ts

+	const currentMaxStorage = storage.maxStorageBytes / Math.pow( 1024, 3 );
+	const maxStorageExcludingAddons = storage.maxStorageBytesExcludingAddons / Math.pow( 1024, 3 );
+	const existingAddOnStorage = currentMaxStorage - maxStorageExcludingAddons;
+
+	return existingAddOnStorage < quantity;


This should work most of the time here. However, I'd like to point out that expecting JS division to always produce integral values is in general not recommended, since they are double-precision floats underneath.

Also, if the sole usage of maxStorageExcludingAddons introduced in D168645-code is for doing this computation, could we simply return existingAddOnStorage from the endpoint instead? That would eliminate the floating number computation here all together as well.

I'm not sure how big of a problem this is in practice, but I take your point that we might as well return existingAddOnStorage directly from the API endpoint, since that's what we ultimately want anyway. I've updated the back-end PR (now at 168696-gh-Automattic/wpcom).

Let me know your thoughts!

southp · 2024-12-16T08:33:17Z

packages/data-stores/src/add-ons/hooks/use-available-storage-add-ons.ts

-	}, [ availableStorageUpgrade, storageAddOns ] );
+		return storageAddOns
+			.filter( ( addOn ) => addOn !== null )
+			.filter( ( addOn ): addOn is AddOnMeta =>


storageAddOns should be always in the type of [AddOnMeta] here, so filtering on the dynamic type here won't be necessary. If not, we should fix useStorageAddOns instead to fully leverage the power of type checking of TS.

My local TS didn't complain without addOn is AddOnMeta, but the CI process did, which is why I added this in 3a40d56

Typescript has (or has had) trouble with inferring types in filtered arrays. So, I'd happily remove this, but I don't think we can without upsetting the CI jobs.

That's unfortunate. In that case, let's leave it there for now.

southp · 2024-12-16T08:34:56Z

packages/data-stores/src/add-ons/hooks/use-storage-add-on-availability.ts

+	const mediaStorage = useSiteMediaStorage( { siteIdOrSlug: selectedSiteId } );
+
+	if ( addOnMeta.productSlug !== PRODUCT_1GB_SPACE ) {
+		return true;


I'm wondering if returning true here is correct, since that means the passed in addOnMeta doesn't belong to a storage add-on, but the hook considers it as an available storage add-on. Should it return false instead?

Tricky. I pondered this for a bit and landed on returning an enum. Let me know what you think, @southp

It's better now. I have a feeling that we can further generalize and simplify it as a general add-on level concept so we don't have to treat storage add-on as a special case here. However, let's not hold ourselves back and further bloat this PR :)

southp · 2024-12-16T08:38:25Z

packages/data-stores/src/add-ons/hooks/use-add-on-purchase-status.ts

+type AddOnPurchaseStatus = {
+	available: boolean;
+	hidden?: boolean;
+	text?: ReturnType< typeof i18n.translate >;


A side note that I think the user-facing text we display shouldn't have been part of this hook. The text is part of UI, so should be derived from the calypso side based on the status value.

It's not related to this PR, though; just wanted to raise it :)

Good point. This is already a meaty PR, though, so I'll leave it for another one

matticbot · 2024-12-16T09:45:54Z

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 f26d/refactor-storage-addon-availability on your sandbox.

fredrikekelund · 2024-12-16T10:27:04Z

I deployed overlapped clean-up before noticing your work though

No worries, the conflicts weren't too bad.

I've addressed your comments, @southp—this should be good for another look now!

southp

Thanks for addressing all of my comments. I really appreciate your effort not only fixing the bug but also making the code better in a balanced way 🙇🏼

fredrikekelund added 4 commits December 13, 2024 12:30

WIP - Refactor availability logic for storage add-ons

ae165be

Simplify

361d982

Simplify

cda2d1b

Fix regression

f267d23

fredrikekelund requested review from taipeicoder, rcrdortiz and a team December 13, 2024 12:32

fredrikekelund self-assigned this Dec 13, 2024

fredrikekelund requested a review from a team as a code owner December 13, 2024 12:32

fredrikekelund requested a review from southp December 13, 2024 12:32

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

Fix type errors

3a40d56

Take new API data into account

9545898

taipeicoder approved these changes Dec 16, 2024

View reviewed changes

Merge branch 'trunk' into f26d/refactor-storage-addon-availability

3b4d4ca

southp requested changes Dec 16, 2024

View reviewed changes

fredrikekelund added 4 commits December 16, 2024 09:44

Fix merge

a77e5e5

Address review comments

8e577d2

Return enum from useStorageAddOnAvailability

e58c31f

Appease TS

3b2d124

fredrikekelund added 2 commits December 16, 2024 11:00

Ensure storage quantity doesn't exceed STORAGE_LIMIT

862f5c1

Adapt front-end to back-end

fc2b76d

fredrikekelund requested a review from southp December 16, 2024 10:26

southp approved these changes Dec 17, 2024

View reviewed changes

Merge branch 'trunk' into f26d/refactor-storage-addon-availability

0232daf

fredrikekelund merged commit 3de2470 into trunk Dec 17, 2024
11 checks passed

fredrikekelund deleted the f26d/refactor-storage-addon-availability branch December 17, 2024 09:51

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 17, 2024

southp mentioned this pull request Dec 20, 2024

Refactor: simplify population of addon metas #97645

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor logic for checking storage add-on availability #97450

Refactor logic for checking storage add-on availability #97450

fredrikekelund commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited by matticbot

Loading

matticbot commented Dec 13, 2024 •

edited

Loading

fredrikekelund commented Dec 13, 2024 •

edited

Loading

southp commented Dec 16, 2024

taipeicoder left a comment

southp Dec 16, 2024

fredrikekelund Dec 16, 2024

southp Dec 16, 2024

fredrikekelund Dec 16, 2024

southp Dec 16, 2024

fredrikekelund Dec 16, 2024 •

edited

Loading

southp Dec 16, 2024

fredrikekelund Dec 16, 2024

southp Dec 17, 2024

southp Dec 16, 2024

fredrikekelund Dec 16, 2024

southp Dec 17, 2024

southp Dec 16, 2024

fredrikekelund Dec 16, 2024

matticbot commented Dec 16, 2024 •

edited

Loading

fredrikekelund commented Dec 16, 2024

southp left a comment

Refactor logic for checking storage add-on availability #97450

Refactor logic for checking storage add-on availability #97450

Conversation

fredrikekelund commented Dec 13, 2024 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Dec 13, 2024 • edited by matticbot Loading

matticbot commented Dec 13, 2024 • edited Loading

fredrikekelund commented Dec 13, 2024 • edited Loading

southp commented Dec 16, 2024

taipeicoder 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

Choose a reason for hiding this comment

fredrikekelund Dec 16, 2024 • 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

matticbot commented Dec 16, 2024 • edited Loading

fredrikekelund commented Dec 16, 2024

southp left a comment

Choose a reason for hiding this comment

fredrikekelund commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited by matticbot

Loading

matticbot commented Dec 13, 2024 •

edited

Loading

fredrikekelund commented Dec 13, 2024 •

edited

Loading

fredrikekelund Dec 16, 2024 •

edited

Loading

matticbot commented Dec 16, 2024 •

edited

Loading