site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Missing logic condition for error: 'Looks like you're already using Google Analytics within your Google Tag Manager configuration' message

Open adamdunnage opened this issue 3 years ago • 7 comments

Bug Description

The following error message:

"Error: Looks like you're already using Google Analytics within your Google Tag Manager configuration. However, it's Analytics property UA-#########-# is different from the Analytics property , which is currently selected in the plugin. You need to configure the same Analytics property in both places."

displays when editing the Tag Manager settings within Site Kit under these circumstances:

  • Tag Manager container has been manually created on the GTM site with an Analytics tag set up as part of it. This GTM snippet is manually placed on the website.
  • GTM module within Site Kit has been connected with that same GTM container.
  • The Analytics module connection has been started but is cancelled before being completed.

If the Analytics module is not yet connected then I am not sure why this message would show because there shouldn't be an Analytics UA code to find which shows in the error message as the UA code it tries to return is blank. I am not sure this message should be showing in the GTM settings or if it should be in the Analytics settings.

It looks like there may be a missing condition in the logic for this.

Steps to reproduce

  1. Create a fresh new site on https://app.instawp.io/onboard.
  2. Install Site Kit on site and connect to Analytics to create new account.
  3. From Tag Manager site set up account and container for new site with Analytics ID from newly set up account in step 2.
  4. Manually insert Tag Manger code to site head and body of the site and save.
  5. Disconnect Analytics from Site Kit.
  6. Connect Tag Manager in Site Kit.
  7. Start Analytics set up in Site Kit but cancel before completing.
  8. Go to Tag Manger settings in Site Kit and edit.

Screenshots

https://drive.google.com/file/d/11lT7U7IT5P56bhGal1LJfr8xlx0GWAEh/view?usp=sharing

Additional Context

This only happens when the Analytics module has been started but not completed. If it has not been started or has been completed setup then this error does not show when editing GTM settings.


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

Acceptance criteria

  • Tag Manager should only consider the current Analytics property ID if Analytics is connected rather than only active

Implementation Brief

  • Using assets/js/modules/tagmanager/components/common/FormInstructions.js,
    • Check if the Analytics module is connected by querying the modules data store via the isModuleConnected selector, passing analytics as parameter.
    • Update the check for ! analyticsModuleActive && singleAnalyticsPropertyID to check for ! analyticsModuleConnected && singleAnalyticsPropertyID.

Test Coverage

  • No new tests to be added.

QA Brief

Changelog entry

adamdunnage avatar Dec 08 '21 10:12 adamdunnage

The problem here is that we use the active state of the module for the condition rather than connected state: https://github.com/google/site-kit-wp/blob/29e0605f8b91484245bf6b4eaefb39ab4bc31ede/assets/js/modules/tagmanager/components/common/FormInstructions.js#L66

aaemnnosttv avatar Jan 24 '22 12:01 aaemnnosttv

cc @felixarntz ACs ^

aaemnnosttv avatar Jan 24 '22 12:01 aaemnnosttv

Hi @asvinb are you still planning on working on this? If not, please unassign yourself so someone else can pick this up - thanks!

FlicHollis avatar May 26 '22 08:05 FlicHollis

@FlicHollis Picking it back again now. I started taking a look a while ago then switched to other higher priority tickets.

asvinb avatar Jun 09 '22 12:06 asvinb

IB ✔️

eugene-manuilov avatar Jun 10 '22 15:06 eugene-manuilov

@aaemnnosttv @asvinb @eugene-manuilov

Does this issue still actually exist? I tried to replicate it following the replication steps but couldn't. Also, it looks like this error message no longer exists in the codebase anymore. Am I missing something?

Thank you!

nfmohit avatar Aug 22 '22 06:08 nfmohit

@nfmohit I believe the message has changed but the relevant condition is this one I think:

https://github.com/google/site-kit-wp/blob/12d7bc1f78c5706add69e7e45de4c64a3e4effef/assets/js/modules/tagmanager/components/common/FormInstructions.js#L71-L73

It is probably an odd edge case to be able to satisfy singleAnalyticsPropertyID === analyticsPropertyID without Analytics being connected so there might not be anything really left to fix here.

Assigning back to @adamdunnage to confirm or close.

aaemnnosttv avatar Aug 23 '22 19:08 aaemnnosttv

I wasn't able to recreate this by following the steps to recreate in the main body of this issue. https://recordit.co/rLQ2PeDfzK

One of the reasons I was not able to recreate is Site Kit doesn't recognize an existing GA tag if this tag was added via Tag Manager and the Tag Manager snippet was added to a site by manually inserting the GTM code snippets (see #4103).

I also tried various configurations to recreate this, including the following:

  • Cancelling GA set up before and after selecting your Google account
  • Allowing for SK to insert the GTM snippet and not allowing SK to insert the GTM snippet (in both cases GTM was added manually)

@adamdunnage Maybe you can take a look at this when you're back? If you look at 4103 SK shouldn't recognize a UA snippet if added via a manually inserted GTM code snippet

jamesozzie avatar Sep 08 '22 15:09 jamesozzie

Thanks so much @jamesozzie @aaemnnosttv for now I will remove from sprints until Adam can take a look. Thanks all! cc @eclarke1

FlicHollis avatar Sep 08 '22 15:09 FlicHollis

@adamdunnage just a reminder on this one please, would you mind taking a look

eclarke1 avatar Oct 17 '22 09:10 eclarke1

@eclarke1 @FlicHollis @jamesozzie @aaemnnosttv Apologies for missing this one! I have followed the steps to reproduce once more and can no longer recreate this. This flow looks to now be working as expected so happy to close this one off.

adamdunnage avatar Oct 17 '22 09:10 adamdunnage