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.

Sign up for GitHub

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

Jump to bottom

Add conditional check to replace launch-site task with LYS task #1509

Merged
merged 5 commits into from
Sep 6, 2024
Merged

Add conditional check to replace launch-site task with LYS task #1509

merged 5 commits into from
Sep 6, 2024

Conversation

ilyasfoo
Copy link
Contributor

@ilyasfoo ilyasfoo commented Sep 3, 2024 •
edited
Loading

Changes proposed in this Pull Request:

Closes #1497

  • Replace launch-site task with LYS task when LYS feature is enabled.
  • Refactor and merge add_tasks function to replace_tasks to consolidate logic and enable easier tasks sorting

I decided to skip adding a redirect from /wp-admin/admin.php?page=wc-admin&task=launch_site because it doesn't seem to be a simple endeavour and it only serves a tiny edge case like users having the link opened.

This PR does not handle free trial detection yet.

How to test the changes in this Pull Request:

Test LYS feature enabled

  1. Use a WPCOM atomic e-commerce site
  2. Enable LYS feature flag (via beta tester or you can set these lines to true).
  3. Go to Homescreen
  4. Observe LYS task is displayed and launch-site task is hidden
  5. Observe LYS task is displayed after Add a domain task

Test LYS feature disabled

  1. Disable LYS feature flag (via beta tester or you can set these lines to false).
  2. Go to Homescreen
  3. Observe launch-site task is displayed and LYS task is hidden
  4. Observe launch-site task is displayed after Add a domain task

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.

@ilyasfoo ilyasfoo force-pushed the update/1497-replace-launch-site-task-with-lys branch from 62d7e30 to a7507f1 Compare September 3, 2024 08:02
@ilyasfoo ilyasfoo requested review from a team, psealock and rjchow September 3, 2024 08:54
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 3, 2024 •
edited
Loading

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

ilyasfoo
ilyasfoo commented Sep 3, 2024
View reviewed changes
includes/class-wc-calypso-bridge-setup-tasks.php
*/
public function add_tasks() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entirely merged this function into replace_tasks

ilyasfoo
ilyasfoo commented Sep 3, 2024
View reviewed changes
includes/class-wc-calypso-bridge-setup-tasks.php
if ( ! Features::is_enabled( 'launch-your-store' ) ) {
require_once WC_CALYPSO_BRIDGE_PLUGIN_PATH . '/includes/tasks/class-wc-calypso-task-launch-site.php';
$launch_site_task = new \Automattic\WooCommerce\Admin\Features\OnboardingTasks\Tasks\LaunchSite( $lists['setup'] );
$lists['setup']->tasks[$index + 1] = $launch_site_task;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$index + 1 because index was bumped up since we added domain task above

@psealock
Copy link
Contributor

psealock commented Sep 4, 2024

Test LYS feature disabled
Disable LYS feature flag (via beta tester or you can set these lines to true).

@ilyasfoo Can you explain this further? Do you mean leave them uncommented or set $features['launch-your-store'] = true;?

If the feature flag is set to true, then I get the same results as the first section, which makes sense.

image

If I leave that code as-is, I get the following:

image

@ilyasfoo
Copy link
Contributor Author

ilyasfoo commented Sep 4, 2024

@ilyasfoo Can you explain this further? Do you mean leave them uncommented or set $features['launch-your-store'] = true;?
@psealock Sorry that was a copy-pasta error. It should be leave the code as-is, fixed it in description

chihsuan
chihsuan approved these changes Sep 5, 2024
View reviewed changes
Copy link
Member

@chihsuan chihsuan 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! 👍

@psealock
Copy link
Contributor

psealock commented Sep 6, 2024

Observe launch-site task is displayed after Add a domain task

I'm seeing that I've already launched

image

So I follow the link and go back coming soon mode, but the task list is the same. Should I be seeing something else? I'm wondering if there is a sync issue.

image

@ilyasfoo
Copy link
Contributor Author

ilyasfoo commented Sep 6, 2024

So I follow the link and go back coming soon mode, but the task list is the same. Should I be seeing something else? I'm wondering if there is a sync issue.

@psealock Good question! This is expected.

The launch-site task checks for launch status, not coming soon status. WPCOM has a separate launch status (launch-status option) and coming soon status (blog_public, wpcom_public_coming_soon and wpcom_coming_soon options)

psealock
psealock approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@psealock psealock 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 the clarification @ilyasfoo, this is all working well then. 🚢

@ilyasfoo ilyasfoo merged commit 132f81c into master Sep 6, 2024
5 checks passed
@ilyasfoo ilyasfoo mentioned this pull request Sep 10, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chihsuan chihsuan chihsuan approved these changes

@psealock psealock psealock approved these changes

@rjchow rjchow Awaiting requested review from rjchow rjchow was automatically assigned from Automattic/ghidorah

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[LYS] Replace launch-site task with LYS
3 participants
@ilyasfoo @psealock @chihsuan