Copy over relevant parts of the `Analytics` class into `Analytics_4`

[nfmohit]

Feature Description

In an effort to use analytics-4 as the singular Analytics module, the goal is to make the Analytics class obsolete and only use the Analytics_4 class going forward.

Even though most of the Analytics class is no longer relevant after the removal of UA, there are still some parts that are still relevant, and thus should be copied over to Analytics_4. Some notable pieces are:

1. handle_provisioning_callback method: This method co-exists between both Analytics and Analytics_4 classes. They should be merged into one removing the need to use a hook to trigger the other.
2. get_debug_fields method: Since the accountID setting will be a part of Analytics_4, its get_debug_fields method should be updated to include debug information for this setting.
3. register method: This method includes different callbacks to hooks that should be carefully evaluated and merged.
4. is_tracking_disabled & print_tracking_opt_out methods: These methods implement the Analytics tracking opt-out for certain user groups. They should be merged accordingly.
5. Analytics\Advanced_Tracking class: This class should be migrated to the Analytics_4 namespace, but not be instantiated, as it will be updated to work with GA4 as a part of #7145.

---

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

Acceptance criteria

• Methods and pieces of the Modules\Analytics class that are still relevant after the removal of UA should be copied over to the Modules\Analytics_4 class.
• It should be ensured that Site Kit can work as expected with the Modules\Analytics_4 class as the only singular Analytics module class keeping in mind that the Modules\Analytics class will no longer be instantiated/will be removed soon (currently planned with #7932).
• Pieces to be copied should include but might not be limited to the following:
  1. handle_provisioning_callback method: This method co-exists between both Analytics and Analytics_4 classes. They should be merged into one removing the need to use a hook to trigger the other.
  2. get_debug_fields method: Since the accountID setting will be a part of Analytics_4, its get_debug_fields method should be updated to include debug information for this setting.
  3. register method: This method includes different callbacks to hooks that should be carefully evaluated and merged.
  4. is_tracking_disabled & print_tracking_opt_out methods: These methods implement the Analytics tracking opt-out for certain user groups. They should be merged accordingly.
  5. Analytics\Advanced_Tracking class: This class should be migrated to the Analytics_4 namespace, but not be instantiated, as it will be updated to work with GA4 as a part of #7145.
• Any other methods/pieces that are not listed above should be carefully evaluated to ensure that the analytics-4 module doesn't need it to function. If any other method is needed for the analytics-4 module (e.g. for REST endpoints, populating/updating module settings, etc.), it should be copied to and adapted for the analytics-4 module.
• If a piece copied to Modules\Analytics_4 retains its functionality even without being present in the Modules\Analytics class, and if it is deemed safe to remove it from Modules\Analytics, it should be eliminated.
• It should be ensured that no duplication is introduced due to the migration here, e.g. duplicate tags, duplicate hooks, etc.

Implementation Brief

• From includes/Modules/Analytics.php
  • Merge/Update the methods mentioned in AC
  • From register method
  • Move the check for reseting data available on propertyID change, to the Analytics 4 settings change check
  • Move PROVISION_ACCOUNT_TICKET_ID, READONLY_SCOPE, PROVISION_SCOPE and EDIT_SCOPE constants
  • Copy over the GET:accounts-properties-profiles endpoint definition and it's usage in create_data_request (see comment)
• In includes/Modules/Analytics_4.php
  • Replace constants usage from old Analytics module Analytics::X to be used from within the Analytics 4 class where needed

Test Coverage

• Update tests/phpunit/integration/Modules/Analytics_4Test.php tests, copy/adapt relevant parts from AnalyticsTest

QA Brief

• This issue changes a lot of code that handles:
  • the creation of a new GA4 property,
  • saving GA4 settings,
  • outputting "tracking opt-out" script within the site
  • outputting the actual GA4 snippet itself and
  • outputting debug fields in Site Info
• So a full GA4 end-to-end test should be performed to ensure all of the above behaviour remains the same as it is now. Comparing the above functionality with the existing main branch for regressions would be a good idea.

QA Eng

• Within includes/Modules/Analytics_4.php, within the setup_info() method, set internal to false. Now go to the Settings Admin screen and you will see a row for Analytics 4. Click on this row and click the 'Edit' button. Ensure the new analytics-4/SettingsEdit component is rendered correctly.
• Try saving and editing all settings and then test the Site health info new field "Analytics 4 ads conversion ID" to ensure it is saved correctly (to make sure this comment is covered).

Changelog entry

• Relocate infrastructure from legacy Analytics module.

[tofumatt]

@nfmohit Because this deals with functionality after the removal of UA, but doesn't explicitly specify what that is in the AC nor is blocked by a UA removal issue, can you add the points you mention in the issue description to the ACs? Just so it's extra-clear what should be moved over and what shouldn't be 🙂

Then this should be good-to-go 👍🏻

[nfmohit]

@nfmohit Because this deals with functionality after the removal of UA, but doesn't explicitly specify what that is in the AC nor is blocked by a UA removal issue, can you add the points you mention in the issue description to the ACs? Just so it's extra-clear what should be moved over and what shouldn't be 🙂

Then this should be good-to-go 👍🏻

Thank you for the kind review, @tofumatt. I intentionally wanted to leave this out as open-ended so that the specific discovery could be a part of the IB process. There might be other small pieces still needed outside of the methods/pieces that are mentioned in the issue description which might've been missed while authoring the design doc. According to your feedback, I have included them in the ACs, but also left another point about being cautious of any other piece that might still be needed. LMKWYT, thanks!

[tofumatt]

Sounds good, that makes sense 👍🏻

Moving to IB 🙂

[techanvil]

Hey @zutigrm, thanks for drafting this IB. A few notes:

• The points about googlesitekit_analytics_adsense_linked and exception_to_error() are not needed as these relate to the UA AdSense linking functionality which is no longer relevant. GA4 does now provide the ability to link to AdSense but works a bit differently, and we have an epic underway to add support for it.
• The googlesitekit_dashboard_sharing_data filter hook in Analytics doesn't actually do anything useful - in fact, it's clear we missed this aspect in this recent PR and the add_filter() call should have been removed entirely. Seeing as the Analytics class is being removed anyway, we can leave it there but there's no need to copy it to Analytics_4.
• I think we'll still need the GET:accounts-properties-profiles endpoint until we fully move from the analytics getAccounts() to analytics-4 getAccountSummaries(), please could you check this and update as necessary?
• We'll also need to migrate the check for accountID in is_connected().
• Lastly, it's by no means essential, but you could update the first two permalinks as the linked code has changed slightly (or at least the surrounding context of it has).

[zutigrm]

@techanvil Thank you for the review.

exception_to_error

This seems to be needed, as it is used in Analytics 4 method here

I think we'll still need the GET:accounts-properties-profiles endpoint until we fully move from the analytics getAccounts() to analytics-4 getAccountSummaries(), please could you check this and update as necessary?

This is being done as part of #7637, which is switching to account summaries, so GET:accounts-properties-profiles doesn't seem to be needed

We'll also need to migrate the check for accountID in is_connected().

This will be done as part of #7932 where final migration and removal of old analytics module is done, which also turns off the internal property and makes it only Analytics module, which makes more sense that is_connected will be checked against accountID, but I can include it here is well, if it makes sense to be done as part of this issue?

Let me know what you think

[techanvil]

Thanks @zutigrm.

exception_to_error

This seems to be needed, as it is used in Analytics 4 method here

This Analytics_4 method calls the version of exception_to_error() defined in the superclass, it doesn't need the overridden version that Analytics provides.

I think we'll still need the GET:accounts-properties-profiles endpoint until we fully move from the analytics getAccounts() to analytics-4 getAccountSummaries(), please could you check this and update as necessary?

This is being done as part of #7637, which is switching to account summaries, so GET:accounts-properties-profiles doesn't seem to be needed

Good spot there, however, if you review the search results for Analytics getAccounts() vs the IB for #7637, there will still be some remaining usage of it. As the description for that issue alludes to, it's just a start of the migration from getAccounts() to getAccountSummaries() so we'll still need this in the short term at least.

We'll also need to migrate the check for accountID in is_connected().

This will be done as part of #7932 where final migration and removal of old analytics module is done, which also turns off the internal property and makes it only Analytics module, which makes more sense that is_connected will be checked against accountID, but I can include it here is well, if it makes sense to be done as part of this issue?

Good spot, and seeing as the change to is_connected() is specced in the IB for that issue, we can leave it out of this one.

[zutigrm]

@techanvil Got it, thanks! IB updated

[techanvil]

Thanks @zutigrm! IB LGTM. :white_check_mark:

[techanvil]

Hey @jimmymadon @zutigrm - apologies for missing this during the IB review, but I've just realised we are creating a GA4 version of getAccounts() in https://github.com/google/site-kit-wp/issues/7637, which uses accountSummaries under the hood, so we should in fact no longer need the GET:accounts-properties-profiles endpoint after all.

[nfmohit]

@aaemnnosttv @jimmymadon I have added the following to the ACs for clarification:

• If a piece copied to Modules\Analytics_4 retains its functionality even without being present in the Modules\Analytics class, and if it is deemed safe to remove it from Modules\Analytics, it should be eliminated.
• It should be ensured that no duplication is introduced due to the migration here, e.g. duplicate tags, duplicate hooks, etc.

Please let me know if that looks good, thank you!

[mohitwp]

QA Update ⚠️

@jimmymadon I noticed that under on dev environment under Site health info new field "Analytics 4 ads conversion ID" is added. This field is not exist on latest environment. I added Ads conversion id under settings and it also showing under Analytics 4 snippet but site heath info field still shows "None" even after adding the Ad conversion id.

• Tested on dev environment.
• Created new GA4 a/c on fresh site.
• Verified all Ga4 settings under edit component.
• Verified that all fields are getting save as per selection.
• Verified that user is able to create new property.
• Verified that user is able to disconnect and connect analytics.
• Verified analytics functionality with latest environment.
• Verified analytics Opt Out Snippet.
• Verified analytics snippet.
• Also, verified snippet on site on which UA property is connected by switching on to dev environment.
• Analytics and opt out snippet is same as latest environment.

GA4 snippet with Key metrics  Ga4 snippet with Ads conversion  GA4 opt out snippet 

[jimmymadon]

@mohitwp This is normal as when you edited the settings above as a regression test, you were still editing the settings of the old Analytics module (not the Analytics 4) module. I have just updated the QAB to add a note for QA Eng to test this by saving Analytics 4 module settings.

[mohitwp]

QA Update ✅

Thanks @jimmymadon !

• Tested on dev environment.
• Created new GA4 a/c on fresh site.
• Verified all Ga4 settings under edit component.
• Verified that all fields are getting save as per selection.
• Verified that user is able to create new property.
• Verified that user is able to disconnect and connect analytics.
• Verified analytics functionality with latest environment.
• Verified analytics Opt Out Snippet.
• Verified analytics snippet.
• Also, verified snippet on site on which UA property is connected by switching on to dev environment.
• Analytics and opt out snippet is same as latest environment.

GA4 snippet with Key metrics  Ga4 snippet with Ads conversion  GA4 opt out snippet 

[mxbclang]

@mohitwp Should this now be in Approval? Thanks!

[mohitwp]

@bethanylang It's require QA:Engreview.

[mxbclang]

@mohitwp Ah, missed that, thank you!

[hussain-t]

QA:Eng Verified ✅

• Verified that the Analytics 4 module has been listed in the settings screen.
• Verified the settings edit displays the relevant data and resembles the data in the settings view.
• Verified creating/modifying accounts, properties, web datastream, and other settings work as expected.
• Verified the Analytics 4 ads conversion ID field is available in the Site Health.
• Note: The accountID in the Account Select and the Property Select had to be sourced from the MODULES_ANALYTICS_4 datastore to get the settings edit screen to work properly. It will be fixed in #7932