Current selection can pull metrics list from previous answer

[wpdarren]

Bug Description

ACR Bug bash ticket in Asana can be found here. The second part of this ticket can be found in Asana here.

When tailored metrics site purpose answer is edited, if for example currently saved answer is Sell products, then you choose Publish a blog, and then go to Provide services and click on the Apply changes CTA current metrics selection is not pulling the current selection.

Screen.Recording.2025-01-14.at.15.22.49.mov

What changing the answer to the "What is the main purpose of the site?" question in the Settings page, the list of current metrics changes unexpectedly while the modal is saving.

For example when changing from "Sell products" to "Publish a blog", the modal opens up with this list of current metrics:

When the "Update metrics selection" CTA is clicked the list of current metrics is updated to the following while the selection is being saved:

Changing the answer again in order to show the modal, the current metrics are now as expected (i.e. the "New tailored metrics" list in the first modal):

---

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

Acceptance criteria

• Switching between site purpose answers in the admin settings, and trying to apply changes should always trigger the modal with correct metrics in the current selection
  • It should accurately show tailored list of metrics for saved answer - reflecting what's currently in the suggested list
  • It should persist the current suggested metrics during saving/after saving is done, when Update metrics selection CTA is clicked

Implementation Brief

• Update assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.js
  • Source the value of currently saved purpose from CORE_USER datastore using getSavedUserInputSettings selector, for the existing savedPurpose
  • Replace the savedPurpose[ 0 ] argument in getAnswerBasedMetrics

    site-kit-wp/assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.js

    Line 102 in 944cad1

    return select( CORE_USER ).getAnswerBasedMetrics( savedPurpose[ 0 ] );

    with savedPurpose?.purpose?.values?.[ 0 ]
• In assets/js/components/user-input/UserInputPreview.js
  • Apply same changes as above, by sourcing different value for savedPurpose and using it in getAnswerBasedMetrics selector for currentMetrics
  • Rename the variable

    site-kit-wp/assets/js/components/user-input/UserInputPreview.js

    Line 149 in 944cad1

    if ( savedPurpose?.length ) {

    to savedPurposeSnapshot and source the value from USER_INPUT_QUESTIONS_PURPOSE key of the FORM_USER_INPUT_QUESTION_SNAPSHOT form like it was before for now modified savedPurpose
  • Update

    site-kit-wp/assets/js/components/user-input/UserInputPreview.js

    Lines 131 to 133 in 944cad1

    	const newMetrics = useSelect( ( select ) => {
    	return select( CORE_USER ).getKeyMetrics();
    	} );

    selector, it should also include potential new events if they are detected using the same arguments/approach as here

    site-kit-wp/assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.js

    Lines 63 to 88 in 944cad1

    	const includeConversionTailoredMetrics = useSelect( ( select ) => {
    	const isGA4Connected =
    	select( CORE_MODULES ).isModuleConnected( 'analytics-4' );
    	if ( ! isGA4Connected ) {
    	return false;
    	}
    	const haveConversionEventsForTailoredMetrics =
    	select(
    	MODULES_ANALYTICS_4
    	).haveConversionEventsForTailoredMetrics();
    	if ( haveConversionEventsForTailoredMetrics ) {
    	return select( MODULES_ANALYTICS_4 ).getDetectedEvents() || [];
    	}
    	return [];
    	} );
    	const newMetrics = useSelect( ( select ) => {
    	return select( CORE_USER ).getAnswerBasedMetrics(
    	null,
    	includeConversionTailoredMetrics
    	);
    	} );

    • You can extract logic from includeConversionTailoredMetrics and add it as a new selector, say shouldIncludeConversionTailoredMetrics in assets/js/modules/analytics-4/datastore/conversion-reporting.js so it can be re-used in both files
• Existing logic around FORM_USER_INPUT_QUESTION_SNAPSHOT form value should remain in other parts of the codebase, as it is primary used for accurately reseting the focus after modal changes as most reliable value.

Test Coverage

• Update assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.stories.js stories by using setUserInputSetting action to change site purpose

QA Brief

• Setup Site Kit with Analytics module and conversionReporting feature flag enabled
• Setup KMW using tailored metrics
• Go to settings and edit site purpose - change site purpose few times before clicking apply CTA
• Verify that current mtrics are correctly representing available suggested metrics based on currently saved metrics, not the wrong list of metrics
• Verify that current metrics is remaining the same during the saving

Changelog entry

• Fix bug that could cause metrics not to update properly if answers were changed several times before saving changes.

[10upsimon]

@zutigrm IB LGTM ✅ This one took a while for me to get my head around. Given that we're both very close to the form snapshot approach, let's aim to keep this issue amongst us if possible.

[kelvinballoo]

QA Update ⚠

I noticed a regression which is better explained through video.

Please refer to the attached video.

10034.test.wrong.mov

[10upsimon]

@tofumatt this one is back with you for Merge Review given that it's targeted against main for release 1.146.0. Thanks!

[kelvinballoo]

QA Update ⚠

Thanks @zutigrm .

Tested the main regression I noticed before and this is now working as expected. Changing of site purpose is also displaying the modal with the correct metrics. Tested a few of those site purpose and not only what is on the video attached. ✅

10034.new.-.01.mov

_____________________________________________________________________________

🚨 That said I want to raise 2 other items. These are possibly out of scope for this ticket.
We can raise new tickets for those if you agree.

ITEM 1: ⚠
When Site purpose is 'Other', current metrics are understandably blank.
In that case, I think it's best to hide the 'Suggested tab' because it's blank but it will write 'No metrics were selected yet'.

____________________________________________________________________

ITEM 2: ⚠
Here's the scenario:

• Let's say we select a particular metric e.g. Publish a Blog.

• If we change the site purpose to 'Other'

• It will save straightaway without any popup modal. I assume that's ok because technically there is no metrics associated with it.

  • Can you confirm this though? Because on 'Latest Release' branch, a modal would pop up with current metrics as blank. This looks like a regression.

• Now, if I change the site purpose to 'Publish a Blog' again, a modal would popup with the same current and new metrics.

• I was not expecting this.

  

A video is attached for reference for both issues.
Note: those were tested on both develop and main branch.

10034.issues.mov

[zutigrm]

@kelvinballoo Thanks,

I have opened a new issue #10182 to define handling of other for both cases (item 1 and item 2), since this issue fixes persisting the lists in confirm modal, the new one will define the fix for parts of the logic detecting the difference extending on the new logic introduced here.

For item 2 I also raised this with Sigal, to see if we want to include some message instead of showing empty list, etc

[kelvinballoo]

QA update ✅

Thanks @zutigrm . Noted on that.

Moving ticket to approval since the main regression is now working as expected. Changing of site purpose is also displaying the modal with the correct metrics.
Tested good on a few of those site purpose and not only what is on the video attached. ✅

10034.new.-.01.mov