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

Improve default Analytics property selection when selecting an Analytics account #3291

Closed
felixarntz opened this issue May 4, 2021 · 14 comments
Labels
Module: Analytics Google Analytics module related issues P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented May 4, 2021

This enhancement purely relates to the existing Analytics module and the modules/analytics store, but it is still part of the GA4 epic since it ensures certain prerequisites to bring the same functionality to GA4 are in place.

Currently, when selecting an Analytics account (in the module setup or settings), the first property will be initially selected. This experience should be improved to be as follows:

  • If one of the properties within the account matches the current site's URL, that property should be initially selected.
  • If none of the properties within the account matches the current site's URL, no property should be initially selected.

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

Acceptance criteria

  • A new selector getSiteURLPermutations() should be added to the core/site store. It should create return all 4 "permutations" of the current site URL, including/excluding www. and with http:// or https://. No trailing slash should be included at the end of each permutation.
  • A new action matchPropertyByURL( properties, url ) should be implemented in the modules/analytics store, with a purpose similar to the action of the same name in the modules/analytics-4 store (see Allow matching a GA4 property by data in its web data stream #3168).
    • It should simply iterate over the given list of property objects until it finds a property that has a websiteUrl which matches the given url (if url is a string) or matches any of the given values in url (if url is an array of strings).
  • The existing logic to potentially make a cascading (UA) Analytics property selection in the selectAccount action in the modules/analytics store should be enhanced to try to find a matching property rather than just selecting the first property:
    • It should wait for the modules/analytics selector getProperties and, once properties have been fetched, rely on the new matchPropertyByURL action in the same store (see above) passing the getSiteURLPermutations array of URLs to attempt to find a matching UA property for the current site URL. If one is found, the selectProperty action should be called with it so that it is set.
    • In other words, at the end of this action, either the matched property should be selected or no property should be selected at all.
  • The logic in the getProperties resolver that potentially selects the first property if none is selected yet should be removed, since it is no longer relevant - this should have always happened from within selectAccount as it is the appropriate action to trigger any automated sub-selections.

Implementation Brief

  • Create a new selector in the core/site store called getSiteURLPermutations, which should:
    • select the current site URL using the getReferenceSiteURL selector
    • return an array of all four permutations of the URL:
      • with and without www.
      • with http:// or https://
    • No trailing slash should be included at the end of the returned URLs
    • Add unit tests for this new selector
  • Create a new action, matchPropertyByURL in the Analytics store, which accepts properties and url arguments
    • properties should be an array of property objects
    • url should either be a URL string or an array of URL strings
    • It should iterate over the given list properties until it finds a property that has a websiteUrl which matches
      • the given url (if url is a string)
      • any of the given values in url (if url is an array of strings)
    • Return the matching property if one is found, otherwise return null.
  • Inside the selectAccount selector in the Analytics datastore, after the getProperties selector is called:
    • call the new getSiteURLPermutations selector to get an array of URL permutations
    • call the new matchPropertyByURL action, passing the array of URLs
    • if a matching property is returned, call the selectProperty action with it so that it is set.
    • This means that at the end of the selectAccount action, either the matched property should be selected or no property should be selected at all.
    • Add unit test coverage for this new behaviour
  • Inside the Analytics module's getProperties resolver, remove the logic that selects the first property if none is already selected.

Test Coverage

  • Unit test coverage should be added for the new getSiteURLPermutations selector and the behaviour in selectAccount. Existing tests for getProperties may need adjusting.

Visual Regression Changes

  • Images dealing with the Analytics module / settings flows that include selecting an account may need updating.

QA Brief

  • Create a new site on https://jurassic.ninja/
  • Go to Google Analytics and create a new property for the newly created site
  • Go back to the jurassic site and install Site Kit plugin there
  • Install tester plugin as well and select Site Kit version for this ticket
  • Activate the Analytics module and select your account that contains the newly created property for this site.
  • Make sure that the correct property has been selected automatically once the correct account has been selected.

Extra criteria for follow-up QA

  • When switching to an account where no property using the current URL is present, no property should be initially selected (i.e. the dropdown should just be empty and show "Property").
    • The only case where it should default to "Set up a new property" is if the account has no UA properties at all (i.e. "Set up a new property" is the only option available).

Changelog entry

  • Enhance default Analytics property selection when selecting an Analytics account.
@tofumatt
Copy link
Collaborator

Create a new action, matchPropertyByURL in the Analytics store, which accepts properties and url arguments

I don't see the intended return value of this action in the IB. In the GA4 implementation of a similar action we return the property via getProperty(): https://github.com/google/site-kit-wp/pull/3311/files#diff-6710dc1eb58e3617c6fe1582bf4de6f061f0c38b85f2548688a8936056f098afR191-R193

Is the intention here to have the matchPropertyByURL() action also set the selected property if it finds one by dispatching an action?

From the IB it looks like it should be returning a property if a matching one is found, as below I see:

if a matching property is returned, call the selectProperty action with it so that it is set.

So sounds like the matchPropertyByURL action should return a property.


Otherwise this looks good, but can you clarify how the action should return when it finds a match? I think what I expect is what you implied, but it was a bit unclear so I think could use clarification.

@tofumatt tofumatt assigned johnPhillips and unassigned tofumatt May 11, 2021
@johnPhillips
Copy link
Contributor

@tofumatt Good catch - thank you 🙏

So sounds like the matchPropertyByURL action should return a property.

From reading the ACs, I guess my assumption was that the action is passed an array of properties and it would be like an array filter: return the matching property if there is one or some falsey value if there isn't (I see the GA4 one returns null for example).

Does that sound good? I've updated the IB with this for now, but let me know if it needs changing or I have misunderstood.

@johnPhillips johnPhillips removed their assignment May 12, 2021
@tofumatt tofumatt self-assigned this May 13, 2021
@tofumatt
Copy link
Collaborator

That makes perfect sense and is what I expect. Returning null makes sense to me, and it's good to keep it consistent with similar methods elsewhere in the app.

IB ✅

@fhollis fhollis added this to the Sprint 50 milestone May 24, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented May 24, 2021

QA Update: Confirm with Engineer ⚠️

@eugene-manuilov when I select the account, it does not default to this property for the URL.

It's still showing as Set up a new property I've tried this on a few new sites.

Does it possibly need time to pick up the new property in the API? I also noticed that it has changed where you now need to create a stream for web with the URL, not sure if that is a new change but not noticed it before.

image

image

@eugene-manuilov
Copy link
Collaborator

@wpdarren you need to create the regular UA property, not GA4 as you created above.

@wpdarren
Copy link
Collaborator

QA Update: Pass ✅

Verified:

  • When you select the account the property related to the URL appears as default.

image

@eugene-manuilov thank you, I'd not realised that by default it creates it as GA4, and that you have to select UA property.

@wpdarren wpdarren removed their assignment May 24, 2021
@felixarntz felixarntz reopened this Jun 4, 2021
@felixarntz felixarntz self-assigned this Jun 4, 2021
@felixarntz
Copy link
Member Author

Approval ❌

@eugene-manuilov @aaemnnosttv This is not working as expected in my testing:

  1. I'm on https://beckandgalo-wordpress-amp.pantheonsite.io/.
  2. On pageload of the GA setup, it shows the correct account and UA property.
    Screen Shot 2021-06-03 at 5 11 09 PM
  3. Then I change the account to another one where I also have access to a UA property for the same URL. But now it shows "Set up new property" even though a UA property that should be relevant for my current URL is also present in the account.
    Screen Shot 2021-06-03 at 5 14 12 PM
    • This is problematic in two ways: Not only is the property that I expect not selected, but also instead it selects "Set up new property" as fallback, whereas it should not select anything at all in case there was no match.

Not sure this is an edge-case, but there is definitely something off here.

@eugene-manuilov
Copy link
Collaborator

... Not only is the property that I expect not selected ...

Hm... Unfortunately, I can't reproduce it on my end. It selects the property correctly for me...

... but also instead it selects "Set up new property" as fallback, whereas it should not select anything at all in case there was no match.

Ok, makes sense. I have created a follow-up PR to fix this: #3481. @felixarntz do you mind reviewing it?

@felixarntz
Copy link
Member Author

@eugene-manuilov Left some feedback, the PR mostly looks good for the fallback issue. I left some follow-up questions to clarify my scenario further - please try to recreate exactly that if you haven't yet.

@eugene-manuilov
Copy link
Collaborator

This fixes the fallback selection issue as expected - the PR needs to be based on main and against main though, since it's for a release issue :)

@felixarntz I had to create a new branch and create a new PR #3484 because I couldn't rebase that branch :(

About my other problem, have you tried to recreate the exact same situation I've described in 3291 (comment)?

Yes, it seems to work on my end. Even don't know why it doesn't work for you 🤔

@felixarntz
Copy link
Member Author

@eugene-manuilov Merged the PR. I just figured out why the property switching didn't work for me: It was simply because the one property I was using didn't actually have the websiteUrl populated, that field was empty for that property (it was using the URL as property name, but that's not what Site Kit looks at of course) 🤦‍♂️

So this is all good to go! 🚢

@felixarntz felixarntz removed their assignment Jun 4, 2021
@cole10up cole10up self-assigned this Jun 6, 2021
@cole10up
Copy link

cole10up commented Jun 6, 2021

### QA ✅

Retested and setup a new account and property:
image

Installed and activated SK with the user that has this account and property

Noticed the property is missing after selecting the new account with the property that had been setup.
image

Revisited my property setup and adjusted to a 'universal Analytics property'

Tried my setup again and confirmed the property was displayed properly.

image

Confirmed configuration completed and functions properly.

image

Initially sent the ticket back to execution as I missed the 'universal Analytics property' flipper when setting up the property. Sending to approval.

@cole10up cole10up assigned felixarntz and unassigned cole10up and felixarntz Jun 6, 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 P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants