Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable launch your store on trial sites #1507

Merged
merged 11 commits into from
Sep 5, 2024

Conversation

chihsuan
Copy link
Member

@chihsuan chihsuan commented Aug 29, 2024

Changes proposed in this Pull Request:

Closes #1504.

Apply all of the below for trial plans (both ecommerce and business trial plans)

@ilyasfoo I'm using wc_calypso_bridge_is_ecommerce_trial_plan() to check if the store is on a trial plan. I'm not sure if this is sufficient. Please let me know if there is a better way to check if the store is on any trial plan. 🙏

  • Hide Site visibility settings tabs
  • Hide Launch your store task
  • Disable WPCOM coming soon page override implemented in this PR
  • Disable the coming soon banner shown in frontend
  • Hide site visibility badge
  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Setting up a free eCommerce trial site for development peapX7-1D4-p2
  2. Use master branch
  3. Enable LYS feature flag (via beta tester or you can remove these lines).
  4. Go to /wp-admin/admin.php?page=wc-settings&tab=site-visibility
  5. Enable Coming Soon mode
  6. Checkout this branch
  7. Go to /wp-admin/admin.php?page=wc-settings&tab=site-visibility and confirm you're redirected to /wp-admin/admin.php?page=wc-settings
  8. Confirm Site visibility settings tabs are hidden
  9. Go to WooCommerce > Home and confirm Launch your store task is hidden
  10. Go to the frontend and confirm the coming soon banner is not shown
  11. Go to the frontend in incognito mode and confirm the LYS coming soon page is not shown
  12. Confirm site visibility badge is not shown

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@chihsuan chihsuan self-assigned this Aug 29, 2024
@chihsuan chihsuan requested review from a team, ilyasfoo and moon0326 August 29, 2024 08:48
Copy link

github-actions bot commented Aug 29, 2024

Size Change: 0 B

Total Size: 201 kB

ℹ️ View Unchanged
Filename Size
./build/53.js 1.08 kB
./build/index.css 878 B
./build/index.js 126 kB
./build/marketing.js 58.3 kB
./build/payment-gateway-suggestions.css 1.24 kB
./build/payment-gateway-suggestions.js 6.57 kB
./build/plugins.js 3.93 kB
./build/style-index.css 2.15 kB
./build/style-marketing.css 800 B

compressed-size-action

Copy link
Contributor

@moon0326 moon0326 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tested well 👍 🚀

Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilyasfoo I'm using wc_calypso_bridge_is_ecommerce_trial_plan() to check if the store is on a trial plan. I'm not sure if this is sufficient. Please let me know if there is a better way to check if the store is on any trial plan. 🙏

@chihsuan That's a good question! I tested in my business plan site and indeed this did not return true. I wrote a function that detects it, but it's only lightly tested with my business plan site:

public static function is_ecommerce_trial_plan() {
	if ( ! function_exists( 'wpcom_get_site_purchases' ) ) {
		return false;
	}

	if ( is_null( self::$is_wpcom_business_trial_plan ) ) {
		self::$is_wpcom_business_trial_plan = false;
	} else {
		return self::$is_wpcom_business_trial_plan;
	}

	$all_site_purchases      = wpcom_get_site_purchases();
	$plan_purchases          = array_filter(
		$all_site_purchases,
		function ( $purchase ) {
			return 'bundle' === $purchase->product_type;
		}
	);

	if ( 1 === count( $plan_purchases ) ) {
		// We have exactly one plan
		$plan_purchase = reset( $plan_purchases );
		if ( 'wp-bundle-hosting-trial' === $plan_purchase->billing_product_slug ) {
			self::$is_wpcom_business_trial_plan = true;
		}
	}

	return self::$is_wpcom_business_trial_plan ?? false;
}

@chihsuan
Copy link
Member Author

chihsuan commented Sep 3, 2024

That's a good question! I tested in my business plan site and indeed this did not return true. I wrote a function that detects it, but it's only lightly tested with my business plan site:

Thanks @ilyasfoo, I'll try that out and look into it more. 🙌

@chihsuan chihsuan force-pushed the update/disable-lys-on-trial-sites branch from 2025944 to b524143 Compare September 3, 2024 06:08
- Update trial plan checks
@chihsuan chihsuan force-pushed the update/disable-lys-on-trial-sites branch from e4a48f7 to 5f5352d Compare September 3, 2024 08:00
@chihsuan
Copy link
Member Author

chihsuan commented Sep 3, 2024

@ilyasfoo 👋 I added a helper function wc_calypso_bridge_is_trial_plan in 5f5352d. It checks if the plan is a trial plan by reusing is_ecommerce_trial_plan and checking the plan slug against a list of trial plans wp-bundle-hosting-trial, wp-bundle-migration-trial (See fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Snqzva%2Qcyhtvaf%2Sjcpbz%2Qovyyvat%2Scebqhpgf%2Sohfvarff%2Qgevny%2Qohaqyr.cuc%3Se%3Q29r2n341%23274%2Q277-og).

I tested the code on a business plan site, and it worked as expected. Could you please take a look at the change and possibly review it? Thanks! 🙏

Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @chihsuan! Apologies for missing this in the issue description, but we should also hide the admin bar badge 🙏

image

We should also revert the coming soon should_show_a8c_coming_soon_page to use WPCOM's coming soon when it's a trial site.

}

/**
* Check if the site has a specific plan.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate comment here

*
* @return bool True if the site has the specified plan, false otherwise.
*/
private static function has_plan( $plan, $exact_one_plan = false ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Would it be better if we support passing array for $plan? I'm unsure how performant is this check, and we have a prior art in checking for multiple plan string.

Also, I think it's better to have $exact_one_plan check to true since it's how it was done before in similar checks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Updated it here 0ec8e66

@adrianduffell
Copy link
Contributor

We should also revert the coming soon should_show_a8c_coming_soon_page to use WPCOM's coming soon when it's a trial site.

@ilyasfoo @chihsuan is the concern here that free trial users could customize the Woo template too much? I suppose they could add a checkout block or something 🤔

I'm happy with this for now, but I think it would be worth returning to when we create a newsletter signup template. I think it would be good to allow free trial users to have access to that. Instead, perhaps in Core we need to place direct protections on the checkout when coming soon mode is active.

@ilyasfoo
Copy link
Contributor

ilyasfoo commented Sep 4, 2024

@ilyasfoo @chihsuan is the concern here that free trial users could customize the Woo template too much? I suppose they could add a checkout block or something 🤔

@adrianduffell Good point, my original concern was that it was showing the live site since we were in live mode and WPCOM coming soon was bypassed. I think free trial users being able to add a checkout block is also a valid concern.

I'm happy with this for now, but I think it would be worth returning to when we create a newsletter signup template. I think it would be good to allow free trial users to have access to that. Instead, perhaps in Core we need to place direct protections on the checkout when coming soon mode is active.

I agree, let's revisit this in the future 👍

@adrianduffell
Copy link
Contributor

my original concern was that it was showing the live site since we were in live mode and WPCOM coming soon was bypassed

@ilyasfoo With #1506, would the woo coming soon mode now always be on for free trial sites? (by inheriting the WPCOM value)

@ilyasfoo
Copy link
Contributor

ilyasfoo commented Sep 5, 2024

With #1506, would the woo coming soon mode now always be on for free trial sites? (by inheriting the WPCOM value)

@adrianduffell Yes, the state will be coming soon since the site is "Unlaunched"

@chihsuan
Copy link
Member Author

chihsuan commented Sep 5, 2024

Thanks for working on this, @chihsuan! Apologies for missing this in the issue description, but we should also hide the admin bar badge 🙏

image We should also revert the coming soon `should_show_a8c_coming_soon_page` to use WPCOM's coming soon when it's a trial site.

Hey @ilyasfoo, I've pushed the changes to hide the admin bar badge and to use WPCOM's coming soon page for trial sites. Could you take a look again? Thanks. 🙏

You will need to use WooCommerce trunk or 9.3 beta to test the admin badge.

wp plugin install https://github.com/woocommerce/woocommerce/releases/download/nightly/woocommerce-trunk-nightly.zip --activate

@chihsuan chihsuan requested a review from ilyasfoo September 5, 2024 03:47
Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, @chihsuan! This is testing well, LGTM! 🚢

class-wc-calypso-bridge-dotcom-features.php Outdated Show resolved Hide resolved
@chihsuan chihsuan merged commit aea181a into master Sep 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LYS] Disable launch your store on trial sites
4 participants