Remove unifiedDashboard feature flag and unused code

[tofumatt]

Feature Description

With the Unified Dashboard epic complete, we should remove its feature flag and unused code from the repo.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• All usage of the unifiedDashboard feature flag should be removed, and all code that relies on unifiedDashboard === false should be removed.
• The individual Module Screen PHP code (eg. /wp-admin/admin.php?page=googlesitekit-module-analytics) and accompanying JS code should be removed as it is no longer in use once Unified Dashboard is the default.
• All relevant E2E tests should continue to pass, as the tests that are failing with the feature flags enabled are mostly testing functionality that is still relevant for the Unified Dashboard. The failing E2E tests are mostly around the "Post search" input and search functionality on the dashboard, and the date range picker. When the feature flags are always enabled (see this PR/test run: https://github.com/google/site-kit-wp/runs/6086002534?check_suite_focus=true#step:13:426) those tests fail and should be updated.
• All stories that use "Legacy" or old-style dashboard UIs should be removed with VRTs updated, and new stories should be put in their place or added.

Implementation Brief

Before removing the feature flag from production code, force-enable the flag and fix (or remove) the failing tests. Then, remove the feature flag and related code. Additional tests may be removed as part of this process.

Following this approach should provide a bit more certainty that the removal of the feature flag from production code doesn't break anything.

Fix / Remove Tests

To force enable the flag, patch the JS function isFeatureEnabled and the PHP method Feature_Flags::enabled, as seen here:

site-kit-wp/assets/js/features/index.js

Lines 33 to 39 in e5cb521

	export const isFeatureEnabled = (
	feature,
	_enabledFeatures = enabledFeatures
	) => {
	if ( feature === 'unifiedDashboard' ) {
	return true;
	}

site-kit-wp/includes/Core/Util/Feature_Flags.php

Lines 40 to 47 in e5cb521

	public static function enabled( $feature ) {
	if ( ! $feature || ! is_string( $feature ) || empty( static::$features ) ) {
	return false;
	}
	if ( 'unifiedDashboard' === $feature ) {
	return true;
	}

At the time of writing the test failures comprise:

E2E tests

8 test failures across the following 4 files (see example run):

tests/e2e/specs/modules/search-console/dashboard-date-range.test.js
tests/e2e/specs/dashboard/search.test.js
tests/e2e/specs/modules/search-console/admin-bar.test.js
tests/e2e/specs/modules/analytics/dashboard-date-range.test.js

6 of these tests (spanning the first two files) are failing because they are trying to reference the old URLSearchWidget component, and will need to be updated to exercise the new search bar. The failing test in admin-bar.test.js expects to navigate to the old Entity Dashboard, and needs updating to look for a DOM element present on the new version instead. The failing test in dashboard-date-range.test.js is trying to exercise the legacy Module Dashboard and can be removed.

PHPUnit

3 failing tests, over 3 files, each in the test_register test (see example run):

tests/phpunit/integration/Modules/AdSenseTest.php
tests/phpunit/integration/Modules/AnalyticsTest.php
tests/phpunit/integration/Modules/Search_ConsoleTest.php

Each of these has the same cause, $this->register_screen_hook(); being no longer invoked once the unifiedDashboard feature flag is removed. They can be fixed by removing the check for the screen in googlesitekit_module_screens.

Ultimately, the googlesitekit_module_screens filter will no longer be used so all references to googlesitekit_module_screens in the test_register tests can in fact be removed, and the test_register_unified_dashboard tests can also be removed.

JS Tests

15 failing across 6 files (see example run):

Of these, four files have tests that specifically test for the case where unifiedDashboard is not enabled. These can be removed; the files also contain tests where unifiedDashboard is enabled and these can be cleaned up a bit to reflect this is now the default/only case. These files are:

assets/js/components/DashboardEntryPoint.test.js
assets/js/modules/adsense/datastore/base.test.js
assets/js/modules/analytics/datastore/base.test.js
assets/js/modules/search-console/datastore/base.test.js

The remaining two files test components which have conditional behaviour relating to the unifiedDashboard flag and the tests should be updated accordingly. These files are:

assets/js/components/UserMenu.test.js
assets/js/googlesitekit/widgets/components/WidgetAreaRenderer.test.js

Remove Feature Flag

• Remove unifiedDashboard from feature-flags.json.
• Remove all references to this feature flag, keeping the execution path where the flag is true and removing where false. Remove any unused/unreachable code. Take care to remove components which are no longer in use.

At the time of writing that entails (as entry points):

• 4 files under includes/
• 3 files under tests/phpunit/
• 1 file under tests/e2e
• 26 files under assets/js (excluding tests and stories: there are no story files that reference this feature flag)
• 4 test files under assets/js (as mentioned above under Fix Tests)

Remove Module Screens and Legacy Widgets

PHP:

• Remove the class Module_With_Screen_Trait and all code that is only used by or in conjunction with this class. For example all references to googlesitekit_module_screens, Permissions::VIEW_MODULE_DETAILS and Module::prepare_info_for_js can be removed.

JS:

• Remove assets/js/googlesitekit-module.js and all components / styles that are only used by this entry point and not shared elsewhere.
• Remove the googlesitekit-module entry point from webpack.config.js and remove the Script instance for googlesitekit-module from includes/Core/Assets/Assets.php.
• Remove all use of screenWidgetContext and related code.
• Remove the following widget contexts. For each of these contexts, examine the widget areas registered to that context via registerWidgetArea. For each of these areas, remove the area and examine the widget components registered to that area via registerWidget. For each of these components, check whether it's still being used by a non-legacy context/area, and delete the component if it's no longer in use.

// Legacy dashboard
CONTEXT_DASHBOARD
// Legacy entity dashboard
CONTEXT_PAGE_DASHBOARD
// Legacy module screens
CONTEXT_MODULE_ANALYTICS
CONTEXT_MODULE_ADSENSE
CONTEXT_MODULE_SEARCH_CONSOLE

Storybook

• Review and update Storybook to ensure there are no out-dated or broken stories.
• Update the VRT reference images.

QA Brief

• Setup and configure the plugin on a brand new site or reset the plugin on an existing one.
  • Setup all the modules.
• Check that the main dashboard, entity dashboard and settings screens work fine.

Changelog entry

• N/A

[eclarke1]

Just to highlight here, that Zero Data States will not be rolled out to 100% of users until May 9 at the earliest, so is it a little premature to get this in Sprint 74, should this perhaps be done in Sprint 75 once it has been launched to 100% of people for at least 2 weeks?
cc @felixarntz @aaemnnosttv WDYT?

[tofumatt]

I think there's a fair bit of work in this issue, so it won't hurt to get started early. I don't think it'll introduce too many conflicts, and we can wait to actually merge it if it's ready early 🙂

[techanvil]

Following on from the discussion we had about splitting this into two tickets, I have reviewed the feature flags in question and don't see any problem with splitting this to two, which should be implemented in sequence, i.e. first remove unifiedDashboard and once this is merged, remove zeroDataStates.

I'd suggest we create a new story for zeroDataStates and modify this one to cover unifiedDashboard alone.

cc @eclarke1 @aaemnnosttv @felixarntz

[aaemnnosttv]

@techanvil that sounds good to me, especially since ZDS is still rolling out – let's not block the removal of unifiedDashboard because of it. Unless these need to be done together then I would prefer them to happen separately anyways. I'll create a new issue for ZDS and update the ACs for both.

[aaemnnosttv]

@techanvil thanks for the comprehensive IB! This looks like it covers about everything, just a few points to address:

• Remove the class Module_With_Screen_Trait and all code that is only used by or in conjunction with this class. For example all references to googlesitekit_module_screens, Permissions::VIEW_MODULE_DETAILS and Module::prepare_info_for_js can be removed.

The Module::prepare_info_for_js method should not be removed here, only the screenID in its return value is no longer relevant.

• Remove assets/js/googlesitekit-module.js and all components / styles that are only used by this entry point and not shared elsewhere.

For any entrypoints that we remove, these should also be removed in Webpack as well as their asset definitions in Assets – for googlesitekit-module this was only referenced in the trait so we just need to remove its Script instance as well in Assets::get_assets.

[techanvil]

Thanks @aaemnnosttv -

• Remove the class Module_With_Screen_Trait and all code that is only used by or in conjunction with this class. For example all references to googlesitekit_module_screens, Permissions::VIEW_MODULE_DETAILS and Module::prepare_info_for_js can be removed.

The Module::prepare_info_for_js method should not be removed here, only the screenID in its return value is no longer relevant.

I can't see where Module::prepare_info_for_js is being used outside of Module_With_Screen_Trait (apart from the tests for Module itself) - am I missing some sort of obscure method call (or indeed, something obvious)?

• Remove assets/js/googlesitekit-module.js and all components / styles that are only used by this entry point and not shared elsewhere.

For any entrypoints that we remove, these should also be removed in Webpack as well as their asset definitions in Assets – for googlesitekit-module this was only referenced in the trait so we just need to remove its Script instance as well in Assets::get_assets.

Thanks for pointing this out - I have added a corresponding point to the IB 👍

[aaemnnosttv]

I can't see where Module::prepare_info_for_js is being used outside of Module_With_Screen_Trait (apart from the tests for Module itself) - am I missing some sort of obscure method call (or indeed, something obvious)?

@techanvil looks like you are correct, I thought this was used by Modules but I was confusing it with prepare_module_data_for_response.

IB ✅

[mohitwp]

QA update ✅

Verified:

• UnifiedDashboard Feature flag removed successfully.
• Verified on new site as well as on old site.
• Verified main, entity, and settings page
• All above pages and UFD is working fine.