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

Handling Other site purpose answer for tailored metrics #10182

Open
3 tasks
zutigrm opened this issue Feb 6, 2025 · 6 comments
Open
3 tasks

Handling Other site purpose answer for tailored metrics #10182

zutigrm opened this issue Feb 6, 2025 · 6 comments
Labels
P1 Medium priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@zutigrm
Copy link
Collaborator

zutigrm commented Feb 6, 2025

Feature Description

When Other is selected as site purpose for tailored metrics, hide Suggested tab in selection panel

Also when switching between other and rest of site purpose answers in KMW admin settings, the confirm change modal should list correct metrics - empty for current or new, depending if we are editing from, or to other answer

Originally raised in this comment

While testing around I spotted a bug when user input is used, selecting other as site purpose will throw console error - complete setup will not execute on last step. This should be included here as well


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

Acceptance criteria

  • The suggested group is hidden in the selection panel, when there is no valuable recommendation to show.
  • The confirmation metric changes modal does not appear when the user changes the site purpose setting to other.
  • The confirmation metric changes modal shows up with currently selected metrics and corresponding recommendations when the user changes the site purpose from other to anything else.
  • Selecting the other answer throws no console error during setup.

Implementation Brief

  • In assets/js/components/user-input/util/constants.js
    • In USER_INPUT_PURPOSE_TO_CONVERSION_EVENTS_MAPPING add other to the object, with value of empty array
      • This will fix the bug from last point in the AC
  • Update assets/js/components/KeyMetrics/ChipTabGroup/index.js
    • Where dynamicGroups is defined, before KEY_METRICS_GROUP_SUGGESTED is added to the array, besides checking for isUserInputCompleted, check also if answerBasedMetrics (which is already defined), is not empty array
  • In assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.js
    • Where currentMetrics is defined, before getAnswerBasedMetrics is returned, check if savedPurpose?.purpose?.values?.[ 0 ] is other, and return metrics using getKeyMetrics selector from CORE_USER datastore
    • In the existing check
      if (
      savedPurpose?.purpose?.values?.[ 0 ] &&
      currentMetricsSnapshot === null
      ) {
      include additional AND condition to the list - currentMetrics !== undefined

Test Coverage

  • No updates needed

QA Brief

Changelog entry

@zutigrm zutigrm added P1 Medium priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature labels Feb 6, 2025
@zutigrm zutigrm self-assigned this Feb 6, 2025
@zutigrm zutigrm removed their assignment Feb 6, 2025
@eugene-manuilov eugene-manuilov self-assigned this Feb 19, 2025
@eugene-manuilov
Copy link
Collaborator

@zutigrm, please, update IB to solve AC requirements.

Update assets/js/components/KeyMetrics/ChipTabGroup/index.js

  • Where dynamicGroups is defined, before KEY_METRICS_GROUP_SUGGESTED is added to the array, besides checking for isUserInputCompleted, check also if saved purpose is not other

    • You can retrieve purpose answer using getUserInputSettings selector from CORE_USER datastore, and then access it under savedPurpose?.purpose?.values

Instead of checking the site purpose, we need to check whether there are any suggestions available. If at least one suggestions is available, then add that group, otherwise not.

@zutigrm
Copy link
Collaborator Author

zutigrm commented Feb 21, 2025

Thanks @eugene-manuilov IB updated

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 21, 2025
@eugene-manuilov
Copy link
Collaborator

  • In assets/js/googlesitekit/datastore/user/key-metrics.js
    • Extract the default metrics list into a new selector getFallbackMetricsList
  • In assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.js
    • Where currentMetrics is defined, before getAnswerBasedMetrics is returned, check if savedPurpose?.purpose?.values?.[ 0 ] is other, and return getFallbackMetricsList metrics if true

@zutigrm, I believe you added add this for the case when other is selected, right? We don't need to show the modal when other is selected.

@zutigrm
Copy link
Collaborator Author

zutigrm commented Feb 21, 2025

I believe you added add this for the case when other is selected, right? We don't need to show the modal when other is selected.

I reversed the initial approach, to accommodate for AC change, specifically this point:

The confirmation metric changes modal shows up with currently selected metrics and corresponding recommendations when the user changes the site purpose from other to anything else

So when other is currently saved and we are changing to anything else we want to show default 4 metrics

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 21, 2025
@eugene-manuilov
Copy link
Collaborator

I believe you added add this for the case when other is selected, right? We don't need to show the modal when other is selected.

I reversed the initial approach, to accommodate for AC change, specifically this point:

The confirmation metric changes modal shows up with currently selected metrics and corresponding recommendations when the user changes the site purpose from other to anything else

So when other is currently saved and we are changing to anything else we want to show default 4 metrics

No, that should be anything else's recommendations, not the default 4 metrics.

@eugene-manuilov
Copy link
Collaborator

IB ✔

@eugene-manuilov eugene-manuilov removed their assignment Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

2 participants