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

Continue to pre-select matching second property when changing the primary property #3549

Closed
eclarke1 opened this issue Jun 10, 2021 · 6 comments
Labels
Module: Analytics Google Analytics module related issues P2 Low priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Milestone

Comments

@eclarke1
Copy link
Collaborator

eclarke1 commented Jun 10, 2021

Bug Description

This applies to only the case where the GA account has both types of properties, UA and GA4.

In the particular case below, the site I'm operating on already has both a matching UA property and a matching GA4 property that were created earlier.

  1. When I go to the setup, the UA property for my site is correctly pre-selected in the primary dropdown (which has all properties). The blue notice where I can then selecting the matching GA4 property also has my GA4 property pre-selected as expected (see screenshot 1).
  2. The actual problem: When I then change the primary property (e.g. to the matching GA4 property), the secondary dropdown in the blue notice remains empty, even though I would expect it to once again pre-select the matching property, in this case the UA property (see screenshot 2).
  3. This appears to happen no matter what property in the same account you select - the matching logic for the secondary property dropdown only kicks in on the initial load.

Related Asana task (also features a screen recording)


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

Acceptance criteria

  • In the ga4transitional version of the Analytics setup flow: When selecting another property than the pre-selected one in the primary dropdown, the logic to check if there is also a matching property for the secondary dropdown (i.e. of the respective other type, UA or GA4) should also be run.
    • Right now, the secondary property selection just gets emptied, without checking whether there is a match.
    • Example:
      1. Initially, the primary dropdown shows a UA property for the current site URL and the secondary dropdown shows a GA4 property for the current site URL (in the same account of course).
      2. If you now change the primary property to be a GA4 property (e.g. the same property that is selected in the secondary dropdown), instead of resetting the secondary dropdown to be empty, the logic to check if there is a matching UA property should be re-triggered, which should result in the UA property that was initially pre-selected in the primary dropdown now appearing pre-selected in the secondary dropdown.

Implementation Brief

  • Using assets/js/modules/analytics-4/datastore/properties.js:
    • Extract the functionality to match a property by URL from the matchAndSelectProperty action into a new matchAccountProperty( accountID ) action.
    • Update the matchAndSelectProperty action to use the matchAccountProperty action to find matching property.
  • Using assets/js/modules/analytics-4/datastore/webdatastreams.js:
    • Add a new action matchWebDataStream( propertyID ) that waits till web data streams are loaded for a propertyID and returns the result of the getMatchingWebDataStream( propertyID ) selector call.
  • Using assets/js/modules/analytics/datastore/properties.js:
    • Add a new action findMatchedProperty( accountID ) that waits for properties, finds a property that matches the current reference site URL and returns it.
  • Using assets/js/modules/analytics/datastore/profiles.js:
    • Add a new action findPropertyProfile( accountID, propertyID, defaultProfileID = '' ) that does the following:
      • Pulls profiles for provided accountID and propertyID.
      • Tries to find a profile which ID matches defaultProfileID. If such a profile is found, return it.
      • Returns the first profile from the profiles array.
  • Using assets/js/modules/analytics/datastore/accounts.js:
    • Update the selectAccount action to use the findMatchedProperty( accountID ) action to find matching UA property.
  • Using assets/js/modules/analytics/components/common/PropertySelectIncludingGA4.js:
    • Update the onChange callback to set appropriate property, profile, web datastream and measurement IDs instead of empty strings using the aforementioned new actions.

P.S.: the majority of work is done in the bug/3549-continue-to-preselect-second-property branch, the remaining part is to implement js tests.

Test Coverage

  • Add js tests for new actions.
  • Try to update the should change between UA and GA4 properties test in the PropertySelectIncludingGA4.test.js file to work with the new approach.

Visual Regression Changes

  • N/A

QA Brief

  • Connect the Analytics module and go to the Analytics setup form.
  • Select an account that has both UA and GA4 properties that match the current website URL
  • Make sure that when you select GA4 property in the top dropdown, the appropriate UA property is selected in the dropdown in the blue area. Then select UA property at the top dropdown and make sure that the appropriate GA4 property is preselected in the dropdown in the blue area.

Changelog entry

  • Fix a bug in Analytics setup where a matching secondary property would not be automatically selected when the primary property was changed.
@eclarke1 eclarke1 added Type: Bug Something isn't working P1 Medium priority labels Jun 10, 2021
@felixarntz felixarntz added P2 Low priority and removed P1 Medium priority labels Jun 15, 2021
@felixarntz felixarntz removed their assignment Jun 15, 2021
@eugene-manuilov eugene-manuilov self-assigned this Jun 17, 2021
@fhollis fhollis added this to the Sprint 53 milestone Jun 24, 2021
@eugene-manuilov eugene-manuilov added the Module: Analytics Google Analytics module related issues label Jun 25, 2021
@aaemnnosttv
Copy link
Collaborator

@eugene-manuilov – the PR-as-IB approach should generally be reserved for issues with a trivial change and/or highly explicit ACs, such that the IB would be a rehashed AC. In this case, I think an IB is appropriate to include given the scope and complexity of the changes. This is relevant when looking back on an issue as well 🙂

Looking at your PR, there are substantial changes to actions in the Analytics stores, but no added or updated tests to cover these changes. There was also one very relevant test for PropertySelectIncludingGA4 "should change between UA and GA4 properties" which was removed (as you mentioned). Given the relevance of the test, it would be great if we could find a way to keep it if it didn't take too much to maintain. Otherwise, I think we at least need to add tests for the actions that were added/updated/renamed that don't seem to have any yet.

Please add some details around the proposed changes to the IB and let's also ensure the changes are covered by tests.

@eugene-manuilov
Copy link
Collaborator

the PR-as-IB approach should generally be reserved for issues with a trivial change and/or highly explicit ACs, such that the IB would be a rehashed AC. In this case, I think an IB is appropriate to include given the scope and complexity of the changes. This is relevant when looking back on an issue as well

@aaemnnosttv yes, I know this. I wanted to squeeze this ticket into the 1.36.0 release. That's why I decided to go with the PR-as-IB approach.

Please add some details around the proposed changes to the IB and let's also ensure the changes are covered by tests.

IB is updated. Also had to increase the estimate from 7 to 19 because I expect we will need more time for tests.

@aaemnnosttv
Copy link
Collaborator

Thanks @eugene-manuilov – overall IB looks good, just one question.

findPropertyProfile( accountID, propertyID, defaultProfileID = '' )

What do you think about leaving the defaultProfileID off, and getting this from the property internally? We don't store this so the property would have to be loaded anyways. Internally it would just wait for properties, get the property by ID and then see if it has a defaultProfileId? I think we'd have to do the same outside of this action to use that so I think it would make sense to move inside 🤔

@eugene-manuilov
Copy link
Collaborator

What do you think about leaving the defaultProfileID off, and getting this from the property internally? We don't store this so the property would have to be loaded anyways. Internally it would just wait for properties, get the property by ID and then see if it has a defaultProfileId? I think we'd have to do the same outside of this action to use that so I think it would make sense to move inside

It won't work, because we pass defaultProfileID of a matched property, not a selected one.

@aaemnnosttv
Copy link
Collaborator

Thanks @eugene-manuilov – per our conversation, we can leave that action as you have it defined now and revisit that later when we have more uses of it 👍

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment Jul 12, 2021
@aaemnnosttv aaemnnosttv removed their assignment Jul 13, 2021
@wpdarren wpdarren self-assigned this Jul 14, 2021
@fhollis fhollis added the Rollover Issues which role over to the next sprint label Jul 19, 2021
@fhollis fhollis modified the milestones: Sprint 53, Sprint 54 Jul 19, 2021
@wpdarren
Copy link
Collaborator

QA Update: Pass ✅

Verified:

  • The account defaulted because it matched the current website URL. The account has both UA and GA4 properties.
  • When you select GA4 property in the top dropdown, the UA property is selected in the dropdown in the blue area.
  • When you select UA property at the top dropdown the GA4 property is preselected in the dropdown in the blue area.
ga44.mp4

@wpdarren wpdarren removed their assignment Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P2 Low priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants