Add country validation to subscription inclusion #7777

octaedro · 2021-10-08T19:30:21Z

This is a follow-up to PR 7734 to add a country validation.

The Subscription inclusion will be visible only for US stores and this PR adds the changes necessary to accomplish that.

The requested changes here also required a small refactor to how we handle the product types.

No changelog is necessary.

Detailed test instructions:

Checkout this branch.
Deactivate and delete WooCommerce Payments if you have it installed.
Go to step one of the store profiler and select France (or any country other than the US) as the store Country / Region.
Go to step three of the store profiler (Product Types) and verify Subscriptions is shown as a paid extension (with a price chip).
Check Subscriptions and continue with the OBW.
Go back to the Home screen by pressing Skip setup store details in the step one of the store profiler. Check that the task item Add Subscriptions to my store is visible in the setup task list.
Press Add my products in the setup task list.
Select Start with a template. Verify that the option Subscription product is not visible in the popup.
Go to step one of the store profiler and select US as the store Country / Region.
Verify Subscriptions is shown as free (without a price chip). Also, verify that the text

The following extensions will be added to your site for free: WooCommerce Payments. An account is required to use this feature

is visible at the bottom when WooCommerce Payments is not installed.

Check Subscriptions and pressContinue and verify that the WooCommerce Payments plugin is installed and activated and it's not shown in the Free features list

Verify that the WooCommerce Payments plugin is being shown in the Free features list when the store country is other than the US.
Go back to the Home screen by pressing Skip setup store details in step one of the store profiler. Check that the task item Add Subscriptions to my store is not visible in the setup task list. It should be visible if the store is from any country other than the US.
Press Add my products in the setup task list.
Select Start with a template. Verify that the option Subscription product is visible in the popup (verify that this is not being shown when the store is from a country other than the US)

UPDATE

Verify that this error is not happening anymore.

louwie17

@octaedro Things were testing well, and overall the changes look great!
I just left a couple small comments, and one question in relation to the getTaskLists invalidation call, otherwise close to ready 👍

louwie17 · 2021-10-12T18:47:22Z

client/homescreen/activity-panel/index.js

 	useEffect( () => {
 		if ( panelsData.isTaskListHidden !== undefined ) {
+			if ( ! panelsData.isTaskListHidden ) {
+				invalidateResolutionForStoreSelector( 'getTaskLists' );


This seems to cause the getTaskLists request to be triggered twice when loading the homescreen for the first time.
Was there a specific reason for adding this?

I added the invalidateResolutionForStoreSelector here thinking about the following use case:

The user sets US as the store's country in step one of the OBW; then they select Subscriptions in step three (Product Types) and press Continue, after that they go to the first step and press Skip setup store details.
They won't be able to see the task item Add Subscriptions to my store in the setup task list.

Then, they go back to the OBW, select another country, and press Skip setup store details. The task item Add Subscriptions to my store should be visible in the setup task list.

If we don't invalidate the resolution the user will see the old list instead of the updated one until they refresh the page.

Do you think it would be better for us to add the resolution invalidation in the OBW instead of here?

Do you think it would be better for us to add the resolution invalidation in the OBW instead of here?

Yeah I think doing it in the OBW is a better idea, either when the user finishes it or skips it.
This might be the easiest, I was thinking otherwise to only do it when the country changes, but that's not correct given some tasks depend on the other selected options (like subscriptions as you mentioned).

Sounds good to me. I moved the invalidateResolutionForStoreSelector in the commit 83266f1

louwie17 · 2021-10-12T18:58:55Z

packages/data/src/onboarding/selectors.ts

 };

+export const getProductTypes = ( state: OnboardingState ): Product[] => {
+	return state.productTypes || [];


Given this is in a selector that gets memoized, could you define the empty array as a constant outside of this function and return that?

const EMPTY_ARRAY = []; export const getProductTypes = ( state: OnboardingState ): Product[] => { return state.productTypes || EMPTY_ARRAY; }

Good idea! I added this change in the commit 02ee720

louwie17 · 2021-10-12T19:00:47Z

packages/data/src/onboarding/selectors.ts

+export type Product = {
+	default: boolean;
+	label: string;
+	product: string;


product seems to be a number.
Both default and product are also optional, so just add ? before :

Nice catch! I added this in the commit 5a53635

octaedro · 2021-10-12T21:35:50Z

Thank you @louwie17 for your review.
I addressed the suggested changes. Could you give this PR another look?

louwie17

Thanks for the changes @octaedro, it looks great and still tested well 😄

louwie17 · 2021-10-12T23:31:01Z

@octaedro @mattsherman these changes do look good, but it is a rather large change to be cherry-picking into our RC version. So just wanted to ask if it is necessary to cherry-pick this? and if so, if we could forgo any of the refactored changes? It might require a couple more thorough tests if we do go ahead.

mattsherman · 2021-10-13T02:20:39Z

@octaedro @mattsherman these changes do look good, but it is a rather large change to be cherry-picking into our RC version.

It is quite a large change. Much larger than I would have expected to add country validation!

So just wanted to ask if it is necessary to cherry-pick this?

Yes, it is necessary, as the subscription support needs to be in there in this release, and it needs to be limited to US only. Unfortunately, this requirement was not known initially when the support was added.

and if so, if we could forgo any of the refactored changes? It might require a couple more thorough tests if we do go ahead.

@octaedro @louwie17 What other areas than this particular scenario should we test?

mattsherman

Tested and works as expected. Code looks good, just a minor comment or two that doesn't need to be addressed in this PR, given that it has already been tested and reviewed and our timeline.

This PR is quite large to cherry-pick, but given the requirements and timeline, we will do so.

mattsherman · 2021-10-13T15:04:40Z

...le-wizard/steps/business-details/flows/selective-bundle/selective-extensions-bundle/index.js

 import './style.scss';
 import sanitizeHTML from '~/lib/sanitize-html';
 import { setAllPropsToValue } from '~/lib/collections';
+import { getCountryCode } from '../../../../../../dashboard/utils';


This feels really fragile (using so many relative parent paths). Is there no other way to reference this that doesn't use parent paths?

It doesn't need to be fixed in this PR, since it is working now and has been tested/reviewed, but it would be good to cleanup in a follow up PR.

mattsherman · 2021-10-13T15:07:13Z

client/profile-wizard/steps/product-types/index.js

 * Internal dependencies
 */
 import { createNoticesFromResponse } from '~/lib/notices';
+import { getCountryCode } from '../../../dashboard/utils';


Same comment as above.

Co-authored-by: Fernando Marichal <[email protected]>

…erce-admin#7777) * Add country validation * Add OnboardingProductTypes * Add OnboardingProductTypes * Add country validation to product task * Add `productTypes` data handling * Add country validation and new productTypes handling * Fix to get `productTypes` from a SSOT * Add `invalidateResolution` for `getTaskLists` * Fixed testing instructions * Fix `isTaskListHidden` issue * Fixed product type * Added constant `EMPTY_ARRAY` to `selectors.ts` * Fixed constant `EMPTY_ARRAY` * Moved `invalidateResolutionForStoreSelector` into OBW * Updated testing instructions * Updated testing instructions * Fixed testing instructions Co-authored-by: Fernando Marichal <[email protected]>

octaedro added 8 commits October 8, 2021 15:39

Add country validation

504fbd9

Add OnboardingProductTypes

fb90c21

Add OnboardingProductTypes

d025468

Add country validation to product task

3c35c8a

Add productTypes data handling

ee3cbf3

Add country validation and new productTypes handling

18ebf95

Fix to get productTypes from a SSOT

8a3e9f4

Add invalidateResolution for getTaskLists

4d2c243

octaedro added the status: in progress label Oct 8, 2021

octaedro self-assigned this Oct 8, 2021

probot-autolabeler bot added the Packages label Oct 8, 2021

github-actions bot added the needs changelog entry label Oct 8, 2021

james-allan mentioned this pull request Oct 11, 2021

Switch on WCPay Subscriptions feature flag after onboarding completion Automattic/woocommerce-payments#3086

Closed

github-actions bot removed the needs changelog entry label Oct 11, 2021

octaedro added 2 commits October 11, 2021 10:56

Fixed testing instructions

b40daf3

Fix isTaskListHidden issue

2b91e18

octaedro changed the title ~~Update/7445 add country validation~~ Add country validation to subscription inclusion Oct 11, 2021

octaedro added status: needs review and removed status: in progress labels Oct 11, 2021

octaedro requested a review from a team October 11, 2021 18:22

mattsherman added this to the 2.8.0 milestone Oct 11, 2021

mattsherman added the release: cherry-pick label Oct 11, 2021

octaedro mentioned this pull request Oct 12, 2021

[GlobalStep - 2.8.0 Beta-2] "WooCommerce Payments" extension is visible under extensions suggestions on "Free Features" screen when "Subscriptions Inclusion" feature is enabled. #7775

Closed

3 tasks

louwie17 reviewed Oct 12, 2021

View reviewed changes

octaedro added 4 commits October 12, 2021 17:29

Fixed product type

5a53635

Added constant EMPTY_ARRAY to selectors.ts

62edf8f

Fixed constant EMPTY_ARRAY

02ee720

Moved invalidateResolutionForStoreSelector into OBW

83266f1

louwie17 approved these changes Oct 12, 2021

View reviewed changes

louwie17 added status: ready to merge and removed status: needs review labels Oct 12, 2021

octaedro added 2 commits October 13, 2021 12:03

Updated testing instructions

def6075

Updated testing instructions

01c1e32

mattsherman approved these changes Oct 13, 2021

View reviewed changes

Fixed testing instructions

9e53d4e

octaedro merged commit 6e8a98a into main Oct 13, 2021

octaedro deleted the update/7445_add_country_validation branch October 13, 2021 16:15

octaedro mentioned this pull request Oct 13, 2021

Enable Subscriptions inclusion for every environment #7799

Merged

mattsherman added status: cherry picked and removed release: cherry-pick labels Oct 13, 2021

mattsherman added a commit that referenced this pull request Oct 13, 2021

Add country validation to subscription inclusion (#7777)

aafa5a4

Co-authored-by: Fernando Marichal <[email protected]>

This was referenced Oct 17, 2021

Add tests to Subscriptions inclusion #7804

Merged

Add country validation for old task list #7821

Closed

Add country validation to subscription inclusion #7777

Add country validation to subscription inclusion #7777

Uh oh!

Conversation

octaedro commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Detailed test instructions:

UPDATE

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octaedro commented Oct 12, 2021

Uh oh!

louwie17 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louwie17 commented Oct 12, 2021

Uh oh!

mattsherman commented Oct 13, 2021

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

octaedro commented Oct 8, 2021 •

edited

Loading

louwie17 left a comment •

edited

Loading