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

Update tailored metrics to also include new ACR metrics #9437

Closed
2 tasks done
zutigrm opened this issue Sep 30, 2024 · 18 comments
Closed
2 tasks done

Update tailored metrics to also include new ACR metrics #9437

zutigrm opened this issue Sep 30, 2024 · 18 comments
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@zutigrm
Copy link
Collaborator

zutigrm commented Sep 30, 2024

Feature Description

Accompanying the update to the KMW area to support up to 8 metric widgets, tailored metrics included based on the site purpose answer, will also be expanded to include ACR metric widgets.

See ACR tailored metrics based on the site purpose answer section in the design doc


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

Acceptance criteria

  • Given the ACR feature is enabled, when a user answers the User Input questionnaire, then the answer-based tailored metrics selected for the user should be 8 (instead of the usual 4 when ACR is not enabled).
  • The new set of 8 tailored metrics for each answer is given in the table below. Note that these include a mix of old and new metrics:
    • Image
  • If the KMW was already set up by the user and then the ACR feature is enabled, then the answer based metrics returned should not change (the original set of 4 metrics should persist).

Implementation Brief

  • Update includes/Core/Key_Metrics/Key_Metrics_Settings.php
    • Add new setting to the list includeConversionTailoredMetrics, false by default
      • This setting will be used to confirm that existing user who has tailored metrics setup, will not see ACR widgets automatically as soon as the new events are detected
  • In assets/js/googlesitekit/datastore/user/key-metrics.js
    • Update getAnswerBasedMetrics selector
      • Expand on the existing metrics by assigning ACR metrics under their respective site purpose answer, following the list provided in the AC. Each new group of ACR metrics should be conditionally added to the existing list, if either includeConversionTailoredMetrics KMW setting is true OR if user input is completed - retrieve this value from isUserInputCompleted selector from CORE_USER datastore.
        • This will ensure that ACR widgets are included if it is a new KMW setup using user input

Test Coverage

  • Update assets/js/googlesitekit/datastore/user/key-metrics.test.js
    • Specifically expand on tests for getAnswerBasedMetrics selector to include ACR metrics
  • Update tests/phpunit/integration/Core/Key_Metrics/Key_Metrics_SettingsTest.php , expand on settings list to include new setting

QA Brief

Note: For the latest follow up, verify that when feature flag is disabled, only 4 old tailored metrics are showing

  • Setup Site Kit with Analytics module and conversionReporting feature flag enabled
  • Setup Key Metrics, using tailored metrics option
  • Verify that selected site purpose answer matches the tailored metrics from the AC list
  • Go to Serttings > Admin Settings and under key metrics settings section, edit the site purpose answer, and verify that for each answer tailored metrics list is matching the ones in AC table
  • Note: one exception is that Top device driving purchases KMW is not yet merged in the codebase, so it will be missing from the list at this time

Changelog entry

  • Update tailored metrics to also include new ACR metrics.
@zutigrm zutigrm added P0 High priority Type: Enhancement Improvement of an existing feature Team S Issues for Squad 1 labels Sep 30, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Sep 30, 2024
@10upsimon 10upsimon self-assigned this Sep 30, 2024
@10upsimon
Copy link
Collaborator

@zutigrm AC looks good to me, moving to IB.

@10upsimon 10upsimon removed their assignment Sep 30, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Sep 30, 2024
@binnieshah binnieshah added the Next Up Issues to prioritize for definition label Oct 2, 2024
@10upsimon 10upsimon self-assigned this Oct 2, 2024
@10upsimon
Copy link
Collaborator

@zutigrm IB ✅ Moving to EB

@10upsimon 10upsimon removed their assignment Oct 2, 2024
@zutigrm zutigrm removed the Next Up Issues to prioritize for definition label Oct 2, 2024
@zutigrm zutigrm assigned zutigrm and 10upsimon and unassigned zutigrm Oct 3, 2024
@10upsimon
Copy link
Collaborator

@zutigrm IB ✅ Moving to EB

@10upsimon 10upsimon removed their assignment Oct 3, 2024
@jimmymadon jimmymadon assigned jimmymadon and 10upsimon and unassigned jimmymadon Oct 6, 2024
@jimmymadon
Copy link
Collaborator

@10upsimon @zutigrm I feel, the includeConversionTailoredMetrics is a "Key Metrics" specific setting used only to include tailored metrics or not. As such, it might be better placed in the Key_Metrics_Settings class instead of GA4 settings which contain settings specific to Google Analytics within SK.

This isn't a big deal but I've updated the IB here for this after speaking to @10upsimon and having another look at the existing settings in both classes.

@jimmymadon
Copy link
Collaborator

jimmymadon commented Oct 9, 2024

@andreylipattsev @sigal-teller Currently, we have 4 "answer-based" metrics selected depending on the answer that a user selects to the User Input Questionnaire. These are listed below.

Existing Answer Based Metrics
  • Publish a blog
    • Returning visitors
    • New visitors
    • Top traffic source
    • Most engaged traffic source
  • Publish news content
    • Pages per visit
    • Visit length
    • Visits per visitor
    • Most engaging pages
  • Share a business card or portfolio to represent me or my company online or Other
    • New visitors
    • Top traffic source
    • Most engaged traffic source
    • Top performing keywords
  • Sell products or services
    • Popular products (or Popular content if product post types aren't tracked)
    • Most engaged traffic source
    • Top performing keywords
    • Top traffic source
  • Monetize content
    • Popular content
    • Most engaged traffic source
    • New visitors
    • Top traffic source

I just wanted to flag a few things as per the current ACs which include new ACR widget tiles:

  1. There is a varying number of tiles being added. This seems to be fine but just wanted to make sure this is good UX and we are consciously making this decision. Some users will end up with 6 tiles and some will end up with more.
  2. The Sell products or services section is containing 5 new ACR tiles. This would make the total 9 and that will be a problem as we want to allow only 8 in total. We can remove one of the old tiles here but for a user that already has KMW setup, they will end up having an existing tile changed! So should we stick to just 4 new tiles or remove an older one? Which final 8 should make the cut here?
  3. The Monetize content answer has no new ACR tiles added. Which new ACR tiles should be added if this option were to be selected?

c.c. @10upsimon @zutigrm @aaemnnosttv

@jimmymadon
Copy link
Collaborator

@10upsimon I have updated the ACs as per the discussion in this Slack thread and after @andreylipattsev consequently updated the Key Metrics Mapping.

@10upsimon
Copy link
Collaborator

Thanks @jimmymadon I noticed the IB still references GA4 settings instead of KMW settings:

Each new group of ACR metrics should be conditionally added to the existing list, if either includeConversionTailoredMetrics GA4 setting is true OR if user input is completed

For the sake of time, I am going to update that and approve so long.

@10upsimon
Copy link
Collaborator

@jimmymadon AC & IB ✅ moving to EB, cc @zutigrm

@10upsimon 10upsimon removed their assignment Oct 11, 2024
@benbowler benbowler self-assigned this Oct 11, 2024
@tofumatt tofumatt assigned tofumatt and kelvinballoo and unassigned tofumatt Oct 15, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Verified for each metric/site type, the Key metrics appear accordingly:

  • Publish Blog

    Image

    Image

  • Publish News

    Image

    Image

  • Monetize content

    Image

    Image

  • Portfolio site/other default

    Image

    Image

  • eCom

    Image

    Image

  • If the KMW was already set up by the user and then the ACR feature is enabled, then the answer based metrics returned does not change (the original set of 4 metrics persists).

    9437.-.metrics.selected.persist.480p.mov

@kelvinballoo kelvinballoo removed their assignment Oct 16, 2024
@techanvil
Copy link
Collaborator

Hey @benbowler, as per my comment on the PR, the followup PR has caused the assets/js/googlesitekit/datastore/user/key-metrics.test.js suite to fail.

Please can you raise another followup to fix the tests?

@techanvil
Copy link
Collaborator

techanvil commented Oct 16, 2024

Having merged the followup PR and confirmed all JS tests pass in main post-merge, I'm moving this back to Approval.

Image

@techanvil techanvil removed their assignment Oct 16, 2024
@zutigrm zutigrm self-assigned this Oct 17, 2024
@zutigrm
Copy link
Collaborator Author

zutigrm commented Oct 17, 2024

Moving it back to execution, as we need to guard new list of tailored metrics with feature flag, it seems I forgot to include it in AC as well. I will quickly add a follow up

@zutigrm zutigrm removed their assignment Oct 17, 2024
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Oct 17, 2024
@wpdarren wpdarren self-assigned this Oct 17, 2024
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • Tested that more than 4 tiles appear before refreshing the main branch when the conversionReporting feature flag is not enabled.
  • When I refreshed the main branch, only 4 key metric tiles appear when the conversionReporting feature flag is not enabled.
  • Quickly tested that when conversionReporting is enabled then the 8 key metric tiles appear.
Screenshots

Image
Image
Image

@wpdarren wpdarren removed their assignment Oct 17, 2024
@tofumatt tofumatt self-assigned this Oct 17, 2024
@tofumatt
Copy link
Collaborator

Moving this back to Execution to fix unit test failures.

@tofumatt tofumatt removed their assignment Oct 17, 2024
@tofumatt
Copy link
Collaborator

Just needed a unit test tweak, no user-facing change has been made so moving back to Approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

10 participants