Refactor Key Metric display logic for selection panel, widget area and view only dashboard

[jimmymadon]

Bug Description

In #7741, we introduced logic for view only users that filters out key metrics which require custom dimensions, but now those custom dimensions are unavailable (since view only users can't do much about this).

We are now directly calling displayInList() in the getKeyMetricSettings selector for view-only users which has side-effects when the displayInList implementation contains calls to check if isKeyMetricActive(), like in the case of Most Popular Products and for widgets that use shouldDisplayWidgetWithConversionEvent(). It causes a infinite recursion as seen with the calls below:

getKeyMetrics()
  getUserPickedMetrics()
    getKeyMetricsSettings()
      displayInList() // for "Most popular products" and widgets where shouldDisplayWidgetWithConversionEvent() is used
        isKeyMetricActive()
          getKeyMetrics()

However, this logic was introduced in a central getKeyMetricsSettings() selector even though the main purpose here was to filter out only userPickedMetrics, i.e. metrics the user had once saved but they no longer can benefit from since the custom dimensions required have been deleted or modified. So this should be thoughtfully refactored into the getUserPickedMetrics selector or somewhere more suitable.

site-kit-wp/assets/js/googlesitekit/datastore/user/key-metrics.js

Lines 467 to 501 in 8a891c4

	const isViewOnly = ! select( CORE_USER ).isAuthenticated();
	if ( isViewOnly ) {
	// Filter out widget slugs that depend on unavailable custom dimensions.
	const filteredWidgetSlugs = keyMetricsSettings.widgetSlugs.filter(
	( slug ) => {
	const widget = KEY_METRICS_WIDGETS[ slug ];
	if ( ! widget ) {
	return false;
	}
	if ( widget.displayInList ) {
	return widget.displayInList( select, isViewOnly );
	}
	return true;
	}
	);
	// If only one widget remains after filtering, return an empty array.
	// This prevents hiding the widget area when only one widget is available.
	// This triggers the `getKeyMetrics` selector to use the default widgets instead.
	if ( filteredWidgetSlugs.length === 1 ) {
	return {
	...keyMetricsSettings,
	widgetSlugs: [],
	};
	}
	return {
	...keyMetricsSettings,
	widgetSlugs: filteredWidgetSlugs,
	};
	}

We do have a clear separation of concerns when it comes to logic:

• that checks if a key metric should be displayed in the metrics selection panel - displayInList()
• that checks if a key metric should be displayed within the widget area - isKeyMetricActive()

But we have created a slight overlap in some places to hack things a certain way which makes things redundant and confusing. This can be re-visited and cleaned up.

Steps to reproduce

1. Activate the Woo Commerce plugin on any wordpress site.
2. Setup Site Kit, connect Analytics and set up Key Metrics as an Admin.
3. Share Analytics using Dashboard Sharing.
4. Now login as a view only user and in the Key Metrics widget, select Pick your own metrics.
5. Select the Most popular products by pageviews widget is in the list.
6. See the error

Screen recording

Screen.Recording.2024-10-23.at.20.32.57.mov

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

Acceptance criteria

• View only users should not see any errors when selecting ANY Key Metric widget tile.
  • These specifically include the Most popular products by pageviews widget tile, widget tiles that require Custom Dimensions and the new ACR metric tiles.
• The displayInList() callback function is not used for purposes other than displaying a key metric widget tile in the Metrics Selection panel.

Once completed, review #8703 which appears to be an earlier report of the same issue.

Implementation Brief

Fixing the bug

• In assets/js/components/KeyMetrics/key-metrics-widgets.js:

  • Create a new displayInWidgetArea callback property for all the key metrics that have a requiredCustomDimension. Set this property to call the shouldDisplayWidgetWithCustomDimensions() function. In the future, this callback can be used by other tiles to add any additional requirements for widgets to be rendered in the widget area which would further override the "user picked" or "answer based" selection.

• In assets/js/googlesitekit/datastore/user/key-metrics.js, :

  • Within getKeyMetricSettings() selector: Remove the whole isViewOnly check and snippet to filter out metrics. This will now be handled within the getUserPickedMetrics below.
  • Within getUserPickedMetrics() selector: Filter the widgetSlugs result. Using the KEY_METRICS_WIDGETS data, check if the widget has a displayInWidgetArea property and use the boolean function for filtering. Pass the isViewOnlyDashboard value.

Generic Refactoring

• Rename the displayInList property to displayInSelectionPanel for more clarity as "list" is too generic.

• In getAnswerBasedMetrics selector: Use a new selector for fetching the showConversionTailoredMetrics:
  

  site-kit-wp/assets/js/googlesitekit/datastore/user/key-metrics.js

  Lines 287 to 292 in e1ecae0

  	const keyMetricSettings = select( CORE_USER ).getKeyMetricsSettings();
  	const isUserInputCompleted = select( CORE_USER ).isUserInputCompleted();
  	const showConversionTailoredMetrics =
  	( keyMetricSettings?.includeConversionTailoredMetrics ||
  	isUserInputCompleted ) &&
  	isFeatureEnabled( 'conversionReporting' );

• In getKeyMetrics selector: Remove unnecessary filtering of answer based metrics for ACR feature flag as this is already done within the getAnswerBasedMetrics selector.
  

  site-kit-wp/assets/js/googlesitekit/datastore/user/key-metrics.js

  Lines 242 to 250 in e1ecae0

  	if ( answerBasedMetrics.length ) {
  	if ( ! isFeatureEnabled( 'conversionReporting' ) ) {
  	return answerBasedMetrics.filter( ( slug ) => {
  	return keyMetricsGA4WidgetsNonACR.includes( slug );
  	} );
  	}
  	return answerBasedMetrics;
  	}

• In getKeyMetrics selector: Move the filtering of userPickedMetrics for the ACR feature flag to a callback function that can be added to the new displayInWidgetArea property of all the new ACR metrics in key-metrics-widgets. Remove the keyMetricsGA4WidgetsNonACR constant if not used.
  

  site-kit-wp/assets/js/googlesitekit/datastore/user/key-metrics.js

  Lines 227 to 231 in e1ecae0

  	if ( ! isFeatureEnabled( 'conversionReporting' ) ) {
  	return userPickedMetrics.filter( ( slug ) => {
  	return keyMetricsGA4WidgetsNonACR.includes( slug );
  	} );
  	}

Test Coverage

No new tests needed.

QA Brief

• The only user facing change in this issue is that the "AdSense Top Earning Content" Key Metric tile is now always displayed in the Metrics Selection panel. Before, it was incorrectly "hidden" when the AdSense or Analytics module were disconnected, instead of the metric being shown but disabled. This allows the key metric tile to be "removed" if it was previously selected but the module was disconnected later.
• The Key Metrics functionality should be thoroughly tested in addition to the AC:
  • Answering the user input questionnaire and seeing "answer-based" metrics.
  • Selecting user picked metrics - test all metrics in selection panel (up to 4 at a time when ACR feature is disabled, and 8 at a time when it is enabled).
  • Try disabling modules after selecting metrics. Verify that the selected metrics can be deselected but new metrics which rely on disconnected module is not "enabled".
  • Selecting all metrics on the view-only dashboard.
  • Test all of the above with and without the conversionReporting feature flag.

Changelog entry

• Fix dashboard error when selecting specific key metrics on the view-only dashboard.

[tofumatt]

ACs here are good, and thanks for the great explanation/context for the issue 👍🏻

Moving to IB 🙂

[tofumatt]

IB ✅

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified that issue error is not showing on 'View only dashboard' when user selects 'Most popular products by pageviews' KM tile
• Verified KM and user input functionality when 'conversionInfra' feature flag is enabled.
• Verified KM and user input functionality when 'conversionInfra' feature flag is not enabled.
• Verified Top earning pages KM tile related scenarios.

Top earning pages KM tile related scenarios

Top earning pages KM tile when AdSense is disconnected

Top earning pages KM tile when both Analytics and AdSense are disconnected

Top earning pages KM tile when Analytics is disconnected

If AdSense or Analytics module gets disconnect later and 'Top earning pages' KM tile is already selected

Recording.1581.mp4

Key metric complete flow when 'conversionInfra' feature flag is not enabled

Recording.1580.mp4

Recording.1581.mp4

Recording.1583.mp4

Key metric complete flow when 'conversionInfra' feature flag is enabled

Recording.1584.mp4

Recording.1585.mp4

Recording.1586.mp4

View only dashboard when user selects 'Most popular products by pageviews' KM tile

Recording.1582.mp4