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

Add Analytics setup/settings logic to determine Google Tag settings #6081

Closed
felixarntz opened this issue Oct 27, 2022 · 5 comments
Closed
Labels
Module: Analytics Google Analytics module related issues P0 High priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Oct 27, 2022

With the new Google Tag logic in place, the Analytics module now needs to determine the Google Tag ID to use whenever the GA4 web data stream / measurement ID changes. Concretely, this could be the case in the module setup, or settings, or the GA4 activation banner logic.


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

Acceptance criteria

If the gteSupport feature flag is enabled:

  • Whenever the GA4 web data stream is set/updated (including when a new one has been just created) in the modules/analytics-4 store (concretely, the webDataStreamID and measurementID settings), logic should run to populate the corresponding Google Tag data:
    • The getGoogleTagContainer( measurementID ) selector from Implement Google Tag container lookup and destinations list JS selectors #6079 should be used to identify the Google tag container.
    • This will return a container object, from which the following values need to be stored as module settings (see Add Google Tag settings to the Analytics_4 module #6077 which introduced these settings):
      • The accountId and containerId fields on the object should be stored as googleTagAccountID and googleTagContainerID respectively.
      • To set the googleTagID setting, the following logic should be used (based on the tagIds field):
        • If the tagIds list contains any tag starting in "GT-", use that one. (If there are multiple "GT-" tags, use any one, e.g. the first one.)
        • Otherwise, if there is any tag ID that is the same as the GA4 measurement ID, use that one.
        • Otherwise, if there is any other tag ID that starts in "G-", use that one.
        • Otherwise, use any tag ID in there (unlikely but possible - this would be a tag ID starting in "AW-").
    • Saving the GA4 module settings should then naturally save those 3 additional settings as well.
  • The same logic as above also needs to be implemented on the server-side for the Analytics provisioning callback (concretely in Analytics_4::handle_provisioning_callback()) where the new web data stream is being created.
    • Since the same logic is needed on the server and client for different use-cases, potentially this should be implemented in another REST data point that handles all of it, so that the logic happens on the server, but can be triggered on demand from the client.
  • From an end user perspective, the above should result in those 3 settings to be set whenever a user goes through the Analytics setup, modifies the GA4 settings, or goes through the GA4 activation banner with the feature flag enabled.

Implementation Brief

Note, taking the direction from the AC to implemented a REST data point to share logic, this IB does not make use of the getGoogleTagContainer, rather the container lookup is performed on the server side via the REST data point.

Within includes/Modules/Analytics_4.php:

  • Create a new protected method get_google_tag_settings_for_measurement_id.
    • This should take a $measurement_id parameter.
    • Reusing the logic from the REST data points implemented in Implement Google Tag container lookup and destinations list REST data points #6078 (extract this logic to separate methods if needs be):
      • Retrieve the Google Tag container for the measurement ID.
      • Create an array with keys googleTagAccountID and googleTagContainerID with the values of the container fields accountId and containerId.
      • Set the googleTagID key on this array with the value according to the logic detailed in the AC.
    • Return this array as the method's return value.
  • Create a new REST data point, GET:google-tag-settings.
    • This should expect a measurementID parameter in the request payload.
      • As the data point name is more generic this could be expanded to receive other parameters in future.
    • Call get_google_tag_settings_for_measurement_id with the measurement ID and return the result.
    • If an error occurs, return a suitable error status code and message.
  • If the gteSupport feature flag is enabled:
    • In the handle_provisioning_callback method, call get_google_tag_settings_for_measurement_id with the web data stream's measurement ID and merge the result into the module settings.

Within assets/js/modules/analytics-4/datastore/properties.js:

  • Create a fetch data store called getGoogleTagSettings for the new GET:google-tag-settings data point.
  • Add a new action creator generator function *updateSettingsForMeasurementID.
    • This should take a measurementID parameter.
    • Dispatch setMeasurementID with the passed measurementID.
    • If the gteSupport feature flag is enabled:
      • If measurementID is empty dispatch setGoogleTagAccountID, setGoogleTagContainerID and setGoogleTagID with empty strings and return.
      • Otherwise dispatch fetchGetGoogleTagSettings, wait for the result, and on success dispatch setGoogleTagAccountID, setGoogleTagContainerID and setGoogleTagID with the respective properties of the result.

Across the codebase:

  • Search for all invocations of setMeasurementID. At present these are in the following files and all are suitable candidates for replacing with updateSettingsForMeasurementID.
    • assets/js/modules/analytics/components/common/PropertySelectIncludingGA4.js
    • assets/js/modules/analytics/components/settings/GA4SettingsControls.js
    • assets/js/modules/analytics-4/datastore/properties.js
    • assets/js/modules/analytics-4/datastore/settings.js
  • For each setMeasurementID call:
    • Verify it's a scenario where the Google Tag settings should also be updated, and replace it with updateSettingsForMeasurementID.
    • Ensure that any resulting error is displayed/handled according to the error handling for that scenario.

Test Coverage

  • Add PHPUnit tests for the PHP additions and changes.
  • Add Jest tests for the new data store functions.
  • Fix any failing tests.

QA Brief

  • No regression should be spotted while gteSupport feature flag is not enabled in the following areas of the pluggin:
    • Analytics module setup with GA4.
      • Creating a new property and selecting existing one.
    • Analytics Settings
      • Changing Analytics and GA4 properties.
  • With gteSupport feature flag enabled:

Changelog entry

  • Determine Google Tag settings when configuring Analytics.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Oct 27, 2022
@felixarntz felixarntz self-assigned this Oct 27, 2022
@felixarntz felixarntz removed their assignment Oct 27, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Nov 16, 2022
@tofumatt tofumatt self-assigned this Nov 24, 2022
@tofumatt
Copy link
Collaborator

Looks good to me, I think this approach works 👍🏻

IB ✅

@tofumatt tofumatt removed their assignment Nov 24, 2022
@kuasha420 kuasha420 self-assigned this Jan 4, 2023
@kuasha420 kuasha420 removed their assignment Jan 23, 2023
@nfmohit nfmohit assigned nfmohit and kuasha420 and unassigned nfmohit Jan 23, 2023
@kuasha420 kuasha420 assigned nfmohit and unassigned kuasha420 Jan 23, 2023
@nfmohit nfmohit removed their assignment Jan 23, 2023
@techanvil techanvil self-assigned this Jan 24, 2023
@kuasha420 kuasha420 assigned kuasha420 and techanvil and unassigned techanvil and kuasha420 Jan 24, 2023
techanvil added a commit that referenced this issue Jan 25, 2023
…settings

Enhance/#6081 analytics gte tag settings
@techanvil techanvil removed their assignment Jan 25, 2023
@mohitwp mohitwp self-assigned this Jan 31, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Feb 8, 2023

QA Update ⚠️

@kuasha420 I completed the testing as per QAB and also tested the #6080.
But AC have this point -The same logic as above also needs to be implemented on the server-side for the Analytics provisioning callback (concretely in Analytics_4::handle_provisioning_callback()) where the new web data stream is being created. Since the same logic is needed on the server and client for different use-cases, potentially this should be implemented in another REST data point that handles all of it, so that the logic happens on the server, but can be triggered on demand from the client.

I'm not sure about this point and if anything related to this is included in the QAB and wondered if it needed a QA:Eng label on it ?

@kuasha420
Copy link
Contributor

@mohitwp Nice catch. I think we've implemented it here but a QA:Eng pass would remove all doubt. So, feel free to add the label and proceed accordingly! Cheers.

@mohitwp
Copy link
Collaborator

mohitwp commented Feb 8, 2023

QA Update ✅

  • Tested on dev.

  • Verified the Analytics setup when gteSupport feature flag is disabled.

  • Tested the Analytics set up with new and existing property.

  • Tested the analytics settings view and functionality.

  • When gteSupport feature flag is active

  • Verified Amend GA4 tag placement logic to use Google Tag ID if set #6080 as steps mentioned under it's QAB.

  • Verified tag placement for GA4 is the Google Tag ID, instead of the GA4 measurement ID when gteSupport feature flag is active.

  • If the gteSupport feature flag is disabled, verified that the Google Tag ID is replaced with the fallback GA4 measurement ID.

  • Verified below point in AC.

To set the googleTagID setting, the following logic should be used (based on the tagIds field):
If the tagIds list contains any tag starting in "GT-", use that one. (If there are multiple "GT-" tags, use any one, e.g. the first one.)
Otherwise, if there is any tag ID that is the same as the GA4 measurement ID, use that one.
Otherwise, if there is any other tag ID that starts in "G-", use that one.
  • Tested after changing the analytics a/c and property when gteSupport feature flag is active and verified the source code.
  • Tested analytics set up.
  • Tested analytics set up through GA4 activation banner when GA4 feature flag is active.
  • Tested analytics settings.
  • Combine tag and change destination at another site and checked that after disconnecting and connecting from same a/c Google tags changes or not.
  • I'm not able to generate any condition where gtag starts with AW.

Assigning QA:eng after discussion with @kuasha420 . AC have some technical points which require confirmation from QA:eng

When getSupport feature flag is not active and analytics is connected.

image

When getSupport feature flag is active and analytics is connected.

image

image

gtag starting with g

image

@mohitwp mohitwp removed their assignment Feb 8, 2023
@mohitwp mohitwp added the QA: Eng Requires specialized QA by an engineer label Feb 8, 2023
@jimmymadon jimmymadon self-assigned this Feb 8, 2023
@jimmymadon
Copy link
Collaborator

QA Eng ✅

  • Performed an additional code review and verified that the logic to determine Google Tag settings is performed in the backend. The same logic (Analytics_4::get_google_tag_settings_for_measurement_id()) is used within Analytics_4::handle_provisioning_callback and in the new GET:google-tag-settings endpoint which is used to populate the Google tag settings in the front-end.

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 QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants