GLA inaccessible when WooCommerce Beta Tester plugin is active

martynmjones commented 10 months ago

Describe the bug:

When the WooCommerce Beta Tester plugin is active on a site, Google Listings & Ads stops working and instead an error is displayed reading Sorry, you are not allowed to access this page.

Steps to reproduce:

Install GLA and WooCommerce Beta Tester
Activate both
Go to Marketing > Google Listings & Ads

eason9487 commented 10 months ago

The root cause is the same as https://github.com/woocommerce/google-listings-and-ads/issues/1059#issuecomment-966791638. WooCommerce Beta Tester has added 'wc-admin-app' as its dependency since version 7.8.0.

Given that this is the second time the same issue has happened, even modifying the WooCommerce Beta Tester won't completely avoid it in the future.

I believe any other extension does the same thing may cause it again, so this needs to be investigated further to see if there is a better way to ensure that GLA is loaded before 'wc-admin-app', or that the addFilter callback is invoked correctly even if GLA is loaded after 'wc-admin-app'.

Ref: https://github.com/woocommerce/woocommerce/pull/38113

eason9487 commented 4 months ago

Possible solutions

After exploring this issue further, I suppose there are currently three ways to (temporarily) solve it.

1. Quick fix from the extension side

Set the $priority for the admin_enqueue_scripts action to be less than the default of 10, so that it can be output and run earlier than the 'wc-admin-app' script.

This solution may not work if any script that depends on 'wc-admin-app' is enqueued with a higher priority than this extension.

2. Quick fix from the WooCommerce Core side

WooCommerce Beta Tester depends on 'wc-admin-app' because it requires the config data of inline script window.wcAdminFeatures, and 'wc-admin-app' also requires window.wcAdminFeatures.

Considering that window.wcAdminFeatures is purely a config data object and has no dependencies on others, the approach is:

Move window.wcAdminFeatures up into the <head>
Remove WooCommerce Beta Tester's dependency on 'wc-admin-app'

I tested this solution by replacing the WC_ADMIN_APP constant with a 'utils' string as the following diff. It should be a better way to ensure the inline script is in the <head>.

However, this approach doesn't strictly manage their dependencies and still has some fragility.

index 0fb59cb992..f61ad2edb5 100644
--- a/plugins/woocommerce-beta-tester/woocommerce-beta-tester.php
+++ b/plugins/woocommerce-beta-tester/woocommerce-beta-tester.php
@@ -92,8 +92,6 @@ function add_extension_register_script() {
                );
        $script_url        = plugins_url( $script_path, __FILE__ );

-       $script_asset['dependencies'][] = WC_ADMIN_APP; // Add WCA as a dependency to ensure it loads first.
-
        wp_register_script(
                'woocommerce-admin-test-helper',
                $script_url,
diff --git a/plugins/woocommerce/src/Admin/Features/Features.php b/plugins/woocommerce/src/Admin/Features/Features.php
index d09549657a..b3ee4a5d4d 100644
--- a/plugins/woocommerce/src/Admin/Features/Features.php
+++ b/plugins/woocommerce/src/Admin/Features/Features.php
@@ -309,7 +309,7 @@ class Features {
                foreach ( $features as $key ) {
                        $enabled_features[ $key ] = self::is_enabled( $key );
                }
-               wp_add_inline_script( WC_ADMIN_APP, 'window.wcAdminFeatures = ' . wp_json_encode( $enabled_features ), 'before' );
+               wp_add_inline_script( 'utils', 'window.wcAdminFeatures = ' . wp_json_encode( $enabled_features ), 'before' );
        }

3. More robust fix from the WooCommerce Core side and it requires careful validation

Firstly, add a new React hook usePages to woocommerce-admin/client/layout/controller.js:

export function usePages() {
    const [ pages, setPages ] = useState( [] );

    useEffect( () => {
        // Set the initial pages.
        setPages( getPages() );

        // Handler for new pages being added after the initial filter has been run,
        // so that if any routing pages are added later, they can still be rendered
        // instead of falling back to an error page.
        const namespace = `woocommerce/woocommerce/watch_${ PAGES_FILTER }`;
        const handleHookAdded = ( hookName ) => {
            if ( hookName === PAGES_FILTER && didFilter( PAGES_FILTER ) > 0 ) {
                setPages( getPages() );
            }
        };

        addAction( 'hookAdded', namespace, handleHookAdded );

        return () => {
            removeAction( 'hookAdded', namespace );
        };
    }, [] );

    return pages;
}

Secondly, replace the getPages() in woocommerce-admin/client/layout/index.js with the return of usePages():

@@ -313,6 +313,7 @@ const Layout = compose(

 const _PageLayout = () => {
        const { currentUserCan } = useUser();
+       const pages = usePages();

        // get the basename, usually 'wp-admin/' but can be something else if the site installation changed it
        const path = document.location.pathname;
@@ -321,7 +322,7 @@ const _PageLayout = () => {
        return (
                <HistoryRouter history={ getHistory() }>
                        <Routes basename={ basename }>
-                               { getPages()
+                               { pages
                                        .filter(
                                                ( page ) =>
                                                        ! page.capability ||

When a routing page is added in this way and the page is browsed directly, the error message "Sorry, you are not allowed to access this page" is displayed for a short moment.

https://github.com/woocommerce/google-listings-and-ads/assets/17420811/aa92b72b-9caf-4b0f-850e-bbac403ecdf5

However, I think this is an acceptable solution to get rid of the dependency handling such as the above two, so it's still better overall.

chihsuan commented 4 months ago

Thanks, @eason9487 for the detailed analysis.

From what I understand, the root cause of the issue is that the WooCommerce assets are loaded before the GLA assets, so the GLA filters are not being applied, and the page is not rendered correctly. Is that correct? 🤔

If so, can we load GLA assets to the WooCommerce dependency list before the WooCommerce assets? Would that be a viable solution?

function add_dependency_to_registered_script() {
    if ( ! defined( 'WC_ADMIN_APP' ) ) {
        return;
    }

    global $wp_scripts;

    if ( isset( $wp_scripts->registered[ WC_ADMIN_APP ] ) ) {
        $wp_scripts->registered[ WC_ADMIN_APP ]->deps[] = 'google-listings-and-ads';
    }
}
add_action( 'admin_enqueue_scripts', 'add_dependency_to_registered_script', 20 );

eason9487 commented 4 months ago

Hi @chihsuan, thanks for the suggestion!

From what I understand, the root cause of the issue is that the WooCommerce assets are loaded before the GLA assets, so the GLA filters are not being applied, and the page is not rendered correctly. Is that correct?

This is the cause of the issue, but it's hard to say if it's the root cause because they depend on the action/filter mechanism in @wordpress/hooks, rather than 'wc-admin-app' directly depending on this extension, and vice versa. This is also the reason I thought it would be better to decouple them by adjusting the use of @wordpress/hooks filter.

If so, can we load GLA assets to the WooCommerce dependency list before the WooCommerce assets? Would that be a viable solution?

I tried a solution with the same concept yesterday and it works for this extension. The reasons I didn't mention it because:

I'm not pretty sure if it's a good idea to make 'wc-admin-app' programmatically depends on an extension which intuitively depends on 'wc-admin-app'. In particular, their conceptual relationship should be closer to an extension depending on 'wc-admin-app', so I would like to avoid this strict dependency.
Since this dependency handling was not mentioned in the relevant documentation, all (third-party) extensions built in the same way must also add this adjustment. Based on this, while this solution can solve the issue, it would probably require more effort to get the affected extensions to add the same tweak to deal with the impediments to the Coming Soon Mode of WooCoomerce Core. This doesn't seem ideal.
- https://github.com/woocommerce/woocommerce/blob/8.8.3/packages/js/create-woo-extension/includes/admin/setup.php.mustache#L33-L47
- https://github.com/woocommerce/woocommerce/blob/8.8.3/docs/extension-development/working-with-woocommerce-admin-pages.md
Not at the moment, but I'm a little worried about the potential for it to become a circular dependency in the future.

adrianduffell commented 4 months ago

I like the principle of option 3 -- extensions should be able add pages after WC_ADMIN_APP has loaded. However the flash of "Sorry, you are not allowed to access this page" feels like a showstopper for me. Besides a filter, is there a different mechanism we could add to Core to allow pages to be added after WCA has loaded? 🤔

I also feel it's unintuitive to have WCA depend on GLA, but it is seems like the true relationship right now as WCA needs GLA to have its filters set up first. I'm ok with @chihsuan's idea if want to use it as a stopgap first.

chihsuan commented 4 months ago

Thanks for the clarification! I see your point. @eason9487

I agree that ideally, we should avoid having WCA depend on GLA, but like @adrianduffell mentioned. Currently, GLA does need to be loaded first for WCA to work. 🤔

Besides a filter, is there a different mechanism we could add to Core to allow pages to be added after WCA has loaded?

I think the difficulty here is that WCA needs to know if there are pages to display or not. If there are pages, it should show a loading spinner before the extension scripts are loaded and the filter is applied. Otherwise, it should show a 404 page. I can imagine that other mechanisms would still end up with the same issue.

I'm not sure if there is a good way, but there might be a possible solution that displays a loading spinner in the NoMatch route for a few seconds before showing a 404 page. This way, it would give the extension scripts time to load and apply the filter. It's not ideal but it could be a workaround.

eason9487 commented 4 months ago

Hi @adrianduffell,

However the flash of "Sorry, you are not allowed to access this page" feels like a showstopper for me.

The chance of unexpected error page flashes could be minimized by delaying its rendering by maybe 3-5 seconds.

The delay in rendering error page I think might be acceptable because most of the time users shouldn't run into an incorrect routing page. And if, unfortunately, an actual mismatch routing occurs, then a 3-5 second delay shouldn't be a problem, after all, there are more serious errors out there.

chihsuan commented 4 months ago

Solution 3 + delay is a good compromise to me. If we go with this, it might be worth testing the performance impact of the delay.

adrianduffell commented 4 months ago

I think a 3+ delay is better than what we have now, but it still feels like a bug 😅 If I understand, a scenario can still exist where one extension can add the same logic as Beta Tester, and cause GLA and other extensions to all have that delay to load pages 🤔

chihsuan commented 4 months ago

@adrianduffell From my understanding, users will not need to wait for 3-5 seconds for the actual page to load, but only for the error page to render.

When other extensions add the same logic as Beta Tester, the scripts' loading orders are changed. So, the admin app will be loaded first, then the error page will be rendered with a delay (loading UI), and once the GLA is loaded and the filter is added, the error page will be immediately replaced with the actual page. I think we can't avoid this scenario unless we make admin app dependent on GLA.

adrianduffell commented 4 months ago

@chihsuan thanks for clarifying it. I think I understand it better -- in theory, would it take the same amount of time for a GLA page to render, just now there would be a loading UI for 3-5s beforehand?

Does the 3-5s refer to duration between the WCA and GLA scripts completing?

chihsuan commented 4 months ago

The 3-5 second delay refers to the duration before rendering the error page.

There will be a delay of 3-5 seconds before the error page is rendered if a routing mismatch occurs. During this delay, users will see a loading UI.
If the GLA page loads correctly, the loading UI will be replaced by the actual page without showing the error page. The total time for the GLA page to render should remain roughly the same based on the duration between the WCA and GLA scripts completing.

chihsuan commented 4 months ago

It come to my mind that if we don't want to delay the error page rendering, we could consider introducing a new PHP filter to allow extensions to register their pages; it then can be used to check if the requested page is valid or not. This way, we can avoid the delay in rendering the error page. However, this approach might need extra work to be implemented and tested.

adrianduffell commented 4 months ago

Ha, ok sorry I get it now 😅 So it's the error page that would take 3-5s -- I think that's ok.

woocommerce / google-listings-and-ads