Storage Add-On: Fix spotlight plan CTA upgrade behavior #84289

jeyip · 2023-11-17T11:27:31Z

Related to https://github.com/Automattic/martech/issues/2418

Proposed Changes

Adds an undefined value check for a given selected storage option when the page is first initialized

Testing Instructions

I spent quite a bit of time attempting to replicate the original bug ( different browsers, throttled connections, mobile vs. desktop etc. ). I wasn't able to successfully reproduce it which means that I can't 100% confirm that the issue is fixed. Given the reported behavior though, I do believe that the problem should be addressed with this PR.

With that in mind, we can still smoke test intended behavior to ensure that storage add-on dropdown functionality is still correct.

Check out this branch
Visit /start/plans
Ensure that the storage add-on dropdown continues to behave properly
- Add-on prices are still displayed in the dropdown
- A storage add-on upgrade can be selected
- Storage add-on is correctly added to the cart at checkout

Visit Calypso admin /plans
Confirm, again, that the storage add-on dropdown continues to behave properly ( but don't purchase the storage add-on upgrade )

Upgrade site plan to either business or commerce
Confirm that the add-on upgrade CTA in the spotlight plan behaves as expected

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md 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-ajp-p2)?

github-actions · 2023-11-17T11:27:46Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

matticbot · 2023-11-17T11:28:41Z

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

Sections (~10 bytes added 📈 [gzipped])

name                  parsed_size           gzip_size
update-design-flow          +21 B  (+0.0%)      +10 B  (+0.0%)
plugins                     +21 B  (+0.0%)      +10 B  (+0.0%)
plans                       +21 B  (+0.0%)      +10 B  (+0.0%)
link-in-bio-tld-flow        +21 B  (+0.0%)      +10 B  (+0.0%)
jetpack-app                 +21 B  (+0.0%)      +10 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 (~10 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected        +21 B  (+0.0%)      +10 B  (+0.0%)
async-load-signup-steps-plans                          +21 B  (+0.0%)      +10 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.

southp · 2023-11-24T02:22:42Z

client/my-sites/plans-grid/components/actions.tsx

-	const nonDefaultStorageOptionSelected = defaultStorageOption !== selectedStorageOptionForPlan;
+	const nonDefaultStorageOptionSelected =
+		selectedStorageOptionForPlan !== undefined ||
+		selectedStorageOptionForPlan !== defaultStorageOption;


I'm also having trouble reproducing the original issue. However, shouldn't this be selectedStorageOptionForPlan !== undefined && selectedStorageOptionForPlan !== defaultStorageOption; ? Currently it reads like as long as a storage option is picked and then nonDefaultStorageOptionSelected will be true, which doesn't sound like what we intend here :)

Ah you're absolutely right.

I drafted this PR relatively quickly during the tail end of the dotcom meetup, but I'll take a moment, re-review the changes, and make sure things make more sense. Thanks for pointing this out @southp 👍

chriskmnds · 2023-11-24T09:54:53Z

@jeyip which issue are you not able to reproduce? Not sure if I'm doing something wrong, but Case 2 mentioned in the linked issue (so from my comment #83891 (review)) still happens for me with this image.

Purchased a new Business plan on a fresh site
I can see Upgrade button for the Spotlight plan when 50GB (default) value is selected.
Clicking on it does nothing (and probably fires the same event from Storage Add-Ons: Track spotlight plan card add-on upgrade CTA #83891)

Screen.Recording.2023-11-24.at.11.50.48.AM.mov

and if I purchase the 100GB upgrade on the same site & plan, this is how it behaves now:

Screen.Recording.2023-11-24.at.11.52.04.AM.mov

jeyip · 2023-12-01T16:53:23Z

@jeyip which issue are you not able to reproduce? Not sure if I'm doing something wrong, but Case 2 mentioned in the linked issue (so from my comment #83891 (review)) still happens for me with this image.

Purchased a new Business plan on a fresh site
I can see Upgrade button for the Spotlight plan when 50GB (default) value is selected.
Clicking on it does nothing (and probably fires the same event from #83891)

Thanks for the testing instructions 🙏 I was unable to replicate case 1 and case 2 on trunk. If you're able to reproduce it that's super helpful, and it points to a lapse in some logic that's written.

Reading things back, I noticed that you were using the live image. I also see that you're testing with a fresh site. Lemme try again with the steps you outlined

Edit:

I created a fresh site with a business plan using calypso live and am still unable to reproduce the issue with this branch. 🤔
I attempted to replicate the problem on the other branch where you originally spotted the issue Storage Add-Ons: Track spotlight plan card add-on upgrade CTA #83891 (review). No luck
Taking a look at the existing logic and trying to work backwards from there to see what gaps might exist 🕵️ Also tried different currencies without luck ( since I noticed your GIF uses IDR )

jeyip · 2023-12-01T22:18:31Z

@chriskmnds still unable to reproduce, but I'll keep testing different conditions and other ways that my testing environment is might be different than yours. While I'm doing this, would you be able to do me a favor and log out the following code in L262 of my-sites/plans-grid/components/actions.tsx in your local dev environment while reproducing the bug?

console.log( selectedStorageOptionForPlan, defaultStorageOption );

wp-calypso/client/my-sites/plans-grid/components/actions.tsx

Lines 254 to 262 in 74f4d92

    
           const canPurchaseStorageAddOns = storageAddOnsForPlan?.some( 
        
           	( storageAddOn ) => ! storageAddOn?.purchased && ! storageAddOn?.exceedsSiteStorageLimits 
        
           ); 
        
           const storageAddOnCheckoutHref = storageAddOnsForPlan?.find( 
        
           	( addOn ) => 
        
           		selectedStorageOptionForPlan && addOn?.featureSlugs?.includes( selectedStorageOptionForPlan ) 
        
           )?.checkoutLink; 
        
           const nonDefaultStorageOptionSelected = defaultStorageOption !== selectedStorageOptionForPlan;

After reviewing your GIFs, it seems like the only way we'd see the unexpected behavior is if wpcom-plans-UI state mistakenly believes we've clicked on a storage add-on and updated selectedStorageOptionForPlan, when in reality, as you've demonstrated in your videos, we haven't.

southp · 2023-12-08T10:54:59Z

I've spent some time investigating this today. Even though I didn't get anything concrete yet, I noticed that the state of selected storage add-on option is not restrained within the selected site. i.e. if I select 100G for the Business plan on my site A, then I'll see 100G is selected when I acceess /plans on my site B or in whereever the pricing grid is shown. This might not directly relate to this, but I have a gut feeling that it's a risky behavior.

I'll see if I can find something more next week if no one beat me to it.

chriskmnds · 2023-12-08T11:09:03Z

I've spent some time investigating this today. Even though I didn't get anything concrete yet, I noticed that the state of selected storage add-on option is not restrained within the selected site. i.e. if I select 100G for the Business plan on my site A, then I'll see 100G is selected when I acceess /plans on my site B or in whereever the pricing grid is shown. This might not directly relate to this, but I have a gut feeling that it's a risky behavior.

I'll see if I can find something more next week if no one beat me to it.

Indeed. I have a similar suspicion. I haven't got around to testing but will try and get to it in a bit

chriskmnds · 2023-12-08T12:48:20Z

While I'm doing this, would you be able to do me a favor and log out the following code in L262 of my-sites/plans-grid/components/actions.tsx in your local dev environment while reproducing the bug?

@jeyip this is what is being logged when I reach the following state in the UI:

I believe I can reproduce the above consistently right now, with the following steps:

new site, upgrade to Business
purchase 100GB storage add-on
cancel and remove the add-on subscription completely

The following happens when visiting /plans after and interacting with the dropdown:

Screen.Recording.2023-12-08.at.2.33.52.PM.mov

and this is what happens when I reload the page after that point (with the same console logger) - it switches from "manage plan" to "upgrade" (the wrong state):

Screen.Recording.2023-12-08.at.2.38.34.PM.mov

I am not sure if it's related to what @southp mentioned above. That part seems critical too, although I wonder if it would be as pervasive given we don't persist the respective data-store property to local storage (so it might only occur when switching sites within page session, I think?)

jeyip · 2023-12-08T16:55:23Z

I've spent some time investigating this today. Even though I didn't get anything concrete yet, I noticed that the state of selected storage add-on option is not restrained within the selected site. i.e. if I select 100G for the Business plan on my site A, then I'll see 100G is selected when I acceess /plans on my site B or in whereever the pricing grid is shown. This might not directly relate to this, but I have a gut feeling that it's a risky behavior.

I'll see if I can find something more next week if no one beat me to it.

Indeed. I have a similar suspicion. I haven't got around to testing but will try and get to it in a bit

Ahhh this makes a lot of sense. I'll see if I can't explore this further 🤔

Thanks for the thoughts @southp + @chriskmnds!

jeyip · 2023-12-13T00:01:04Z

It's toward the end of my day, but I'm returning to this now. Hopefully will have things pinpointed some time tomorrow 🕵️

jeyip · 2023-12-15T22:32:42Z

Pausing focus on this for the time being to help shepherd the dropdown term selector experiment https://github.com/Automattic/growth-foundations/issues/296

southp · 2024-03-15T13:04:58Z

@jeyip Could you help me confirm if this is still relevant? I have a feeling that it might be a combination of some of the issues in #85773 :)

jeyip · 2024-03-19T19:07:33Z

Could you help me confirm if this is still relevant? I have a feeling that it might be a combination of some of the issues in #85773 :)

Did a cursory review and this PR seems irrelevant now. I'll close it for the time being and we can reopen if we understand things otherwise.

jeyip self-assigned this Nov 18, 2023

jeyip added the Storage Addon label Nov 18, 2023

jeyip marked this pull request as ready for review November 18, 2023 08:59

jeyip requested a review from a team as a code owner November 18, 2023 08:59

jeyip requested a review from southp November 18, 2023 08:59

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

jeyip changed the title ~~Storage Add-On: Check for an undefined selectedStorageOption on initialization~~ Storage Add-On: Fix spotlight plan CTA upgrade behavior Nov 18, 2023

southp reviewed Nov 24, 2023

View reviewed changes

chriskmnds mentioned this pull request Nov 24, 2023

Plans Grid Refactor: Move action override related props to the relevant data structure #83976

Merged

7 tasks

jeyip added 2 commits December 1, 2023 11:01

Check for an undefined selectedStorageOption on initialization

bf60996

Fix non default storage option selected condition

b140e41

jeyip force-pushed the fix/storage-add-on-upgrade-CTA-behaviour branch from 9ea177f to b140e41 Compare December 1, 2023 19:01

jeyip closed this Mar 19, 2024

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 Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage Add-On: Fix spotlight plan CTA upgrade behavior #84289

Storage Add-On: Fix spotlight plan CTA upgrade behavior #84289

jeyip commented Nov 17, 2023 •

edited

Loading

github-actions bot commented Nov 17, 2023 •

edited by matticbot

Loading

matticbot commented Nov 17, 2023 •

edited

Loading

southp Nov 24, 2023

jeyip Nov 24, 2023

chriskmnds commented Nov 24, 2023

jeyip commented Dec 1, 2023 •

edited

Loading

jeyip commented Dec 1, 2023 •

edited

Loading

southp commented Dec 8, 2023

chriskmnds commented Dec 8, 2023

chriskmnds commented Dec 8, 2023 •

edited

Loading

jeyip commented Dec 8, 2023 •

edited

Loading

jeyip commented Dec 13, 2023

jeyip commented Dec 15, 2023

southp commented Mar 15, 2024

jeyip commented Mar 19, 2024

Storage Add-On: Fix spotlight plan CTA upgrade behavior #84289

Storage Add-On: Fix spotlight plan CTA upgrade behavior #84289

Conversation

jeyip commented Nov 17, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Nov 17, 2023 • edited by matticbot Loading

matticbot commented Nov 17, 2023 • edited Loading

southp Nov 24, 2023

Choose a reason for hiding this comment

jeyip Nov 24, 2023

Choose a reason for hiding this comment

chriskmnds commented Nov 24, 2023

jeyip commented Dec 1, 2023 • edited Loading

jeyip commented Dec 1, 2023 • edited Loading

southp commented Dec 8, 2023

chriskmnds commented Dec 8, 2023

chriskmnds commented Dec 8, 2023 • edited Loading

jeyip commented Dec 8, 2023 • edited Loading

jeyip commented Dec 13, 2023

jeyip commented Dec 15, 2023

southp commented Mar 15, 2024

jeyip commented Mar 19, 2024

jeyip commented Nov 17, 2023 •

edited

Loading

github-actions bot commented Nov 17, 2023 •

edited by matticbot

Loading

matticbot commented Nov 17, 2023 •

edited

Loading

jeyip commented Dec 1, 2023 •

edited

Loading

jeyip commented Dec 1, 2023 •

edited

Loading

chriskmnds commented Dec 8, 2023 •

edited

Loading

jeyip commented Dec 8, 2023 •

edited

Loading