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

Ecommerce Admin Menu: Apply changes to the site with nav redesign #1449

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

arthur791004
Copy link
Contributor

@arthur791004 arthur791004 commented Mar 7, 2024

Changes proposed in this Pull Request:

Closes https://github.com/Automattic/dotcom-forge/issues/5919

The feature, Ecommerce Admin Menu, relies on the Atomic_Admin_Menu class from Jetpack and hook the new navigation class to the jetpack_admin_menu_class filter. However, both class and filter won't be loaded after the nav redesign. So, this PR proposes to restore the Ecommerce Admin Menu when the nav redesign is enabled.

Before After
Top Level image image
Jetpack image image
Woo image image
Tools image image
Settings image image

How to test the changes in this Pull Request:

  1. Create a new WoA site with Entrepreneur plan
  2. Build and sync changes from your local to the WoA site
    npm install
    npm run build
    rsync -azP --delete --delete-after --exclude={'.config','.idea','.git','node_modules/'} ${WCCALYPSOBRIDGE_LOCAL_DIR} ${REMOTE_SSH_HOST}:/srv/htdocs/wp-content/mu-plugins/wpcomsh/vendor/automattic/wc-calypso-bridge/
  3. Enable the nav redesign on your site, e.g.: turning on your proxy or seting the wpcom_classic_early_release option to 1 on your site
  4. Make sure the menu works as expected
    • You can see the "My Home" menu, and it displays the "Welcome to your Woo Express store" page
    • You can see the
    • You can see the "Orders" menu
    • The "WooCommerce" menu is renamed to "Extensions"
      • The submenu "Orders" is moved to the top level
      • The submenu "Customers" is moved to the top level
      • The submenu "Extensions" is renamed to "Discover"
      • The submenu "Home" is hidden
      • The submenu "Settings" is moved to "Settings > WooCommerce" menu, and it's right before "Settings > General"
      • The submenu "Status" is moved to "Tools > WooCommerce Status"
    • The "Settings > Jetpack" menu is removed
    • The "Jetpack > Search" menu is removed
    • The "Jetpack > Akismet" menu is moved to "Settings > Anti-Spam"
  5. Follow peapX7-1D4-p2 to create another WoA site with eCommerce trial plan. Or mark your site as a free trial site by mocking the return value of wc_calypso_bridge_is_ecommerce_trial_plan to true
  6. Make sure the following items work as expected
    • The "Plugins" displays the "WC Plugins Upgrade" page
    • The "Feedback" menu is moved to "Jetpack > Feedback"
  7. Disable the nav redesign on your site, e.g.: turning off your proxy or unsetting the wpcom_classic_early_release option
  8. Review the menu items, and it should be the same as before

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.

Copy link

github-actions bot commented Mar 7, 2024

Size Change: 0 B

Total Size: 197 kB

ℹ️ View Unchanged
Filename Size
./build/53.js 1.05 kB
./build/index.css 883 B
./build/index.js 122 kB
./build/marketing.js 58 kB
./build/payment-gateway-suggestions.css 1.25 kB
./build/payment-gateway-suggestions.js 6.46 kB
./build/plugins.js 3.92 kB
./build/style-index.css 2.15 kB
./build/style-marketing.css 805 B

compressed-size-action

@arthur791004 arthur791004 force-pushed the feat/wooexpress-nav-redesign branch from 28e4c9a to ec34a9d Compare March 7, 2024 11:51
@arthur791004 arthur791004 changed the title WIP Ecommerce Admin Menu: Get rid of Jetpack Atomic_Admin_Menu dependency Ecommerce Admin Menu: Apply changes to the site with nav redesign Mar 7, 2024
@arthur791004 arthur791004 force-pushed the feat/wooexpress-nav-redesign branch from ec34a9d to 42dafad Compare March 7, 2024 13:19
@arthur791004 arthur791004 self-assigned this Mar 7, 2024
@arthur791004 arthur791004 force-pushed the feat/wooexpress-nav-redesign branch from f42e472 to da1a6c8 Compare March 8, 2024 06:42
@PanosSynetos
Copy link
Contributor

Hey @arthur791004 , thanks for working on this fix. I've added @xristos3490 from SomewhereWarm as a reviewer, since he developed the menu; maybe he would have something to add.

As for testing steps, I'd recommend adding testing steps with disabled nav redesign to double check there's no regression.

@PanosSynetos PanosSynetos requested a review from xristos3490 March 8, 2024 09:56
@arthur791004
Copy link
Contributor Author

Thank you! I added the step to disable the nav redesign and review whether the menu items are the same as before 🙂

Copy link
Contributor

@lsl lsl 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 fixes, ran out of time to test but the changes lgtm.

Copy link

@miksansegundo miksansegundo left a comment

Choose a reason for hiding this comment

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

The changes apply to the Entrepreneur plan but not to the Creator plan with the Woo plugin installed.

Can you confirm this is expected?

Entrepreneur Creator + Woo plugin
Screenshot 2567-03-13 at 18 00 22 Screenshot 2567-03-13 at 17 57 03

@miksansegundo
Copy link

miksansegundo commented Mar 13, 2024

I guess the "Woo Extensions" menu is expected to expand when "My Home" is active because it was originally its parent.

However, it feels odd to have two menu options active at the same time:

Screenshot 2567-03-13 at 18 19 28

Also, there is an issue with "My Home" menu because it has another "My Home" as a submenu, and the badge background is not noticeable.

Screenshot 2567-03-13 at 19 04 01

@miksansegundo
Copy link

miksansegundo commented Mar 13, 2024

I'm on the Entrepreneur plan and mocked the function wc_calypso_bridge_is_ecommerce_trial_plan to return true.

❌ The "Plugins" displays the "WC Plugins Upgrade" page

I'm not allowed to access it. Can you confirm this? I verified I could access "Plugins" when I stopped mocking that function.

Screenshot 2567-03-13 at 18 32 52

✅ "Feedback" is moved to "Jetpack > Feedback"

Screenshot 2567-03-13 at 18 29 42

@miksansegundo
Copy link

❌ Disable the nav redesign on your site, e.g.: turning off your proxy or unsetting the wpcom_classic_early_release option

It seems that the option wpcom_classic_early_release and the proxy check do not work as a flag in this case.

After I updated wpcom_classic_early_release to 0 and disabled the proxy, I still see the changes applied.

Screenshot 2567-03-13 at 19 15 16

When I switch from "classic" to the "default" admin interface, I see two "My Home" submenus.

Screenshot 2567-03-13 at 19 20 42

@candy02058912
Copy link

❌ The "Plugins" displays the "WC Plugins Upgrade" page

I'm not allowed to access it. Can you confirm this? I verified I could access "Plugins" when I stopped mocking that function.

I did:

function wc_calypso_bridge_is_ecommerce_trial_plan() {
	return true;
}

The URL is https://:site/wp-admin/admin.php?page=wc-admin&path=%2Fplugins-upgrade and is showing this as well:

not allowed

@candy02058912
Copy link

candy02058912 commented Mar 13, 2024

✅ Disabling the proxy brings back nav unification

no proxy

Copy link
Member

@xristos3490 xristos3490 left a comment

Choose a reason for hiding this comment

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

Hey, @arthur791004! Great work! 💪

Here are the steps I took to test this:

  • Created a fresh Entrepreneur store and set the wpcom_classic_early_release option to 1.
  • Synced this PR.
  • Everything works as expected. ✅ cc @candy02058912 I couldn't replicate the issue with the Plugins landing page; for me, it shows up as expected (using the exact same mockup on the free trial conditional.) 🤔

Here's a screenshot:
FEfkBt.png

However, I went to the Hosting Configuration panel and changed the "Admin interface style" to "Default". As a result, I noticed a few issues. I'm not sure if this is relevant or if you plan to remove this view completely. If not, I think we should address these issues. Here's what I found:

  • The "My home" item appears twice.
  • The Plugins item appears twice (when I mock the trial conditional; both items are working tho.)
  • The "Orders" menu item bleeds into the Calypso's menu.

Moreover, from what is worth, I'm seeing the same results using an old "Woo Express: Performance" store.

Screenshots:

8rO3VC.png PMrE7v.png
  • (Bonus, and most probably unrelated to this PR) The Calypso's submenu highlighting shows this blue background instead of green text.

Copy link

@candy02058912 candy02058912 left a comment

Choose a reason for hiding this comment

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

Seems like we're almost there (regarding from the other comments as well) 👍

Also tested after activating WooPayments, the menu is showing in the correct spot.
The advanced settings are also reflected.

setting

@arthur791004
Copy link
Contributor Author

arthur791004 commented Mar 14, 2024

@miksansegundo

The changes apply to the Entrepreneur plan but not to the Creator plan with the Woo plugin installed.

Yes. The menu is only for the sites with an eCommerce plan (Entrepreneur). See

if ( ! wc_calypso_bridge_has_ecommerce_features() ) {
.

I guess the "Woo Extensions" menu is expected to expand when "My Home" is active because it was originally its parent. However, it feels odd to have two menu options active at the same time:

Interesting...it looks good on my side.

My Home is active Extension is active
image image

Also, there is an issue with "My Home" menu because it has another "My Home" as a submenu, and the badge background is not noticeable.

I checked on my WooCommerce site without nav redesign and found it's an existing issue related to the Modern color scheme. We can open another PR to resolve it 🙂

I'm on the Entrepreneur plan and mocked the function wc_calypso_bridge_is_ecommerce_trial_plan to return true.
❌ The "Plugins" displays the "WC Plugins Upgrade" page
I'm not allowed to access it. Can you confirm this? I verified I could access "Plugins" when I stopped mocking that function.

Hmmm...it's weird. It works on my site 😂

❌ Disable the nav redesign on your site, e.g.: turning off your proxy or unsetting the wpcom_classic_early_release option
It seems that the option wpcom_classic_early_release and the proxy check do not work as a flag in this case.
After I updated wpcom_classic_early_release to 0 and disabled the proxy, I still see the changes applied.

It's the correct behavior. The menu should be applied to the WooCommerce site

When I switch from "classic" to the "default" admin interface, I see two "My Home" submenus.

I'll take a look at this issue.

@arthur791004
Copy link
Contributor Author

The URL is https://:site/wp-admin/admin.php?page=wc-admin&path=%2Fplugins-upgrade and is showing this as well:

@candy02058912 It's weird. I cannot reproduce this issue either 🥲

@arthur791004
Copy link
Contributor Author

However, I went to the Hosting Configuration panel and changed the "Admin interface style" to "Default". As a result, I noticed a few issues. I'm not sure if this is relevant or if you plan to remove this view completely. If not, I think we should address these issues. Here's what I found:

@xristos3490 Thanks for the review! I made 2be0730 to address the following issues:

✅ The "My home" item appears twice.
✅ The Plugins item appears twice (when I mock the trial conditional; both items are working tho.)
✅ The "Orders" menu item bleeds into the Calypso's menu.

Copy link

@fushar fushar left a comment

Choose a reason for hiding this comment

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

I tested the changes by comparing with nav unification.

✅ My Home

Nav unification Classic view
image
image
image
image

✅ Jetpack

Nav unification Classic view
image image

✅ Top Level

Nav unification Classic view
image image

✅ Tools (can see WooCommerce Status menu, order is a bit different but I think it's OK)

Nav unification Classic view
image image

✅ Settings (can see WooCommerce menu)

Nav unification Classic view
image image

On Trial plan (by mocking wc_calypso_bridge_is_ecommerce_trial_plan to true)

❌ The "Plugins" displays the "WC Plugins Upgrade" page

Got not allowed instead (both nav unification and classic view)

image

✅ The "Feedback" menu is moved to "Jetpack > Feedback"

Nav unification Classic view
image image

Issues

❓ If My Home menu is active, somehow the Extensions menu is also active:

Nav unification Classic view
image image

@arthur791004
Copy link
Contributor Author

On Trial plan (by mocking wc_calypso_bridge_is_ecommerce_trial_plan to true)
❌ The "Plugins" displays the "WC Plugins Upgrade" page
Got not allowed instead (both nav unification and classic view)

❓ If My Home menu is active, somehow the Extensions menu is also active:

It's weird... I created a new WooCommerce (Entrepreneur) site but I couldn't reproduce the above 2 issues 🫠

@fushar
Copy link

fushar commented Mar 14, 2024

It's weird... I created a new WooCommerce (Entrepreneur) site but I couldn't reproduce the above 2 issues 🫠

I created a new Woo Express Trial site (woo.com/express) and could not reproduce the issues as well.

Maybe mocking wc_calypso_bridge_is_ecommerce_trial_plan() is not enough? I haven't dug into the codebase though.

@candy02058912
Copy link

candy02058912 commented Mar 14, 2024

@fushar @miksansegundo
I was FINALLY able to see the page 🎉
You'll have to run

npm install
npm run build

in wc-calypso-bridge/
And then rsync to the site and visit /wp-admin/admin.php?page=wc-admin&path=%2Fplugins-upgrade

image

@arthur791004
Copy link
Contributor Author

Referring to #1449 (comment), it also resolves the My Home and Extensions menus are active 😂

@taipeicoder
Copy link

taipeicoder commented Mar 14, 2024

❓ If My Home menu is active, somehow the Extensions menu is also active:

Seeing the same on my end. I think it's because the page are both wc-admin:

Screenshot 2024-03-14 at 4 28 17 PM

Copy link

@candy02058912 candy02058912 left a comment

Choose a reason for hiding this comment

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

Retested things after running #1449 (comment)

When I switch from "classic" to the "default" admin interface, I see two "My Home" submenus.

✅ Fixed

default

❓ If My Home menu is active, somehow the Extensions menu is also active:

✅ Fixed

Default style
myhome extension

Classic style
classic

❌ The "Plugins" displays the "WC Plugins Upgrade" page

✅ Fixed
See #1449 (comment)

You can see the "My Home" menu, and it displays the "Welcome to your Woo Express store" page

image

You can see the "Orders" menu

image

The "WooCommerce" menu is renamed to "Extensions"

  • The submenu "Orders" is moved to the top level
  • The submenu "Customers" is moved to the top level
  • The submenu "Extensions" is renamed to "Discover"
  • The submenu "Home" is hidden
  • The submenu "Settings" is moved to "Settings > WooCommerce" menu, and it's right before "Settings > General"
  • The submenu "Status" is moved to "Tools > WooCommerce Status"

all checks

The "Settings > Jetpack" menu is removed

The "Jetpack > Search" menu is removed

The "Jetpack > Akismet" menu is moved to "Settings > Anti-Spam"

image

Copy link

@fushar fushar left a comment

Choose a reason for hiding this comment

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

You'll have to run
npm install
npm run build
in wc-calypso-bridge/
And then rsync to the site and visit /wp-admin/admin.php?page=wc-admin&path=%2Fplugins-upgrade

Oh my god... thanks @candy02058912!!! Now everything works perfectly!! 😂

Copy link

@taipeicoder taipeicoder left a comment

Choose a reason for hiding this comment

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

Tested:

  • Create a new WoA site with Entrepreneur plan
  • Build and sync changes from your local to the WoA site
  • Enable the nav redesign on your site, e.g.: turning on your proxy or seting the wpcom_classic_early_release option to 1 on your site
  • ✅ Make sure the menu works as expected
    • ✅ You can see the "My Home" menu, and it displays the "Welcome to your Woo Express store" page
    • ✅ You can see the "Orders" menu
    • ✅ The "WooCommerce" menu is renamed to "Extensions"
    • ✅ The submenu "Orders" is moved to the top level
    • ✅ The submenu "Customers" is moved to the top level
    • ✅ The submenu "Extensions" is renamed to "Discover"
    • ✅ The submenu "Home" is hidden
    • ✅ The submenu "Settings" is moved to "Settings > WooCommerce" menu, and it's right before "Settings > General"
    • ✅ The submenu "Status" is moved to "Tools > WooCommerce Status"
    • ✅ The "Settings > Jetpack" menu is removed
    • ✅ The "Jetpack > Search" menu is removed
    • ✅ The "Jetpack > Akismet" menu is moved to "Settings > Anti-Spam"
  • Follow peapX7-1D4-p2 to create another WoA site with eCommerce trial plan. Or mark your site as a free trial site by mocking the return value of wc_calypso_bridge_is_ecommerce_trial_plan to true
    • Make sure the following items work as expected
      • ✅ The "Plugins" displays the "WC Plugins Upgrade" page
      • ✅ The "Feedback" menu is moved to "Jetpack > Feedback"
  • ✅ Disable the nav redesign on your site, e.g.: turning off your proxy or unsetting the wpcom_classic_early_release option
  • ✅ Review the menu items, and it should be the same as before

Copy link
Member

@xristos3490 xristos3490 left a comment

Choose a reason for hiding this comment

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

Thanks @arthur791004! Everything works as expected now. We are almost ready!

I noticed the "My Home" item appears within the Hosting menu. Is this expected? 🤔

See
Vwu2al.png

@xristos3490 xristos3490 self-requested a review March 15, 2024 08:21
@lsl
Copy link
Contributor

lsl commented Mar 15, 2024

I noticed the "My Home" item appears within the Hosting menu. Is this expected? 🤔

This is calypso's My Home, and it's the default route for the "Hosting" menu item as well.

...

Ah that's right, Woo Express wants to use wc-admin as the sites "My Home", that doesn't really work with the new menu setup.

We'll need to figure this out separately (issue). I'm not sure if the change should go here or in our menu as we would want to do something on the calypso side too. Probably just need to make /plans the home route.

Comment on lines +243 to +263
// Replace "Hosting" (/home) link with "Hosting" (/plans).
$this->update_menu(
'wpcom-hosting-menu',
esc_url( "https://wordpress.com/plans/{$this->domain}" ),
esc_attr__( 'Hosting', 'wc-calypso-bridge' ),
'manage_options',
'dashicons-cloud',
3
);

// Remove "Hosting" submenu item created by the above.
remove_submenu_page(
'wpcom-hosting-menu',
esc_url( "https://wordpress.com/plans/{$this->domain}" )
);

// Remove "My Home" submenu item.
remove_submenu_page(
'wpcom-hosting-menu',
esc_url( "https://wordpress.com/home/{$this->domain}" )
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot_2024-03-18_17-22-27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested this change and it looks good!

✅ The “My Home” of Hosting is removed
✅ The “Hosting” menu points to the /plans page on Calypso

@arthur791004 arthur791004 merged commit 912f5da into master Mar 19, 2024
5 checks passed
@arthur791004 arthur791004 deleted the feat/wooexpress-nav-redesign branch March 19, 2024 07:30
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.

8 participants